Skip to content

Commit 593166b

Browse files
NomDeTomNomDeTom
committed
gps-toggle-noreboot
1 parent 45d89b7 commit 593166b

3 files changed

Lines changed: 148 additions & 15 deletions

File tree

‎docs/admin-config-save-gating.md‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,23 @@ directly from `config` each send/schedule cycle:
117117
| `broadcast_smart_minimum_distance` | No | Read live in the smart-position path |
118118
| `position_flags` | No | Read live per outgoing position |
119119
| `fixed_position` | No | Read live; also has dedicated live admin handlers |
120-
| `gps_mode`, `gps_enabled` | **Yes** | GPS driver state |
120+
| `gps_mode` (`ENABLED` ↔ `DISABLED`) | No | Driver toggled live, as the menus do (see below) |
121+
| `gps_mode` (any `NOT_PRESENT` transition) | **Yes** | Driver object only exists from boot |
122+
| `gps_enabled` | **Yes** | GPS driver state |
121123
| `gps_update_interval`, `gps_attempt_time` | **Yes** | GPS subsystem timing |
122124
| `rx_gpio`, `tx_gpio`, `gps_en_gpio` | **Yes** | GPIO pin (re)assignment |
123125

124126
The gate neutralizes the live fields in a copy and reboots if any _other_ byte differs, so a
125127
newly-added `PositionConfig` field reboots until it is explicitly cleared as live - fail-safe
126128
for schema growth.
127129

130+
`gps_mode` is neutralized conditionally, by `isLiveGpsModeToggle()`, and only because the same
131+
case then calls `gps->enable()`/`gps->disable()` - the pattern `MenuHandler::GPSToggleMenu()`
132+
and InkHUD's `TOGGLE_GPS` have always used. Suppressing the reboot **without** driving the
133+
driver would leave the receiver in its old state until the next boot. `NOT_PRESENT` stays on
134+
the reboot path in both directions: `gps` is constructed once at boot and only when `gps_mode`
135+
is not `NOT_PRESENT` ([`main.cpp`](../src/main.cpp)), so there is nothing to enable.
136+
128137
---
129138

130139
## Edit transactions - where both axes nearly got lost
@@ -330,8 +339,14 @@ reconfigure that appears only at the commit, not during the individual sets.
330339
### 2. Position live-apply - expanding the Tier-2 live set (outstanding)
331340
332341
The only reason to touch a node for the _reboot_ work: decide whether the GPS-timing fields
333-
left on the reboot path (`gps_mode`, `gps_enabled`, `gps_update_interval`, `gps_attempt_time` -
334-
item 3 above) can actually apply live and be reclassified.
342+
left on the reboot path (`gps_enabled`, `gps_update_interval`, `gps_attempt_time` - item 3
343+
above) can actually apply live and be reclassified.
344+
345+
`gps_mode` has already been reclassified for the `ENABLED` ↔ `DISABLED` pair, on the strength
346+
of the driver call now made alongside it. **That one still wants a hardware pass**: toggle GPS
347+
off and on from the app and confirm the receiver actually stops and restarts (fix lost, then
348+
reacquired) with no reboot banner - the failure mode to look for is config saying one thing
349+
while the receiver keeps doing the other.
335350
336351
For each candidate field, one at a time, on a GPS-equipped node: change only that field (over
337352
serial, as above) and observe.

‎src/modules/AdminModule.cpp‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -869,6 +869,16 @@ static void reconcileAccelerometerThread(bool wasOn, bool nowOn, bool otherFeatu
869869
}
870870
#endif
871871

872+
// Only ENABLED <-> DISABLED can be applied live: the GPS object is built once at boot, and only
873+
// when gps_mode is not NOT_PRESENT (main.cpp), so NOT_PRESENT transitions need the reboot.
874+
static bool isLiveGpsModeToggle(meshtastic_Config_PositionConfig_GpsMode from, meshtastic_Config_PositionConfig_GpsMode to)
875+
{
876+
auto runnable = [](meshtastic_Config_PositionConfig_GpsMode m) {
877+
return m == meshtastic_Config_PositionConfig_GpsMode_ENABLED || m == meshtastic_Config_PositionConfig_GpsMode_DISABLED;
878+
};
879+
return from != to && runnable(from) && runnable(to);
880+
}
881+
872882
// A "regenerate keys" client sends a blank SecurityConfig holding only the new private key, rather than the
873883
// config it read from us. Detect that shape - new private key, every other field at its proto default - so it
874884
// isn't mistaken for "and clear everything else".
@@ -956,17 +966,21 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
956966
config.has_position = true;
957967
// Reboot only when a field that can't be applied live changed. PositionModule reads these fields
958968
// directly from config every send/schedule cycle (verified in PositionModule.cpp), so changing
959-
// only them takes effect with no restart. Everything else - GPS driver mode/timing and GPIO pin
960-
// assignments - stays on the reboot path. Fails safe: neutralize the known-live fields in a copy,
969+
// only them takes effect with no restart; gps_mode joins them when the driver call below applies
970+
// it. Everything else - GPS timing and GPIO pin assignments - stays on the reboot path.
971+
// Fails safe: neutralize the known-live fields in a copy,
961972
// then reboot if any *other* byte differs, so a newly-added PositionConfig field reboots until it
962973
// is explicitly cleared as live here. See docs/admin-config-save-gating.md.
974+
const bool gpsToggledLive = isLiveGpsModeToggle(config.position.gps_mode, c.payload_variant.position.gps_mode);
963975
{
964976
meshtastic_Config_PositionConfig live = config.position, incoming = c.payload_variant.position;
965977
incoming.position_broadcast_secs = live.position_broadcast_secs;
966978
incoming.position_broadcast_smart_enabled = live.position_broadcast_smart_enabled;
967979
incoming.broadcast_smart_minimum_distance = live.broadcast_smart_minimum_distance;
968980
incoming.position_flags = live.position_flags;
969981
incoming.fixed_position = live.fixed_position;
982+
if (gpsToggledLive)
983+
incoming.gps_mode = live.gps_mode;
970984
requiresReboot = (memcmp(&live, &incoming, sizeof(live)) != 0);
971985
}
972986
// If we have turned off the GPS (disabled or not present) and we're not using fixed position,
@@ -981,6 +995,16 @@ void AdminModule::handleSetConfig(const meshtastic_Config &c, bool fromOthers)
981995
saveChanges(SEGMENT_NODEDATABASE, false, /*radioAffected=*/false);
982996
}
983997
config.position = c.payload_variant.position;
998+
#if !MESHTASTIC_EXCLUDE_GPS
999+
// Driving the driver is what earns the suppressed reboot: writing gps_mode alone leaves the
1000+
// receiver in its old state until next boot. As MenuHandler::GPSToggleMenu(), minus the beep.
1001+
if (gpsToggledLive && gps) {
1002+
if (config.position.gps_mode == meshtastic_Config_PositionConfig_GpsMode_ENABLED)
1003+
gps->enable();
1004+
else
1005+
gps->disable();
1006+
}
1007+
#endif
9841008
break;
9851009
} // case meshtastic_Config_position_tag
9861010
case meshtastic_Config_power_tag:
@@ -1936,7 +1960,7 @@ void AdminModule::expireStaleEditTransaction()
19361960
deferredRadioAffected = false;
19371961
// No reboot: the settings are already live in RAM and the client that would expect one is gone.
19381962
if (segments)
1939-
saveChanges(segments, /*shouldReboot=*/false, expiredRadio);
1963+
saveChanges(segments, false, expiredRadio);
19401964
flushChannelWarnings();
19411965
}
19421966

‎test/test_admin_radio/test_main.cpp‎

Lines changed: 103 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2385,14 +2385,102 @@ static void test_setConfigPosition_liveFieldChange_doesNotReboot()
23852385
TEST_ASSERT_EQUAL_UINT32(0, rebootAtMsec);
23862386
}
23872387

2388-
static void test_setConfigPosition_gpsModeChange_schedulesReboot()
2388+
// A GPIO reassignment is boot-only however it arrives: the pin is claimed during GPS construction.
2389+
static void test_setConfigPosition_gpioChangeStillReboots()
2390+
{
2391+
config.position.rx_gpio = 0; // known start state
2392+
rebootAtMsec = 0;
2393+
meshtastic_Config c = meshtastic_Config_init_zero;
2394+
c.which_payload_variant = meshtastic_Config_position_tag;
2395+
c.payload_variant.position = config.position;
2396+
c.payload_variant.position.rx_gpio = 17;
2397+
sendSetConfig(c);
2398+
2399+
TEST_ASSERT_NOT_EQUAL(0, rebootAtMsec);
2400+
}
2401+
2402+
// -----------------------------------------------------------------------
2403+
// gps_mode: ENABLED <-> DISABLED is applied live (AdminModule drives gps->enable()/disable(),
2404+
// as the on-device menus have always done), so it must not reboot. Every transition involving
2405+
// NOT_PRESENT still must, because the driver object only exists from boot.
2406+
// -----------------------------------------------------------------------
2407+
2408+
static void test_setConfigPosition_gpsEnableIsLive_doesNotReboot()
2409+
{
2410+
config.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_DISABLED; // known start state
2411+
rebootAtMsec = 0;
2412+
ConfigChangedCounter counter;
2413+
counter.observe(&service->configChanged);
2414+
2415+
meshtastic_Config c = meshtastic_Config_init_zero;
2416+
c.which_payload_variant = meshtastic_Config_position_tag;
2417+
c.payload_variant.position = config.position;
2418+
c.payload_variant.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_ENABLED;
2419+
sendSetConfig(c);
2420+
2421+
TEST_ASSERT_EQUAL_INT(meshtastic_Config_PositionConfig_GpsMode_ENABLED, (int)config.position.gps_mode); // persisted
2422+
TEST_ASSERT_EQUAL_UINT32(0, rebootAtMsec);
2423+
TEST_ASSERT_EQUAL_INT(0, counter.count); // and still no reason to touch the radio
2424+
}
2425+
2426+
static void test_setConfigPosition_gpsDisableIsLive_doesNotReboot()
2427+
{
2428+
config.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_ENABLED; // known start state
2429+
config.position.fixed_position = true; // keep the nested clearLocalPosition save out of this test
2430+
rebootAtMsec = 0;
2431+
2432+
meshtastic_Config c = meshtastic_Config_init_zero;
2433+
c.which_payload_variant = meshtastic_Config_position_tag;
2434+
c.payload_variant.position = config.position;
2435+
c.payload_variant.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_DISABLED;
2436+
sendSetConfig(c);
2437+
2438+
TEST_ASSERT_EQUAL_INT(meshtastic_Config_PositionConfig_GpsMode_DISABLED, (int)config.position.gps_mode);
2439+
TEST_ASSERT_EQUAL_UINT32(0, rebootAtMsec);
2440+
}
2441+
2442+
// The live exemption is exactly one pair. Declaring the GPS absent tears down state that only
2443+
// boot rebuilds, so it stays on the reboot path...
2444+
static void test_setConfigPosition_gpsToNotPresent_stillReboots()
2445+
{
2446+
config.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_ENABLED; // known start state
2447+
config.position.fixed_position = true;
2448+
rebootAtMsec = 0;
2449+
meshtastic_Config c = meshtastic_Config_init_zero;
2450+
c.which_payload_variant = meshtastic_Config_position_tag;
2451+
c.payload_variant.position = config.position;
2452+
c.payload_variant.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_NOT_PRESENT;
2453+
sendSetConfig(c);
2454+
2455+
TEST_ASSERT_NOT_EQUAL(0, rebootAtMsec);
2456+
}
2457+
2458+
// ...and so does the reverse: there is no driver to enable until a boot has constructed one.
2459+
static void test_setConfigPosition_gpsFromNotPresent_stillReboots()
2460+
{
2461+
config.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_NOT_PRESENT; // known start state
2462+
rebootAtMsec = 0;
2463+
meshtastic_Config c = meshtastic_Config_init_zero;
2464+
c.which_payload_variant = meshtastic_Config_position_tag;
2465+
c.payload_variant.position = config.position;
2466+
c.payload_variant.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_ENABLED;
2467+
sendSetConfig(c);
2468+
2469+
TEST_ASSERT_NOT_EQUAL(0, rebootAtMsec);
2470+
}
2471+
2472+
// The exemption is for gps_mode alone - it must not smuggle a genuinely boot-only field past the
2473+
// memcmp fail-safe when both change in the same set.
2474+
static void test_setConfigPosition_gpsToggleWithGpioChange_stillReboots()
23892475
{
23902476
config.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_DISABLED; // known start state
2477+
config.position.rx_gpio = 0;
23912478
rebootAtMsec = 0;
23922479
meshtastic_Config c = meshtastic_Config_init_zero;
23932480
c.which_payload_variant = meshtastic_Config_position_tag;
23942481
c.payload_variant.position = config.position;
23952482
c.payload_variant.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_ENABLED;
2483+
c.payload_variant.position.rx_gpio = 17;
23962484
sendSetConfig(c);
23972485

23982486
TEST_ASSERT_NOT_EQUAL(0, rebootAtMsec);
@@ -2536,10 +2624,11 @@ static void test_transaction_loraSet_stillReloadsRadioAtCommit()
25362624
}
25372625

25382626
// ...and the reboot axis on its own: a boot-only field inside a transaction reboots at the commit
2539-
// without dragging the radio reconfigure along with it.
2540-
static void test_transaction_gpsModeSet_reboots_butDoesNotReloadRadio()
2627+
// without dragging the radio reconfigure along with it. rx_gpio rather than gps_mode - the latter
2628+
// is applied live now, so it would no longer exercise the reboot axis at all.
2629+
static void test_transaction_gpioSet_reboots_butDoesNotReloadRadio()
25412630
{
2542-
config.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_DISABLED; // known start state
2631+
config.position.rx_gpio = 0; // known start state
25432632
rebootAtMsec = 0;
25442633
ConfigChangedCounter counter;
25452634
counter.observe(&service->configChanged);
@@ -2548,7 +2637,7 @@ static void test_transaction_gpsModeSet_reboots_butDoesNotReloadRadio()
25482637
meshtastic_Config c = meshtastic_Config_init_zero;
25492638
c.which_payload_variant = meshtastic_Config_position_tag;
25502639
c.payload_variant.position = config.position;
2551-
c.payload_variant.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_ENABLED;
2640+
c.payload_variant.position.rx_gpio = 17;
25522641
sendSetConfig(c);
25532642

25542643
TEST_ASSERT_EQUAL_UINT32(0, rebootAtMsec); // deferred until the commit
@@ -2676,14 +2765,14 @@ static void test_abandonedTransaction_loraSet_stillReloadsRadioOnExpiry()
26762765
// the client that asked for the restart is, by definition, gone.
26772766
static void test_abandonedTransaction_rebootingFieldDoesNotRebootOnExpiry()
26782767
{
2679-
config.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_DISABLED; // known start state
2768+
config.position.rx_gpio = 0; // a genuinely boot-only field, so the drop is a real drop
26802769
rebootAtMsec = 0;
26812770

26822771
sendBeginEdit();
26832772
meshtastic_Config c = meshtastic_Config_init_zero;
26842773
c.which_payload_variant = meshtastic_Config_position_tag;
26852774
c.payload_variant.position = config.position;
2686-
c.payload_variant.position.gps_mode = meshtastic_Config_PositionConfig_GpsMode_ENABLED;
2775+
c.payload_variant.position.rx_gpio = 17;
26872776
sendSetConfig(c);
26882777

26892778
testAdmin->ageEditTransaction();
@@ -2904,15 +2993,20 @@ void setup()
29042993
RUN_TEST(test_setConfigBluetooth_noopSet_doesNotReboot);
29052994
RUN_TEST(test_setConfigBluetooth_realChange_schedulesReboot);
29062995
RUN_TEST(test_setConfigPosition_liveFieldChange_doesNotReboot);
2907-
RUN_TEST(test_setConfigPosition_gpsModeChange_schedulesReboot);
2996+
RUN_TEST(test_setConfigPosition_gpioChangeStillReboots);
2997+
RUN_TEST(test_setConfigPosition_gpsEnableIsLive_doesNotReboot);
2998+
RUN_TEST(test_setConfigPosition_gpsDisableIsLive_doesNotReboot);
2999+
RUN_TEST(test_setConfigPosition_gpsToNotPresent_stillReboots);
3000+
RUN_TEST(test_setConfigPosition_gpsFromNotPresent_stillReboots);
3001+
RUN_TEST(test_setConfigPosition_gpsToggleWithGpioChange_stillReboots);
29083002

29093003
// Edit-transaction deferral (the path phone apps use)
29103004
RUN_TEST(test_transaction_favoriteNode_doesNotReloadRadioAtCommit);
29113005
RUN_TEST(test_transaction_toggleMutedNode_doesNotReloadRadioAtCommit);
29123006
RUN_TEST(test_transaction_moduleConfigSet_doesNotReloadRadioAtCommit);
29133007
RUN_TEST(test_transaction_positionGpsOffNestedSave_doesNotReloadRadioAtCommit);
29143008
RUN_TEST(test_transaction_loraSet_stillReloadsRadioAtCommit);
2915-
RUN_TEST(test_transaction_gpsModeSet_reboots_butDoesNotReloadRadio);
3009+
RUN_TEST(test_transaction_gpioSet_reboots_butDoesNotReloadRadio);
29163010
RUN_TEST(test_transaction_liveOnlyBatch_neitherRebootsNorReloads);
29173011
RUN_TEST(test_transaction_flagsDoNotLeakIntoTheNextTransaction);
29183012
RUN_TEST(test_commitWithoutBegin_persistsButDoesNotReloadOrReboot);

0 commit comments

Comments
 (0)