Fix serial protobuf corruption on short USB CDC writes (#10976)

* Fix serial protobuf corruption on short writes

SerialConsole shared raw debug output and framed protobuf traffic on one
HWCDC stream. Raw text could interleave inside an active frame when API
logging was disabled. Separately, HWCDC deliberately returns a short write
after bounded backpressure; StreamAPI abandoned that frame after PhoneAPI
had already advanced, so the next 0x94c3 header landed inside the previous
declared payload.

Suppress all unframed output after protobuf mode starts and honor the
existing config-replay log pause. For HWCDC, retain short frame tails in the
persistent tx buffers and finish them on later loop passes before dequeuing
another FromRadio packet. Logs remain best-effort and only start when their
complete frame fits; main packets are deferred rather than dropped if a
synchronous log is pending. Do not call HWCDC flush for framed serial output,
since its no-progress path can discard queued bytes. TCP and non-HWCDC
transports keep their existing behavior.

Validated on Cardputer ADV (200-node DB) and Heltec Tracker V2: 400 initial
full-DB sessions plus 140 final sessions across both API-log settings, zero
malformed frames/timeouts/incomplete DBs; 2-20s forced reader stalls resume
with complete DBs; abrupt stalled-client replacement 5/5 per board; 150s
post-close reboot counts stable. Builds pass for Cardputer, Heltec, tbeam,
and rak4631.

* Add serial frame continuation regression tests

Extract the HWCDC pending/deferred frame state machine into a small
transport-independent helper so native tests exercise the same production
logic used by SerialConsole. Keep framing, persistent buffer ownership and
locking in SerialConsole.

Cover short-tail continuation, deferred main-frame ordering, best-effort log
admission, bounded zero-progress calls, generic StreamAPI failure semantics,
PhoneAPI advancement gating, framed-log gating and raw output suppression.
The coverage suite passes 31/31 suites and 582/582 tests.

* Address review: guard flush, assert deferred invariant, dedup framing

- Make SerialConsole::flush() a no-op in protobuf mode: HWCDC::flush()'s
  no-progress path discards queued TX bytes, which would tear a framed
  stream when the sleep path flushes with a stalled host.
- Assert the single required-frame producer invariant in
  StreamFrameWriter::writeFrame() instead of silently dropping a second
  required frame.
- Hoist 0x94C3 header construction into StreamAPI::buildFrameHeader() so
  SerialConsole no longer re-hardcodes the framing constants.

* Test retained serial tail across client replacement

Model a replacement client arriving while an older required frame has an
unwritten tail. Require the old frame to complete before the new frame starts,
so a new 0x94C3 header can never land inside the old declared payload.

* Document serial frame APIs and regression tests

Add concise Doxygen comments for the frame continuation hooks, production
helper, native test doubles, and regression scenarios. Document why retained
frame tails intentionally survive client disconnects: HWCDC may still hold the
accepted prefix, so dropping metadata could insert a new frame header inside
the old declared payload.
This commit is contained in:
p0ns
2026-07-11 06:27:50 -05:00
committed by GitHub
co-authored by GitHub
parent 8e27a8c715
commit ca833d944c
8 changed files with 675 additions and 30 deletions
+31 -9
View File
@@ -9,6 +9,7 @@
#define START2 0xc3
#define HEADER_LEN 4
/// Poll the underlying stream, drain output, and update connection state.
int32_t StreamAPI::runOncePart()
{
auto result = readStream();
@@ -17,6 +18,7 @@ int32_t StreamAPI::runOncePart()
return result;
}
/// Consume supplied input bytes, drain output, and update connection state.
int32_t StreamAPI::runOncePart(char *buf, uint16_t bufLen)
{
auto result = readStream(buf, bufLen);
@@ -48,6 +50,11 @@ int32_t StreamAPI::readStream(const char *buf, uint16_t bufLen)
void StreamAPI::writeStream()
{
if (canWrite) {
// A transport that retained a short frame must complete it before
// getFromRadio() advances the PhoneAPI state to the next packet.
if (!finishPendingFrame())
return;
uint32_t len;
do {
// Send every packet we can
@@ -58,6 +65,7 @@ void StreamAPI::writeStream()
}
}
/// Parse supplied bytes through the framed ToRadio receive state machine.
int32_t StreamAPI::handleRecStream(const char *buf, uint16_t bufLen)
{
uint16_t index = 0;
@@ -167,20 +175,27 @@ int32_t StreamAPI::readStream()
}
}
/// Encode the stream marker and big-endian payload length.
size_t StreamAPI::buildFrameHeader(uint8_t *buf, size_t payloadLen)
{
buf[0] = START1;
buf[1] = START2;
buf[2] = (payloadLen >> 8) & 0xff;
buf[3] = payloadLen & 0xff;
return payloadLen + HEADER_LEN;
}
/**
* Send the current txBuffer over our stream
*/
bool StreamAPI::writeFrame(uint8_t *buf, size_t len)
/// Write one framed payload using the transport's failure semantics.
bool StreamAPI::writeFrame(uint8_t *buf, size_t len, bool bestEffort)
{
(void)bestEffort;
if (len == 0 || !canWrite)
return false;
buf[0] = START1;
buf[1] = START2;
buf[2] = (len >> 8) & 0xff;
buf[3] = len & 0xff;
auto totalLen = len + HEADER_LEN;
const size_t totalLen = buildFrameHeader(buf, len);
// Serialize write-readiness checks, writes and write-failure handling
// against concurrent stream writes/close.
concurrency::LockGuard guard(&streamLock);
@@ -197,11 +212,13 @@ bool StreamAPI::writeFrame(uint8_t *buf, size_t len)
return false;
}
/// Emit the prepared main PhoneAPI payload as required output.
bool StreamAPI::emitTxBuffer(size_t len)
{
return writeFrame(txBuf, len);
return writeFrame(txBuf, len, false);
}
/// Emit the initial reboot notification as a framed FromRadio payload.
void StreamAPI::emitRebooted()
{
// In case we send a FromRadio packet
@@ -213,8 +230,13 @@ void StreamAPI::emitRebooted()
emitTxBuffer(pb_encode_to_bytes(txBuf + HEADER_LEN, meshtastic_FromRadio_size, &meshtastic_FromRadio_msg, &fromRadioScratch));
}
/// Encode and emit one protobuf LogRecord using the dedicated log buffers.
void StreamAPI::emitLogRecord(meshtastic_LogRecord_Level level, const char *src, const char *format, va_list arg)
{
// A retained short log frame still points into txBufLog, so do not overwrite it.
if (!canEncodeLogRecord())
return;
// IMPORTANT: do NOT touch `fromRadioScratch` or `txBuf` here - those
// belong to the main packet-emission path and a LOG_ firing during
// `writeStream()` would corrupt an in-flight encode. We keep a
@@ -237,7 +259,7 @@ void StreamAPI::emitLogRecord(meshtastic_LogRecord_Level level, const char *src,
size_t len =
pb_encode_to_bytes(txBufLog + HEADER_LEN, meshtastic_FromRadio_size, &meshtastic_FromRadio_msg, &fromRadioScratchLog);
writeFrame(txBufLog, len);
writeFrame(txBufLog, len, true);
}
/// Hookable to find out when connection changes
+14 -3
View File
@@ -91,12 +91,24 @@ class StreamAPI : public PhoneAPI
/// Low level function to emit a protobuf encapsulated log record
void emitLogRecord(meshtastic_LogRecord_Level level, const char *src, const char *format, va_list arg);
/// Return whether the transport can accept a frame of the requested size.
virtual bool canWriteFrame(size_t frameLen) { return true; }
/// Let transports recover from or close after an incomplete write.
virtual void onFrameWriteFailed(size_t frameLen, size_t writtenLen) {}
private:
bool writeFrame(uint8_t *buf, size_t len);
/// Fill in the 4-byte 0x94C3 length header; returns the total frame length.
static size_t buildFrameHeader(uint8_t *buf, size_t payloadLen);
/// Complete retained transport output before dequeuing another PhoneAPI packet.
virtual bool finishPendingFrame() { return true; }
/// 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.
virtual bool writeFrame(uint8_t *buf, size_t len, bool bestEffort);
concurrency::Lock streamLock;
private:
/// Dedicated scratch + tx buffer for LogRecord emission.
///
/// The main packet emission path (`writeStream` -> `getFromRadio` ->
@@ -117,5 +129,4 @@ class StreamAPI : public PhoneAPI
/// interleave on the wire.
meshtastic_FromRadio fromRadioScratchLog = {};
uint8_t txBufLog[MAX_STREAM_BUF_SIZE] = {0};
concurrency::Lock streamLock;
};
+65
View File
@@ -0,0 +1,65 @@
#include "StreamFrameWriter.h"
#include <cassert>
/// Start a frame or retain required output behind an incomplete frame.
bool StreamFrameWriter::writeFrame(Stream &stream, uint8_t *frame, size_t frameLen, bool bestEffort)
{
if (!finishPendingFrame(stream)) {
// Single required-frame producer invariant: writeStream() is gated on
// finishPendingFrame(), so a second required frame can never arrive here.
assert(bestEffort || !deferredFrame);
// Preserve required output that was encoded while another frame tail
// was pending. Best-effort output is dropped instead.
if (!bestEffort && !deferredFrame) {
deferredFrame = frame;
deferredFrameLen = frameLen;
}
return false;
}
// Never start best-effort output unless its complete frame fits.
if (bestEffort && stream.availableForWrite() < (int)frameLen)
return false;
size_t written = stream.write(frame, frameLen);
if (written == frameLen)
return true;
pendingFrame = frame;
pendingFrameLen = frameLen;
pendingFrameOffset = written;
return false;
}
/// Continue the pending tail with at most one Stream::write() call.
bool StreamFrameWriter::finishPendingFrame(Stream &stream)
{
if (!pendingFrame)
return true;
size_t remaining = pendingFrameLen - pendingFrameOffset;
size_t written = stream.write(pendingFrame + pendingFrameOffset, remaining);
if (written > remaining)
written = remaining;
pendingFrameOffset += written;
if (pendingFrameOffset < pendingFrameLen)
return false;
pendingFrame = nullptr;
pendingFrameLen = 0;
pendingFrameOffset = 0;
// Promote required output without starting another write in this call.
if (deferredFrame) {
pendingFrame = deferredFrame;
pendingFrameLen = deferredFrameLen;
deferredFrame = nullptr;
deferredFrameLen = 0;
return false;
}
return true;
}
+26
View File
@@ -0,0 +1,26 @@
#pragma once
#include "Stream.h"
#include <cstddef>
#include <cstdint>
/** Caller-owned frame storage must remain valid and unchanged until isIdle(). */
class StreamFrameWriter
{
public:
/// Start a complete frame, or retain a required frame behind pending output.
bool writeFrame(Stream &stream, uint8_t *frame, size_t frameLen, bool bestEffort);
/// Make one bounded attempt to finish pending output.
bool finishPendingFrame(Stream &stream);
/// Return true when no frame buffer is retained.
bool isIdle() const { return pendingFrame == nullptr && deferredFrame == nullptr; }
private:
uint8_t *pendingFrame = nullptr;
size_t pendingFrameLen = 0;
size_t pendingFrameOffset = 0;
uint8_t *deferredFrame = nullptr;
size_t deferredFrameLen = 0;
};