Conversation
gpsResetPin was uint32_t and assigned from a truth test:
static uint32_t gpsResetPin = 0;
...
if(PIN_GPS_EN){ gpsResetPin = PIN_GPS_EN; }
Two defects fall out of that. PIN_GPS_EN is -1 on boards with no enable
pin, which passes the truth test and stores 4294967295 in an unsigned
pin variable; start_gps() and stop_gps() then call pinMode() and
digitalWrite() with it on every gps on/off. And 0 is a legitimate pin
number that the same test rejects, so a board wiring the enable line to
pin 0 would silently never store it.
Today the first case is harmless only because the core discards it:
cores/nRF5/wiring_digital.c checks 'if (ulPin >= PINS_COUNT) return;'
before use. Relying on that is fragile, and it leaves start_gps() and
stop_gps() silently doing nothing on RAK4631 with no indication why.
Make gpsResetPin signed with -1 meaning 'no enable pin', test the macro
with >= 0 so -1 is excluded and 0 is accepted, and guard both toggles so
the no-pin case is explicit and logged rather than relying on a bounds
check further down.
Behaviour is unchanged on every board in tree. rak4631 is the only
variant defining PIN_GPS_EN=-1, and it goes from an out-of-range write
that the core drops to an explicit skip. The gat562 boards define 33 and
are unaffected, rak3401 is excluded by the existing ifndef, and no
variant uses pin 0 today, so that part is a latent fix.
Deliberately not changed: the serial branch does not fall back to the
ioPin that woke the receiver, the way the I2C branch does. On RAK4631
that pin is WB_IO2, the shared 3V3_S slot rail, so reusing it would make
stop_gps() power down the RTC and every other slot module. Whether a GPS
toggle should drive a shared rail is a design question for maintainers,
not something to change here.
Verified on RAK4631 + RAK12002: 'gps off' and 'gps on' log the new
no-enable-pin line, the CLI reply is unchanged, and the clock keeps
running across both, confirming the slot rail is untouched. Build
checked on RAK_4631_repeater and
GAT562_Mesh_Tracker_Pro_companion_radio_usb, the two RAK_WISBLOCK_GPS
board families that differ here.
|
Warning Review limit reachedNext included review available in 13 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes are small, well-scoped, and concretely prevent invalid pin operations without altering the intended GPS power behavior for the affected boards.
Pull request overview
This PR fixes GPS enable-pin handling on RAK WisBlock GPS builds by treating the enable pin as a pin number (with -1 meaning “no pin”) instead of relying on truthiness, which previously mis-handled PIN_GPS_EN == -1 and would also reject a legitimate pin 0.
Changes:
- Change
gpsResetPinfromuint32_tto signedintand initialize it to-1to represent “no enable pin”. - Replace the
if (PIN_GPS_EN)truth test withif (PIN_GPS_EN >= 0)so-1is excluded and0is accepted. - Guard
start_gps()/stop_gps()pin toggling ongpsResetPin >= 0and log an explicit message when no enable pin is configured.
File summaries
| File | Description |
|---|---|
| src/helpers/sensors/EnvironmentSensorManager.cpp | Corrects GPS enable-pin storage and guards pin toggling to avoid invalid pin writes (notably when PIN_GPS_EN == -1). |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Iteration branch for the GPS enable-pin fix. Targets my own fork's
devfor reviewbefore a squashed version goes upstream as a draft. Not intended to merge here.
Raised by a reviewer on meshcore-dev#3423 (upstream). Kept separate because it changes pin
handling rather than reporting.
Problem
A truth test on a pin number, assigned into an unsigned variable. Two defects:
PIN_GPS_ENis-1on boards with no enable pin.if(-1)passes, sogpsResetPinbecomes 4294967295.start_gps()/stop_gps()then callpinMode()anddigitalWrite()with that on everygps on/gps off.0is a legitimate pin number that the same test rejects, so a board wiringthe enable line to pin 0 would silently never store it.
Case 1 is harmless today only because the core drops it
(
cores/nRF5/wiring_digital.c:28-32):Relying on that is fragile, and it leaves
start_gps()/stop_gps()silentlyineffective on RAK4631 with nothing to say why.
Change
gpsResetPinbecomesint, initialised to-1meaning "no enable pin".>= 0, excluding-1and accepting0.Behaviour is unchanged on every board in tree
PIN_GPS_ENrak4631-1gat562_*33rak3401#ifndef RAK_34010rak4631is the only variant in the tree definingPIN_GPS_EN=-1, and no variant usespin 0, so that part is a latent fix.
Deliberately NOT changed
The reviewer on meshcore-dev#3423 suggested "always setting
gpsResetPinto the testedioPin,and only overriding it when
PIN_GPS_ENis present and not-1".I did not do that, and I think it would be a regression. On RAK4631 the pin that wakes
the receiver is
WB_IO2(pin 34), the shared 3V3_S WisBlock slot rail, confirmed onhardware:
stop_gps()drivesgpsResetPinLOW, and it is called at boot on stock firmware viathe
#ifndef FORCE_GPS_ALIVEpath. So storingioPinthere would pull the slot raillow on every boot, powering down the RAK12002 RTC and anything else in the slots.
Whether a GPS toggle should drive a shared rail is a design question for maintainers.
This PR keeps the current power behaviour exactly and only makes the pin handling
honest.
Testing
Hardware, RAK4631 + RAK12002:
The clock still runs across both toggles, confirming the slot rail is untouched.
Builds:
RAK_4631_repeaterandGAT562_Mesh_Tracker_Pro_companion_radio_usb, the twoRAK_WISBLOCK_GPSfamilies that differ in this code.Review checklist
start_gps()/stop_gps()fall through to_location->begin()/stop()as the non-RAK pathdoes?
intvsint8_tfor a pin number;intmatches the-1sentinel style used byGPS_ENelsewhere.overlapping hunks, but one will need a trivial rebase if the other merges first.