diff --git a/src/detect/ReClockI2C.h b/src/detect/ReClockI2C.h index 24a166d53..2501224a0 100644 --- a/src/detect/ReClockI2C.h +++ b/src/detect/ReClockI2C.h @@ -13,7 +13,8 @@ https://github.com/sandeepmistry/arduino-nRF5/blob/master/libraries/Wire/Wire.h#L50 https://github.com/earlephilhower/arduino-pico/blob/master/libraries/Wire/src/Wire.h#L60 https://github.com/stm32duino/Arduino_Core_STM32/blob/main/libraries/Wire/src/Wire.h#L103 - For cases when I2C speed is different to the ones defined by sensors (see defines in sensor classes) + For cases when I2C speed is different to the ones defined by sensors + (see defines in sensor classes) we need to reclock I2C and set it back to the previous established speed. Only for cases where we can know it (ESP32 or known screen) we can do this. */ @@ -27,10 +28,16 @@ class ReClockI2C { this->i2cBus = i2cBus; this->port = port; - this->previousClock = 0; } - bool setClock(uint32_t desiredClock) + // Sets the I2C clock to desiredClock and returns whatever clock was active + // beforehand, so the caller can hand it back to restoreClock() later. The + // previous clock is returned rather than stored on this object, so callers + // that nest calls (see ReClockI2CGuard) each keep their own restoration + // value instead of clobbering a single shared one. + // Returns 0 if the clock was already at desiredClock, or if the previous + // clock couldn't be determined - in both cases there's nothing to restore. + uint32_t setClock(uint32_t desiredClock) { uint32_t currentClock = this->getClock(); @@ -41,36 +48,27 @@ class ReClockI2C if (currentClock != desiredClock) { LOG_TRACE("Changing I2C clock to %uHz", desiredClock); this->i2cBus->setClock(desiredClock); - // If the clock is 0Hz, we still store it - // We'll check in restoreClock function - setPreviousClock(currentClock); - LOG_TRACE("Stored previous clock I2C clock: %uHz", this->previousClock); - return true; + LOG_TRACE("Previous I2C clock: %uHz", currentClock); + return currentClock; } LOG_TRACE("I2C clock was already %uHz. Skipping", desiredClock); - setPreviousClock(0); - return false; + return 0; } - bool restoreClock() + void restoreClock(uint32_t previousClock) { - if (this->previousClock) { - LOG_TRACE("Restoring I2C clock to %uHz", this->previousClock); - i2cBus->setClock(this->previousClock); - setPreviousClock(0); - return true; + if (previousClock) { + LOG_TRACE("Restoring I2C clock to %uHz", previousClock); + i2cBus->setClock(previousClock); + return; } LOG_TRACE("I2C clock was unknown. Not restored"); - return false; } private: TwoWire *i2cBus{}; ScanI2C::I2CPort port{}; - uint32_t previousClock = 0; - - void setPreviousClock(uint32_t clock) { this->previousClock = clock; } uint32_t getClock() { @@ -95,4 +93,23 @@ class ReClockI2C } }; +/* Helper for ReClockI2C: sets the clock on construction and restores it on + destruction, so a caller with multiple early-return paths doesn't need to + remember to call restoreClock() on each one. + */ +class ReClockI2CGuard +{ + public: + ReClockI2CGuard(ReClockI2C &reClock, uint32_t desiredClock) : reClock(reClock), previousClock(reClock.setClock(desiredClock)) + { + } + ~ReClockI2CGuard() { reClock.restoreClock(previousClock); } + ReClockI2CGuard(const ReClockI2CGuard &) = delete; + ReClockI2CGuard &operator=(const ReClockI2CGuard &) = delete; + + private: + ReClockI2C &reClock; + uint32_t previousClock; +}; + #endif diff --git a/src/modules/Telemetry/Sensor/ADS1X15Sensor.cpp b/src/modules/Telemetry/Sensor/ADS1X15Sensor.cpp index 1b22e6358..1a8e923f1 100644 --- a/src/modules/Telemetry/Sensor/ADS1X15Sensor.cpp +++ b/src/modules/Telemetry/Sensor/ADS1X15Sensor.cpp @@ -14,21 +14,18 @@ bool ADS1X15Sensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) LOG_INFO("Init sensor: %s (address: 0x%x)", sensorName, dev->address.address); _bus = bus; - _port = dev->address.port; + _address = dev->address.address; _deviceType = dev->type; #ifdef ADS1X15_I2C_CLOCK_SPEED + _port = dev->address.port; reClockI2C.setup(_bus, _port); - reClockI2C.setClock(ADS1X15_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, ADS1X15_I2C_CLOCK_SPEED); #endif /* ADS1X15_I2C_CLOCK_SPEED */ status = ads1x15.begin(_address, _bus); -#ifdef ADS1X15_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* ADS1X15_I2C_CLOCK_SPEED */ - initI2CSensor(); return status; @@ -104,15 +101,11 @@ bool ADS1X15Sensor::getMetrics(meshtastic_Telemetry *measurement) { // Done here and not in getMeasurements to avoid the back-and-forth 4-8 times one after the other #ifdef ADS1X15_I2C_CLOCK_SPEED - reClockI2C.setClock(ADS1X15_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, ADS1X15_I2C_CLOCK_SPEED); #endif /* ADS1X15_I2C_CLOCK_SPEED */ struct _ADS1X15Measurements m = getMeasurements(); -#ifdef ADS1X15_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* ADS1X15_I2C_CLOCK_SPEED */ - switch (_deviceType) { case ScanI2C::DeviceType::ADS1X15: { measurement->variant.environment_metrics.has_adc_voltage_ch0 = true; diff --git a/src/modules/Telemetry/Sensor/DS248XSensor.cpp b/src/modules/Telemetry/Sensor/DS248XSensor.cpp index d0e138552..fe0877e6d 100644 --- a/src/modules/Telemetry/Sensor/DS248XSensor.cpp +++ b/src/modules/Telemetry/Sensor/DS248XSensor.cpp @@ -63,13 +63,11 @@ bool DS248XSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) #ifdef DS248X_I2C_CLOCK_SPEED reClockI2C.setup(_bus, _port); - reClockI2C.setClock(DS248X_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, DS248X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, DS248X_I2C_CLOCK_SPEED); #endif /* DS248X_I2C_CLOCK_SPEED */ if (!ds248x.begin(bus, _address)) { -#ifdef DS248X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* DS248X_I2C_CLOCK_SPEED */ return false; } @@ -151,9 +149,6 @@ bool DS248XSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) } if (initError && retry == numRetries) { -#ifdef DS248X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* DS248X_I2C_CLOCK_SPEED */ LOG_ERROR("%s: Max retries for one-wire init (%u/%u). Aborting", sensorName, retry, numRetries); return false; } @@ -173,10 +168,6 @@ bool DS248XSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) delay(500); } -#ifdef DS248X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* DS248X_I2C_CLOCK_SPEED */ - initI2CSensor(); return status; } @@ -190,7 +181,8 @@ bool DS248XSensor::isValidROM(const uint8_t *rom) float DS248XSensor::readTemperatureROM(const uint8_t *rom) { #ifdef DS248X_I2C_CLOCK_SPEED - reClockI2C.setClock(DS248X_I2C_CLOCK_SPEED); + LOG_DEBUG("%s: reclock speed %uHz", sensorName, DS248X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, DS248X_I2C_CLOCK_SPEED); #endif /* DS248X_I2C_CLOCK_SPEED */ uint8_t data[9]{}; @@ -219,10 +211,6 @@ float DS248XSensor::readTemperatureROM(const uint8_t *rom) } } -#ifdef DS248X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* DS248X_I2C_CLOCK_SPEED */ - if (!ok) { LOG_WARN("%s: One-wire transaction failed", sensorName); return DS248X_INVALID_TEMPERATURE; diff --git a/src/modules/Telemetry/Sensor/HM330XSensor.cpp b/src/modules/Telemetry/Sensor/HM330XSensor.cpp index 91c0267e7..206b89d1d 100644 --- a/src/modules/Telemetry/Sensor/HM330XSensor.cpp +++ b/src/modules/Telemetry/Sensor/HM330XSensor.cpp @@ -17,21 +17,16 @@ bool HM330XSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) #ifdef HM330X_I2C_CLOCK_SPEED _port = dev->address.port; reClockI2C.setup(_bus, _port); - reClockI2C.setClock(HM330X_I2C_CLOCK_SPEED); + + LOG_INFO("%s: reclock speed %uHz", sensorName, HM330X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, HM330X_I2C_CLOCK_SPEED); #endif /* HM330X_I2C_CLOCK_SPEED */ if (hm330x.init(_bus) != HM330XErrorCode::NO_ERROR) { -#ifdef HM330X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* HM330X_I2C_CLOCK_SPEED */ LOG_WARN("%s error in sensor init", sensorName); return false; } -#ifdef HM330X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* HM330X_I2C_CLOCK_SPEED */ - status = 1; LOG_INFO("%s Enabled", sensorName); @@ -77,21 +72,15 @@ int32_t HM330XSensor::pendingForReadyMs() bool HM330XSensor::getMetrics(meshtastic_Telemetry *measurement) { #ifdef HM330X_I2C_CLOCK_SPEED - reClockI2C.setClock(HM330X_I2C_CLOCK_SPEED); + LOG_DEBUG("%s: reclock speed %uHz", sensorName, HM330X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, HM330X_I2C_CLOCK_SPEED); #endif /* HM330X_I2C_CLOCK_SPEED */ if (hm330x.read_sensor_value(buffer, 29)) { LOG_WARN("%s: read result failed", sensorName); -#ifdef HM330X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* HM330X_I2C_CLOCK_SPEED */ return false; } -#ifdef HM330X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* HM330X_I2C_CLOCK_SPEED */ - if (hm330x.checksum_calc(buffer) != HM330XErrorCode::NO_ERROR) { LOG_ERROR("%s: Checksum error", sensorName); return false; diff --git a/src/modules/Telemetry/Sensor/PMSA003ISensor.cpp b/src/modules/Telemetry/Sensor/PMSA003ISensor.cpp index 8b9151379..16aa5e7d1 100644 --- a/src/modules/Telemetry/Sensor/PMSA003ISensor.cpp +++ b/src/modules/Telemetry/Sensor/PMSA003ISensor.cpp @@ -24,23 +24,18 @@ bool PMSA003ISensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) #ifdef PMSA003I_I2C_CLOCK_SPEED _port = dev->address.port; reClockI2C.setup(_bus, _port); - reClockI2C.setClock(PMSA003I_I2C_CLOCK_SPEED); + + LOG_INFO("%s: reclock speed %uHz", sensorName, PMSA003I_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, PMSA003I_I2C_CLOCK_SPEED); #endif /* PMSA003I_I2C_CLOCK_SPEED */ _bus->beginTransmission(_address); if (_bus->endTransmission() != 0) { LOG_WARN("%s not found on I2C at 0x12", sensorName); -#ifdef PMSA003I_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* PMSA003I_I2C_CLOCK_SPEED */ sleep(); return false; } -#ifdef PMSA003I_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* PMSA003I_I2C_CLOCK_SPEED */ - status = 1; LOG_INFO("%s: Enabled", sensorName); sleep(); @@ -57,15 +52,13 @@ bool PMSA003ISensor::getMetrics(meshtastic_Telemetry *measurement) } #ifdef PMSA003I_I2C_CLOCK_SPEED - reClockI2C.setClock(PMSA003I_I2C_CLOCK_SPEED); + LOG_DEBUG("%s: reclock speed %uHz", sensorName, PMSA003I_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, PMSA003I_I2C_CLOCK_SPEED); #endif /* PMSA003I_I2C_CLOCK_SPEED */ _bus->requestFrom(_address, (uint8_t)PMSA003I_FRAME_LENGTH); if (_bus->available() < PMSA003I_FRAME_LENGTH) { LOG_WARN("%s: read failed: incomplete data (%d bytes)", sensorName, _bus->available()); -#ifdef PMSA003I_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* PMSA003I_I2C_CLOCK_SPEED */ return false; } @@ -73,10 +66,6 @@ bool PMSA003ISensor::getMetrics(meshtastic_Telemetry *measurement) buffer[i] = _bus->read(); } -#ifdef PMSA003I_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* PMSA003I_I2C_CLOCK_SPEED */ - if (buffer[0] != 0x42 || buffer[1] != 0x4D) { LOG_WARN("%s: frame header invalid: 0x%02X 0x%02X", sensorName, buffer[0], buffer[1]); return false; diff --git a/src/modules/Telemetry/Sensor/SCD30Sensor.cpp b/src/modules/Telemetry/Sensor/SCD30Sensor.cpp index c2631f5b0..d203837bb 100644 --- a/src/modules/Telemetry/Sensor/SCD30Sensor.cpp +++ b/src/modules/Telemetry/Sensor/SCD30Sensor.cpp @@ -18,16 +18,15 @@ bool SCD30Sensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) #ifdef SCD30_I2C_CLOCK_SPEED _port = dev->address.port; reClockI2C.setup(_bus, _port); - reClockI2C.setClock(SCD30_I2C_CLOCK_SPEED); + + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD30_I2C_CLOCK_SPEED); #endif /* SCD30_I2C_CLOCK_SPEED */ scd30.begin(*_bus, _address); if (!startMeasurement()) { - LOG_ERROR("%s: Periodic measurement start failed", sensorName); -#ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD30_I2C_CLOCK_SPEED */ + LOG_ERROR("%s: Failed to start periodic measurement", sensorName); return false; } @@ -35,10 +34,6 @@ bool SCD30Sensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) LOG_WARN("%s: Can't determine ASC state", sensorName); } -#ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD30_I2C_CLOCK_SPEED */ - if (state == SCD30_MEASUREMENT) { status = 1; } else { @@ -55,21 +50,15 @@ bool SCD30Sensor::getMetrics(meshtastic_Telemetry *measurement) float co2, temperature, humidity; #ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD30_I2C_CLOCK_SPEED); + LOG_DEBUG("%s: reclock speed %uHz", sensorName, SCD30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD30_I2C_CLOCK_SPEED); #endif /* SCD30_I2C_CLOCK_SPEED */ if (scd30.readMeasurementData(co2, temperature, humidity) != SCD30_NO_ERROR) { - LOG_ERROR("%s: Measurement read failed", sensorName); -#ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD30_I2C_CLOCK_SPEED */ + LOG_ERROR("%s: Failed to read measurement data", sensorName); return false; } -#ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD30_I2C_CLOCK_SPEED */ - if (co2 == 0) { LOG_ERROR("%s: Invalid CO₂ reading", sensorName); return false; @@ -359,15 +348,12 @@ bool SCD30Sensor::isActive() uint32_t SCD30Sensor::wakeUp() { #ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD30_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD30_I2C_CLOCK_SPEED); #endif /* SCD30_I2C_CLOCK_SPEED */ startMeasurement(); -#ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD30_I2C_CLOCK_SPEED */ - return 0; } @@ -378,14 +364,11 @@ uint32_t SCD30Sensor::wakeUp() void SCD30Sensor::sleep() { #ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD30_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD30_I2C_CLOCK_SPEED); #endif /* SCD30_I2C_CLOCK_SPEED */ stopMeasurement(); - -#ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD30_I2C_CLOCK_SPEED */ } bool SCD30Sensor::canSleep() @@ -409,7 +392,8 @@ AdminMessageHandleResult SCD30Sensor::handleAdminMessage(const meshtastic_MeshPa AdminMessageHandleResult result; #ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD30_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD30_I2C_CLOCK_SPEED); #endif /* SCD30_I2C_CLOCK_SPEED */ switch (request->which_payload_variant) { @@ -462,10 +446,6 @@ AdminMessageHandleResult SCD30Sensor::handleAdminMessage(const meshtastic_MeshPa result = AdminMessageHandleResult::NOT_HANDLED; } -#ifdef SCD30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD30_I2C_CLOCK_SPEED */ - return result; } diff --git a/src/modules/Telemetry/Sensor/SCD4XSensor.cpp b/src/modules/Telemetry/Sensor/SCD4XSensor.cpp index 261f8259b..92cb696e6 100644 --- a/src/modules/Telemetry/Sensor/SCD4XSensor.cpp +++ b/src/modules/Telemetry/Sensor/SCD4XSensor.cpp @@ -19,7 +19,8 @@ bool SCD4XSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) #ifdef SCD4X_I2C_CLOCK_SPEED _port = dev->address.port; reClockI2C.setup(_bus, _port); - reClockI2C.setClock(SCD4X_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD4X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD4X_I2C_CLOCK_SPEED); #endif /* SCD4X_I2C_CLOCK_SPEED */ scd4x.begin(*_bus, _address); @@ -29,9 +30,6 @@ bool SCD4XSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) // Stop periodic measurement if (!stopMeasurement()) { -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ return false; } @@ -41,35 +39,22 @@ bool SCD4XSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) if (sensorVariant == SCD4X_SENSOR_VARIANT_SCD41) { LOG_INFO("%s: Found SCD41", sensorName); if (!powerUp()) { - LOG_ERROR("%s: powerUp() failed", sensorName); -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ + LOG_ERROR("%s: Error trying to execute powerUp()", sensorName); return false; } } if (!getASC(ascActive)) { - LOG_ERROR("%s: Can't check if ASC enabled", sensorName); -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ + LOG_ERROR("%s: Unable to check if ASC is enabled", sensorName); return false; } // Start measurement in selected power mode (low power by default) if (!startMeasurement()) { - LOG_ERROR("%s: Can't start measurement", sensorName); -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ + LOG_ERROR("%s: Couldn't start measurement", sensorName); return false; } -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ - if (state == SCD4X_MEASUREMENT) { status = 1; } else { @@ -93,7 +78,8 @@ bool SCD4XSensor::getMetrics(meshtastic_Telemetry *measurement) float temperature, humidity; #ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD4X_I2C_CLOCK_SPEED); + LOG_DEBUG("%s: reclock speed %uHz", sensorName, SCD4X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD4X_I2C_CLOCK_SPEED); #endif /* SCD4X_I2C_CLOCK_SPEED */ bool dataReady = false; @@ -109,19 +95,12 @@ bool SCD4XSensor::getMetrics(meshtastic_Telemetry *measurement) } if (error != SCD4X_NO_ERROR || !dataReady) { -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ LOG_ERROR("SCD4X: Data is not ready"); return false; } error = scd4x.readMeasurement(co2, temperature, humidity); -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ - LOG_DEBUG("Got %s readings: co2=%u, co2_temp=%.2f, co2_hum%.2f", sensorName, co2, temperature, humidity); if (error != SCD4X_NO_ERROR) { LOG_DEBUG("%s: Error getting measurements: %u", sensorName, error); @@ -634,28 +613,19 @@ bool SCD4XSensor::powerDown() } #ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD4X_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD4X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD4X_I2C_CLOCK_SPEED); #endif /* SCD4X_I2C_CLOCK_SPEED */ if (!stopMeasurement()) { -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ return false; } if (scd4x.powerDown() != SCD4X_NO_ERROR) { - LOG_ERROR("%s: sleep() failed", sensorName); -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ + LOG_ERROR("%s: Error trying to execute sleep()", sensorName); return false; } -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ - state = SCD4X_OFF; return true; } @@ -701,21 +671,15 @@ uint32_t SCD4XSensor::wakeUp() { #ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD4X_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD4X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD4X_I2C_CLOCK_SPEED); #endif /* SCD4X_I2C_CLOCK_SPEED */ if (startMeasurement()) { co2MeasureStarted = millis(); -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ return SCD4X_WARMUP_MS; } -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ - return 0; } @@ -726,14 +690,11 @@ uint32_t SCD4XSensor::wakeUp() void SCD4XSensor::sleep() { #ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD4X_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD4X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD4X_I2C_CLOCK_SPEED); #endif /* SCD4X_I2C_CLOCK_SPEED */ stopMeasurement(); - -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ } /** @@ -771,7 +732,8 @@ AdminMessageHandleResult SCD4XSensor::handleAdminMessage(const meshtastic_MeshPa AdminMessageHandleResult result; #ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.setClock(SCD4X_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SCD4X_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SCD4X_I2C_CLOCK_SPEED); #endif /* SCD4X_I2C_CLOCK_SPEED */ // TODO: potentially add selftest command? @@ -837,10 +799,6 @@ AdminMessageHandleResult SCD4XSensor::handleAdminMessage(const meshtastic_MeshPa // Start measurement mode this->startMeasurement(); -#ifdef SCD4X_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SCD4X_I2C_CLOCK_SPEED */ - return result; } diff --git a/src/modules/Telemetry/Sensor/SENXXSensor.cpp b/src/modules/Telemetry/Sensor/SENXXSensor.cpp index 42b8a4ae3..d63d44850 100644 --- a/src/modules/Telemetry/Sensor/SENXXSensor.cpp +++ b/src/modules/Telemetry/Sensor/SENXXSensor.cpp @@ -174,6 +174,7 @@ bool SENXXSensor::probe(TwoWire *bus, uint8_t address, ScanI2C::I2CPort port) #ifdef SENXX_I2C_CLOCK_SPEED _port = port; reClockI2C.setup(_bus, _port); + ReClockI2CGuard clockGuard(reClockI2C, SENXX_I2C_CLOCK_SPEED); #endif /* SENXX_I2C_CLOCK_SPEED */ if (!findModel()) { @@ -215,23 +216,12 @@ bool SENXXSensor::sendCommand(uint16_t command, uint8_t *buffer, uint8_t byteNum } } -#ifdef SENXX_I2C_CLOCK_SPEED - LOG_DEBUG("%s: Attempting to reclock speed to %uHz", sensorName, SENXX_I2C_CLOCK_SPEED); - reClockI2C.setClock(SENXX_I2C_CLOCK_SPEED); -#endif /* SENXX_I2C_CLOCK_SPEED */ - - // Transmit the data // Note: this delay is necessary to allow for long-buffers delay(20); _bus->beginTransmission(_address); size_t writtenBytes = _bus->write(toSend, bufferSize); uint8_t i2c_error = _bus->endTransmission(); -#ifdef SENXX_I2C_CLOCK_SPEED - LOG_DEBUG("%s: restoring clock speed", sensorName); - reClockI2C.restoreClock(); -#endif /* SENXX_I2C_CLOCK_SPEED */ - if (writtenBytes != bufferSize) { LOG_ERROR("%s: Error writing on I2C bus", sensorName); return false; @@ -246,18 +236,9 @@ bool SENXXSensor::sendCommand(uint16_t command, uint8_t *buffer, uint8_t byteNum uint8_t SENXXSensor::readBuffer(uint8_t *buffer, uint8_t byteNumber) { -#ifdef SENXX_I2C_CLOCK_SPEED - LOG_DEBUG("%s: Attempting to reclock speed to %uHz", sensorName, SENXX_I2C_CLOCK_SPEED); - reClockI2C.setClock(SENXX_I2C_CLOCK_SPEED); -#endif /* SENXX_I2C_CLOCK_SPEED */ - size_t readBytes = _bus->requestFrom(_address, byteNumber); if (readBytes != byteNumber) { LOG_ERROR("%s: Error reading I2C bus", sensorName); -#ifdef SENXX_I2C_CLOCK_SPEED - LOG_DEBUG("%s: restoring clock speed", sensorName); - reClockI2C.restoreClock(); -#endif /* SENXX_I2C_CLOCK_SPEED */ return 0; } @@ -270,21 +251,12 @@ uint8_t SENXXSensor::readBuffer(uint8_t *buffer, uint8_t byteNumber) uint8_t calcCRC = senxxCRC(&buffer[i - 2]); if (recvCRC != calcCRC) { LOG_ERROR("%s: Checksum error while receiving msg", sensorName); -#ifdef SENXX_I2C_CLOCK_SPEED - LOG_DEBUG("%s: restoring clock speed", sensorName); - reClockI2C.restoreClock(); -#endif /* SENXX_I2C_CLOCK_SPEED */ return 0; } readBytes -= 3; receivedBytes += 2; } -#ifdef SENXX_I2C_CLOCK_SPEED - LOG_DEBUG("%s: restoring clock speed", sensorName); - reClockI2C.restoreClock(); -#endif /* SENXX_I2C_CLOCK_SPEED */ - return receivedBytes; } @@ -320,6 +292,9 @@ void SENXXSensor::sleep() LOG_INFO("%s: Not going to sleep, fan cleaning is in progress", sensorName); return; } +#ifdef SENXX_I2C_CLOCK_SPEED + ReClockI2CGuard clockGuard(reClockI2C, SENXX_I2C_CLOCK_SPEED); +#endif /* SENXX_I2C_CLOCK_SPEED */ idle(true); } @@ -690,6 +665,14 @@ void SENXXSensor::reconcileTimeDependentState(uint32_t now) } uint32_t SENXXSensor::wakeUp() +{ +#ifdef SENXX_I2C_CLOCK_SPEED + ReClockI2CGuard clockGuard(reClockI2C, SENXX_I2C_CLOCK_SPEED); +#endif /* SENXX_I2C_CLOCK_SPEED */ + return wakeUpInternal(); +} + +uint32_t SENXXSensor::wakeUpInternal() { LOG_DEBUG("%s: Waking up sensor", sensorName); @@ -789,6 +772,7 @@ bool SENXXSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) #ifdef SENXX_I2C_CLOCK_SPEED _port = dev->address.port; reClockI2C.setup(_bus, _port); + ReClockI2CGuard clockGuard(reClockI2C, SENXX_I2C_CLOCK_SPEED); #endif /* SENXX_I2C_CLOCK_SPEED */ delay(50); // without this there is an error on the deviceReset function @@ -1141,6 +1125,12 @@ int32_t SENXXSensor::wakeUpTimeMs() int32_t SENXXSensor::pendingForReadyMs() { +#ifdef SENXX_I2C_CLOCK_SPEED + // Only the SENXX_MEASUREMENT/SENXX_CLEANING branches below touch I2C, but this is only + // ever called while isActive() (i.e. one of those, or SENXX_MEASUREMENT_2, which doesn't), + // so bracketing unconditionally here is simpler than guarding each branch separately. + ReClockI2CGuard clockGuard(reClockI2C, SENXX_I2C_CLOCK_SPEED); +#endif /* SENXX_I2C_CLOCK_SPEED */ uint32_t now = millis(); uint32_t sincePmMeasureStarted = now - pmMeasureStarted; LOG_DEBUG("%s: Since measure started: %ums", sensorName, sincePmMeasureStarted); @@ -1203,6 +1193,10 @@ bool SENXXSensor::getMetrics(meshtastic_Telemetry *measurement) return false; } +#ifdef SENXX_I2C_CLOCK_SPEED + ReClockI2CGuard clockGuard(reClockI2C, SENXX_I2C_CLOCK_SPEED); +#endif /* SENXX_I2C_CLOCK_SPEED */ + uint8_t response; response = getMeasurements(); @@ -1525,6 +1519,9 @@ AdminMessageHandleResult SENXXSensor::handleAdminMessage(const meshtastic_MeshPa switch (request->which_payload_variant) { case meshtastic_AdminMessage_sensor_config_tag: { +#ifdef SENXX_I2C_CLOCK_SPEED + ReClockI2CGuard clockGuard(reClockI2C, SENXX_I2C_CLOCK_SPEED); +#endif /* SENXX_I2C_CLOCK_SPEED */ bool ok = true; bool wasActive = isActive(); @@ -1582,7 +1579,9 @@ AdminMessageHandleResult SENXXSensor::handleAdminMessage(const meshtastic_MeshPa } if (wasActive) { - this->wakeUp(); + // Not this->wakeUp() - we're already inside this function's own + // ReClockI2CGuard, and that guard isn't reentrant (see its comment). + this->wakeUpInternal(); } } } else { diff --git a/src/modules/Telemetry/Sensor/SENXXSensor.h b/src/modules/Telemetry/Sensor/SENXXSensor.h index 1bc80efd8..94c1930b3 100644 --- a/src/modules/Telemetry/Sensor/SENXXSensor.h +++ b/src/modules/Telemetry/Sensor/SENXXSensor.h @@ -255,6 +255,11 @@ class SENXXSensor : public TelemetrySensor, public CO2CalibrationSensor bool readPNValues(bool cumulative); bool readValues(); + // Actual wakeUp() logic, factored out so handleAdminMessage() can resume + // measurement after a calibration pause without nesting a second I2C-clock + // guard inside its own (see ReClockI2CGuard's reentrancy note). + uint32_t wakeUpInternal(); + // Monotonic (millis()) timers for warmup/poll intervals. Deliberately not // wall-clock (getTime()) based: getTime() can jump discontinuously the moment the RTC // quality improves mid-session (see checkRTCQualityImproved()), which would corrupt diff --git a/src/modules/Telemetry/Sensor/SFA30Sensor.cpp b/src/modules/Telemetry/Sensor/SFA30Sensor.cpp index 90671e229..743130d48 100644 --- a/src/modules/Telemetry/Sensor/SFA30Sensor.cpp +++ b/src/modules/Telemetry/Sensor/SFA30Sensor.cpp @@ -17,33 +17,24 @@ bool SFA30Sensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) #ifdef SFA30_I2C_CLOCK_SPEED _port = dev->address.port; reClockI2C.setup(_bus, _port); - reClockI2C.setClock(SFA30_I2C_CLOCK_SPEED); + LOG_INFO("%s: reclock speed %uHz", sensorName, SFA30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SFA30_I2C_CLOCK_SPEED); #endif /* SFA30_I2C_CLOCK_SPEED */ sfa30.begin(*_bus, _address); delay(20); if (this->isError(sfa30.deviceReset())) { -#ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SFA30_I2C_CLOCK_SPEED */ return false; } state = State::IDLE; if (this->isError(sfa30.startContinuousMeasurement())) { -#ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SFA30_I2C_CLOCK_SPEED */ return false; } LOG_INFO("%s starting measurement", sensorName); -#ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SFA30_I2C_CLOCK_SPEED */ - status = 1; state = State::ACTIVE; measureStarted = millis(); @@ -66,7 +57,8 @@ bool SFA30Sensor::isError(uint16_t response) void SFA30Sensor::sleep() { #ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.setClock(SFA30_I2C_CLOCK_SPEED); + LOG_DEBUG("%s: reclock speed %uHz", sensorName, SFA30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SFA30_I2C_CLOCK_SPEED); #endif /* SFA30_I2C_CLOCK_SPEED */ // Note - not recommended for this sensor on a periodic basis @@ -74,10 +66,6 @@ void SFA30Sensor::sleep() LOG_ERROR("%s: Can't stop measurement", sensorName); }; -#ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SFA30_I2C_CLOCK_SPEED */ - LOG_DEBUG("%s: stop measurement", sensorName); state = State::IDLE; measureStarted = 0; @@ -86,21 +74,15 @@ void SFA30Sensor::sleep() uint32_t SFA30Sensor::wakeUp() { #ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.setClock(SFA30_I2C_CLOCK_SPEED); + LOG_DEBUG("%s: reclock speed %uHz", sensorName, SFA30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SFA30_I2C_CLOCK_SPEED); #endif /* SFA30_I2C_CLOCK_SPEED */ LOG_DEBUG("Waking %s", sensorName); if (this->isError(sfa30.startContinuousMeasurement())) { -#ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SFA30_I2C_CLOCK_SPEED */ return 0; } -#ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SFA30_I2C_CLOCK_SPEED */ - state = State::ACTIVE; measureStarted = millis(); return SFA30_WARMUP_MS; @@ -142,21 +124,15 @@ bool SFA30Sensor::getMetrics(meshtastic_Telemetry *measurement) float temperature = 0.0; #ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.setClock(SFA30_I2C_CLOCK_SPEED); + LOG_DEBUG("%s: reclock speed %uHz", sensorName, SFA30_I2C_CLOCK_SPEED); + ReClockI2CGuard clockGuard(reClockI2C, SFA30_I2C_CLOCK_SPEED); #endif /* SFA30_I2C_CLOCK_SPEED */ if (this->isError(sfa30.readMeasuredValues(hcho, humidity, temperature))) { LOG_WARN("%s: No values", sensorName); -#ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SFA30_I2C_CLOCK_SPEED */ return false; } -#ifdef SFA30_I2C_CLOCK_SPEED - reClockI2C.restoreClock(); -#endif /* SFA30_I2C_CLOCK_SPEED */ - measurement->variant.air_quality_metrics.has_form_temperature = true; measurement->variant.air_quality_metrics.has_form_humidity = true; measurement->variant.air_quality_metrics.has_form_formaldehyde = true;