Fixes on 0x69 address detection#10715
Merged
oscgonfer merged 21 commits intoJul 3, 2026
Merged
Conversation
* Change SEN5X detection method, using class itself * MPU6050 register check * BMX160 default removed
oscgonfer
added
tech debt
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves I2C device detection on shared address 0x69, removing fallback assumptions and making detection more explicit—especially for SEN5X air quality sensors and MPU6050 IMUs.
Changes:
- Add a
SEN5XSensor::probe(...)method and use it from the I2C scanner instead of a custom product-name probe. - Add an MPU6050
WHO_AM_Iregister value check. - Remove the previous “default to BMX160/MPU6050” behavior when detection fails.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/modules/Telemetry/Sensor/SEN5XSensor.h |
Exposes a new probe(...) API to support scanner-based detection. |
src/modules/Telemetry/Sensor/SEN5XSensor.cpp |
Implements probe(...) by resetting the sensor and identifying the model. |
src/detect/ScanI2CTwoWire.cpp |
Replaces custom SEN5X probing with SEN5XSensor::probe() and adds MPU6050 WHO_AM_I detection while removing old default fallbacks. |
Comment on lines
+140
to
145
| #include "../modules/Telemetry/Sensor/SEN5XSensor.h" | ||
| bool probeSEN5X(TwoWire *i2cBus, uint8_t address, ScanI2C::I2CPort port) | ||
| { | ||
| uint8_t cmd[] = {0xD0, 0x14}; | ||
| uint8_t response[48] = {0}; | ||
|
|
||
| i2cBus->beginTransmission(address); | ||
| i2cBus->write(cmd, 2); | ||
| if (i2cBus->endTransmission() != 0) | ||
| return ""; | ||
|
|
||
| delay(20); | ||
| if (i2cBus->requestFrom(address, (uint8_t)48) != 48) | ||
| return ""; | ||
|
|
||
| for (int i = 0; i < 48 && i2cBus->available(); ++i) { | ||
| response[i] = i2cBus->read(); | ||
| } | ||
|
|
||
| char productName[33] = {0}; | ||
| int j = 0; | ||
| for (int i = 0; i < 48 && j < 32; i += 3) { | ||
| if (response[i] >= 32 && response[i] <= 126) | ||
| productName[j++] = response[i]; | ||
| else | ||
| break; | ||
|
|
||
| if (response[i + 1] >= 32 && response[i + 1] <= 126) | ||
| productName[j++] = response[i + 1]; | ||
| else | ||
| break; | ||
| } | ||
|
|
||
| return String(productName); | ||
| SEN5XSensor sen5xsensor = SEN5XSensor(); | ||
| return sen5xsensor.probe(i2cBus, address, port); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Not quite sure I agree with this. I understand the module coupling, but there are two considerations:
- The sensor has quite a bit of details that are handled in the class directly (changing bus speed when sending commands, for one) that would need to be replicated in a separate class
- The sensor also has an internal firmware versioning, which is also detected in the class. If in the future there are new firmware versions for the sensor, we would only need to update de class, and not the probing on a different function.
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (26)
Build artifacts expire on 2026-08-02. Updated for |
* Make ReClockI2C API class * Use new API in AirQualityTelemetry module * Minor changes on some logs
* Define map for sensors to re-scan * Add re-scan on runOnce
Contributor
Firmware Size Report22 targets | vs
Show 17 more target(s)
Updated for b7d039b |
# Conflicts: # src/graphics/Screen.cpp # src/modules/Telemetry/Sensor/PMSA003ISensor.h # src/modules/Telemetry/Sensor/SCD30Sensor.h # src/modules/Telemetry/Sensor/SCD4XSensor.h # src/modules/Telemetry/Sensor/SEN5XSensor.cpp # src/modules/Telemetry/Sensor/SEN5XSensor.h # src/modules/Telemetry/Sensor/SFA30Sensor.h
ScanI2CTwoWire is compiled out on builds that define MESHTASTIC_EXCLUDE_I2C (e.g. native-wasm/portduino), while the AirQuality module still builds there. Skip the re-scan when I2C is excluded; there is nothing to scan.
📝 WalkthroughWalkthroughSEN5X I2C detection now uses a new ChangesSEN5X probe and I2C re-scan
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AirQualityTelemetryModule
participant ScanI2CTwoWire
participant SEN5XSensor
participant AddI2CSensorTemplate
AirQualityTelemetryModule->>ScanI2CTwoWire: i2cScanFinished(firstTime)
ScanI2CTwoWire->>SEN5XSensor: probeSEN5X(bus, address, port)
SEN5XSensor->>SEN5XSensor: findModel()
SEN5XSensor-->>ScanI2CTwoWire: true/false
ScanI2CTwoWire-->>AirQualityTelemetryModule: detected devices
AirQualityTelemetryModule->>AddI2CSensorTemplate: addSensor(dev)
AddI2CSensorTemplate->>AddI2CSensorTemplate: check for duplicate address/port
AddI2CSensorTemplate-->>AirQualityTelemetryModule: return early or register sensor
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/modules/Telemetry/AirQualityTelemetry.cpp (1)
147-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
std::make_uniqueover rawnew.As per coding guidelines, "Use C++17 features when appropriate."
std::make_uniqueis the idiomatic C++14/17 way to construct owned objects and avoids a nakednew.♻️ Proposed refactor
- auto i2cScanner = std::unique_ptr<ScanI2CTwoWire>(new ScanI2CTwoWire()); + auto i2cScanner = std::make_unique<ScanI2CTwoWire>();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/Telemetry/AirQualityTelemetry.cpp` at line 147, Replace the manual heap allocation in the AirQualityTelemetry setup with the idiomatic C++17 ownership helper: update the i2cScanner initialization in the code path that creates ScanI2CTwoWire to use std::make_unique instead of std::unique_ptr constructed from new. This keeps ownership semantics the same while removing the naked new and aligning with the existing C++17 style used in this module.Source: Coding guidelines
src/modules/Telemetry/Sensor/AddI2CSensorTemplate.h (1)
14-21: 📐 Maintainability & Code Quality | 🔵 TrivialMinor: duplicate
NONEcheck across the two conditionals.
dev.type != ScanI2C::DeviceType::NONEis evaluated here (Line 15) and again as part of the condition on Line 23. Could combine into a single early-return path for clarity, but the current structure is still correct and readable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/Telemetry/Sensor/AddI2CSensorTemplate.h` around lines 14 - 21, The duplicate `ScanI2C::DeviceType::NONE` check in `AddI2CSensorTemplate` should be simplified by using a single early-return flow. Update the logic around the device scan and sensor deduplication so the `dev.type != ScanI2C::DeviceType::NONE` guard is evaluated only once, and then perform the `sensors` address/port duplicate check within that path for clearer control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/modules/Telemetry/AirQualityTelemetry.cpp`:
- Line 147: Replace the manual heap allocation in the AirQualityTelemetry setup
with the idiomatic C++17 ownership helper: update the i2cScanner initialization
in the code path that creates ScanI2CTwoWire to use std::make_unique instead of
std::unique_ptr constructed from new. This keeps ownership semantics the same
while removing the naked new and aligning with the existing C++17 style used in
this module.
In `@src/modules/Telemetry/Sensor/AddI2CSensorTemplate.h`:
- Around line 14-21: The duplicate `ScanI2C::DeviceType::NONE` check in
`AddI2CSensorTemplate` should be simplified by using a single early-return flow.
Update the logic around the device scan and sensor deduplication so the
`dev.type != ScanI2C::DeviceType::NONE` guard is evaluated only once, and then
perform the `sensors` address/port duplicate check within that path for clearer
control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00df52d3-5e09-4cf7-b04e-a0a7a3ff387b
📒 Files selected for processing (11)
src/detect/ScanI2CTwoWire.cppsrc/modules/Telemetry/AirQualityTelemetry.cppsrc/modules/Telemetry/AirQualityTelemetry.hsrc/modules/Telemetry/Sensor/AddI2CSensorTemplate.hsrc/modules/Telemetry/Sensor/PMSA003ISensor.hsrc/modules/Telemetry/Sensor/SCD30Sensor.hsrc/modules/Telemetry/Sensor/SCD4XSensor.hsrc/modules/Telemetry/Sensor/SEN5XSensor.cppsrc/modules/Telemetry/Sensor/SEN5XSensor.hsrc/modules/Telemetry/Sensor/SFA30Sensor.hsrc/modules/Telemetry/Sensor/TelemetrySensor.h
💤 Files with no reviewable changes (1)
- src/modules/Telemetry/Sensor/SFA30Sensor.h
caveman99
reviewed
Jul 3, 2026
Contributor
Author
|
All good on my end too for this one @caveman99 |
caveman99
pushed a commit
that referenced
this pull request
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes some detection issues on devices with shared address 0x69
BMX160 detection has recently been improved by @caveman99, but there was some leftover code that assumed BMX160 as a default if everything else failed. Similar to MPU6050, which a WHO_AM_I register check was added.
The SEN5X sensor detection process is also improved, avoiding a custom probe for the model, but instead using the class methods, which account for all the I2C speed shebang (#9898 and #10593).
Changes:
🤝 Attestations
Tagging @caveman99 for checks if possible with the BMX160 code.
Caution
Rebased onto #9898 and #10593
Summary by CodeRabbit
New Features
Bug Fixes