mirror of
https://github.com/alexhopeoconnor/firmware.git
synced 2026-10-13 15:11:20 +10:00
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.
This commit is contained in:
@@ -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()
|
||||
{
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
*/
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user