Possible issue with temperature offset logic (and maybe with altitude)

[melkati]

I am very confused with the temperature offsets. I think we have a problem with the logic, in some sensors...

I'm not sure what's going on yet, but something's not right... probably it's a mix of issues...

In my preliminary research I found (unconfirmed, more research is needed) some things to check or fix:

• [x] 1. On "Sensor Init" (as CO2scdXXInit()) we are always calling the function to set an offset. We must probably first get the offset stored on the sensors EEPROM and if it's the same don't set the offset again (maybe this is the reason at each start sensors are taking a while to stabilize the temperature).
• [x] 2. On sensor init we are calling the function to set or read the offset (and probably the Altitude, but this is another history) before registering the sensor on the Sensorslib so it just errors: CO2scdXXInit() first get the offsets at the sensors EEPROM then compare with toffset (internal variable used by Sensorlib to keep the actual offset) and if necessary, set the new offset but as the sensors aren't registered on Sensorslib these calls just fail.
• [x] 3. Looks like SCD30 stores the offsets automatically after calling their set_offset function and keeps it between power cicles, but SCD4x doesn't keep the data and a call to the "Persist Settings" function is needed so settings on sensor are not lost after power cycling.
• [x] 4. On the SCD4x sensors the EEPOM is guaranteed to last for 2000 write cycles. So, we must be very cautious to decide if the "Persist Settings" function should be called from the library when it's needed (more control provided by the library) or from the program using the library (prone to implementation errors but moving the responsibility to the main program's developer).
• [x] 5. We have not implemented Sensors::getTempOffset() so that you don't have to call sensors.scd30.getTemperatureOffset() or equivalent directly.
• [x] 6. The toffset variable is like a "free electron", it causes confusion and there are temptations to access it directly. It would be necessary to decide and, if necessary, make it private and implement methods to read and modify it in a controlled way.
• [ ] 7. There is an inconsistency for sensors other than SCD30, SCD4x and Sen5X as Sensorlib just ignores offsets for other sensors. Maybe Sensorlib must do a simple subtraction or addition for other sensors to be coherent and give broad compatibility and abstraction to all of them).

Finally, I would like to point out that I want to implement support for low power sensors, starting with SCD41 (which is already "half supported" and is getting a lot of traction) and will have to be taken into account to make it easier later on.

I would like to invite @hpsaturn, @roberbike and @Coscolin to the discussion.

[melkati]

I moved the sensor registration up in Sensors::CO2scd30Init() and Sensors::CO2scd4xInit() to fix (2).

I don't see any side effects. Tested with both sensors in CO2 Gadget and looks like it's working fine.

https://github.com/melkati/canairio_sensorlib/commit/42329340867bc088a6653f97509f18c757cef30d https://github.com/melkati/canairio_sensorlib/commit/6da72010a3cd7a37673c4d7907ed23f48213b38c

In Sensors::CO2scd30Init() I also changed In Sensors::CO2scd30Init() to multiply toffset * 100 to convert into hundredths of a degree C to comply with Sensirion's API:

https://github.com/melkati/canairio_sensorlib/commit/6da72010a3cd7a37673c4d7907ed23f48213b38c#r138802382

Maybe we should do the same for Sensors::sen5xInit() but I don't have one to test.

@hpsaturn, I sent the PR #204 directed to master (as there isn't another branch to work on) but I think you can change the branch easily before merging. This way I can keep on working without having at the end only one PR with changes to different things.

[melkati]

I just sent a PR to fix last commit and (5). https://github.com/kike-canaries/canairio_sensorlib/pull/204

I didn't add sen5x to Sensors::getTempOffset() and I don't know how it works and don't have one to test.

[hpsaturn]

Maybe @roberbike knows about this sensor.

[BRKMK]

Yes, I have one sen5x from an ikea device. I will test it in a few days.

[melkati]

I'm centered today on SCD30 time to stabilize the temperature.

For (1), no matter what I do, Sensors are still taking a while to stabilize the temperature.

I reviewed the code and looks like the checks are already in place and are working fine (there was a bug comparing the offset saved at the SCD30 with the one in Sensorlib but I already fixed it in https://github.com/kike-canaries/canairio_sensorlib/pull/204/commits/6da72010a3cd7a37673c4d7907ed23f48213b38c).

I would appreciate a second, or even third, pair of eyes reviewing it.

I carefully removed all references to scd30.setTemperatureOffset() in CO2 Gadget, Sensorlib and even Adafruit_SCD30 (commented out code in all three) and sensor is still taking a while to stabilize the temperature. Maybe my SCD30 is malfunctioning? Maybe that is the way this sensor works? I would thank you if you could test it with your SCD30 sensor (if you have one) @hpsaturn @BRKMK @Coscolin...

You can use my fork to test: https://github.com/melkati/canairio_sensorlib/tree/fixOffset

[hpsaturn]

Thanks @melkati On my side, right now I don't have SCD30 here to perform tests. Only I have a new SCD40 builtin sensor, included in the AirQ unit of M5. I'm going to check if is possible replicate the issue there. On the other hand, I could help in the review code of any advance. Thanks again.

[melkati]

I did the same tests with an SCD40 sensor and the same thing happened.

One tip: the programmer using the library must set the float variable sensors.toffset, prior to sensors.init(), to the same temperature offset the program using the library has stored as "actual offset temperature" for everything to be "on sync".

Maybe we should create a method to set this variable before calling sensors.init() so programmers do not have to access the variable directly.

I can't find any problem with manually setting the variable, before calling sensors.init(). @hpsaturn, what do you think?

[hpsaturn]

Yes, I agree. It is normal in some libraries. If we need that, please do it.

[melkati]

Yes, I agree. It is normal in some libraries. If we need that, please do it.

Are initTOffset(float) and float = getTOffset() good names?

It will not check if offset is positive or negative, as some other libraries may need a negative value, although I think we must keep to positives offset to standardize, following the Sensirion path.

[hpsaturn]

Yes, these names are good. I guess that we should admit the two possibilities, positives and negatives.

[melkati]

Just sent PR with these two methods. I didn't make tOffset variable private to maintain backward compatibility. Maybe we should do it.

[melkati]

On my side, this PR is ready for merging.

CO2 Gadget is using this branch (in my fork) and nobody reported issues with sensors, so I think it's working fine.

Point 7. is still pending to improve in the future. I tried to open it on their own issue, so it's not forgotten, but I receive an error:

[hpsaturn]

Thanks @melkati for your contributions and time. Then I'm going to close this issue. Thanks!