Initial implementation of serialHal - #10351
Conversation
github-actions
Bot
added
needs-review
jp-bennett
added
enhancement
| } | ||
| } | ||
|
|
||
| void SerialHalDevice::handleCommand(const uint8_t *buf, size_t len, StreamAPI *streamApi) |
There was a problem hiding this comment.
It would be possible to just check for an existing radiolib instance here, and delete it if found. This could be a parallel or alternative approach to the config.lora config entry.
Firmware Size Report22 targets | vs
Show 17 more target(s)
Updated for cae717b |
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (30)
Build artifacts expire on 2026-08-22. Updated for |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a serial-based LoRa transport for Portduino, with StreamAPI framing, device-side command handling, host-side SerialHal transport, log suppression, and configuration for serial-only radio operation. ChangesSerial HAL Transport
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant HostApp
participant SerialPort
participant StreamAPI
participant SerialHalDevice
participant RadioGPIO_SPI
HostApp->>SerialPort: write framed SerialHalCommand
SerialPort->>StreamAPI: deliver SERIALHAL_MAGIC frame
StreamAPI->>SerialHalDevice: dispatch command payload
SerialHalDevice->>RadioGPIO_SPI: perform GPIO, interrupt, or SPI operation
RadioGPIO_SPI-->>SerialHalDevice: return operation result
SerialHalDevice->>StreamAPI: emit framed response
StreamAPI->>SerialPort: write response
SerialPort-->>HostApp: deliver SerialHalResponse
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/mesh/StreamAPI.cpp (1)
83-130: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicated framing state machine has already drifted between the two copies.
handleRecStream()andreadStream()implement the same SerialHal/ToRadio discrimination logic almost verbatim, butreadStream()includes aLOG_WARNon frame detection (line 175) thathandleRecStream()lacks — concrete evidence of the risk duplicated logic poses. Extract the shared per-byte state machine into a single private helper both call into.♻️ Suggested direction
-int32_t StreamAPI::handleRecStream(const char *buf, uint16_t bufLen) -{ - ... -} +int32_t StreamAPI::handleRecStream(const char *buf, uint16_t bufLen) +{ + for (...) processIncomingByte((uint8_t)buf[index]); + return 0; +}Then have
readStream()'s inline loop call the sameprocessIncomingByte()helper.Also applies to: 135-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/StreamAPI.cpp` around lines 83 - 130, The per-byte framing logic in handleRecStream() and readStream() is duplicated and already diverging, so extract the shared SerialHal/ToRadio state machine into one private helper (for example, a processIncomingByte-style method) and have both callers delegate to it. Keep the frame type discrimination, length validation, payload dispatch, and serialHalRxActive/RedirectablePrint::setSerialHalLogSuppressed state changes in that shared helper so behavior stays identical in both paths and future changes only need to be made once.src/mesh/RadioInterface.cpp (1)
383-395: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReplace the C-style downcast here
src/mesh/RadioInterface.cpp:394—SerialHalandCh341Halare separateRadioLibHalimplementations, notLockingArduinoHalderivatives, but this path still passesRadioLibHALthrough(LockingArduinoHal *). That’s undefined behavior; make the factory/constructor acceptRadioLibHal*or split the portduino backends instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/RadioInterface.cpp` around lines 383 - 395, The factory path in RadioInterface currently forces RadioLibHAL through a C-style cast to LockingArduinoHal* before calling loraModuleInterface, but SerialHal and Ch341Hal are not LockingArduinoHal-derived types. Update the RadioInterface setup so loraModuleInterface accepts a RadioLibHal* (or otherwise dispatches by backend type) and remove the unsafe downcast, using the existing RadioLibHAL allocation branches as the point of integration.
🧹 Nitpick comments (1)
src/platform/portduino/PortduinoGlue.cpp (1)
883-892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider validating
SerialDeviceis set whenspidev: serial.If a user sets
spidev: serialwithoutSerialDevice,lora_serial_devicestays empty, and the failure only surfaces later as a generic serial-open error insideSerialHal/checkError, eventually leading to the opaque "No radio" exit ininitLoRa(). An early, explicit check here would give a clearer startup error.💡 Suggested check
portduino_config.lora_spi_dev = yamlConfig["Lora"]["spidev"].as<std::string>("spidev0.0"); + if (portduino_config.lora_spi_dev == "serial" && portduino_config.lora_serial_device.empty()) { + std::cerr << "Lora.spidev is 'serial' but Lora.SerialDevice is not set!" << std::endl; + exit(EXIT_FAILURE); + } if (portduino_config.lora_spi_dev != "ch341" && portduino_config.lora_spi_dev != "serial") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/portduino/PortduinoGlue.cpp` around lines 883 - 892, The LoRa config parsing in PortduinoGlue::initLoRa should explicitly validate that Lora.SerialDevice is provided whenever Lora.spidev is set to serial, instead of letting lora_serial_device remain empty. Add an early check right after reading yamlConfig["Lora"] values and before the SerialHal/open path so startup fails with a clear config error, using the existing portduino_config.lora_serial_device and portduino_config.lora_spi_dev settings to locate the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mesh/SerialHalDevice.cpp`:
- Around line 367-378: The framing magic bytes are duplicated in SerialHalDevice
and StreamAPI, which risks request/response framing drifting if one copy
changes. Move the shared START1 and SERIALHAL_MAGIC definitions out of the local
constexprs in SerialHalDevice::emitSerialHalResponse and the corresponding
definitions in StreamAPI into a common header (for example, SerialHalFraming.h),
then include that header from both translation units so both sides use the exact
same constants.
- Line 320: The preprocessor guard in SerialHalDevice needs to be changed
because `#if !ARCH_PORTDUINO` fails when `ARCH_PORTDUINO` is provided as a bare
define. Update the conditional around the Portduino-specific block to use a
definition check such as `defined(ARCH_PORTDUINO)` or `ifndef` so the build
stays compatible with that target; keep the fix localized to the existing
`SerialHalDevice` guard.
- Around line 28-33: Make InterruptSlot::pending an ISR-safe synchronized flag
instead of volatile, since it is written in the interrupt path and read/cleared
in pumpInterruptEvents() under interruptMutex. Update the InterruptSlot
definition and the interrupt/event handling in SerialHalDevice.cpp to use
std::atomic<bool> or an equivalent atomic flag primitive, and adjust the
set/read/clear operations accordingly so cross-core visibility is guaranteed.
In `@src/mesh/StreamAPI.cpp`:
- Around line 314-333: emitSerialHalResponse bypasses the class’s normal
frame-write path and skips both write gating and partial-write handling. Update
StreamAPI::emitSerialHalResponse to route through writeFrame() or to mirror its
canWrite/canWriteFrame checks and invoke onFrameWriteFailed() on failure, and
verify the return value from stream->write() so short writes are treated as
errors rather than success. Also align the logging in emitSerialHalResponse with
the existing frame-emission behavior instead of using a warning for a normal
emit path.
In `@src/platform/portduino/SerialHal.cpp`:
- Around line 190-252: The application-level failure handling in
SerialHal::sendRequest is treating device-reported command errors as fatal
transport errors, which leaves inError stuck on and blocks all later HAL calls.
Update SerialHal::sendRequest so only genuine I/O problems use setTransportError
and set inError, while a non-OK meshtastic_SerialHalResponse_Result from the
device should be logged and returned as a normal request failure without
disabling future calls. Keep the existing recovery/reset behavior tied to
successful sendRequest/openPort paths and ensure methods like pinMode,
digitalWrite, digitalRead, attachInterrupt, detachInterrupt, and spiTransfer can
continue after a single bad command.
- Around line 190-252: sendRequest currently only removes entries from
pendingResponses when a matching response arrives, so timed-out or late replies
can linger and later be reused after txId wraps. Update SerialHal::readerLoop
and SerialHal::sendRequest to track which transaction_id values are actively in
flight, and only cache responses for those IDs; discard any late response that
has no waiter, including reserved ID 0. Ensure the cleanup path in sendRequest
removes the in-flight mark on timeout, success, and error so stale entries
cannot satisfy a future caller.
- Around line 174-188: The crc16 helper in SerialHal is currently unused dead
code, while SerialHal framing still only emits and parses START1, MAGIC, LEN_H,
LEN_L, and payload. Either wire crc16() into the framing logic in SerialHal and
the corresponding host/device parser so both ends send and validate a CRC, or
remove crc16() entirely if the protocol does not use it. Use SerialHal::crc16
and the framing code in SerialHal to locate the update points.
- Around line 387-422: The SerialHal::spiTransfer implementation silently
truncates transfers because cmd.data.bytes is limited to 512 bytes, so any len
larger than that drops data on both the request and response paths. Update
spiTransfer to either reject oversized transfers with an explicit error path or
split the operation into multiple chunks, and make sure the handling around
sendRequest, txLen, and copyLen no longer zero-fills hidden truncation without
signaling failure.
- Around line 62-119: `SerialHal` has unsynchronized shared state around the
port lifecycle, so `fd` and `hasWarned` can be raced by `sendRequest()`,
`readFrame()`, and `closePort()`. Use the existing `fdMutex` to guard all
reads/writes of `fd` and the warning state, especially in `openPort()`,
`closePort()`, and `setTransportError()`, so reentrant callbacks cannot close or
reopen the port while the reader thread is active.
In `@src/RedirectablePrint.cpp`:
- Around line 24-39: The global process-wide suppression in
RedirectablePrint::setSerialHalLogSuppressed / isSerialHalLogSuppressed is
letting one SerialHal frame mute unrelated logs, including LOG_ERROR, across all
outputs. Move the suppression state to the active StreamAPI/instance instead of
the shared atomic, or change RedirectablePrint::log so error-level messages
still pass through regardless of suppression. Also add a timeout/reset fallback
so suppression cannot remain stuck if framing never completes.
---
Outside diff comments:
In `@src/mesh/RadioInterface.cpp`:
- Around line 383-395: The factory path in RadioInterface currently forces
RadioLibHAL through a C-style cast to LockingArduinoHal* before calling
loraModuleInterface, but SerialHal and Ch341Hal are not
LockingArduinoHal-derived types. Update the RadioInterface setup so
loraModuleInterface accepts a RadioLibHal* (or otherwise dispatches by backend
type) and remove the unsafe downcast, using the existing RadioLibHAL allocation
branches as the point of integration.
In `@src/mesh/StreamAPI.cpp`:
- Around line 83-130: The per-byte framing logic in handleRecStream() and
readStream() is duplicated and already diverging, so extract the shared
SerialHal/ToRadio state machine into one private helper (for example, a
processIncomingByte-style method) and have both callers delegate to it. Keep the
frame type discrimination, length validation, payload dispatch, and
serialHalRxActive/RedirectablePrint::setSerialHalLogSuppressed state changes in
that shared helper so behavior stays identical in both paths and future changes
only need to be made once.
---
Nitpick comments:
In `@src/platform/portduino/PortduinoGlue.cpp`:
- Around line 883-892: The LoRa config parsing in PortduinoGlue::initLoRa should
explicitly validate that Lora.SerialDevice is provided whenever Lora.spidev is
set to serial, instead of letting lora_serial_device remain empty. Add an early
check right after reading yamlConfig["Lora"] values and before the
SerialHal/open path so startup fails with a clear config error, using the
existing portduino_config.lora_serial_device and portduino_config.lora_spi_dev
settings to locate the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 02a5b4cd-8da8-49d3-8219-0f1d66ac455a
📒 Files selected for processing (12)
src/RedirectablePrint.cppsrc/RedirectablePrint.hsrc/main.cppsrc/mesh/RadioInterface.cppsrc/mesh/SerialHalDevice.cppsrc/mesh/SerialHalDevice.hsrc/mesh/StreamAPI.cppsrc/mesh/StreamAPI.hsrc/platform/portduino/PortduinoGlue.cppsrc/platform/portduino/PortduinoGlue.hsrc/platform/portduino/SerialHal.cppsrc/platform/portduino/SerialHal.h
| struct InterruptSlot { | ||
| bool used = false; | ||
| uint32_t pin = 0; | ||
| uint32_t mode = 0; | ||
| volatile bool pending = false; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file with line numbers and nearby context.
git ls-files src/mesh/SerialHalDevice.cpp
wc -l src/mesh/SerialHalDevice.cpp
sed -n '1,240p' src/mesh/SerialHalDevice.cpp
# Search for the interrupt slot and mutex usage in this file.
rg -n "InterruptSlot|pending|interruptMutex|isr[0-7]|pumpInterruptEvents" src/mesh/SerialHalDevice.cppRepository: meshtastic/firmware
Length of output: 7885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect lock semantics and the interrupt attach/detach paths.
git ls-files concurrency/Lock.h concurrency/Lock.cpp src/mesh/SerialHalDevice.cpp
printf '\n=== concurrency/Lock.h ===\n'
sed -n '1,220p' concurrency/Lock.h
printf '\n=== concurrency/Lock.cpp ===\n'
sed -n '1,220p' concurrency/Lock.cpp
printf '\n=== SerialHalDevice interrupt-related sections ===\n'
sed -n '240,390p' src/mesh/SerialHalDevice.cppRepository: meshtastic/firmware
Length of output: 276
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the lock implementation and inspect the interrupt-related code paths.
git ls-files | rg '(^|/)(Lock|lock)\.(h|hpp|cpp)$|src/mesh/SerialHalDevice\.cpp$'
printf '\n=== lock-related files ===\n'
fd -a 'Lock.h' .
fd -a 'Lock.cpp' .
fd -a 'lock.h' .
fd -a 'lock.cpp' .
printf '\n=== SerialHalDevice interrupt sections ===\n'
sed -n '240,390p' src/mesh/SerialHalDevice.cppRepository: meshtastic/firmware
Length of output: 5749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/concurrency/Lock.h
printf '\n=== Lock.cpp ===\n'
sed -n '1,220p' src/concurrency/Lock.cpp
printf '\n=== Interrupt related code around the shared flag ===\n'
sed -n '1,220p' src/mesh/SerialHalDevice.cppRepository: meshtastic/firmware
Length of output: 7226
src/mesh/SerialHalDevice.cpp:28-33,106-170 — Make InterruptSlot::pending atomic. The ISR sets it outside interruptMutex, while pumpInterruptEvents() reads and clears it under the mutex. volatile does not provide cross-core synchronization, so use std::atomic<bool> or another ISR-safe flag primitive here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mesh/SerialHalDevice.cpp` around lines 28 - 33, Make
InterruptSlot::pending an ISR-safe synchronized flag instead of volatile, since
it is written in the interrupt path and read/cleared in pumpInterruptEvents()
under interruptMutex. Update the InterruptSlot definition and the
interrupt/event handling in SerialHalDevice.cpp to use std::atomic<bool> or an
equivalent atomic flag primitive, and adjust the set/read/clear operations
accordingly so cross-core visibility is guaranteed.
| return; | ||
| } | ||
|
|
||
| #if !ARCH_PORTDUINO |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'ARCH_PORTDUINO' --type=cpp -C1 | rg -v 'defined\(ARCH_PORTDUINO\)|ifdef ARCH_PORTDUINO'Repository: meshtastic/firmware
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant regions in the target file
sed -n '1,90p' src/mesh/SerialHalDevice.cpp
printf '\n---\n'
sed -n '300,340p' src/mesh/SerialHalDevice.cpp
# Find all preprocessor uses of ARCH_PORTDUINO across the repo
printf '\n=== ARCH_PORTDUINO occurrences ===\n'
rg -n -C 2 'ARCH_PORTDUINO' .
# Probe whether the build defines it as a bare macro or with a value in config/build files
printf '\n=== possible macro definitions ===\n'
rg -n '^\s*#\s*define\s+ARCH_PORTDUINO\b|^-DARCH_PORTDUINO\b|ARCH_PORTDUINO=' . --glob '!**/build/**' --glob '!**/dist/**'Repository: meshtastic/firmware
Length of output: 50375
🏁 Script executed:
python3 - <<'PY'
import subprocess, tempfile, textwrap, os, sys
cases = {
"undefined": "",
"empty_define": "`#define` ARCH_PORTDUINO\n",
"zero_define": "`#define` ARCH_PORTDUINO 0\n",
"one_define": "`#define` ARCH_PORTDUINO 1\n",
}
for name, prefix in cases.items():
src = prefix + textwrap.dedent("""
`#if` !ARCH_PORTDUINO
int x = 1;
`#else`
int x = 2;
`#endif`
""")
with tempfile.NamedTemporaryFile("w", suffix=".c", delete=False) as f:
f.write(src)
path = f.name
try:
p = subprocess.run(
["cpp", "-P", path],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
)
print(f"== {name} ==")
print("rc:", p.returncode)
if p.stdout:
print("stdout:", p.stdout.strip())
if p.stderr:
print("stderr:", p.stderr.strip())
finally:
os.unlink(path)
PYRepository: meshtastic/firmware
Length of output: 710
🏁 Script executed:
python3 - <<'PY'
import shutil
for cmd in ["clang", "clang++", "gcc", "g++", "cpp"]:
print(cmd, shutil.which(cmd))
PYRepository: meshtastic/firmware
Length of output: 230
🏁 Script executed:
python3 - <<'PY'
import subprocess, tempfile, textwrap, os
cases = {
"undefined": "",
"empty_define": "`#define` ARCH_PORTDUINO\n",
"zero_define": "`#define` ARCH_PORTDUINO 0\n",
"one_define": "`#define` ARCH_PORTDUINO 1\n",
}
for name, prefix in cases.items():
src = prefix + textwrap.dedent("""
`#if` !ARCH_PORTDUINO
int x = 1;
`#else`
int x = 2;
`#endif`
""")
with tempfile.NamedTemporaryFile("w", suffix=".c", delete=False) as f:
f.write(src)
path = f.name
try:
p = subprocess.run(
["clang", "-E", "-P", path],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
)
print(f"== {name} ==")
print("rc:", p.returncode)
print("stdout:", p.stdout.strip() or "<empty>")
print("stderr:", p.stderr.strip() or "<empty>")
finally:
os.unlink(path)
PYRepository: meshtastic/firmware
Length of output: 504
Use defined() here
#if !ARCH_PORTDUINO breaks when ARCH_PORTDUINO is passed as a bare -DARCH_PORTDUINO flag, which is how this target is configured. Switch this to #if !defined(ARCH_PORTDUINO) / #ifndef ARCH_PORTDUINO to keep the Portduino build compiling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mesh/SerialHalDevice.cpp` at line 320, The preprocessor guard in
SerialHalDevice needs to be changed because `#if !ARCH_PORTDUINO` fails when
`ARCH_PORTDUINO` is provided as a bare define. Update the conditional around the
Portduino-specific block to use a definition check such as
`defined(ARCH_PORTDUINO)` or `ifndef` so the build stays compatible with that
target; keep the fix localized to the existing `SerialHalDevice` guard.
| // Build frame with StreamAPI framing: START1 SERIALHAL_MAGIC LEN_H LEN_L [payload] | ||
| constexpr uint8_t START1 = 0x94; | ||
| constexpr uint8_t SERIALHAL_MAGIC = 0xA5; | ||
|
|
||
| uint8_t hdr[4]; | ||
| hdr[0] = START1; | ||
| hdr[1] = SERIALHAL_MAGIC; | ||
| hdr[2] = (uint8_t)((responseLen >> 8) & 0xFF); // LEN_H | ||
| hdr[3] = (uint8_t)(responseLen & 0xFF); // LEN_L | ||
|
|
||
| // Emit via StreamAPI (this uses the internal txBuf + framing) | ||
| streamApi->emitSerialHalResponse(hdr, sizeof(hdr), encoded, responseLen); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Framing magic bytes duplicated across two translation units.
START1/SERIALHAL_MAGIC are redefined here as local constexpr values identical to the #defines in src/mesh/StreamAPI.cpp (lines 10-12). If either definition changes independently, response framing silently desyncs from request framing detection. Move these to a shared header (e.g. a small SerialHalFraming.h) included by both files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mesh/SerialHalDevice.cpp` around lines 367 - 378, The framing magic bytes
are duplicated in SerialHalDevice and StreamAPI, which risks request/response
framing drifting if one copy changes. Move the shared START1 and SERIALHAL_MAGIC
definitions out of the local constexprs in
SerialHalDevice::emitSerialHalResponse and the corresponding definitions in
StreamAPI into a common header (for example, SerialHalFraming.h), then include
that header from both translation units so both sides use the exact same
constants.
| void StreamAPI::emitSerialHalResponse(const uint8_t *hdr, size_t hdrLen, const uint8_t *payload, size_t payloadLen) | ||
| { | ||
| if (hdr == nullptr || hdrLen != 4 || payload == nullptr || payloadLen > meshtastic_SerialHalResponse_size) { | ||
| LOG_ERROR("StreamAPI: Invalid SerialHal response parameters"); | ||
| return; | ||
| } | ||
|
|
||
| // Build complete frame in a temporary buffer | ||
| uint8_t frame[4 + meshtastic_SerialHalResponse_size]; | ||
| memcpy(frame, hdr, hdrLen); | ||
| memcpy(frame + hdrLen, payload, payloadLen); | ||
|
|
||
| size_t totalLen = hdrLen + payloadLen; | ||
|
|
||
| // Serialize stream writes against other emit operations via streamLock | ||
| concurrency::LockGuard guard(&streamLock); | ||
| stream->write(frame, totalLen); | ||
| stream->flush(); | ||
| LOG_WARN("StreamAPI: Emitted SerialHal response frame (len=%zu)", totalLen); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
emitSerialHalResponse bypasses writeFrame()'s gating and partial-write handling.
Every other emission path in this class routes through writeFrame(), which (per the class's own documented rationale about torn frames) checks canWrite/canWriteFrame() and reports failures via onFrameWriteFailed(). This new method calls stream->write()/stream->flush() directly: it ignores canWrite (so subclasses that disable output, e.g. read-only consoles, can't suppress this), and it never checks the number of bytes actually written, so a partial write is silently treated as success. Also note LOG_WARN here is a guaranteed no-op given the suppression window noted above, and reports what is effectively expected/successful behavior — LOG_DEBUG would be more appropriate if it weren't suppressed.
🐛 Suggested fix
- // Serialize stream writes against other emit operations via streamLock
- concurrency::LockGuard guard(&streamLock);
- stream->write(frame, totalLen);
- stream->flush();
- LOG_WARN("StreamAPI: Emitted SerialHal response frame (len=%zu)", totalLen);
+ if (!canWrite || !canWriteFrame(totalLen))
+ return;
+ if (!writeFrame(frame, totalLen))
+ return; // writeFrame already invokes onFrameWriteFailed on partial writes📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void StreamAPI::emitSerialHalResponse(const uint8_t *hdr, size_t hdrLen, const uint8_t *payload, size_t payloadLen) | |
| { | |
| if (hdr == nullptr || hdrLen != 4 || payload == nullptr || payloadLen > meshtastic_SerialHalResponse_size) { | |
| LOG_ERROR("StreamAPI: Invalid SerialHal response parameters"); | |
| return; | |
| } | |
| // Build complete frame in a temporary buffer | |
| uint8_t frame[4 + meshtastic_SerialHalResponse_size]; | |
| memcpy(frame, hdr, hdrLen); | |
| memcpy(frame + hdrLen, payload, payloadLen); | |
| size_t totalLen = hdrLen + payloadLen; | |
| // Serialize stream writes against other emit operations via streamLock | |
| concurrency::LockGuard guard(&streamLock); | |
| stream->write(frame, totalLen); | |
| stream->flush(); | |
| LOG_WARN("StreamAPI: Emitted SerialHal response frame (len=%zu)", totalLen); | |
| } | |
| void StreamAPI::emitSerialHalResponse(const uint8_t *hdr, size_t hdrLen, const uint8_t *payload, size_t payloadLen) | |
| { | |
| if (hdr == nullptr || hdrLen != 4 || payload == nullptr || payloadLen > meshtastic_SerialHalResponse_size) { | |
| LOG_ERROR("StreamAPI: Invalid SerialHal response parameters"); | |
| return; | |
| } | |
| // Build complete frame in a temporary buffer | |
| uint8_t frame[4 + meshtastic_SerialHalResponse_size]; | |
| memcpy(frame, hdr, hdrLen); | |
| memcpy(frame + hdrLen, payload, payloadLen); | |
| size_t totalLen = hdrLen + payloadLen; | |
| if (!canWrite || !canWriteFrame(totalLen)) | |
| return; | |
| if (!writeFrame(frame, totalLen)) | |
| return; // writeFrame already invokes onFrameWriteFailed on partial writes | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mesh/StreamAPI.cpp` around lines 314 - 333, emitSerialHalResponse
bypasses the class’s normal frame-write path and skips both write gating and
partial-write handling. Update StreamAPI::emitSerialHalResponse to route through
writeFrame() or to mirror its canWrite/canWriteFrame checks and invoke
onFrameWriteFailed() on failure, and verify the return value from
stream->write() so short writes are treated as errors rather than success. Also
align the logging in emitSerialHalResponse with the existing frame-emission
behavior instead of using a warning for a normal emit path.
| bool SerialHal::openPort() | ||
| { | ||
| closePort(); | ||
| fd = ::open(device.c_str(), O_RDWR | O_NOCTTY | O_SYNC); | ||
| if (fd < 0) { | ||
| return false; | ||
| } | ||
|
|
||
| termios tty = {}; | ||
| if (tcgetattr(fd, &tty) != 0) { | ||
| closePort(); | ||
| return false; | ||
| } | ||
|
|
||
| // Force raw mode to avoid line discipline byte mangling on binary frames. | ||
| cfmakeraw(&tty); | ||
|
|
||
| cfsetospeed(&tty, toTermiosBaud(baud)); | ||
| cfsetispeed(&tty, toTermiosBaud(baud)); | ||
|
|
||
| tty.c_cflag = (tty.c_cflag & ~CSIZE) | CS8; | ||
| tty.c_cc[VMIN] = 0; | ||
| tty.c_cc[VTIME] = 0; | ||
| tty.c_cflag |= (CLOCAL | CREAD); | ||
| tty.c_cflag &= ~(PARENB | PARODD); | ||
| tty.c_cflag &= ~CSTOPB; | ||
| tty.c_cflag &= ~CRTSCTS; | ||
|
|
||
| if (tcsetattr(fd, TCSANOW, &tty) != 0) { | ||
| closePort(); | ||
| return false; | ||
| } | ||
|
|
||
| tcflush(fd, TCIOFLUSH); | ||
| inError = false; | ||
| startReaderThread(); | ||
| return true; | ||
| } | ||
|
|
||
| void SerialHal::closePort() | ||
| { | ||
| stopReaderThread(); | ||
| if (fd >= 0) { | ||
| ::close(fd); | ||
| fd = -1; | ||
| } | ||
| } | ||
|
|
||
| void SerialHal::setTransportError(const char *msg) | ||
| { | ||
| if (!inError.load() || !hasWarned) { | ||
| LOG_ERROR("SerialHal: %s (%s)", msg, device.c_str()); | ||
| } | ||
| inError = true; | ||
| hasWarned = true; | ||
| portduino_status.LoRa_in_error = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
ast-grep outline src/platform/portduino/SerialHal.cpp --view expandedRepository: meshtastic/firmware
Length of output: 208
🏁 Script executed:
ast-grep outline src/platform/portduino/SerialHal.h --view expandedRepository: meshtastic/firmware
Length of output: 206
🏁 Script executed:
rg -n "fdMutex|hasWarned|fd\b|checkError|sendRequest|waitForReadable|readExact|readFrame|startReaderThread|stopReaderThread" src/platform/portduino/SerialHal.cpp src/platform/portduino/SerialHal.hRepository: meshtastic/firmware
Length of output: 5162
🏁 Script executed:
sed -n '1,220p' src/platform/portduino/SerialHal.hRepository: meshtastic/firmware
Length of output: 3102
🏁 Script executed:
sed -n '1,220p' src/platform/portduino/SerialHal.cppRepository: meshtastic/firmware
Length of output: 5786
🏁 Script executed:
sed -n '220,640p' src/platform/portduino/SerialHal.cppRepository: meshtastic/firmware
Length of output: 10714
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('src/platform/portduino/SerialHal.cpp')
for start,end in [(1,220),(220,640)]:
print(f"\n--- {start}-{end} ---")
lines = p.read_text().splitlines()
for i in range(start-1, min(end, len(lines))):
print(f"{i+1}:{lines[i]}")
PYRepository: meshtastic/firmware
Length of output: 18651
🏁 Script executed:
sed -n '1,140p' src/platform/portduino/SerialHal.hRepository: meshtastic/firmware
Length of output: 3102
🏁 Script executed:
sed -n '100,180p' src/platform/portduino/SerialHal.cppRepository: meshtastic/firmware
Length of output: 2120
🏁 Script executed:
sed -n '430,620p' src/platform/portduino/SerialHal.cppRepository: meshtastic/firmware
Length of output: 5020
🏁 Script executed:
sed -n '1,120p' src/platform/portduino/SerialHal.hRepository: meshtastic/firmware
Length of output: 3102
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
for path, ranges in {
'src/platform/portduino/SerialHal.h': [(1,120)],
'src/platform/portduino/SerialHal.cpp': [(100,180),(420,620)],
}.items():
lines = Path(path).read_text().splitlines()
for start,end in ranges:
print(f"\n== {path} {start}-{end} ==")
for i in range(start-1, min(end, len(lines))):
print(f"{i+1}:{lines[i]}")
PYRepository: meshtastic/firmware
Length of output: 11582
Serialize SerialHal’s port state
fd is read and written from sendRequest(), readFrame(), and closePort() on different threads, and hasWarned is shared too. fdMutex is already declared, but it needs to guard the fd lifecycle and the warning flag so a callback-driven reentry can’t close/reopen the port while the reader thread is active.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/portduino/SerialHal.cpp` around lines 62 - 119, `SerialHal` has
unsynchronized shared state around the port lifecycle, so `fd` and `hasWarned`
can be raced by `sendRequest()`, `readFrame()`, and `closePort()`. Use the
existing `fdMutex` to guard all reads/writes of `fd` and the warning state,
especially in `openPort()`, `closePort()`, and `setTransportError()`, so
reentrant callbacks cannot close or reopen the port while the reader thread is
active.
| uint16_t SerialHal::crc16(const uint8_t *data, size_t len) const | ||
| { | ||
| uint16_t crc = 0xFFFF; | ||
| for (size_t i = 0; i < len; ++i) { | ||
| crc ^= ((uint16_t)data[i] << 8); | ||
| for (int bit = 0; bit < 8; ++bit) { | ||
| if (crc & 0x8000) { | ||
| crc = (uint16_t)((crc << 1) ^ 0x1021); | ||
| } else { | ||
| crc = (uint16_t)(crc << 1); | ||
| } | ||
| } | ||
| } | ||
| return crc; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and inspect the surrounding protocol code.
git ls-files src/platform/portduino/SerialHal.cpp
wc -l src/platform/portduino/SerialHal.cpp
sed -n '150,240p' src/platform/portduino/SerialHal.cpp
sed -n '420,520p' src/platform/portduino/SerialHal.cpp
# Search for CRC usage in the file and nearby protocol code.
rg -n "\bcrc16\b|CRC|checksum|START1|MAGIC|LEN_H|LEN_L" src/platform/portduino/SerialHal.cpp
rg -n "\bcrc16\b|CRC|checksum" src/platform/portduinoRepository: meshtastic/firmware
Length of output: 7361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search the repo for any use of SerialHal::crc16 or serial framing CRC handling.
rg -n --hidden --glob '!**/.git/**' "SerialHal::crc16|crc16\(|CRC|checksum|SERIALHAL_MAGIC|HEADER_SIZE" src/platform/portduino srcRepository: meshtastic/firmware
Length of output: 9830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the device-side framing and the StreamAPI definitions for any CRC field.
sed -n '1,260p' src/mesh/SerialHalDevice.h
sed -n '330,430p' src/mesh/SerialHalDevice.cpp
sed -n '1,260p' src/mesh/StreamAPI.h
sed -n '1,260p' src/mesh/StreamAPI.cppRepository: meshtastic/firmware
Length of output: 22172
Add CRC to SerialHal framing or remove the unused helper
crc16() is dead code, and the SerialHal frames on both host and device sides are still just START1, MAGIC, LEN_H, LEN_L, [payload]. If CRC is part of the protocol, it needs to be added to both ends; otherwise this helper should be removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/portduino/SerialHal.cpp` around lines 174 - 188, The crc16
helper in SerialHal is currently unused dead code, while SerialHal framing still
only emits and parses START1, MAGIC, LEN_H, LEN_L, and payload. Either wire
crc16() into the framing logic in SerialHal and the corresponding host/device
parser so both ends send and validate a CRC, or remove crc16() entirely if the
protocol does not use it. Use SerialHal::crc16 and the framing code in SerialHal
to locate the update points.
| bool SerialHal::sendRequest(const meshtastic_SerialHalCommand &cmd, meshtastic_SerialHalResponse *response) | ||
| { | ||
| if (fd < 0 && !openPort()) { | ||
| setTransportError("serial open failed"); | ||
| return false; | ||
| } | ||
|
|
||
| uint8_t encoded[meshtastic_SerialHalCommand_size] = {0}; | ||
| const size_t payloadLen = | ||
| pb_encode_to_bytes(encoded, sizeof(encoded), &meshtastic_SerialHalCommand_msg, static_cast<const void *>(&cmd)); | ||
| if (payloadLen == 0 || payloadLen > 0xFFFF) { | ||
| setTransportError("serial command encode failed"); | ||
| return false; | ||
| } | ||
|
|
||
| // Build frame with StreamAPI canonical framing: START1 SERIALHAL_MAGIC LEN_H LEN_L [payload] | ||
| std::vector<uint8_t> frame; | ||
| frame.resize(HEADER_SIZE + payloadLen); | ||
|
|
||
| frame[0] = START1; | ||
| frame[1] = SERIALHAL_MAGIC; | ||
| frame[2] = (uint8_t)((payloadLen >> 8) & 0xFF); // LEN_H (big-endian) | ||
| frame[3] = (uint8_t)(payloadLen & 0xFF); // LEN_L | ||
| memcpy(frame.data() + HEADER_SIZE, encoded, payloadLen); | ||
|
|
||
| { | ||
| std::lock_guard<std::mutex> writeGuard(writeMutex); | ||
| if (!writeAll(frame.data(), frame.size())) { | ||
| setTransportError("serial write failed"); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| meshtastic_SerialHalResponse got = meshtastic_SerialHalResponse_init_zero; | ||
| { | ||
| std::unique_lock<std::mutex> lock(stateMutex); | ||
| const auto timeout = std::chrono::milliseconds(timeoutMs); | ||
| const bool arrived = responseCv.wait_for(lock, timeout, [&]() { return pendingResponses.count(cmd.transaction_id) > 0; }); | ||
| if (!arrived) { | ||
| setTransportError("serial response timeout"); | ||
| LOG_WARN("SerialHal: response timeout for transaction_id %u, cmd type %u", cmd.transaction_id, cmd.type); | ||
| return false; | ||
| } | ||
|
|
||
| got = pendingResponses[cmd.transaction_id]; | ||
| pendingResponses.erase(cmd.transaction_id); | ||
| } | ||
|
|
||
| if (got.result != meshtastic_SerialHalResponse_Result_OK) { | ||
| setTransportError("serial response reported error"); | ||
| LOG_WARN("SerialHal: response error: %s, %u, %u", got.error, cmd.type, cmd.data.size); | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| if (response != nullptr) { | ||
| *response = got; | ||
| } | ||
|
|
||
| inError = false; | ||
| hasWarned = false; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Application-level response errors permanently disable the whole HAL.
Any got.result != Result_OK (e.g. an invalid pin, unsupported command, or a single bad transfer reported by the device) calls setTransportError, which sets inError = true. Every public HAL method (pinMode, digitalWrite, digitalRead, attachInterrupt, detachInterrupt, spiTransfer) starts with checkError() and returns early once inError is set (lines 256, 272, 288, 306, 327, 389). Since inError is only cleared on a successful sendRequest() call (line 249) or a successful openPort() (line 96, unreachable while fd >= 0), no future call can ever reach sendRequest() again to clear it. A single non-fatal, application-level error therefore permanently disables the radio for the life of the process, indistinguishable from a genuine transport failure.
🛠️ Suggested direction
if (got.result != meshtastic_SerialHalResponse_Result_OK) {
- setTransportError("serial response reported error");
+ // Application-level error: report but don't poison the transport.
LOG_WARN("SerialHal: response error: %s, %u, %u", got.error, cmd.type, cmd.data.size);
-
return false;
}Reserve setTransportError/inError for genuine I/O failures (write/read errors, timeouts), so a single bad command doesn't permanently disable subsequent, otherwise-valid requests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bool SerialHal::sendRequest(const meshtastic_SerialHalCommand &cmd, meshtastic_SerialHalResponse *response) | |
| { | |
| if (fd < 0 && !openPort()) { | |
| setTransportError("serial open failed"); | |
| return false; | |
| } | |
| uint8_t encoded[meshtastic_SerialHalCommand_size] = {0}; | |
| const size_t payloadLen = | |
| pb_encode_to_bytes(encoded, sizeof(encoded), &meshtastic_SerialHalCommand_msg, static_cast<const void *>(&cmd)); | |
| if (payloadLen == 0 || payloadLen > 0xFFFF) { | |
| setTransportError("serial command encode failed"); | |
| return false; | |
| } | |
| // Build frame with StreamAPI canonical framing: START1 SERIALHAL_MAGIC LEN_H LEN_L [payload] | |
| std::vector<uint8_t> frame; | |
| frame.resize(HEADER_SIZE + payloadLen); | |
| frame[0] = START1; | |
| frame[1] = SERIALHAL_MAGIC; | |
| frame[2] = (uint8_t)((payloadLen >> 8) & 0xFF); // LEN_H (big-endian) | |
| frame[3] = (uint8_t)(payloadLen & 0xFF); // LEN_L | |
| memcpy(frame.data() + HEADER_SIZE, encoded, payloadLen); | |
| { | |
| std::lock_guard<std::mutex> writeGuard(writeMutex); | |
| if (!writeAll(frame.data(), frame.size())) { | |
| setTransportError("serial write failed"); | |
| return false; | |
| } | |
| } | |
| meshtastic_SerialHalResponse got = meshtastic_SerialHalResponse_init_zero; | |
| { | |
| std::unique_lock<std::mutex> lock(stateMutex); | |
| const auto timeout = std::chrono::milliseconds(timeoutMs); | |
| const bool arrived = responseCv.wait_for(lock, timeout, [&]() { return pendingResponses.count(cmd.transaction_id) > 0; }); | |
| if (!arrived) { | |
| setTransportError("serial response timeout"); | |
| LOG_WARN("SerialHal: response timeout for transaction_id %u, cmd type %u", cmd.transaction_id, cmd.type); | |
| return false; | |
| } | |
| got = pendingResponses[cmd.transaction_id]; | |
| pendingResponses.erase(cmd.transaction_id); | |
| } | |
| if (got.result != meshtastic_SerialHalResponse_Result_OK) { | |
| setTransportError("serial response reported error"); | |
| LOG_WARN("SerialHal: response error: %s, %u, %u", got.error, cmd.type, cmd.data.size); | |
| return false; | |
| } | |
| if (response != nullptr) { | |
| *response = got; | |
| } | |
| inError = false; | |
| hasWarned = false; | |
| return true; | |
| } | |
| bool SerialHal::sendRequest(const meshtastic_SerialHalCommand &cmd, meshtastic_SerialHalResponse *response) | |
| { | |
| if (fd < 0 && !openPort()) { | |
| setTransportError("serial open failed"); | |
| return false; | |
| } | |
| uint8_t encoded[meshtastic_SerialHalCommand_size] = {0}; | |
| const size_t payloadLen = | |
| pb_encode_to_bytes(encoded, sizeof(encoded), &meshtastic_SerialHalCommand_msg, static_cast<const void *>(&cmd)); | |
| if (payloadLen == 0 || payloadLen > 0xFFFF) { | |
| setTransportError("serial command encode failed"); | |
| return false; | |
| } | |
| // Build frame with StreamAPI canonical framing: START1 SERIALHAL_MAGIC LEN_H LEN_L [payload] | |
| std::vector<uint8_t> frame; | |
| frame.resize(HEADER_SIZE + payloadLen); | |
| frame[0] = START1; | |
| frame[1] = SERIALHAL_MAGIC; | |
| frame[2] = (uint8_t)((payloadLen >> 8) & 0xFF); // LEN_H (big-endian) | |
| frame[3] = (uint8_t)(payloadLen & 0xFF); // LEN_L | |
| memcpy(frame.data() + HEADER_SIZE, encoded, payloadLen); | |
| { | |
| std::lock_guard<std::mutex> writeGuard(writeMutex); | |
| if (!writeAll(frame.data(), frame.size())) { | |
| setTransportError("serial write failed"); | |
| return false; | |
| } | |
| } | |
| meshtastic_SerialHalResponse got = meshtastic_SerialHalResponse_init_zero; | |
| { | |
| std::unique_lock<std::mutex> lock(stateMutex); | |
| const auto timeout = std::chrono::milliseconds(timeoutMs); | |
| const bool arrived = responseCv.wait_for(lock, timeout, [&]() { return pendingResponses.count(cmd.transaction_id) > 0; }); | |
| if (!arrived) { | |
| setTransportError("serial response timeout"); | |
| LOG_WARN("SerialHal: response timeout for transaction_id %u, cmd type %u", cmd.transaction_id, cmd.type); | |
| return false; | |
| } | |
| got = pendingResponses[cmd.transaction_id]; | |
| pendingResponses.erase(cmd.transaction_id); | |
| } | |
| if (got.result != meshtastic_SerialHalResponse_Result_OK) { | |
| LOG_WARN("SerialHal: response error: %s, %u, %u", got.error, cmd.type, got.data.size); | |
| return false; | |
| } | |
| if (response != nullptr) { | |
| *response = got; | |
| } | |
| inError = false; | |
| hasWarned = false; | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/portduino/SerialHal.cpp` around lines 190 - 252, The
application-level failure handling in SerialHal::sendRequest is treating
device-reported command errors as fatal transport errors, which leaves inError
stuck on and blocks all later HAL calls. Update SerialHal::sendRequest so only
genuine I/O problems use setTransportError and set inError, while a non-OK
meshtastic_SerialHalResponse_Result from the device should be logged and
returned as a normal request failure without disabling future calls. Keep the
existing recovery/reset behavior tied to successful sendRequest/openPort paths
and ensure methods like pinMode, digitalWrite, digitalRead, attachInterrupt,
detachInterrupt, and spiTransfer can continue after a single bad command.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect relevant regions with line numbers.
git ls-files src/platform/portduino/SerialHal.cpp
wc -l src/platform/portduino/SerialHal.cpp
sed -n '1,140p' src/platform/portduino/SerialHal.cpp
printf '\n--- lines 140-280 ---\n'
sed -n '140,280p' src/platform/portduino/SerialHal.cpp
printf '\n--- lines 480-580 ---\n'
sed -n '480,580p' src/platform/portduino/SerialHal.cppRepository: meshtastic/firmware
Length of output: 10936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for all transaction_id/pendingResponses usage to understand ownership and cleanup.
rg -n "pendingResponses|transaction_id|txId|responseCv|readerLoop|sendRequest" src/platform/portduino/SerialHal.cppRepository: meshtastic/firmware
Length of output: 1486
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the SerialHal class members to confirm transaction-id type and any cleanup hooks.
file=src/platform/portduino/SerialHal.h
wc -l "$file"
sed -n '1,220p' "$file"Repository: meshtastic/firmware
Length of output: 3140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether pendingResponses is cleared anywhere else in the class or on reconnect/teardown.
rg -n "pendingResponses\.clear|clear\(\)|txId|transaction_id|readerStopRequested|closePort|openPort|stopReaderThread" src/platform/portduino/SerialHal.*Repository: meshtastic/firmware
Length of output: 3312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=src/platform/portduino/SerialHal.h
if [ -f "$file" ]; then
wc -l "$file"
sed -n '1,220p' "$file"
else
echo "missing $file"
fiRepository: meshtastic/firmware
Length of output: 3140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "pendingResponses\.clear|pendingResponses|txId|transaction_id|readerStopRequested|closePort|openPort|stopReaderThread" src/platform/portduino/SerialHal.cpp src/platform/portduino/SerialHal.hRepository: meshtastic/firmware
Length of output: 3220
Drop late responses instead of caching them indefinitely
readerLoop stores every non-zero response in pendingResponses, but sendRequest only erases the entry on success. A timed-out request can leave a stale response behind, and once txId wraps that old entry can satisfy a different caller; 0 is also reserved here, so wraparound needs special handling. Track in-flight IDs and discard responses that no thread is waiting for.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/portduino/SerialHal.cpp` around lines 190 - 252, sendRequest
currently only removes entries from pendingResponses when a matching response
arrives, so timed-out or late replies can linger and later be reused after txId
wraps. Update SerialHal::readerLoop and SerialHal::sendRequest to track which
transaction_id values are actively in flight, and only cache responses for those
IDs; discard any late response that has no waiter, including reserved ID 0.
Ensure the cleanup path in sendRequest removes the in-flight mark on timeout,
success, and error so stale entries cannot satisfy a future caller.
| void SerialHal::spiTransfer(uint8_t *out, size_t len, uint8_t *in) | ||
| { | ||
| if (checkError()) { | ||
| return; | ||
| } | ||
|
|
||
| if (len == 0) { | ||
| return; | ||
| } | ||
|
|
||
| meshtastic_SerialHalCommand cmd = meshtastic_SerialHalCommand_init_zero; | ||
| cmd.transaction_id = txId.fetch_add(1); | ||
| cmd.type = meshtastic_SerialHalCommand_Type_SPI_TRANSFER; | ||
|
|
||
| const size_t maxTx = sizeof(cmd.data.bytes); | ||
| const size_t txLen = len < maxTx ? len : maxTx; | ||
| cmd.data.size = txLen; | ||
| if (out != nullptr) { | ||
| memcpy(cmd.data.bytes, out, txLen); | ||
| } else { | ||
| memset(cmd.data.bytes, 0, txLen); | ||
| } | ||
|
|
||
| meshtastic_SerialHalResponse response = meshtastic_SerialHalResponse_init_zero; | ||
| if (!sendRequest(cmd, &response)) { | ||
| return; | ||
| } | ||
|
|
||
| if (in != nullptr) { | ||
| size_t copyLen = response.data.size < len ? response.data.size : len; | ||
| memcpy(in, response.data.bytes, copyLen); | ||
| if (copyLen < len) { | ||
| memset(in + copyLen, 0, len - copyLen); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the generated bytes-field capacity for the SerialHalCommand data field.
fd -i 'serial_hal.pb.h' | xargs -r rg -n 'data' -A3Repository: meshtastic/firmware
Length of output: 1617
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation around spiTransfer and related request handling.
FILE="src/platform/portduino/SerialHal.cpp"
wc -l "$FILE"
sed -n '340,470p' "$FILE"
# Find call sites and any chunking/truncation logic related to spiTransfer.
rg -n "spiTransfer\(|SPI_TRANSFER|data\.size|maxTx|bytes\[512\]|sendRequest\(" src -S
# Inspect the generated protobuf header more closely for field semantics.
PB="src/platform/portduino/serial_hal.pb.h"
if [ -f "$PB" ]; then
sed -n '1,140p' "$PB"
fiRepository: meshtastic/firmware
Length of output: 6560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the SerialHalDevice handling for SPI transfer sizing and any rejection/chunking.
sed -n '220,360p' src/mesh/SerialHalDevice.cpp
# Inspect the abstract SPI interface and the portduino implementation that forwards to SerialHal.
sed -n '1,120p' src/mesh/RadioLibInterface.h
sed -n '1,120p' src/mesh/RadioLibInterface.cpp
sed -n '1,260p' src/platform/portduino/USBHal.h
# Search for any explicit SPI length limits or split-transfer logic.
rg -n "512|chunk|split|spiTransfer|transferBytes\(|transfer\(" src -SRepository: meshtastic/firmware
Length of output: 37457
SPI transfers above 512 bytes are silently truncated. cmd.data.bytes is a 512-byte protobuf field, so txLen/copyLen cap the request and response with no error path. Any larger burst transfer will drop the tail and zero-fill the unread bytes; this needs either an explicit failure or chunking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/platform/portduino/SerialHal.cpp` around lines 387 - 422, The
SerialHal::spiTransfer implementation silently truncates transfers because
cmd.data.bytes is limited to 512 bytes, so any len larger than that drops data
on both the request and response paths. Update spiTransfer to either reject
oversized transfers with an explicit error path or split the operation into
multiple chunks, and make sure the handling around sendRequest, txLen, and
copyLen no longer zero-fills hidden truncation without signaling failure.
|
|
||
| namespace | ||
| { | ||
| std::atomic<bool> serialHalLogSuppressed{false}; | ||
| } | ||
|
|
||
| void RedirectablePrint::setSerialHalLogSuppressed(bool suppressed) | ||
| { | ||
| serialHalLogSuppressed.store(suppressed); | ||
| } | ||
|
|
||
| bool RedirectablePrint::isSerialHalLogSuppressed() | ||
| { | ||
| return serialHalLogSuppressed.load(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File map ==\n'
git ls-files 'src/RedirectablePrint.cpp' 'src/mesh/StreamAPI.cpp' | sed 's#^`#-` #'
printf '\n== RedirectablePrint.cpp around suppression + log guard ==\n'
sed -n '1,220p' src/RedirectablePrint.cpp | cat -n
printf '\n== StreamAPI.cpp references to serialHalLogSuppressed ==\n'
rg -n "serialHalLogSuppressed|setSerialHalLogSuppressed|isSerialHalLogSuppressed|SERIALHAL_MAGIC|handleSerialHalCommand" src/mesh/StreamAPI.cpp src/RedirectablePrint.cpp
printf '\n== StreamAPI.cpp around the relevant logic ==\n'
sed -n '1,260p' src/mesh/StreamAPI.cpp | cat -nRepository: meshtastic/firmware
Length of output: 21999
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== RedirectablePrint.cpp around log() ==\n'
sed -n '260,340p' src/RedirectablePrint.cpp | cat -n
printf '\n== Search for other suppression resets or uses ==\n'
rg -n "setSerialHalLogSuppressed\(false\)|isSerialHalLogSuppressed\(\)|serialHalLogSuppressed|serialHalRxActive" src
printf '\n== StreamAPI.cpp timeout / cleanup related methods ==\n'
rg -n "checkConnectionTimeout|close|disconnect|timeout|serialHalRxActive" src/mesh/StreamAPI.cpp
sed -n '340,520p' src/mesh/StreamAPI.cpp | cat -nRepository: meshtastic/firmware
Length of output: 5824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== StreamAPI.cpp around the timeout/cleanup path ==\n'
sed -n '248,320p' src/mesh/StreamAPI.cpp | cat -nRepository: meshtastic/firmware
Length of output: 3920
Global SerialHal log suppression hides real errors.
serialHalLogSuppressed is process-wide, and RedirectablePrint::log() returns before any level filtering, so an in-flight SerialHal frame can suppress unrelated LOG_ERROR output across every destination. The flag is only cleared when StreamAPI finishes or resets framing; if the host stalls mid-frame, suppression can stick indefinitely. Scope this to the active stream/instance, or at least keep errors flowing and add a timeout fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/RedirectablePrint.cpp` around lines 24 - 39, The global process-wide
suppression in RedirectablePrint::setSerialHalLogSuppressed /
isSerialHalLogSuppressed is letting one SerialHal frame mute unrelated logs,
including LOG_ERROR, across all outputs. Move the suppression state to the
active StreamAPI/instance instead of the shared atomic, or change
RedirectablePrint::log so error-level messages still pass through regardless of
suppression. Also add a timeout/reset fallback so suppression cannot remain
stuck if framing never completes.
|
Why not boot into the micro mode like tdek boots into ble/basic mode? |
|
I would love to see this feature! Just looking up last night how to emulate a ch341 with an esp32, glad to know it's already in the process of being implemented! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mesh/StreamAPI.h (1)
47-48: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear SerialHal RX state on every parser abort.
serialHalRxActivebecomestrueas soon asSERIALHAL_MAGICis seen. In the suppliedStreamAPI::readStream()implementation, an oversized length resetsrxPtrbut leaves this flag—and log suppression—enabled. If the peer stops sending, subsequent polls return0indefinitely, causing a busy loop and suppressing logs until another valid frame arrives. Clear the flag and suppression state whenever a SerialHal frame is rejected or abandoned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/StreamAPI.h` around lines 47 - 48, Ensure every SerialHal parser abort or rejection in StreamAPI::readStream() clears both serialHalRxActive and the associated log-suppression state, including the oversized-length path that resets rxPtr. Preserve normal state while a valid SerialHal frame is in progress, but reset all SerialHal RX state when the frame is abandoned so subsequent polls do not remain suppressed.
🧹 Nitpick comments (3)
src/platform/portduino/PortduinoGlue.h (1)
96-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse camelCase for the new configuration members.
These new members use snake_case despite the project convention requiring camelCase. Rename them to
loraSerialDevice,loraSerialBaud, andloraSerialTimeoutMs, updating all references consistently.
As per coding guidelines: “Use PascalCase for classes, camelCase for functions and members, and UPPER_SNAKE_CASE for constants and defines.”Also applies to: 321-326
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/portduino/PortduinoGlue.h` around lines 96 - 98, Rename the new configuration members in PortduinoGlue to loraSerialDevice, loraSerialBaud, and loraSerialTimeoutMs, including their declarations and all references in the additional affected locations, while preserving their existing behavior.Source: Coding guidelines
src/main.cpp (1)
199-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense these comments to the required two lines maximum.
src/main.cpp#L199-L202: retain only that reusingSPI_HSPIprevents a second HSPI initialization from invalidating the LoRa SPI handle.src/main.cpp#L625-L628: state only the PMU-rail ordering requirement.src/main.cpp#L1115-L1124: retain the SPI2 restart reason and whySPI.end()precedesSPI.begin().As per coding guidelines, “Keep comments minimal—one or two lines maximum—and comment only when the reason is not obvious.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.cpp` around lines 199 - 202, Condense the comments at src/main.cpp lines 199-202, 625-628, and 1115-1124 to no more than two lines each: retain only that reusing SPI_HSPI prevents a second HSPI initialization from invalidating the LoRa SPI handle; state only the PMU-rail ordering requirement; and retain the SPI2 restart reason plus why SPI.end() must precede SPI.begin().Source: Coding guidelines
src/mesh/StreamAPI.h (1)
66-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep new StreamAPI comments within the project convention.
Both additions use multi-line explanatory blocks where the repository requires minimal comments.
src/mesh/StreamAPI.h#L66-L76: reduce the response API documentation to one concise line.src/mesh/StreamAPI.h#L96-L101: reduce the handler documentation to one concise line.As per coding guidelines, comments must be minimal—one or two lines maximum and only explain non-obvious rationale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/mesh/StreamAPI.h` around lines 66 - 76, Reduce the multi-line documentation above StreamAPI::emitSerialHalResponse in src/mesh/StreamAPI.h:66-76 to one concise line describing its purpose. Also reduce the handler documentation at src/mesh/StreamAPI.h:96-101 to one concise line, preserving only essential non-obvious context.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/mesh/StreamAPI.h`:
- Around line 47-48: Ensure every SerialHal parser abort or rejection in
StreamAPI::readStream() clears both serialHalRxActive and the associated
log-suppression state, including the oversized-length path that resets rxPtr.
Preserve normal state while a valid SerialHal frame is in progress, but reset
all SerialHal RX state when the frame is abandoned so subsequent polls do not
remain suppressed.
---
Nitpick comments:
In `@src/main.cpp`:
- Around line 199-202: Condense the comments at src/main.cpp lines 199-202,
625-628, and 1115-1124 to no more than two lines each: retain only that reusing
SPI_HSPI prevents a second HSPI initialization from invalidating the LoRa SPI
handle; state only the PMU-rail ordering requirement; and retain the SPI2
restart reason plus why SPI.end() must precede SPI.begin().
In `@src/mesh/StreamAPI.h`:
- Around line 66-76: Reduce the multi-line documentation above
StreamAPI::emitSerialHalResponse in src/mesh/StreamAPI.h:66-76 to one concise
line describing its purpose. Also reduce the handler documentation at
src/mesh/StreamAPI.h:96-101 to one concise line, preserving only essential
non-obvious context.
In `@src/platform/portduino/PortduinoGlue.h`:
- Around line 96-98: Rename the new configuration members in PortduinoGlue to
loraSerialDevice, loraSerialBaud, and loraSerialTimeoutMs, including their
declarations and all references in the additional affected locations, while
preserving their existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22a1ca6e-4cd0-4344-b5b2-12437771e88a
📒 Files selected for processing (7)
src/RedirectablePrint.cppsrc/main.cppsrc/mesh/RadioInterface.cppsrc/mesh/StreamAPI.cppsrc/mesh/StreamAPI.hsrc/platform/portduino/PortduinoGlue.cppsrc/platform/portduino/PortduinoGlue.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/mesh/RadioInterface.cpp
- src/RedirectablePrint.cpp
- src/platform/portduino/PortduinoGlue.cpp
- src/mesh/StreamAPI.cpp
The idea is to use a micro firmware for devices we can't support with the full-fat Meshtastic firmware. |
5 participants
Depends on meshtastic/protobufs#904
This adds two big chunks of code, half of which only lives inside the Portduino platform. It gives handheld nodes the ability to work as a proxy for radiolib running on Native. In other words, a node can effectively emulate a ch341 device.
There's also a pretty obvious path here to build a Meshtastic-micro firmware, that only provides this functionality. This should run on devices that were previously completely unworkable, and at least provide some use-case for them.
Lots of copilot usage here, and probably a few bits where code can be cleaned up as a result, but overall seems pretty sane.
Summary by CodeRabbit
New Features
Bug Fixes
serial_hal_onlystartup behavior to skip LoRa radio initialization and avoid unnecessary radio error reporting.