Adding Humidity and Temperature compensation to SGP40 in preperation for VOC Algorithm

[KeithTheEE]

In short this lets a user provide temperature (in C) and humidity (in percentage) to the SGP40 for a humidity compensated raw value reading.

Humidity compensation is necessary for the VOC algorithm mentioned in #2 and this should help implement the VOC algorithm over the next week or two.

There is a spelling correction and I adjusted the _READ_CMD byte array to match the provided byte array in the SGP40 data sheet (table 9). This shouldn't have a large impact on anyone's code since the byte array was generated used input humidity of 49.999%, and the datasheet example used an input of 50%.

The checksum _check_crc8 was adjusted to call a new function--generate_crc which is basically the original _check_crc8 with the adjustment that it returns the crc byte.

Temperature and Humidity are converted to their 'tick' value according to the equation in the datasheet.

And measure_raw takes in the temperature and humidity, converts them to ticks, then generates a measure command hex array. Once the measure_command is generated it calls raw to handle the communication.

[jposada202020]

Thanks for your contribution I saw that you changed the command lists already. Thanks.
Just some quick comments, and a question

• some debug print are still in the code,
• Also it would be good to document the measure_raw function. just transfer you PR comments

And measure_raw takes in the temperature and humidity, converts them to ticks, then generates a measure command hex array. Once the measure_command is generated it calls raw to handle the communication.

• A good idea will be also to explain in the function documentation why we need to convert temp and humidity to ticks, that would hep maintainers in the future that does not know about the sensor understand what the function does. just transfer you PR comments here also

Temperature and Humidity are converted to their 'tick' value according to the equation in the datasheet.

Did you build the documentation, have not try that kind of table before so not sure how sphinx will treat it. (just curious)

Thanks again :)

[KeithTheEE]

• I missed the print commands, thank you for catching it!

I saw that you changed the command lists already

• When you say changed the command list, are you referring to _READ_CMD? If so that was all Black's doing--it's a nice tool I'll have to start using more often

Did you build the documentation

• I have not tried to do that--is this the guide for building the documentation?

Also it would be good to document the measure_raw function

And that point brings us here (which I think is the heart of the issues of this pull request):

measure_raw is actually an interesting point. Should it be exposed to an end user? I would personally like access to it, but it is sort of an obscure value much like raw is.

Looking just a little further at its structure raw and measure_raw are almost the same thing. But measure_raw takes inputs and raw has a property flag, so to keep code from breaking I broke it into two functions. Ideally it would be one function mimicking the c implementation but I think avoiding breaking changes matters more than adhering to the c function design.

I could get around this by adding a set_current_temp and set_current_humidity functions and removing measure_raw entirely, but I felt that would be both more obscure, and require more code from the end user than a simple functioning taking two input and automatically issuing the i2c commands as is the case with measure_raw.

Then relatedly measure_raw and raw are really similar function names. I could change measure_raw to something more explicit like, measure_raw_with_compensation but that deviates from the datasheet and the c code so I just put something down so more eyes could give feedback.

Looking forward, I could avoid the issue and completely hide measure_raw with a leading _ and then only call it within the measure_voc_index() function I'm planning on working on next.

I felt this was a good point to make a pull request because I think (and could be wrong) that the VOC Algorithm from this point on does not interact with the sgp40. So if a microcontroller is tight on space and can send the raw humidity-compensated value from the sgp40's readings to another system, you could just run the VOC Algorithm on that secondary system.

Anyway, thanks for the feedback! I'll clean up the prints and add some more documentation to the comments!

[jposada202020]

No problem Thank you I have other comments, more related with the code than the purpose of the change. so I figured that I would ask those questions first.

When you say changed the command list, are you referring to _READ_CMD? If so that was all Black's doing--it's a nice tool I'll have to start using more often

yes normally all the Adafruit libraries are setup to crate virtual environments and work with pre-commit, So I strongly recommend using this tool as it will do pylint and black at the same time for you. so the command will be pre-commit run --all-files

I have not tried to do that--is this the guide for building the documentation?

Yes it is. Is the easiest way that would work 95% of the time. Another way is to create a free account in readthedocs and import you repos and build the documentation there. we would need in the other 5% as sometimes documents render locally are no the same as with the CI (but we would get there when we get there)

Non Breaking Changes

Yes, we try to avoid this as much as possible, or to do them in a way of changes will not affect old code, so you would probably see in helpers library a lot of kwargs, as more and more functionalities are added. But if we need to break old code, there has to be a a good reason: major improvement, compatibility, Bug fix.

raw_function

But regarding your question a good way on what to do with the function is here
https://github.com/adafruit/circuitpython/blob/main/docs/design_guide.rst#getterssetters
Me approach will be, is the normal user have a use for this function if the answer is yes, I will expose it, more advanced users will read the code and use private functions if they need them.
Try to use properties, as we do no promote the use of set and get in name functions

Documenting Functions

CircuitPython has a mixed way of documenting functions and parameters, but how to do it is explained in the design guide, or you could use other libraries as an example

Other Unspoken Guidelines

CircuitPython use a lot of tuples instead of lists, and bytearrays, so when you return values, return them as tuples. CircuitPython likes to work with Properties as for the end user is easier to understand.

[KeithTheEE]

I'd like to add an example for the measure_raw function, but because it requires a temperature and humidity value, should I include the sensor as well? I'm currently using a BME280 but the specific temperature and humidity sensor itself shouldn't matter (as long as it can update at 1Hz)

And to that point, how do I adjust the requirements to state that for humidity compensated raw values, (and for the VOC Algorithm) it will need a second sensor?

[jposada202020]

Interesting... we are uncharted waters here then, however I thought the sensor included a T/H already
For the requirements part, in the documentation we need to specify that in order to use that functionality the function needs Temperature and Humidity available from a different sensor.
For the example yes, we could do something like
in this example we are using the BME280.... however you could use other sensor that refreshes at 1 Hz something similar to this:
https://github.com/adafruit/Adafruit_CircuitPython_Display_Button/blob/299c8ccf2b99877a2af9fce95c1a2952bb8f0db0/examples/display_button_simpletest.py#L13
Maybe recommend some sensors that have those capability from this list?
https://github.com/adafruit/Adafruit_CircuitPython_FakeRequests/blob/main/examples/fakerequests_i2c_database.txt

[KeithTheEE]

I messed something up with the documentation but I'm new to sphinx so that's far from surprising.

Additionally, I'm going to remove reference to the adafruit-circuitpython-register library as I didn't end up using it, and to the best of my knowledge, it's never used. So there will be another commit coming soon, plus one commit after that when I figure out why the docs aren't building.

[jposada202020]

If you need help with the documentation I could help with that (I have done my share part of documentation) So I could commit to your branch. It is up to you :)

[KeithTheEE]

Certainly!

I don't actually know where to begin looking for this warning treated as error issue

 Warning, treated as error:
/home/runner/work/Adafruit_CircuitPython_SGP40/Adafruit_CircuitPython_SGP40/adafruit_sgp40.py:docstring of adafruit_sgp40.SGP40:29:'any' reference target not found: sgp.measure_raw
Error: Process completed with exit code 2.

I'm sure it's a simple problem, and I'll pull your commit if you like--but because I want to get better at it I'm going to read through the commit to see the difference between mine and yours and will probably ask you questions if you don't mind.

[jposada202020]

Lets discuss in the dev channel it would help other folks :)

[jposada202020]

Thanks :)

[jposada202020]

Nothing major

Things related to this particular PR
Name of the function
parameter documentation
and quickstart correction

[KeithTheEE]

There's a few more mentions of busio to clean up, but I'm not 100% clear as to why it's being removed? Is it a library/set of functions that are being removed in favor of a simpler 'board' import?

And is bus device different from busio?

[jposada202020]

Thanks. use the busio is deprecated. sometimes it could cause conflicts in the bus. So we underwent a big update in all the libraries and learning guides, to update this. there will be some that have that busio reference, but we are moving away from this.

[jposada202020]

we wait a little to see if other folks want to chime in, But I think we are pretty much good to go.

[KeithTheEE]

Ok those last four changes should remove the comments and usage of busio from everything but the conf.py file which references it in the template generated comment. I think that should cover the full pull request--busio and register have been removed from reference, and the sgp40 now has a measure_raw method which takes in temperature and relative humidity to return a humidity compensated reading.