feature: read temperature from last humidity measurement

[jnsbyr]

No description provided.

[RobTillaart]

Thanks, too be reviewed asap.

[RobTillaart]

Restarted CI build as something went wrong.

[RobTillaart]

@jnsbyr

• please merge latest master (0.4.1)
• please add a small example showing the usage of the new function.

[jnsbyr]

Merged from upstream and rebased. Please check if the name of the new method sounds good to you. Functionality is obvious from datasheet, but an example is better. Will update the merge request when done.

[RobTillaart]

Functionality is obvious from datasheet, but an example is better.

I cannot find the command 0xE0 (0b11100000) in the datasheets I have.
Do you have a link?

[RobTillaart]

Functionality is obvious from datasheet, but an example is better.

I cannot find the command 0xE0 (0b11100000) in the datasheets I have. Do you have a link?

Found the info in the Si7021 datasheet ==> it might not be compatible with SHT20 and SHT21 et al.
There should be a remark added to the readme.md.

[jnsbyr]

I cannot find the command 0xE0 (0b11100000) in the datasheets I have.

May bad, I assumed that all models have this command, not only the Si7021. It's makes for a significant performance boost, reducing the acquisition time by ~50%.

There should be a remark added to the readme.md.

Sure, but additionally there are separate constructors for the supported sensor models. Each constructor could set its model type from an enum "Model" to a member variable "model". Then calling readTemperatureForHumidity() could just return false, maybe set a specific error code you should define. Continue?

[RobTillaart]

... reducing the acquisition time by ~50%.

That is substantial and really an added value.

Sure, but additionally there are separate constructors for the supported sensor models. Each constructor could set its model type from an enum "Model" to a member variable "model". Then calling readTemperatureForHumidity() could just return false, maybe set a specific error code you should define. Continue?

The object oriented way to do it is to only implement the function in the derived 7021 class.

[jnsbyr]

The object oriented way to do it is to only implement the function in the derived 7021 class.

Good idea. Had a look through the datasheets of the supported devices and added the function where supported, effectively all chips from Silicon Labs. The protected method in the base class avoids duplicate code. Also renamed example.

[RobTillaart]

Code looks OK -
Can you add a few lines to the readme.md e.g. like (async interface section)

• float readTemperatureForHumidity() optimized call for temperature that uses the sensors last humidity request.
  Note that the sensor returns a cached temperature value, so keep time between the two calls as small as possible.
  Otherwise the actual temperature may differ a lot.

Questions wrt usage

According to the datasheet the function does Read temperature value from previous RH measurement.

What happens if you call the function e.g. multiple times e.g. with 10 seconds interval.
Does it return the same value over and over?

What happens if I requestTemperature() first and then call readTemperatureForHumidity()?
Does it return the same value or is that an error condition?

Depending on the answers maybe readCachedTemperature is more descriptive ?

[jnsbyr]

Questions ...

What the function basically does is clear from the documentation. But someone who does not read docs might use it differently.

So far I did not make "abuse" tests. I suspect that you get a random or fixed reply if you just call it without doing a humidity acquisition first. After a humidity acquisition you will get the same value if you call it more than once.

I will make some tests, but as I "only" have a Si7021 one cannot be sure that all Si70xx behave in the same way as they have different firmware.

For me it currently boils down to: always call requestHumidity before calling readHumidity or readTemperatureForHumidity. Maybe it is necessary to even call readHumidity before readTemperatureForHumidity returns a valid value.

One could try to force a workflow by tracking the calling state and returning an error value if the wrong calling sequence is used. But I think this will make the code unnecessarily complex. An inline comment and a description in the README should be enough.

Will post the test results over the weekend. Currently I have the Si7021 at almost 100 °C for a few hours per day by enabling the heater to "burn" off excessive humidity/contamination, as my sensor showed an offset of ~10 %. The first burn was only for 1 hour and the offset did not change after cool-down. But with the longer burns the offset seems to decrease. Not sure jet how this will play out ...

[RobTillaart]

I will make some tests, but as I "only" have a Si7021 one cannot be sure that all Si70xx behave in the same way as they have different firmware.

Appreciated and very true (not even for different batches of Si7021 )

Maybe it is necessary to even call readHumidity before readTemperatureForHumidity returns a valid value.

That is what I would expect.
Maybe we must include the humidity call into one bool readTemperatureAndHumidityFast() call?
Then the function can check all, and there is also minimal time between the calls.
Opinion?

[jnsbyr]

Test results with a Si7021 with firmware 2.0:

reset { readTemperatureForHumidity getTemperature }
-> const -46.8 °C

reset { requestTemperature readCachedTemperature getTemperature }
-> readTemperatureForHumidity ERR_READBYTES

reset { requestTemperature { reqTempReady } readCachedTemperature getTemperature }
-> act. temp

reset { requestTemperature { reqTempReady } readTemperature getTemperature readCachedTemperature getTemperature }
-> act. temp / act.temp

reset { requestHumidity readCachedTemperature getTemperature }
-> readTemperatureForHumidity ERR_READBYTES

reset { requestHumidity { reqHumReady } readCachedTemperature getTemperature }
-> act. temp

reset { requestHumidity { reqHumReady } readHumidity getHumidity readCachedTemperature getTemperature }
-> act. humi / act. temp (this is the typical use case)

reset { requestHumidity { reqHumReady } readTemperature getTemperature }
-> var. value 40 .. 41 (neither act. humidity or temperature)

[jnsbyr]

Test interpretation:

• readTemperatureForHumidity can be called multiple times in sequence and returns the last acquired value
• readTemperatureForHumidity cannot be called while a acquisition is in progress
• readTemperatureForHumidity returns the same value as readTemperature after a temperature acquisition
• readTemperature cannot be used instead of readTemperatureForHumidity after a humidity acquisition

Recommendations (assuming the other Si70xx behave in the same way):

• readTemperatureForHumidity() should better be called readLastTemperature() and should get the usage instruction "can be called after requesting temperature or humidity, when used in combination with a humidity request saves the overhead of a separate temperature request".
• there is no need for extra state handling as readTemperatureForHumidity() already returns error code if used out of sequence except immediately after reset

[RobTillaart]

Thanks for the extended tests, in the readme you should link to this PR so the tests can be found easily.

We may assume all SI70xx behave the same (until someone proves otherwise).

---

Name

What about

• bool readCachedTemperature() reads the cached temperature from the sensor. Values for temperature are cached for both temperature and humidity calls. A humidity call always measures the temperature to be able to calculate the relative humidity..

Furthermore the word last in lastTemperature() is already "taken" by

• float getTemperature() computes the temperature in °C based off the latest raw reading, and returns it.

[jnsbyr]

The name "readCachedTemperature" does not sit well with me, as cached typically implies something readily available in the local system, often in RAM. But here it is a register value in the sensor, requiring an I2C bus round trip and this is not really a cache to me. On the other hand the I2C round trip is so much faster than a separate acquisition.

In the end these are only semantics and I do not have a better alternative, so "readCachedTemperature" it will be.

[RobTillaart]

I do not agree it is only semantics, a good function name is self descriptive. But I cannot think of one.
So I agree with you to go forward and merge the PR asap (tomorrow for me)

[jnsbyr]

... a good function name is self descriptive.

definitely

I ... merge the PR asap ...

please wait for the next commit as I still needed to add the necessary modifications (rename, comments)

[jnsbyr]

Making the 2 new links work took me 3 tries as they cannot be tested without committing if you do not use the Github online editor. Github uses different rules for the project internal references in README and issue comments. The shortcuts only work in issue comments.

[RobTillaart]

I will review it and merge it if it looks good.

[jnsbyr]

@RobTillaart: Thanks for accepting this feature! 👍

Just updated the comment above with the test description to show the same method name as the merged source "readCachedTemperature" to avoid confusion.

[RobTillaart]

Good additions are always welcome!
So thank you!

[jnsbyr]

Missed this: readTemperatureForHumidity() needs to be renamed to readCachedTemperature() in keywords.txt.

[RobTillaart]

And in the changelog too.
(I seldom have a perfect PR :)

[jnsbyr]

So many dependencies that the compiler does not check ... I just pushed the 2 changes to my feature branch. Should I open a new merge request?

[RobTillaart]

Should I open a new merge request?

Ok, no need to update version

[jnsbyr]

pull request is ready, also fixed another typo in keywords.txt, see #29