fix(extnotif): make isNagging the only armed flag for the nag cycle (#11828)

* fix(extnotif): make isNagging the only armed flag for the nag cycle

ExternalNotificationModule kept the nag cycle's armed state in two places that
could disagree: the isNagging bool, and nagCycleCutoff reserving UINT32_MAX for
"not armed". handleInputEvent() read only the second one:

    if (nagCycleCutoff != UINT32_MAX) { stopNow(); return 1; }

The field is declared `= 1`, while isNagging starts false, so at boot that test
said "armed" when nothing was nagging. The first input event of every boot was
therefore answered with stopNow() and a non-zero return - and a non-zero return
ends the observer chain (Observable::notifyObservers in src/Observer.h returns on
the first one), so that event was swallowed from every later observer. The handler
is registered whenever external_notification.enabled, and InputBroker only
short-circuits while nagging() is true, so the event does reach it.

The same read had a second failure mode once per ~49.7-day wrap: armNagCycle()
computes `millis() + durationMs`, which can land exactly on UINT32_MAX. When it
does, a real nag is running with isNagging true, but this read says "not armed" and
the module's own handler never stops it. Time::skipZero() cannot help here - it
lifts 0 to 1 and leaves UINT32_MAX alone, which src/UptimeClock.h static_asserts.

So the fix is not a zero guard, it is removing the second opinion. isNagging is
the armed flag - which is what the comment above the expiry check already claimed,
and what the other four reads already use - and nagCycleCutoff is now only ever a
deadline, read after isNagging has been checked. Nothing reserves a value, which
matters because an arm site spelled `millis() + interval` can produce any value
there is, so no value is safe to reserve. That is the shape the TODO(deadline-type)
note in src/mesh/Throttle.h is aiming at, and that note is updated to match rather
than keep describing the sentinel this removes.

Worth knowing for review, though not changed here: InputBroker::handleInputEvent
already calls stopNow() itself when nagging() is true, and returns without
notifying observers. Every path that starts a notification calls armNagCycle()
first, so isNagging is true for the whole life of any real nag. That makes this
handler reachable only when there is nothing to stop - its stopNow() was never
doing useful work. Gated rather than deleted, because removing a public handler
and its observer registration is a bigger call than fixing the defect.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* style(extnotif): trim comments to the house limit

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: nomdetom <nomdetom@protonmail.com>

src/mesh/Throttle.h

@@ -32,7 +32,7 @@ class Throttle
     /// Deadline::in(ms) / .armed() / .passed() / .disarm(). A hand-built `now + interval` could then
     /// no longer land on the sentinel by accident, and "armed" would stay a question separate from
     /// "passed" - the split that has to survive, because which way "inactive" falls is the caller's
-    /// to decide. Same size and cost as the bare uint32_t. The conversion sites, grouped by the four
+    /// to decide. Same size and cost as the bare uint32_t. The conversion sites, grouped by the three
     /// meanings they give the sentinel today:
     ///   0 = unarmed - Power.cpp rebootAtMsec/shutdownAtMsec, GPS.cpp fixHoldEnds, AdminModule.cpp
     ///                 enterDfuAtMsec and the other timerEndsAtMillis()/skipZero() arm sites dodge
@@ -41,8 +41,8 @@ class Throttle
     ///                 guard, so this third state wants naming rather than repeating.
     ///   0 = due now - ethClient.cpp ntp_renew, forced at link-up. A computed renewal now dodges 0,
     ///                 so only a deliberate write still means "due now".
-    ///   UINT32_MAX  - ExternalNotificationModule.cpp nagCycleCutoff, whose armed() also lives in a
-    ///                 second variable (isNagging) and whose arm site can land on the sentinel.
+    /// ExternalNotificationModule.cpp nagCycleCutoff reserves nothing: isNagging is the only armed
+    /// flag and the deadline is read only while it is set.
     static bool deadlinePassed(uint32_t deadlineMs);
 
     /// deadlinePassed() against a caller-supplied "now", for a loop that snapshots the time once and

src/modules/ExternalNotificationModule.cpp

@@ -88,12 +88,11 @@ int32_t ExternalNotificationModule::runOnce()
 #if defined(HAS_I2S_SPEAKER_NRF52)
         isRtttlPlaying = isRtttlPlaying || nrf52RtttlPlayer.isPlaying();
 #endif
-        // isNagging is the armed flag; nagCycleCutoff holds a real deadline only while it is set
-        // (UINT32_MAX once stopped, 1 at boot), so short-circuit before the comparison.
+        // isNagging is the armed flag; nagCycleCutoff is only a deadline while it is set, so
+        // short-circuit before the comparison. `millis() + durationMs` can land on any value.
         const bool nagWindowExpired = !isNagging || Throttle::deadlinePassed(nagCycleCutoff);
         if (nagWindowExpired && !isRtttlPlaying) {
             // Turn off external notification immediately when timeout is reached, regardless of song state
-            nagCycleCutoff = UINT32_MAX;
             ExternalNotificationModule::stopNow();
             isNagging = false;
             return INT32_MAX; // save cycles till we're needed again
@@ -309,9 +308,9 @@ void ExternalNotificationModule::stopNow()
 #endif
 
     // Prevent the state machine from immediately re-triggering outputs after a manual stop.
+    // Clearing isNagging disarms the cycle; nagCycleCutoff is never read without it.
     isNagging = false;
     buzzerShouldAlert = false;
-    nagCycleCutoff = UINT32_MAX;
 
 #ifdef HAS_I2S
     // GPIO0 is used as mclk for I2S audio and set to OUTPUT by the sound library
@@ -624,7 +623,9 @@ void ExternalNotificationModule::handleSetRingtone(const char *from_msg)
 #if !MESHTASTIC_EXCLUDE_INPUTBROKER
 int ExternalNotificationModule::handleInputEvent(const InputEvent *event)
 {
-    if (nagCycleCutoff != UINT32_MAX) {
+    // Testing the deadline instead of isNagging was true at boot, and the non-zero return
+    // swallowed the first input event from every later observer.
+    if (isNagging) {
         stopNow();
         return 1;
     }

src/modules/ExternalNotificationModule.h

@@ -64,7 +64,9 @@ class ExternalNotificationModule : public SinglePortModule, private concurrency:
     int handleInputEvent(const InputEvent *arg);
 #endif
 
-    uint32_t nagCycleCutoff = 1;
+    /// When the current nag cycle ends. Meaningful only while isNagging is set; never test it for a
+    /// magic value.
+    uint32_t nagCycleCutoff = 0;
 
     void setExternalState(uint8_t index = 0, bool on = false);
     bool getExternal(uint8_t index = 0);

test/test_throttle/test_main.cpp

@@ -148,8 +148,12 @@ void test_deadlinePassed_reads_disarmed_sentinels_as_passed()
 {
     Time::setTestMillis(6247);
 
-    TEST_ASSERT_TRUE(Throttle::deadlinePassed(0));          // "inactive" for rebootAtMsec et al
-    TEST_ASSERT_TRUE(Throttle::deadlinePassed(UINT32_MAX)); // "inactive" for nagCycleCutoff
+    TEST_ASSERT_TRUE(Throttle::deadlinePassed(0)); // "inactive" for rebootAtMsec et al
+
+    // UINT32_MAX is not a usable "far future" either - at a low uptime it is a hair BEHIND now, so
+    // it reads as passed like any other past value. ExternalNotificationModule used to reserve it
+    // for "unarmed" and now keeps that state in its isNagging flag instead.
+    TEST_ASSERT_TRUE(Throttle::deadlinePassed(UINT32_MAX));
 
     // The guarded form every caller must use.
     const uint32_t disarmed = 0;