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.

Sign up for GitHub

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

Jump to bottom

Scd4x 3x issue #658

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Scd4x 3x issue #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
736d1ba
Fix SCD4x and SCD30 (alreadyRecentlyRead) + MetroS3 debug target
tyeth Nov 7, 2024
cdb4d02
Swap Previous Period helper functions to ulong
tyeth Nov 7, 2024
b4c83b8
Set SCD4x to not read until T+5s
tyeth Nov 7, 2024
2052621
Switch to using sensor period for T-5sec delay (pollPeriod begin arg)
tyeth Nov 7, 2024
7104bf6
Switch SCD30 to use read() instead of getEvent()
tyeth Nov 8, 2024
8eab892
clang format
tyeth Nov 8, 2024
7ee24c3
clang again
tyeth Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions platformio.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,16 @@ 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_type = debug
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
Expand Down
10 changes: 6 additions & 4 deletions src/components/i2c/WipperSnapper_I2C.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,7 +565,8 @@ bool WipperSnapper_Component_I2C::initI2CDevice(
WS_DEBUG_PRINTLN("VEML7700 Initialized Successfully!");
} else if (strcmp("scd40", msgDeviceInitReq->i2c_device_name) == 0) {
_scd40 = new WipperSnapper_I2C_Driver_SCD4X(this->_i2c, i2cAddress);
if (!_scd40->begin()) {
if (!_scd40->begin(
msgDeviceInitReq->i2c_device_properties[0].sensor_period)) {
WS_DEBUG_PRINTLN("ERROR: Failed to initialize SCD4x!");
_busStatusResponse =
wippersnapper_i2c_v1_BusResponse_BUS_RESPONSE_DEVICE_INIT_FAIL;
Expand Down Expand Up @@ -1328,16 +1329,17 @@ void WipperSnapper_Component_I2C::sensorEventRead(
unsigned long curTime, wippersnapper_signal_v1_I2CResponse *msgi2cResponse,
bool (WipperSnapper_I2C_Driver::*getEventFunc)(sensors_event_t *),
long (WipperSnapper_I2C_Driver::*getPeriodFunc)(),
long (WipperSnapper_I2C_Driver::*getPeriodPrvFunc)(),
void (WipperSnapper_I2C_Driver::*setPeriodPrvFunc)(long),
ulong (WipperSnapper_I2C_Driver::*getPeriodPrvFunc)(),
void (WipperSnapper_I2C_Driver::*setPeriodPrvFunc)(ulong),
Comment on lines +1332 to +1333
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning for the function header to change from long to ulong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@brentru brentru Nov 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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?

wippersnapper_i2c_v1_SensorType sensorType, const char *sensorName,
const char *unit, sensors_event_t event,
float sensors_event_t::*valueMember, bool &sensorsReturningFalse,
int &retries) {
// sensorName used for prefix + error message, units is value suffix
curTime = millis();
if (((*iter)->*getPeriodFunc)() != 0L &&
curTime - ((*iter)->*getPeriodPrvFunc)() > ((*iter)->*getPeriodFunc)()) {
curTime - ((*iter)->*getPeriodPrvFunc)() >
(ulong)((*iter)->*getPeriodFunc)()) {
brentru marked this conversation as resolved.
Show resolved Hide resolved
// within the period, read the sensor
if (((*iter)->*getEventFunc)(&event)) {
float value;
Expand Down
4 changes: 2 additions & 2 deletions src/components/i2c/WipperSnapper_I2C.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,8 @@ class WipperSnapper_Component_I2C {
wippersnapper_signal_v1_I2CResponse *msgi2cResponse,
bool (WipperSnapper_I2C_Driver::*getEventFunc)(sensors_event_t *),
long (WipperSnapper_I2C_Driver::*getPeriodFunc)(),
long (WipperSnapper_I2C_Driver::*getPeriodPrvFunc)(),
void (WipperSnapper_I2C_Driver::*setPeriodPrvFunc)(long),
ulong (WipperSnapper_I2C_Driver::*getPeriodPrvFunc)(),
void (WipperSnapper_I2C_Driver::*setPeriodPrvFunc)(ulong),
wippersnapper_i2c_v1_SensorType sensorType, const char *sensorName,
const char *unit, sensors_event_t event,
float sensors_event_t::*valueMember, bool &sensorsReturningFalse,
Expand Down
Loading