Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scd4x 3x issue #658

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Next Next commit
Fix SCD4x and SCD30 (alreadyRecentlyRead) + MetroS3 debug target
tyeth committed Nov 7, 2024
commit 736d1ba927a8a5ecf00bead489dd0e1b430a9cbc
9 changes: 9 additions & 0 deletions platformio.ini
Original file line number Diff line number Diff line change
@@ -271,6 +271,15 @@ build_flags = -DARDUINO_METRO_ESP32S3 -DBOARD_HAS_PSRAM
board_build.partitions = tinyuf2-partitions-16MB.csv
extra_scripts = pre:rename_usb_config.py

; Adafruit Metro ESP32-S3
[env:adafruit_metro_esp32s3_debug]
extends = common:esp32
board = adafruit_metro_esp32s3
build_flags = -DARDUINO_METRO_ESP32S3 -DBOARD_HAS_PSRAM -DCFG_TUSB_DEBUG=1 -DDEBUG=1 -DESP_LOG_LEVEL=ESP_LOG_VERBOSE -DARDUINO_CORE_DEBUG_LEVEL=5 -DCORE_DEBUG_LEVEL=5 -DARDUHAL_LOG_LEVEL=5 -DARDUINO_USB_CDC_ON_BOOT=1
;set partition to tinyuf2-partitions-16MB.csv as of idf 5.1
board_build.partitions = tinyuf2-partitions-16MB.csv
extra_scripts = pre:rename_usb_config.py

; Adafruit Funhouse ESP32-S2
[env:adafruit_funhouse_esp32s2]
extends = common:esp32
Original file line number Diff line number Diff line change
@@ -59,7 +59,7 @@ class WipperSnapper_I2C_Driver_SCD30 : public WipperSnapper_I2C_Driver {
@returns True if the sensor was recently read, False otherwise.
*/
bool alreadyRecentlyRead() {
return (_lastRead != 0 && millis() - _lastRead) < 1000;
return _lastRead != 0 && millis() - _lastRead < 1000;
}

/*******************************************************************************/
92 changes: 68 additions & 24 deletions src/components/i2c/drivers/WipperSnapper_I2C_Driver_SCD4X.h
Original file line number Diff line number Diff line change
@@ -55,38 +55,78 @@ class WipperSnapper_I2C_Driver_SCD4X : public WipperSnapper_I2C_Driver {
_scd->begin(*_i2c);

// stop previously started measurement
if (_scd->stopPeriodicMeasurement())
if (_scd->stopPeriodicMeasurement() != 0) {
return false;
}

// start measurements
if (_scd->startPeriodicMeasurement())
if (_scd->startPeriodicMeasurement() != 0) {
return false;
}

return true;
}

/********************************************************************************/
/*******************************************************************************/
/*!
@brief Checks if sensor was read within last 1s, or is the first read.
@returns True if the sensor was recently read, False otherwise.
*/
bool alreadyRecentlyRead() {
return _lastRead != 0 && millis() - _lastRead < 1000;
}

/*******************************************************************************/
/*!
@brief Attempts to read the SCD4x's sensor measurements
@returns True if the measurements were read without errors, False
if read errors occured or if sensor did not have data ready.
@brief Checks if the sensor is ready to be read
@returns True if the sensor is ready, False otherwise.
*/
/********************************************************************************/
bool readSensorMeasurements() {
uint16_t error;
/*******************************************************************************/
bool sensorReady() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change function to something that reflects its returning a bool, like IsSensorReady()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rest of this function incl. the fall-thru makes logical sense, it's much better than it was before

bool isDataReady = false;
delay(100);
uint16_t error = _scd->getDataReadyFlag(isDataReady);
if (error != 0 || !isDataReady) {
// failed, one more quick attempt
delay(100);
error = _scd->getDataReadyFlag(isDataReady);
if (error != 0 || !isDataReady) {
return false;
}
}
return true;
}

/*******************************************************************************/
/*!
@brief Reads the sensor.
@returns True if the sensor was read successfully, False otherwise.
*/
/*******************************************************************************/
bool readSensorData() {
// dont read sensor more than once per second
if (alreadyRecentlyRead()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so alreadyRecentlyRead() returning true assets that the sensor is read > 1x per second? Maybe change the function's name to reflect this?

return true;
}

// Check if data is ready
error = _scd->getDataReadyFlag(isDataReady);
if (error || !isDataReady)
if (!sensorReady()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to something that reflects the boolean, IsSensorReady()

return false;
}


// Read SCD4x measurement
error = _scd->readMeasurement(_co2, _temperature, _humidity);
if (error || _co2 == 0)
uint16_t error = _scd->readMeasurement(_co2, _temperature, _humidity);
if (error != 0 || _co2 == 0) {
WS_DEBUG_PRINT("Error reading SCD4x measurements: #");
WS_DEBUG_PRINT(error);
WS_DEBUG_PRINT(" CO2: ");
WS_DEBUG_PRINTLN(_co2);
WS_DEBUG_PRINT("Temp: ");
WS_DEBUG_PRINT(_temperature);
WS_DEBUG_PRINT(" Humidity: ");
WS_DEBUG_PRINTLN(_humidity);
return false;

}
_lastRead = millis();
return true;
}

@@ -101,8 +141,9 @@ class WipperSnapper_I2C_Driver_SCD4X : public WipperSnapper_I2C_Driver {
/*******************************************************************************/
bool getEventAmbientTemp(sensors_event_t *tempEvent) {
// read all sensor measurements
if (!readSensorMeasurements())
if (!readSensorData()) {
return false;
}

tempEvent->temperature = _temperature;
return true;
@@ -119,8 +160,9 @@ class WipperSnapper_I2C_Driver_SCD4X : public WipperSnapper_I2C_Driver {
/*******************************************************************************/
bool getEventRelativeHumidity(sensors_event_t *humidEvent) {
// read all sensor measurements
if (!readSensorMeasurements())
if (!readSensorData()) {
return false;
}

humidEvent->relative_humidity = _humidity;
return true;
@@ -137,18 +179,20 @@ class WipperSnapper_I2C_Driver_SCD4X : public WipperSnapper_I2C_Driver {
/*******************************************************************************/
bool getEventCO2(sensors_event_t *co2Event) {
// read all sensor measurements
if (!readSensorMeasurements())
if (!readSensorData()) {
return false;
}

co2Event->CO2 = (float)_co2;
return true;
}

protected:
SensirionI2CScd4x *_scd; ///< SCD4x driver object
uint16_t _co2; ///< SCD4x co2 reading
float _temperature; ///< SCD4x temperature reading
float _humidity; ///< SCD4x humidity reading
private:
SensirionI2CScd4x *_scd = nullptr; ///< SCD4x driver object
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
};

#endif // WipperSnapper_I2C_Driver_SCD4X