Scd4x 3x issue #658
Scd4x 3x issue #658
Conversation
|
@brentru this is ready for review |
| ulong (WipperSnapper_I2C_Driver::*getPeriodPrvFunc)(), | ||
| void (WipperSnapper_I2C_Driver::*setPeriodPrvFunc)(ulong), |
There was a problem hiding this comment.
What's the reasoning for the function header to change from long to ulong?
There was a problem hiding this comment.
The previous poll period (periodPrv) is stored in the backing field as a long, and initially is set to 24hours ago (negative), but the logic check was shortcutting due to the unsigned long getter and setter methods (achieving the initial read change that introduced it).
Now that I've got a driver that wants update to only be called 5seconds after driver init/begin, which uses the sensor poll period taken away from millis +5secs, we need a way of the previous poll period getter+setter taking negative numbers.
There was a problem hiding this comment.
This makes sense to me as millis() returns a ulong and you want to prevent it from underflowing.
Would we want to change everything within I2C.cpp/I2C_Driver to ulong while we're at it?
If we are going to change something, let's ensure we have matching function signatures everywhere.
There was a problem hiding this comment.
Be careful that you don't make a change that breaks backward compatibility. Could code that uses the current data type break if the type is re-defined?
| sensors_event_t _temperature; ///< Temperature | ||
| sensors_event_t _humidity; ///< Relative Humidity | ||
| sensors_event_t _CO2; ///< CO2 | ||
| float _temperature; ///< Temperature |
There was a problem hiding this comment.
Why are we moving away from sensors_event_t types here?
| bool readSensorMeasurements() { | ||
| uint16_t error; | ||
| /*******************************************************************************/ | ||
| bool sensorReady() { |
There was a problem hiding this comment.
Change function to something that reflects its returning a bool, like IsSensorReady()
There was a problem hiding this comment.
the rest of this function incl. the fall-thru makes logical sense, it's much better than it was before
| /*******************************************************************************/ | ||
| bool readSensorData() { | ||
| // dont read sensor more than once per second | ||
| if (alreadyRecentlyRead()) { |
There was a problem hiding this comment.
Okay so alreadyRecentlyRead() returning true assets that the sensor is read > 1x per second? Maybe change the function's name to reflect this?
| // Check if data is ready | ||
| error = _scd->getDataReadyFlag(isDataReady); | ||
| if (error || !isDataReady) | ||
| if (!sensorReady()) { |
There was a problem hiding this comment.
Change to something that reflects the boolean, IsSensorReady()
| uint16_t _co2 = 0; ///< SCD4x co2 reading | ||
| float _temperature = 20.0f; ///< SCD4x temperature reading | ||
| float _humidity = 50.0f; ///< SCD4x humidity reading | ||
| ulong _lastRead = 0; ///< Last time the sensor was read |
There was a problem hiding this comment.
What's the reason for the default values being defined here, why do they need to be 0, 20.0, 50.0, 0?
If they do need to be pre-defined before the function runs, let's do it in the constructor instead of here.
This adds a metro s3 debug target, and changes the SCD4x to use a single read for all datapoints at once like the SCD30.
Also noticed the alreadyRead condition needed a fix.
Lastly, to get a reliable first read on the SCD4x, I pass in the sensor period to the Begin setup call, allowing the driver to set it's first read to be five seconds time (after init).