From d5d5bad97cdca77da4ad77fd5fde9f84bba34d14 Mon Sep 17 00:00:00 2001 From: Michael Mohr Date: Tue, 11 Aug 2026 23:56:46 -0700 Subject: [PATCH] SEN5X: fix version parsing, VOC index reporting, and read-buffer handling (#11114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * SEN5X: validate read lengths and initialize read buffers readBuffer() returns the number of data bytes written (0 on error). Check the return value against the number of bytes each caller consumes before parsing, and zero-initialize the destination buffers: findModel (5), getMeasurements (2), readValues (16), readPNValues (20), and vocStateFromSensor (SEN5X_VOC_STATE_BUFFER_SIZE). This also resolves maybe-uninitialized compiler warnings. Small simplifications in the same area: - Assign the converted measurement values directly; the isnan() checks on integer intermediates always took the conversion branch, so this preserves behavior. - Fold a redundant state comparison in wakeUp() that immediately followed the assignment of the same value. - Add an explicit 'return false' to the non-FSCom branches of loadState() and saveState(). Co-Authored-By: Claude Fable 5 * SEN5X: correct version parsing, VOC index gating, and cleaning wait - getVersion() requested 3 raw I2C bytes (2 data bytes) but parsed versionBuffer[0..6], so the hardware and protocol versions came from the buffer's initialized-but-unwritten tail. Request the full 12-byte reply (8 data bytes, the layout used by Sensirion's embedded-i2c-sen5x driver) and validate the received length before parsing. Also make the error message specific to the version read. - Use floating-point division when deriving major.minor version numbers so minor versions below 10 are preserved (integer division reported e.g. firmware 2.2 as 2.00). - Gate pm_voc_idx on vocIndex rather than noxIndex, so SEN54 devices (VOC but no NOx) report their VOC index. - Widen the millis() snapshot in startCleaning() to uint32_t so the 10-second fan-cleaning wait always measures elapsed time correctly. Co-Authored-By: Claude Fable 5 * SEN5X: add size checks to I2C helpers, stage VOC state, handle unavailable readings - readBuffer(): accept only request sizes that are a multiple of 3 (2 data bytes + 1 CRC per group), keeping the read loop's size arithmetic in bounds for any future caller. Current callers all comply. - sendCommand(): likewise accept only even payload sizes on the write side. - vocStateFromSensor(): read into a staging buffer and copy to vocState only after the full transfer verifies, so the stored state stays consistent if a read fails partway through. - readValues()/readPNValues(): the sensor reports unavailable values as 0xFFFF (unsigned) / 0x7FFF (signed); map these to the UINT16_MAX / UINT32_MAX / FLT_MAX sentinels that getMetrics() checks, so unavailable channels are omitted from telemetry rather than scaled into numeric readings. Guard the cumulative-to-binned PN subtraction so the sentinels are preserved. - readPNValues(): convert #/cm3 to #/0.1l as raw * 10, retaining the 0.1-resolution digit that dividing before multiplying discarded. Co-Authored-By: Claude Fable 5 * SEN5X: size read buffers in data bytes and document I2C helper conventions readBuffer()'s size parameter is the raw I2C transfer size including CRC bytes, while only the verified data bytes (2/3 of the request) are written to the destination. Two call sites sized their buffers in raw units (findModel: 48 for 32 data bytes; getMeasurements: 3 for 2); both were safe over-allocations. Size them in data bytes so every call site reflects the same convention, and document the raw-vs-data contracts on the readBuffer() and sendCommand() declarations. No functional change. Co-Authored-By: Claude Fable 5 * SEN5X: use named defines for I2C reply buffer sizes Follow the SEN5X_VOC_STATE_BUFFER_SIZE pattern for all reply reads, per review feedback: define each reply's payload size in data bytes, size the destination buffer with it, request + / 2 raw bytes, and compare the received count against the same define. The version and product-name guards now compare against the full reply size rather than the bytes parsed (previously 7 and 5); readBuffer() returns either 0 or the full data count, so the conditions accept and reject the same transfers. Co-Authored-By: Claude Fable 5 * SEN5X: document I2C helper size requirements instead of checking at runtime Per review feedback: drop the runtime even-size and multiple-of-3 checks from sendCommand()/readBuffer() and state the requirements in @brief/@param documentation on the declarations. All callers pass sizes derived from the SEN5X_*_BUFFER_SIZE defines, which satisfy both requirements. Co-Authored-By: Claude Fable 5 * SEN5X: name the sensor's invalid-value constants Per review feedback, define SEN5X_UINT_INVALID (0xFFFF) and SEN5X_INT_INVALID (0x7FFF) for the values the sensor reports when a reading is unavailable, and use them in the readValues()/readPNValues() conversions in place of the numeric literals. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Claude Fable 5 Co-authored-by: oscgonfer Co-authored-by: Thomas Göttgens --- src/modules/Telemetry/Sensor/SEN5XSensor.cpp | 114 +++++++++++-------- src/modules/Telemetry/Sensor/SEN5XSensor.h | 29 ++++- 2 files changed, 92 insertions(+), 51 deletions(-) diff --git a/src/modules/Telemetry/Sensor/SEN5XSensor.cpp b/src/modules/Telemetry/Sensor/SEN5XSensor.cpp index 7a721433c..37df1204b 100644 --- a/src/modules/Telemetry/Sensor/SEN5XSensor.cpp +++ b/src/modules/Telemetry/Sensor/SEN5XSensor.cpp @@ -22,16 +22,18 @@ bool SEN5XSensor::getVersion() } delay(20); // From Sensirion Datasheet - uint8_t versionBuffer[12]{}; - size_t charNumber = readBuffer(&versionBuffer[0], 3); - if (charNumber == 0) { - LOG_ERROR("%s: Error getting data ready flag value", sensorName); + // Version reply layout: fw major/minor, fw debug, hw major/minor, + // protocol major/minor, padding + uint8_t versionBuffer[SEN5X_VERSION_BUFFER_SIZE]{}; + size_t charNumber = readBuffer(&versionBuffer[0], SEN5X_VERSION_BUFFER_SIZE + (SEN5X_VERSION_BUFFER_SIZE / 2)); + if (charNumber < SEN5X_VERSION_BUFFER_SIZE) { + LOG_ERROR("%s: Error getting device version value", sensorName); return false; } - firmwareVer = versionBuffer[0] + (versionBuffer[1] / 10); - hardwareVer = versionBuffer[3] + (versionBuffer[4] / 10); - protocolVer = versionBuffer[5] + (versionBuffer[6] / 10); + firmwareVer = versionBuffer[0] + (versionBuffer[1] / 10.0f); + hardwareVer = versionBuffer[3] + (versionBuffer[4] / 10.0f); + protocolVer = versionBuffer[5] + (versionBuffer[6] / 10.0f); LOG_INFO("%s: Firmware Version: %0.2f", sensorName, firmwareVer); LOG_INFO("%s: Hardware Version: %0.2f", sensorName, hardwareVer); @@ -48,12 +50,11 @@ bool SEN5XSensor::findModel() } delay(50); // From Sensirion Datasheet - const uint8_t nameSize = 48; - uint8_t name[nameSize]; - size_t charNumber = readBuffer(&name[0], nameSize); + uint8_t name[SEN5X_PRODUCT_NAME_BUFFER_SIZE]{}; + size_t charNumber = readBuffer(&name[0], SEN5X_PRODUCT_NAME_BUFFER_SIZE + (SEN5X_PRODUCT_NAME_BUFFER_SIZE / 2)); bool foundModel = false; - if (charNumber == 0) { + if (charNumber < SEN5X_PRODUCT_NAME_BUFFER_SIZE) { LOG_ERROR("%s: Error getting device name", sensorName); return foundModel; } @@ -361,15 +362,18 @@ bool SEN5XSensor::vocStateFromSensor() delay(20); // From Sensirion Datasheet - // Retrieve the data - // Allocate buffer to account for CRC - size_t receivedNumber = readBuffer(&vocState[0], SEN5X_VOC_STATE_BUFFER_SIZE + (SEN5X_VOC_STATE_BUFFER_SIZE / 2)); + // Retrieve the data into a staging buffer so a partial read (e.g. a CRC + // failure halfway through) cannot corrupt the current vocState. + // The requested size accounts for the CRC bytes + uint8_t stateBuffer[SEN5X_VOC_STATE_BUFFER_SIZE]{}; + size_t receivedNumber = readBuffer(&stateBuffer[0], SEN5X_VOC_STATE_BUFFER_SIZE + (SEN5X_VOC_STATE_BUFFER_SIZE / 2)); delay(20); // From Sensirion Datasheet - if (receivedNumber == 0) { + if (receivedNumber < SEN5X_VOC_STATE_BUFFER_SIZE) { LOG_DEBUG("%s: Error getting VOC's state", sensorName); return false; } + memcpy(vocState, stateBuffer, SEN5X_VOC_STATE_BUFFER_SIZE); // Print the state (if debug is on) LOG_DEBUG("%s: VOC state from sensor: [%u, %u, %u, %u, %u, %u, %u, %u]", sensorName, vocState[0], vocState[1], vocState[2], @@ -427,6 +431,7 @@ bool SEN5XSensor::loadState() return okay; #else LOG_ERROR("%s: Filesystem not implemented", sensorName); + return false; #endif } @@ -472,6 +477,7 @@ bool SEN5XSensor::saveState() return okay; #else LOG_ERROR("%s: Filesystem not implemented", sensorName); + return false; #endif } @@ -497,8 +503,7 @@ uint32_t SEN5XSensor::wakeUp() // keep track of how long it has passed pmMeasureStarted = getTime(); state = SEN5X_MEASUREMENT; - if (state == SEN5X_MEASUREMENT) - LOG_INFO("%s: Started measurement mode", sensorName); + LOG_INFO("%s: Started measurement mode", sensorName); return SEN5X_WARMUP_MS_1; } @@ -533,7 +538,7 @@ bool SEN5XSensor::startCleaning() // This message will be always printed so the user knows the device it's not hung LOG_INFO("%s: Started fan cleaning (10 sec)", sensorName); - uint16_t started = millis(); + uint32_t started = millis(); while (millis() - started < 10500) { delay(500); } @@ -658,9 +663,9 @@ bool SEN5XSensor::readValues() LOG_TRACE("%s: Reading PM Values", sensorName); delay(20); // From Sensirion Datasheet - uint8_t dataBuffer[16]{}; - size_t receivedNumber = readBuffer(&dataBuffer[0], 24); - if (receivedNumber == 0) { + uint8_t dataBuffer[SEN5X_READ_VALUES_BUFFER_SIZE]{}; + size_t receivedNumber = readBuffer(&dataBuffer[0], SEN5X_READ_VALUES_BUFFER_SIZE + (SEN5X_READ_VALUES_BUFFER_SIZE / 2)); + if (receivedNumber < SEN5X_READ_VALUES_BUFFER_SIZE) { LOG_ERROR("%s: Error getting values", sensorName); return false; } @@ -676,15 +681,17 @@ bool SEN5XSensor::readValues() int16_t int_vocIndex = static_cast((dataBuffer[12] << 8) | dataBuffer[13]); int16_t int_noxIndex = static_cast((dataBuffer[14] << 8) | dataBuffer[15]); - // Convert values based on Sensirion Arduino lib - sen5xmeasurement.pM1p0 = !isnan(uint_pM1p0) ? uint_pM1p0 / 10 : UINT16_MAX; - sen5xmeasurement.pM2p5 = !isnan(uint_pM2p5) ? uint_pM2p5 / 10 : UINT16_MAX; - sen5xmeasurement.pM4p0 = !isnan(uint_pM4p0) ? uint_pM4p0 / 10 : UINT16_MAX; - sen5xmeasurement.pM10p0 = !isnan(uint_pM10p0) ? uint_pM10p0 / 10 : UINT16_MAX; - sen5xmeasurement.humidity = !isnan(int_humidity) ? int_humidity / 100.0f : FLT_MAX; - sen5xmeasurement.temperature = !isnan(int_temperature) ? int_temperature / 200.0f : FLT_MAX; - sen5xmeasurement.vocIndex = !isnan(int_vocIndex) ? int_vocIndex / 10.0f : FLT_MAX; - sen5xmeasurement.noxIndex = !isnan(int_noxIndex) ? int_noxIndex / 10.0f : FLT_MAX; + // Convert values based on Sensirion Arduino lib. + // Map values the sensor reports as unavailable (SEN5X_UINT_INVALID / + // SEN5X_INT_INVALID) to the sentinels getMetrics() checks for + sen5xmeasurement.pM1p0 = (uint_pM1p0 != SEN5X_UINT_INVALID) ? (uint_pM1p0 / 10) : UINT16_MAX; + sen5xmeasurement.pM2p5 = (uint_pM2p5 != SEN5X_UINT_INVALID) ? (uint_pM2p5 / 10) : UINT16_MAX; + sen5xmeasurement.pM4p0 = (uint_pM4p0 != SEN5X_UINT_INVALID) ? (uint_pM4p0 / 10) : UINT16_MAX; + sen5xmeasurement.pM10p0 = (uint_pM10p0 != SEN5X_UINT_INVALID) ? (uint_pM10p0 / 10) : UINT16_MAX; + sen5xmeasurement.humidity = (int_humidity != SEN5X_INT_INVALID) ? (int_humidity / 100.0f) : FLT_MAX; + sen5xmeasurement.temperature = (int_temperature != SEN5X_INT_INVALID) ? (int_temperature / 200.0f) : FLT_MAX; + sen5xmeasurement.vocIndex = (int_vocIndex != SEN5X_INT_INVALID) ? (int_vocIndex / 10.0f) : FLT_MAX; + sen5xmeasurement.noxIndex = (int_noxIndex != SEN5X_INT_INVALID) ? (int_noxIndex / 10.0f) : FLT_MAX; LOG_TRACE("%s: Got readings: pM1p0=%u, pM2p5=%u, pM4p0=%u, pM10p0=%u", sensorName, sen5xmeasurement.pM1p0, sen5xmeasurement.pM2p5, sen5xmeasurement.pM4p0, sen5xmeasurement.pM10p0); @@ -711,9 +718,9 @@ bool SEN5XSensor::readPNValues(bool cumulative) LOG_TRACE("%s: Reading PN Values", sensorName); delay(20); // From Sensirion Datasheet - uint8_t dataBuffer[20]{}; - size_t receivedNumber = readBuffer(&dataBuffer[0], 30); - if (receivedNumber == 0) { + uint8_t dataBuffer[SEN5X_READ_PM_BUFFER_SIZE]{}; + size_t receivedNumber = readBuffer(&dataBuffer[0], SEN5X_READ_PM_BUFFER_SIZE + (SEN5X_READ_PM_BUFFER_SIZE / 2)); + if (receivedNumber < SEN5X_READ_PM_BUFFER_SIZE) { LOG_ERROR("%s: Error getting PN values", sensorName); return false; } @@ -730,22 +737,29 @@ bool SEN5XSensor::readPNValues(bool cumulative) uint16_t uint_pN10p0 = static_cast((dataBuffer[16] << 8) | dataBuffer[17]); uint16_t uint_tSize = static_cast((dataBuffer[18] << 8) | dataBuffer[19]); - // Convert values based on Sensirion Arduino lib - // Multiply by 100 for converting from #/cm3 to #/0.1l for PN values - sen5xmeasurement.pN0p5 = !isnan(uint_pN0p5) ? uint_pN0p5 / 10 * 100 : UINT32_MAX; - sen5xmeasurement.pN1p0 = !isnan(uint_pN1p0) ? uint_pN1p0 / 10 * 100 : UINT32_MAX; - sen5xmeasurement.pN2p5 = !isnan(uint_pN2p5) ? uint_pN2p5 / 10 * 100 : UINT32_MAX; - sen5xmeasurement.pN4p0 = !isnan(uint_pN4p0) ? uint_pN4p0 / 10 * 100 : UINT32_MAX; - sen5xmeasurement.pN10p0 = !isnan(uint_pN10p0) ? uint_pN10p0 / 10 * 100 : UINT32_MAX; - sen5xmeasurement.tSize = !isnan(uint_tSize) ? uint_tSize / 1000.0f : FLT_MAX; + // Convert values based on Sensirion Arduino lib. + // Raw PN values are #/cm3 with 0.1 resolution; multiplying by 10 + // converts to #/0.1l without the truncation of dividing first. + // Map values the sensor reports as unavailable (SEN5X_UINT_INVALID) to the + // sentinels getMetrics() checks for + sen5xmeasurement.pN0p5 = (uint_pN0p5 != SEN5X_UINT_INVALID) ? ((uint32_t)uint_pN0p5 * 10) : UINT32_MAX; + sen5xmeasurement.pN1p0 = (uint_pN1p0 != SEN5X_UINT_INVALID) ? ((uint32_t)uint_pN1p0 * 10) : UINT32_MAX; + sen5xmeasurement.pN2p5 = (uint_pN2p5 != SEN5X_UINT_INVALID) ? ((uint32_t)uint_pN2p5 * 10) : UINT32_MAX; + sen5xmeasurement.pN4p0 = (uint_pN4p0 != SEN5X_UINT_INVALID) ? ((uint32_t)uint_pN4p0 * 10) : UINT32_MAX; + sen5xmeasurement.pN10p0 = (uint_pN10p0 != SEN5X_UINT_INVALID) ? ((uint32_t)uint_pN10p0 * 10) : UINT32_MAX; + sen5xmeasurement.tSize = (uint_tSize != SEN5X_UINT_INVALID) ? (uint_tSize / 1000.0f) : FLT_MAX; // Remove accumuluative values: // https://github.com/fablabbcn/smartcitizen-kit-2x/issues/85 if (!cumulative) { - sen5xmeasurement.pN10p0 -= sen5xmeasurement.pN4p0; - sen5xmeasurement.pN4p0 -= sen5xmeasurement.pN2p5; - sen5xmeasurement.pN2p5 -= sen5xmeasurement.pN1p0; - sen5xmeasurement.pN1p0 -= sen5xmeasurement.pN0p5; + if (sen5xmeasurement.pN10p0 != UINT32_MAX && sen5xmeasurement.pN4p0 != UINT32_MAX) + sen5xmeasurement.pN10p0 -= sen5xmeasurement.pN4p0; + if (sen5xmeasurement.pN4p0 != UINT32_MAX && sen5xmeasurement.pN2p5 != UINT32_MAX) + sen5xmeasurement.pN4p0 -= sen5xmeasurement.pN2p5; + if (sen5xmeasurement.pN2p5 != UINT32_MAX && sen5xmeasurement.pN1p0 != UINT32_MAX) + sen5xmeasurement.pN2p5 -= sen5xmeasurement.pN1p0; + if (sen5xmeasurement.pN1p0 != UINT32_MAX && sen5xmeasurement.pN0p5 != UINT32_MAX) + sen5xmeasurement.pN1p0 -= sen5xmeasurement.pN0p5; } LOG_TRACE("%s: Got readings: pN0p5=%u, pN1p0=%u, pN2p5=%u, pN4p0=%u, pN10p0=%u, tSize=%.2f", sensorName, @@ -767,10 +781,10 @@ uint8_t SEN5XSensor::getMeasurements() } delay(20); // From Sensirion Datasheet - uint8_t dataReadyBuffer[3]; - size_t charNumber = readBuffer(&dataReadyBuffer[0], 3); - if (charNumber == 0) { - LOG_ERROR("%s: Error getting device version value", sensorName); + uint8_t dataReadyBuffer[SEN5X_DATA_READY_BUFFER_SIZE]{}; + size_t charNumber = readBuffer(&dataReadyBuffer[0], SEN5X_DATA_READY_BUFFER_SIZE + (SEN5X_DATA_READY_BUFFER_SIZE / 2)); + if (charNumber < SEN5X_DATA_READY_BUFFER_SIZE) { + LOG_ERROR("%s: Error getting data ready flag value", sensorName); return 2; } @@ -909,7 +923,7 @@ bool SEN5XSensor::getMetrics(meshtastic_Telemetry *measurement) measurement->variant.air_quality_metrics.has_pm_temperature = true; measurement->variant.air_quality_metrics.pm_temperature = sen5xmeasurement.temperature; } - if (sen5xmeasurement.noxIndex != FLT_MAX) { + if (sen5xmeasurement.vocIndex != FLT_MAX) { measurement->variant.air_quality_metrics.has_pm_voc_idx = true; measurement->variant.air_quality_metrics.pm_voc_idx = sen5xmeasurement.vocIndex; } diff --git a/src/modules/Telemetry/Sensor/SEN5XSensor.h b/src/modules/Telemetry/Sensor/SEN5XSensor.h index 5d84b8916..eeebbd373 100644 --- a/src/modules/Telemetry/Sensor/SEN5XSensor.h +++ b/src/modules/Telemetry/Sensor/SEN5XSensor.h @@ -86,6 +86,18 @@ class SEN5XSensor : public TelemetrySensor #define SEN5X_READ_RAW_VALUES 0x03D2 #define SEN5X_READ_PM_VALUES 0x0413 +// Values the sensor reports when a reading is unavailable +#define SEN5X_UINT_INVALID 0xFFFF +#define SEN5X_INT_INVALID 0x7FFF + +// Reply payload sizes in data bytes; the raw I2C transfer adds one CRC byte +// per 2-byte group, so requests are + / 2 raw bytes +#define SEN5X_VERSION_BUFFER_SIZE 8 +#define SEN5X_PRODUCT_NAME_BUFFER_SIZE 32 +#define SEN5X_DATA_READY_BUFFER_SIZE 2 +#define SEN5X_READ_VALUES_BUFFER_SIZE 16 +#define SEN5X_READ_PM_BUFFER_SIZE 20 + #define SEN5X_VOC_VALID_TIME 600 #define SEN5X_VOC_VALID_DATE 1514764800 @@ -114,8 +126,23 @@ See: https://sensirion.com/resource/application_note/low_power_mode/sen5x #define SEN5X_PN4P0_CONC_THD 100 bool sendCommand(uint16_t command); + /** + * @brief Send a command word followed by a data payload; a CRC byte is + * computed and inserted on the wire after every 2-byte pair. + * @param command 16-bit command code, sent big-endian + * @param buffer payload data bytes, without CRCs + * @param byteNumber payload size in data bytes; must be even + * @return true when the full transfer is written and acknowledged + */ bool sendCommand(uint16_t command, uint8_t *buffer, uint8_t byteNumber = 0); - uint8_t readBuffer(uint8_t *buffer, uint8_t byteNumber); // Return number of bytes received + /** + * @brief Read a reply, verifying and stripping the interleaved CRC bytes. + * @param buffer destination for the data bytes (byteNumber * 2 / 3 of them) + * @param byteNumber raw transfer size including CRCs; must be a multiple + * of 3 (2 data bytes + 1 CRC per group) + * @return the number of data bytes written to buffer, or 0 on any error + */ + uint8_t readBuffer(uint8_t *buffer, uint8_t byteNumber); uint8_t sen5xCRC(const uint8_t *buffer); bool startCleaning(); uint8_t getMeasurements();