add set_gas_heater(); Was available on arduino but not CircuitPython #61

garberw · 2023-03-16T07:11:18Z

translated Bosch code from .cpp to python. Not sure about license.
Having trouble with code checks in pull request and how to fix them and resubmit.

… not CircuitPython.

garberw · 2023-03-16T07:28:51Z

all checks passed. took three attempts.

FoamyGuy · 2023-03-20T16:42:24Z

adafruit_bme680.py

@@ -45,10 +97,34 @@
 except ImportError:
    pass

-__version__ = "0.0.0+auto.0"
+__version__ = "3.4.12"


This version should stay the way it was. All of our libraries have this placeholder value that gets filled in by the actions when releases are made.

yes by all means; was not sure how to do this !!

FoamyGuy · 2023-03-20T16:44:18Z

Thanks for adding this additional functionality. Can you link to the code or project that you worked from where you got the license that was added?

I think the format for it needs to be a bit different, but I'm not positive, will bring it up to some others for input.

garberw · 2023-03-20T17:13:01Z

github adafruit bme680 arduino code including Bosch library
github adafruit bme680 arduino code bme68x.c file from Bosch library

Bosch code includes Bosch license.

My code is just a translation of the relevant parts of the Bosch library (the open source part) from C to Python.
We still need to check some of the bit manipulation. The C code expects uint8_t is different from uint32_t for
example whereas python uses "unlimited precision integers".

USE AT YOUR OWN RISK. I tried it and it runs. Since it is a heater I am not sure how to test if it works !!!!
I don't think it would really be likely to burn out your heater though.

NOTE the adafruit python defaults were the same as the C arduino defaults but until now we had no easy way to change them.

tekktrik · 2023-03-20T18:22:24Z

BSD should be REUSE compliant, for what it's worth. You can add multiple licenses to files by adding the license to LICENSES/ and adding it as an SPDX header: https://github.com/tekktrik/CircuitPython_CSV/blob/main/circuitpython_csv.py I think BSD 3 Clause is roughly equivalent to MIT but may be worth considering if we want to use code requiring multiple licenses.

I can also take a look at some of the typing here, and I think the syntax and exact implementation can be made to conform more to the other those of the other libraries due to implementation differences. For example, I see casts to float where I don't believe they're necessary because CircuitPython division of int and float always results in float so there's no need to cast.

garberw · 2023-03-20T22:41:35Z

Yes the casts to float were more of a form of documentation.

FoamyGuy · 2023-04-24T18:50:37Z

We discussed this in the weeds during 4/24 meeting. Conclusion was that it is okay to bring in BSD-3 licensed code.

I'll work on getting the attribution lines setup and revert the version back to 0s.

FoamyGuy

The latest commit moves the BSD license over to it's own file and is now re-use compliant and passing actions.

The changes look good to me, but I don't have the right hardware for testing the functionality.

tekktrik

Took a quick look, but I also don't have hardware I think. I think I'd like to take a second look at this once some of these points have been resolved just to see if there's anything I missed. I appreciate everyone's work!

tekktrik · 2023-06-05T20:52:38Z

LICENSES/BSD-3-Clause.txt

+@file       bme68x_defs.h
+@date       2021-05-24
+@version    v4.4.6


I think these should be removed.

tekktrik · 2023-06-05T20:53:20Z

adafruit_bme680.py

@@ -14,6 +16,10 @@

 * Author(s): Limor Fried

+
+original Adafruit_BME680 with addition of set_gas_heater().  Other new members are private.


This can be removed, I think we can just use the GitHub release to announce things similar to a changelog.

tekktrik · 2023-06-05T20:54:34Z

adafruit_bme680.py

+        *  @brief  Enable and configure gas reading + heater
+        *  @param  heater_temp
+        *          Desired temperature in degrees Centigrade
+        *  @param  heater_time
+        *          Time to keep heater on in milliseconds
+        *  @return True on success, False on failure


Looks like the Arduino-style docstrings, so instances of these will need to be converted to the typical style for sphinx.

tekktrik · 2023-06-05T20:55:59Z

adafruit_bme680.py

+INT32 = int
+INT16 = int
+INT8 = int
+UINT32 = int
+UINT16 = int
+UINT8 = int


I think using int in all applicable places is fine since there isn't really a distinct "typewise" within Python. If needed, docstrings should explain limits (e.g., 0 - 255). That will save us using type aliases where not strictly needed.

tekktrik · 2023-06-05T20:56:18Z

adafruit_bme680.py

@@ -116,6 +152,43 @@
 )


+# garberw added begin  ===========================


This should be removed since it's a contextual comment.

There's a few comments like these.

they were just meant to highlight changes. remove them for sure.

tekktrik · 2023-06-05T20:57:22Z

adafruit_bme680.py

+    return (reg_data & ~bitname_msk) | (data & bitname_msk)
+
+
+class GasHeaterException(Exception):


This looks like a hyperspecific exception that could just be replaced with something like OSError, most likely.

tekktrik · 2023-06-05T20:59:03Z

adafruit_bme680.py

+        if (heater_temp == 0) or (heater_time == 0):
+            return False


This could be:

Suggested change

if (heater_temp == 0) or (heater_time == 0):

return False

if not heater_temp or not heater_time:

return False

tekktrik · 2023-06-05T21:00:07Z

adafruit_bme680.py

+        op_mode: UINT8 = _BME68X_FORCED_MODE
+        # restrict to enable = True
+        enable: bool = True
+        nb_conv: UINT8 = 0
+        hctrl: UINT8 = _BME68X_ENABLE_HEATER
+        run_gas: UINT8 = 0
+        ctrl_gas_data_0: UINT8 = 0
+        ctrl_gas_data_1: UINT8 = 0
+        ctrl_gas_addr_0: UINT8 = _BME68X_REG_CTRL_GAS_0
+        ctrl_gas_addr_1: UINT8 = _BME68X_REG_CTRL_GAS_1


I think this is using a lot of local variables where using what they equate to might be more efficient. This comment holds for the other methods as well.

tekktrik · 2023-06-05T21:01:24Z

contributors.md

+# SPDX-FileCopyrightText: 2017 ladyada for Adafruit Industries
+#
+# SPDX-License-Identifier: MIT
+
+Contributors to Adafruit_CircuitPython_BME680_modified
+=============================================================
+
+Limor (Ladyada) Fried
+
+William Garber
+many others


It might be worth just adding contributors' names to the SPDX headers as opposed to using a contribuitors file (for consistency reason).

tekktrik · 2023-06-05T21:02:54Z

adafruit_bme680.py

+        except GasHeaterException as exc:
+            raise exc


Since the exception is just being passed through, I don't think there's a need to try the above block. There's another instance of this below as well.

garberw · 2023-06-05T23:06:08Z

+# garberw added begin =========================== these were just meant to be equivalent to doing a diff. go ahead and remove them. Who is making the changes listed below, me or someone else?

…

On 6/5/23 2:06 PM, Alec Delaney wrote: ***@***.**** requested changes on this pull request. Took a quick look, but I also don't have hardware I think. I think I'd like to take a second look at this once some of these points have been resolved just to see if there's anything I missed. I appreciate everyone's work! ------------------------------------------------------------------------ In LICENSES/BSD-3-Clause.txt <#61 (comment)>: > ***@***.*** bme68x_defs.h ***@***.*** 2021-05-24 ***@***.*** v4.4.6 I think these should be removed. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > @@ -14,6 +16,10 @@ * Author(s): Limor Fried + +original Adafruit_BME680 with addition of set_gas_heater(). Other new members are private. This can be removed, I think we can just use the GitHub release to announce things similar to a changelog. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + * @brief Enable and configure gas reading + heater + * @param heater_temp + * Desired temperature in degrees Centigrade + * @param heater_time + * Time to keep heater on in milliseconds + * @return True on success, False on failure Looks like the Arduino-style docstrings, so instances of these will need to be converted to the typical style for sphinx. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > +INT32 = int +INT16 = int +INT8 = int +UINT32 = int +UINT16 = int +UINT8 = int I think using |int| in all applicable places is fine since there isn't really a distinct "typewise" within Python. If needed, docstrings should explain limits (e.g., 0 - 255). That will save us using type aliases where not strictly needed. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > @@ -116,6 +152,43 @@ ) +# garberw added begin =========================== This should be removed since it's a contextual comment. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + Macro to set bits + data2 = data << bitname_pos + set masked bits from data2 in reg_data + """ + return (reg_data & ~bitname_msk) | ((data << bitname_pos) & bitname_msk) + + +def bme_set_bits_pos_0(reg_data, bitname_msk, data): + """ + Macro to set bits starting from position 0 + set masked bits from data in reg_data + """ + return (reg_data & ~bitname_msk) | (data & bitname_msk) + + +class GasHeaterException(Exception): This looks like a hyperspecific exception that could just be replaced with something like |OSError|, most likely. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + if (heater_temp == 0) or (heater_time == 0): + return False This could be: ⬇️ Suggested change - if (heater_temp == 0) or (heater_time == 0): - return False + if not heater_temp or not heater_time: + return False ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + op_mode: UINT8 = _BME68X_FORCED_MODE + # restrict to enable = True + enable: bool = True + nb_conv: UINT8 = 0 + hctrl: UINT8 = _BME68X_ENABLE_HEATER + run_gas: UINT8 = 0 + ctrl_gas_data_0: UINT8 = 0 + ctrl_gas_data_1: UINT8 = 0 + ctrl_gas_addr_0: UINT8 = _BME68X_REG_CTRL_GAS_0 + ctrl_gas_addr_1: UINT8 = _BME68X_REG_CTRL_GAS_1 I think this is using a lot of local variables where using what they equate to might be more efficient. This comment holds for the other methods as well. ------------------------------------------------------------------------ In contributors.md <#61 (comment)>: > +# SPDX-FileCopyrightText: 2017 ladyada for Adafruit Industries +# +# SPDX-License-Identifier: MIT + +Contributors to Adafruit_CircuitPython_BME680_modified +============================================================= + +Limor (Ladyada) Fried + +William Garber +many others It might be worth just adding contributors' names to the SPDX headers as opposed to using a contribuitors file (for consistency reason). ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + except GasHeaterException as exc: + raise exc Since the exception is just being passed through, I don't think there's a need to |try| the above block. There's another instance of this below as well. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > @@ -116,6 +152,43 @@ ) +# garberw added begin =========================== There's a few comments like these. — Reply to this email directly, view it on GitHub <#61 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKX5GECXQBGG673ZB6DXBH3XJZC6FANCNFSM6AAAAAAV4Z3FHQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

garberw · 2023-06-05T23:17:29Z

new to git how to update if you would like me to change it. pull request is closed ?

…

On 6/5/23 2:06 PM, Alec Delaney wrote: ***@***.**** requested changes on this pull request. Took a quick look, but I also don't have hardware I think. I think I'd like to take a second look at this once some of these points have been resolved just to see if there's anything I missed. I appreciate everyone's work! ------------------------------------------------------------------------ In LICENSES/BSD-3-Clause.txt <#61 (comment)>: > ***@***.*** bme68x_defs.h ***@***.*** 2021-05-24 ***@***.*** v4.4.6 I think these should be removed. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > @@ -14,6 +16,10 @@ * Author(s): Limor Fried + +original Adafruit_BME680 with addition of set_gas_heater(). Other new members are private. This can be removed, I think we can just use the GitHub release to announce things similar to a changelog. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + * @brief Enable and configure gas reading + heater + * @param heater_temp + * Desired temperature in degrees Centigrade + * @param heater_time + * Time to keep heater on in milliseconds + * @return True on success, False on failure Looks like the Arduino-style docstrings, so instances of these will need to be converted to the typical style for sphinx. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > +INT32 = int +INT16 = int +INT8 = int +UINT32 = int +UINT16 = int +UINT8 = int I think using |int| in all applicable places is fine since there isn't really a distinct "typewise" within Python. If needed, docstrings should explain limits (e.g., 0 - 255). That will save us using type aliases where not strictly needed. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > @@ -116,6 +152,43 @@ ) +# garberw added begin =========================== This should be removed since it's a contextual comment. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + Macro to set bits + data2 = data << bitname_pos + set masked bits from data2 in reg_data + """ + return (reg_data & ~bitname_msk) | ((data << bitname_pos) & bitname_msk) + + +def bme_set_bits_pos_0(reg_data, bitname_msk, data): + """ + Macro to set bits starting from position 0 + set masked bits from data in reg_data + """ + return (reg_data & ~bitname_msk) | (data & bitname_msk) + + +class GasHeaterException(Exception): This looks like a hyperspecific exception that could just be replaced with something like |OSError|, most likely. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + if (heater_temp == 0) or (heater_time == 0): + return False This could be: ⬇️ Suggested change - if (heater_temp == 0) or (heater_time == 0): - return False + if not heater_temp or not heater_time: + return False ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + op_mode: UINT8 = _BME68X_FORCED_MODE + # restrict to enable = True + enable: bool = True + nb_conv: UINT8 = 0 + hctrl: UINT8 = _BME68X_ENABLE_HEATER + run_gas: UINT8 = 0 + ctrl_gas_data_0: UINT8 = 0 + ctrl_gas_data_1: UINT8 = 0 + ctrl_gas_addr_0: UINT8 = _BME68X_REG_CTRL_GAS_0 + ctrl_gas_addr_1: UINT8 = _BME68X_REG_CTRL_GAS_1 I think this is using a lot of local variables where using what they equate to might be more efficient. This comment holds for the other methods as well. ------------------------------------------------------------------------ In contributors.md <#61 (comment)>: > +# SPDX-FileCopyrightText: 2017 ladyada for Adafruit Industries +# +# SPDX-License-Identifier: MIT + +Contributors to Adafruit_CircuitPython_BME680_modified +============================================================= + +Limor (Ladyada) Fried + +William Garber +many others It might be worth just adding contributors' names to the SPDX headers as opposed to using a contribuitors file (for consistency reason). ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > + except GasHeaterException as exc: + raise exc Since the exception is just being passed through, I don't think there's a need to |try| the above block. There's another instance of this below as well. ------------------------------------------------------------------------ In adafruit_bme680.py <#61 (comment)>: > @@ -116,6 +152,43 @@ ) +# garberw added begin =========================== There's a few comments like these. — Reply to this email directly, view it on GitHub <#61 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKX5GECXQBGG673ZB6DXBH3XJZC6FANCNFSM6AAAAAAV4Z3FHQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

tekktrik · 2023-06-06T00:15:39Z

Ah, I apologize, I did not realize at the time that this wasn't a request for review, but the merging. No worriies, if you'd like to address it, I would open a new PR. Otherwise, it can be a TODO for later :)

garberw · 2023-06-06T03:08:39Z

I am new to github so I do not know how this works.

…

On 6/5/23 5:15 PM, Alec Delaney wrote: Ah, I apologize, I did not realize at the time that this wasn't a request for review, but the merging. No worriies, if you'd like to address it, I would open a new PR. Otherwise, it can be a TODO for later :) — Reply to this email directly, view it on GitHub <#61 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKX5GEFOC4SNSSBJUKXVRADXJZZDRANCNFSM6AAAAAAV4Z3FHQ>. You are receiving this because you authored the thread.Message ID: ***@***.***>

Updating https://github.com/adafruit/Adafruit_CircuitPython_BME680 to 3.5.0 from 3.4.13: > Merge pull request adafruit/Adafruit_CircuitPython_BME680#61 from garberw/main Updating https://github.com/adafruit/Adafruit_CircuitPython_Crickit to 2.3.15 from 2.3.14: > Merge pull request adafruit/Adafruit_CircuitPython_Crickit#37 from OptionalLion411/main Updating https://github.com/adafruit/Adafruit_CircuitPython_MatrixKeypad to 1.2.14 from 1.2.13: > Merge pull request adafruit/Adafruit_CircuitPython_MatrixKeypad#16 from gtbcoding/isssue/13-type-annotation Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries

FoamyGuy · 2023-06-12T14:07:59Z

Sorry for the confusion on this one, I can make a follow up PR with those suggested changes

garberw added 4 commits March 15, 2023 21:28

Added set_gas_heater() which was previously available for arduino but…

0cda977

… not CircuitPython.

added name to contributors.

3ca336e

fixed mistakes found by lint on pull request.

a2b2414

trying to get checker to accept pull request

c31b1f6

jposada202020 requested a review from a team March 16, 2023 11:37

FoamyGuy reviewed Mar 20, 2023

View reviewed changes

seperate bsd-3-clause to it's own file

72dfc38

FoamyGuy approved these changes Jun 5, 2023

View reviewed changes

FoamyGuy merged commit d7909fe into adafruit:main Jun 5, 2023

tekktrik requested changes Jun 5, 2023

View reviewed changes

FoamyGuy mentioned this pull request Jun 12, 2023

feedback from pr61 #62

Merged

FoamyGuy mentioned this pull request Apr 15, 2024

v3.4.12 always returns wrong/nonsenical, static number for temperature, humidity, and pressure on first reading. Gas also seems wrong #69

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add set_gas_heater(); Was available on arduino but not CircuitPython #61

add set_gas_heater(); Was available on arduino but not CircuitPython #61

garberw commented Mar 16, 2023

garberw commented Mar 16, 2023

FoamyGuy Mar 20, 2023

garberw Mar 20, 2023

FoamyGuy commented Mar 20, 2023

garberw commented Mar 20, 2023

tekktrik commented Mar 20, 2023 •

edited

Loading

garberw commented Mar 20, 2023

FoamyGuy commented Apr 24, 2023

FoamyGuy left a comment

tekktrik left a comment

tekktrik Jun 5, 2023

garberw Jun 12, 2023

tekktrik Jun 5, 2023

garberw Jun 12, 2023

tekktrik Jun 5, 2023

garberw Jun 12, 2023

tekktrik Jun 5, 2023

garberw Jun 12, 2023

tekktrik Jun 5, 2023

tekktrik Jun 5, 2023

garberw Jun 12, 2023

tekktrik Jun 5, 2023

tekktrik Jun 5, 2023

tekktrik Jun 5, 2023

tekktrik Jun 5, 2023

tekktrik Jun 5, 2023

garberw commented Jun 5, 2023 via email

garberw commented Jun 5, 2023 via email

tekktrik commented Jun 6, 2023

garberw commented Jun 6, 2023 via email

FoamyGuy commented Jun 12, 2023

		@@ -14,6 +16,10 @@

		* Author(s): Limor Fried


		original Adafruit_BME680 with addition of set_gas_heater(). Other new members are private.

		@@ -116,6 +152,43 @@
		)


		# garberw added begin ===========================

		return (reg_data & ~bitname_msk) \| (data & bitname_msk)


		class GasHeaterException(Exception):

add set_gas_heater(); Was available on arduino but not CircuitPython #61

add set_gas_heater(); Was available on arduino but not CircuitPython #61

Conversation

garberw commented Mar 16, 2023

garberw commented Mar 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FoamyGuy commented Mar 20, 2023

garberw commented Mar 20, 2023

tekktrik commented Mar 20, 2023 • edited Loading

garberw commented Mar 20, 2023

FoamyGuy commented Apr 24, 2023

FoamyGuy left a comment

Choose a reason for hiding this comment

tekktrik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garberw commented Jun 5, 2023 via email

garberw commented Jun 5, 2023 via email

tekktrik commented Jun 6, 2023

garberw commented Jun 6, 2023 via email

FoamyGuy commented Jun 12, 2023

tekktrik commented Mar 20, 2023 •

edited

Loading