From 905482ccceba58af9c8fdb3b67008a756ecff779 Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Fri, 14 Aug 2026 10:41:44 -0500 Subject: [PATCH] fix(serial): don't sleep forever with pending PhoneAPI output on UART consoles (#11500) * fix(serial): don't sleep forever with pending PhoneAPI output on UART consoles Since #11164 bounded the stream drain, a config dump can end a dispatch with output still queued. On UART-console ESP32 boards runOnce() then returns INT32_MAX with no RX pending, and neither rxInt() nor onNowHasData() fires for the remaining output, so the download wedges mid nodeinfo stream until the client happens to send a byte. Add StreamAPI::hasPendingOutput() (transport-retained frame or queued PhoneAPI data) and have SerialConsole::runOnce() short-poll (<=25ms) while it holds instead of sleeping INT32_MAX. The #11164 write budget is unchanged; idle sleep behavior with a drained queue is unchanged. The retained-frame probe also covers the ESP32-S2 USB-CDC branch, which takes the same INT32_MAX path. * test(serial): restore scratch NodeDB via tearDown, trim comments to house style A failed TEST_ASSERT longjmps out of a Unity test without running destructors, so RAII cannot restore the swapped nodeDB pointer; install the scratch NodeDB explicitly and restore/delete it in tearDown(), which runs after every test outcome. Also shorten the new comments to the two-line house limit. --- src/SerialConsole.cpp | 15 ++++++ src/SerialConsole.h | 2 + src/mesh/StreamAPI.cpp | 6 +++ src/mesh/StreamAPI.h | 6 +++ test/test_stream_api/test_main.cpp | 86 ++++++++++++++++++++++++++++-- 5 files changed, 112 insertions(+), 3 deletions(-) diff --git a/src/SerialConsole.cpp b/src/SerialConsole.cpp index a406fcd0dd..24141be28e 100644 --- a/src/SerialConsole.cpp +++ b/src/SerialConsole.cpp @@ -125,6 +125,10 @@ int32_t SerialConsole::runOnce() int32_t delay = runOncePart(); #if defined(SERIAL_HAS_ON_RECEIVE) || defined(CONFIG_IDF_TARGET_ESP32S2) + // Nothing wakes the idle sleep for "TX space freed" or a bounded-drain remainder + // (#11164), so keep polling while the API holds undelivered output. + if (hasPendingOutput()) + return delay < 25 ? delay : 25; // 0 continues a budget slice; else short-poll TX drain return Port.available() ? delay : INT32_MAX; #elif defined(IS_USB_SERIAL) return HWCDC::isPlugged() ? delay : (1000 * 20); @@ -212,6 +216,17 @@ bool SerialConsole::finishPendingFrame() #endif } +/// Report a retained USB CDC frame awaiting TX space. +bool SerialConsole::hasRetainedFrame() +{ +#ifdef IS_USB_SERIAL + concurrency::LockGuard guard(&streamLock); + return !frameWriter.isIdle(); +#else + return false; +#endif +} + /// Protect the retained log buffer from being overwritten. bool SerialConsole::canEncodeLogRecord() { diff --git a/src/SerialConsole.h b/src/SerialConsole.h index eeed25644d..466d6afa96 100644 --- a/src/SerialConsole.h +++ b/src/SerialConsole.h @@ -51,6 +51,8 @@ class SerialConsole : public StreamAPI, public RedirectablePrint, private concur /// Continue retained USB CDC output before PhoneAPI advances. virtual bool finishPendingFrame() override; + /// Report a retained USB CDC frame awaiting TX space. + virtual bool hasRetainedFrame() override; /// Return whether the dedicated log buffer can be safely overwritten. virtual bool canEncodeLogRecord() override; /// Write or retain one framed USB CDC message. diff --git a/src/mesh/StreamAPI.cpp b/src/mesh/StreamAPI.cpp index e20434042c..412a9786a5 100644 --- a/src/mesh/StreamAPI.cpp +++ b/src/mesh/StreamAPI.cpp @@ -33,6 +33,12 @@ int32_t StreamAPI::runOncePart(char *buf, uint16_t bufLen) return result; } +/// Report undelivered output so idle-sleep decisions keep the drain alive. +bool StreamAPI::hasPendingOutput() +{ + return canWrite && (hasRetainedFrame() || available()); +} + /** * Read any rx chars from the link and call handleRecStream */ diff --git a/src/mesh/StreamAPI.h b/src/mesh/StreamAPI.h index c91da4d02f..7968972e1f 100644 --- a/src/mesh/StreamAPI.h +++ b/src/mesh/StreamAPI.h @@ -57,6 +57,10 @@ class StreamAPI : public PhoneAPI virtual int32_t runOncePart(); virtual int32_t runOncePart(char *buf, uint16_t bufLen); + /// True while undelivered output remains (retained frame or queued PhoneAPI data); callers + /// woken only by RX activity must keep polling while set, as drains stop mid-dump (#11164). + bool hasPendingOutput(); + /// Check the current underlying physical link to see if the client is currently connected virtual bool checkIsConnected() override = 0; @@ -104,6 +108,8 @@ class StreamAPI : public PhoneAPI /// Complete retained transport output before dequeuing another PhoneAPI packet. virtual bool finishPendingFrame() { return true; } + /// Return whether the transport retains an incomplete frame awaiting TX space. + virtual bool hasRetainedFrame() { return false; } /// Return whether the dedicated log buffer is available for encoding. virtual bool canEncodeLogRecord() { return true; } /// Frame and write a payload, optionally using best-effort admission. diff --git a/test/test_stream_api/test_main.cpp b/test/test_stream_api/test_main.cpp index 994e82c3d3..fdc87ab8e4 100644 --- a/test/test_stream_api/test_main.cpp +++ b/test/test_stream_api/test_main.cpp @@ -147,6 +147,26 @@ class PhoneAPITestShim : public PhoneAPI bool checkIsConnected() override { return true; } }; +/// Exposes the hasPendingOutput() inputs used by idle-sleep gating. +class PendingOutputStreamAPI : public StreamAPI +{ + public: + /// Construct the shim over a scripted stream. + explicit PendingOutputStreamAPI(Stream *stream) : StreamAPI(stream) {} + + /// Keep connection-timeout handling inactive during tests. + bool checkIsConnected() override { return true; } + + /// Set the transport-writability gate normally controlled by first client contact. + void setCanWrite(bool value) { canWrite = value; } + + bool retainedFrame = false; + + protected: + /// Report the scripted retained-frame state. + bool hasRetainedFrame() override { return retainedFrame; } +}; + /// Exposes framed-log hooks and records best-effort writes. class LogHookStreamAPI : public StreamAPI { @@ -538,7 +558,7 @@ static void queuePendingTimePlaceholderPacket(NodeNum from, uint32_t placeholder service->sendToPhone(packetPool.allocCopy(pending)); } -static void startHandshake(PhoneAPITestShim &api) +static void startHandshake(PhoneAPI &api) { meshtastic_ToRadio request = meshtastic_ToRadio_init_zero; request.which_payload_variant = meshtastic_ToRadio_want_config_id_tag; @@ -566,6 +586,58 @@ static bool drainHandshakeForPacketFrom(PhoneAPITestShim &api, NodeNum from, mes return false; } +// Scratch NodeDB for the config-dump stream; restored by tearDown() rather than RAII +// because a failed TEST_ASSERT longjmps out of the test without running destructors. +static NodeDB *scratchNodeDB = nullptr; +static NodeDB *savedNodeDB = nullptr; + +/// Install a scratch NodeDB; tearDown() restores the previous one after any test outcome. +static void installScratchNodeDB() +{ + savedNodeDB = nodeDB; + scratchNodeDB = new NodeDB(); + nodeDB = scratchNodeDB; +} + +// SerialConsole::runOnce gates its INT32_MAX idle sleep on hasPendingOutput(): pending while +// output is queued or retained (#11164 bounded drain), clear when drained or pre-contact. +static void test_stream_api_pending_output_tracks_queue_and_retained_frame(void) +{ + ScopedMeshService scopedService; + installScratchNodeDB(); + ScriptedStream stream; + PendingOutputStreamAPI api(&stream); + + // Nothing queued and no client yet: an idle console must be allowed to sleep. + TEST_ASSERT_FALSE(api.hasPendingOutput()); + + // A client that has not yet spoken (canWrite false) must not force polling, + // even with a full config dump queued behind the gate. + startHandshake(api); + api.setCanWrite(false); + TEST_ASSERT_FALSE(api.hasPendingOutput()); + + // Once writable, the queued dump is pending output until fully drained. + api.setCanWrite(true); + TEST_ASSERT_TRUE(api.hasPendingOutput()); + unsigned drained = 0; + for (unsigned i = 0; i < 512 && api.hasPendingOutput(); ++i) { + uint8_t responseBytes[meshtastic_FromRadio_size]; + if (api.getFromRadio(responseBytes) != 0) + drained++; + } + TEST_ASSERT_GREATER_THAN_UINT(0, drained); + TEST_ASSERT_FALSE_MESSAGE(api.hasPendingOutput(), "pending output must clear once the dump is drained"); + + // A transport-retained partial frame alone keeps the drain alive. + api.retainedFrame = true; + TEST_ASSERT_TRUE(api.hasPendingOutput()); + api.retainedFrame = false; + TEST_ASSERT_FALSE(api.hasPendingOutput()); + + api.close(); +} + /// Swaps in a scratch NodeDB and the injected clock, restoring both plus the RTC on destruction. /// Unity's TEST_ASSERT longjmps out on failure, so cleanup must not live at the end of the test. class ScopedTimeFixture @@ -715,8 +787,15 @@ static void test_node_heard_during_first_uptime_second_gets_last_heard_backfille /// Unity per-test setup; fixtures are local to each test. void setUp(void) {} -/// Unity per-test teardown; fixtures clean themselves up. -void tearDown(void) {} +/// Unity per-test teardown; restores state that a failed assert's longjmp would leak. +void tearDown(void) +{ + if (scratchNodeDB) { + nodeDB = savedNodeDB; + delete scratchNodeDB; + scratchNodeDB = nullptr; + } +} /// Initialize the native environment and run the stream regression suite. void setup() @@ -735,6 +814,7 @@ void setup() RUN_TEST(test_lockdown_admin_gate_ignores_wire_from); RUN_TEST(test_lockdown_admin_gate_rejects_undecodable_admin); RUN_TEST(test_want_config_includes_status_message_module_config); + RUN_TEST(test_stream_api_pending_output_tracks_queue_and_retained_frame); RUN_TEST(test_time_given_at_handshake_start_reconciles_queued_packet); RUN_TEST(test_time_given_at_handshake_end_does_not_rewrite_already_sent_packet); RUN_TEST(test_node_heard_before_time_gets_last_heard_backfilled);