Add AS3935 lightning sensor support#10931
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds SparkFun AS3935 lightning detector support: a pinned library dependency, new I2C address macros and device enum, an I2C probing routine, a new AS3935Sensor telemetry driver, and EnvironmentTelemetryModule changes enabling immediate telemetry sends triggered by lightning events. Updates protobufs subproject reference. ChangesAS3935 Lightning Sensor Integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ScanI2CTwoWire
participant AS3935Sensor
participant EnvironmentTelemetryModule
participant Mesh
ScanI2CTwoWire->>ScanI2CTwoWire: probe AS3935 candidate addresses
ScanI2CTwoWire->>AS3935Sensor: register found device
AS3935Sensor->>AS3935Sensor: initDevice() configures thresholds and IRQ
loop runOnce polling
AS3935Sensor->>AS3935Sensor: check IRQ pin
AS3935Sensor->>AS3935Sensor: classify interrupt reason
AS3935Sensor->>EnvironmentTelemetryModule: requestImmediateSend()
end
EnvironmentTelemetryModule->>EnvironmentTelemetryModule: runOnce() detects immediateSendRequested
EnvironmentTelemetryModule->>Mesh: sendTelemetry()
Suggested labels: enhancement, sensors, telemetry Suggested reviewers: thebentern, GUVWAF Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
github-actions
Bot
added
the
hardware-support
6504bfe to
bee0a81
Compare
|
f-p both PRs to add _1h suffix to new protobuf field |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/detect/ScanI2CTwoWire.cpp (1)
913-915: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment exceeds two-line limit.
This explanatory comment spans three lines. As per coding guidelines, "Keep code comments minimal: one or two lines maximum, only when the reason is not obvious, and do not restate the next line."
✏️ Suggested trim
- // No WHOAMI register, and a POR-only check can't survive a warm reboot (this - // driver rewrites REG0x00 on init). Instead, write a test pattern to bits[5:1] - // and confirm it reads back - initDevice() overwrites this field right after anyway. + // No WHOAMI register; write a test pattern to bits[5:1] and confirm readback. + // initDevice() overwrites this field right after anyway.🤖 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/detect/ScanI2CTwoWire.cpp` around lines 913 - 915, The inline explanation in initDevice() is too long and repeats implementation details, so trim it to at most two lines while keeping only the essential rationale for the test-pattern write/readback check in ScanI2CTwoWire. Keep the comment minimal and avoid restating what the following code already shows, using the surrounding initDevice logic and REG0x00/WHOAMI context to preserve meaning.Source: Coding guidelines
🤖 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.
Inline comments:
In `@src/modules/Telemetry/Sensor/AS3935Sensor.cpp`:
- Around line 82-107: The interrupt switch in AS3935Sensor::readInterruptReg
handling is using the wrong case labels, so lightning events may not be matched
correctly. Update the switch on interruptReason in AS3935Sensor.cpp to use the
SparkFun AS3935 interrupt constants that correspond to readInterruptReg()
values, specifically LIGHTNING_INT, DISTURBER_INT, and NOISE_INT, so the
lightning, disturber, and noise branches are triggered as intended.
---
Nitpick comments:
In `@src/detect/ScanI2CTwoWire.cpp`:
- Around line 913-915: The inline explanation in initDevice() is too long and
repeats implementation details, so trim it to at most two lines while keeping
only the essential rationale for the test-pattern write/readback check in
ScanI2CTwoWire. Keep the comment minimal and avoid restating what the following
code already shows, using the surrounding initDevice logic and REG0x00/WHOAMI
context to preserve meaning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ac0ce3ef-fa56-46e1-8c5f-5323a433093a
⛔ Files ignored due to path filters (1)
src/mesh/generated/meshtastic/telemetry.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (10)
platformio.iniprotobufssrc/configuration.hsrc/detect/ScanI2C.hsrc/detect/ScanI2CTwoWire.cppsrc/modules/Modules.cppsrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/EnvironmentTelemetry.hsrc/modules/Telemetry/Sensor/AS3935Sensor.cppsrc/modules/Telemetry/Sensor/AS3935Sensor.h
|
@coderabbitai review |
✅ Action performedReview finished.
|
Implements meshtastic#10774: an AS3935Sensor (TelemetrySensor subclass) that reports lightning_strike_count_1h and lightning_distance_km on the normal environment telemetry interval, like a rain gauge - strikes are counted over a fixed rolling ~1h window and read non-destructively, so replying to a peer's telemetry request in between broadcasts can't silently drop counted strikes. The AS3935's IRQ pin (opt-in per board via AS3935_IRQ) is polled with a plain digitalRead() in runOnce(), deliberately not attachInterrupt(): the IRQ line is a level that stays asserted until its interrupt register is read, so polling can't miss an event regardless of timing, matching the SparkFun library's own reference examples. An interrupt would also buy nothing here even setting that aside - classification requires an I2C read (readInterruptReg(), which itself calls delay(2) per the datasheet's settle-time requirement), and blocking I2C/delay() calls aren't safe from ISR context on any of this codebase's target platforms, so the ISR could only ever set a flag for later draining - no less work than just polling the pin directly on the next tick. A genuine lightning classification also requests an immediate out-of-cycle send via a new EnvironmentTelemetryModule:: requestImmediateSend() hook. There's no fixed debounce on the request itself - EnvironmentTelemetryModule's existing airtime/duty-cycle gate already paces every send, so it sends as often as airtime allows rather than an arbitrary fixed rate. The request does expire after 5 minutes unfulfilled, so it can't fire an arbitrarily stale broadcast if airtime was blocked for a long stretch. The AS3935's I2C addresses (0x01-0x03) fall inside the range this codebase's I2C scanner otherwise skips as reserved, so detection is a small dedicated probe gated behind AS3935_IRQ and respecting the caller's address filter, rather than a change to the general scan loop. Presence is confirmed via a register write/readback round-trip rather than a fixed expected value, since the AS3935 has no WHOAMI register and a power-on-reset-only check can't survive a warm reboot that doesn't power-cycle the sensor (initDevice() permanently rewrites that register on first configuration). Generated files under src/mesh/generated/ are intentionally excluded from this commit - they're regenerated from the protobufs submodule by update_protobufs.yml, and hand edits get overwritten and conflict once the companion protobufs PR merges and the submodule pointer updates. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>
e72e437 to
a0fac16
Compare
|
I'm trying to find available hardware for this, but not lucky yet. Have you got any recommendations? I'm curious about how to test this reliably too... |
Seems like the main options are:
I’m waiting for the purple board to make its way here. It’s pretty expensive and I don’t have a use for it if Meshtastic support doesn't land, so I went for the cheap, slow shipping. |
|
Hoping this ends up getting merged in, I've wanted to use these on sensor-role nodes for a while now! |
are you able to build and test? i'm still waiting for my breakout board to arrive |
Unfortunately not yet, as I'm also waiting on a board. I had held out on purchasing originally due to cost of the DFRobot board, but decided to get one after seeing the much cheaper purple board pop up on AliExpress. |
4 participants
What does this PR do?
Implements #10774: support for the AS3935 Franklin lightning-detection IC as an environment telemetry sensor.
Rather than inventing a new "on-demand" sensor/packet type (as discussed in the issue), this reuses the existing
TelemetrySensorscheduling infrastructure:lightning_strike_count_1h/lightning_distance_kmon the normal environment telemetry broadcast, the same way the DFRobot rain gauge reportsrainfall_1h. The window is read non-destructively, so a peer's telemetry request answered between broadcasts can't silently zero out counted strikes.EnvironmentTelemetryModule::requestImmediateSend()hook. There's no fixed debounce on the request itself — the module's existing airtime/duty-cycle gate already paces every send, so it goes out as often as airtime allows rather than at an arbitrary fixed rate. The request does expire after 5 minutes unfulfilled, so it can't fire an arbitrarily stale broadcast if airtime was blocked for a long stretch.Design notes
AS3935_IRQdefine, not yet set on any board) is polled with a plaindigitalRead()inrunOnce(), notattachInterrupt(). This is deliberate, not an oversight — see "Why polling, not an interrupt?" below.initDevice()only writes configuration oncebegin()has actually succeeded, so a device that's detected but fails to initialize isn't left in a half-configured state.ScanI2CTwoWire.cpp, gated behindAS3935_IRQand respecting the caller's address filter rather than widening the general scan loop for every board.initDevice()permanently rewrites that register on first configuration).Throttle::isWithinTimespanMshelper already used throughoutEnvironmentTelemetry.cpp— no new throttling mechanism.Why polling, not an interrupt?
It'd be reasonable to expect an interrupt-capable pin to use
attachInterrupt(), so this is worth calling out explicitly: it wouldn't buy anything here, for two independent reasons.Example1_BasicLightning_I2C,Example2_More_Lightning_Features_I2Cboth use plaindigitalRead()in a loop, neverattachInterrupt()).readInterruptReg()), which itself callsdelay(2)per the datasheet's settle-time requirement — and blocking I2C/delay()calls are not safe to make from ISR context on any of this codebase's target platforms (esp32/nrf52/rp2xx0). So the ISR could only ever set a flag forrunOnce()to drain later — at which point it has done no less work than just checkingdigitalRead()directly on the next tick.Event flow
flowchart TD subgraph AS["AS3935Sensor::runOnce() (polled ~1s)"] CHECKPIN{"digitalRead(AS3935_IRQ)\n== HIGH?"} CLASSIFY["classifyPendingIrq()\nreadInterruptReg() over I2C"] REASON{"what did the\nregister report?"} LOGNOISE["log only"] LOGDIST["log only, ignored"] COUNT["strikeCountWindow++\ndistanceToStorm()"] DISTVALID{"distance valid?\n(not out-of-range)"} SAVEDIST["lastDistanceKm = distance"] SKIPDIST["distance stays unknown"] PUSHCALL["requestImmediateSend()\n(no fixed debounce)"] WINDOWCHECK{"~1h strike window\nelapsed?"} RESET["reset strikeCountWindow\n+ lastDistanceKm"] TICKDONE(("next tick")) CHECKPIN -->|"yes"| CLASSIFY CHECKPIN -->|"no"| WINDOWCHECK CLASSIFY --> REASON REASON -->|"noise floor too high"| LOGNOISE REASON -->|"disturber"| LOGDIST REASON -->|"lightning"| COUNT COUNT --> DISTVALID DISTVALID -->|"yes"| SAVEDIST DISTVALID -->|"no"| SKIPDIST SAVEDIST --> PUSHCALL SKIPDIST --> PUSHCALL PUSHCALL --> WINDOWCHECK LOGNOISE --> WINDOWCHECK LOGDIST --> WINDOWCHECK WINDOWCHECK -->|"yes"| RESET WINDOWCHECK -->|"no"| TICKDONE RESET --> TICKDONE end subgraph ETM["EnvironmentTelemetryModule (existing, unchanged scheduling)"] FLAG["immediateSendRequested = true\n(timestamped)"] STALE{"5 min elapsed\nsince requested?"} GIVEUP(("expire; counter still\nreported next normal broadcast")) AIRTIME{"airtime gate:\nisTxAllowedChannelUtil()\nand isTxAllowedAirUtil()?"} SEND["getMetrics()\nbroadcast interval, phone sync,\nor reply to a peer's request"] REPORT["report lightning_strike_count_1h\n+ lightning_distance_km\n(non-destructive read)"] FLAG --> STALE STALE -->|"yes"| GIVEUP STALE -->|"no"| AIRTIME AIRTIME -->|"yes"| SEND AIRTIME -->|"no, retry\nnext module tick"| STALE SEND --> REPORT end PUSHCALL -.->|"sets flag on\nthe module"| FLAGDependencies
This depends on the companion protobuf schema change adding
lightning_strike_count_1h/lightning_distance_kmtoEnvironmentMetricsandAS3935toTelemetrySensorType:This firmware PR should stay a draft until that protobufs PR merges and the submodule pointer here is updated to the merged commit.
Testing locally before the protobufs PR merges: this branch's submodule pointer already points at the commit adding the
AS3935sensor type and lightning fields, but the generatedsrc/mesh/generated/files are intentionally not committed here (they'd conflict with the real regenerated output once meshtastic/protobufs#981 merges and the pointer is updated — see review discussion below). To build this branch locally: (1)git submodule update --init protobufs; (2) download the nanopb 0.4.9 prebuilt tool for your OS from https://jpa.kapsi.fi/nanopb/download/ into ananopb-0.4.9/directory at the repo root; (3) runbin/regen-protos.shto regeneratesrc/mesh/generated/; (4) build as normal, e.g.pio run -e rak4631.Board support
No board currently defines
AS3935_IRQ— the sensor is entirely opt-in. A board wanting to support this sensor adds#define AS3935_IRQ <gpio>to itsvariants/<board>/variant.h.🤝 Attestations
rak4631(nRF52) firmware build;trunk fmtclean; and direct inspection of the SparkFun_AS3935 Arduino library source (register semantics, constructor/begin signatures, POR defaults) to avoid guessing at the datasheet../bin/run-tests.sh) currently reports RED in my dev environment, but that reproduces identically on the unmodified base branch (pre-existing-fprofile-abs-pathclang/toolchain mismatch, unrelated to this change).AS3935_IRQ, so this feature can't regress any existing board's behavior — it's inert until a board opts in.Closes #10774.
Summary by CodeRabbit
New Features
Bug Fixes