upgrade uart protocol to full avlos spec #393
Conversation
![preloop[bot]](https://avatars.githubusercontent.com/in/2645368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Preloop Code Review
Review Status: ❌ Changes Requested
Summary
This PR replaces the legacy ASCII UART protocol with Avlos binary framing, aligning UART behavior with CAN. The implementation is close, but there is a blocking issue that prevents zero-payload read requests from being accepted, plus a malformed-frame handling gap.
Findings Overview
| Severity | Count | Status |
|---|---|---|
| 🔴 CRITICAL | 0 | 0 new, 0 persisting |
| 🟠 HIGH | 1 | 1 new, 0 persisting |
| 🟡 MEDIUM | 2 | 2 new, 0 persisting |
| 🟢 LOW | 0 | 0 new, 0 persisting |
✅ What Looks Good
- Reuse of Avlos endpoint dispatch mirrors CAN handling, keeping protocol behavior consistent.
- UART docs now describe the binary framing and CRC clearly.
⚠️ Key Issues to Address
- [Quality]: UART read requests with zero payload are rejected due to an incorrect minimum frame size constant.
- Affected file:
firmware/src/uart/uart_interface.h:27
- Affected file:
💡 Suggestions
- Validate payload length (<= 8) before dispatch to prevent malformed frames from invoking endpoints with uninitialized data.
- Add at least a basic UART protocol test (simulation or HITL) to cover read/write framing and CRC validation.
📊 Resolved Since Last Review
- No prior Preloop comments on this PR.
Automated review by Preloop. Human verification recommended for critical findings.
📋 Preloop Review SummaryLast Updated: 2026-01-30 22:37:45Z Current StatusChanges Requested Issue Tracker🔴 Critical Issues
🟠 High Priority Issues
🟡 Medium Priority Issues
🟢 Low Priority / Suggestions
✅ Resolved Issues
Progress: 0 of 3 issues addressed This summary is automatically updated on each review cycle. |
yconst
left a comment
yconst
left a comment
There was a problem hiding this comment.
Addressed comments
📋 Preloop Review SummaryLast Updated: 2026-01-30 23:13:55Z Current StatusChanges Suggested Issue Tracker🔴 Critical Issues
🟠 High Priority Issues
🟡 Medium Priority Issues
🟢 Low Priority / Suggestions
✅ Resolved Issues
Progress: 2 of 4 issues addressed This summary is automatically updated on each review cycle. |
There was a problem hiding this comment.
🔍 Preloop Code Review
Review Status: 💬 Changes Suggested
Summary
The UART path is now aligned with Avlos binary framing, with CRC and length validation added. I only see a documentation gap that should be addressed to help users/hosts adopt the new protocol.
Findings Overview
| Severity | Count | Status |
|---|---|---|
| 🔴 CRITICAL | 0 | 0 new, 0 persisting |
| 🟠 HIGH | 0 | 0 new, 0 persisting |
| 🟡 MEDIUM | 1 | 1 new, 0 persisting |
| 🟢 LOW | 0 | 0 new, 0 persisting |
✅ What Looks Good
- Protocol framing, length checks, and CRC verification are now consistently applied.
- Defensive checks (endpoint bounds, payload sizing) reduce malformed frame exposure.
💡 Suggestions
- Add/update documentation for the UART Avlos framing and host expectations.
📄 Documentation Impact
- README.md / protocol docs: Describe Avlos UART frame format, length semantics, CRC, and supported commands.
- CHANGELOG: Note the UART protocol upgrade and any compatibility implications.
📊 Resolved Since Last Review
UART_FRAME_MIN_SIZE rejected zero‑payload reads✅Oversized payloads could reach endpoint without rejection✅
Automated review by Preloop. Human verification recommended for critical findings.
|
Let's review again |
There was a problem hiding this comment.
🔍 Preloop Code Review
Review Status: ❌ Changes Requested
Summary
This PR upgrades the UART interface to Avlos binary framing with CRC and endpoint dispatch, which is a solid direction. I found one high-severity correctness issue in the low-level RX/TX buffers plus a couple of medium issues around command validation and documentation alignment.
Findings Overview
| Severity | Count | Status |
|---|---|---|
| 🔴 CRITICAL | 0 | 0 new, 0 persisting |
| 🟠 HIGH | 1 | 1 new, 0 persisting |
| 🟡 MEDIUM | 2 | 2 new, 0 persisting |
| 🟢 LOW | 0 | 0 new, 0 persisting |
✅ What Looks Good
- Clean migration to Avlos framing with CRC validation and protocol hash checking.
- Defense-in-depth around payload length and endpoint bounds.
⚠️ Key Issues to Address
- [Quality]: Binary UART buffers stored as
charcan break sync matching on signed-char toolchains.- Affected file:
firmware/src/uart/uart_lowlevel.c:24
- Affected file:
💡 Suggestions
- Validate command bytes explicitly and reject unknown values to avoid unintended reads.
- Consider adding at least a simulation test covering UART framing/CRC parsing to lock in the new protocol.
📄 Documentation Impact
-
docs/interfaces/interfaces.rst: Update UART protocol section to describe the Avlos binary framing (sync/len/hash/CRC) and remove/mark the dot-protocol description.
📊 Resolved Since Last Review
UART min frame size rejected zero-payload reads✅Missing payload length guard could pass uninitialized data✅
Automated review by Preloop. Human verification recommended for critical findings.
firmware/src/uart/uart_lowlevel.c
Outdated
|
|
||
| char uart_rx_buf[96] = {0}; | ||
| // Receive buffer | ||
| char uart_rx_buf[UART_RX_BUFFER_SIZE] = {0}; |
There was a problem hiding this comment.
[HIGH] [Quality]
Affected files:
firmware/src/uart/uart_lowlevel.c:24
UART RX/TX buffers are declared as char, but the protocol is binary and uses the sync byte 0xAA. On toolchains where char is signed, that byte becomes negative in storage and comparisons against UART_SYNC_BYTE later in UART_process_message will fail, dropping all frames. It also risks sign-extension for bytes >= 0x80 during TX.
Recommendation:
Use uint8_t for RX/TX buffers and message arrays (and update related declarations in uart_lowlevel.h plus any casts in the ISR) so byte values are preserved consistently across platforms.
| memcpy(buffer, &uart_rx_msg[UART_FRAME_HEADER_SIZE], payload_len); | ||
| } | ||
|
|
||
| // Map command to Avlos command type |
There was a problem hiding this comment.
[MEDIUM] [Quality]
Affected files:
firmware/src/uart/uart_interface.c:171
Invalid command bytes currently fall back to AVLOS_CMD_READ, so malformed frames can trigger unintended reads. This deviates from stricter validation and makes fuzzing/line noise harder to diagnose.
Recommendation:
Reject unknown cmd values before mapping:
| // Map command to Avlos command type | |
| if (cmd != AVLOS_CMD_READ && cmd != AVLOS_CMD_WRITE) | |
| { | |
| return; // Invalid command | |
| } | |
| Avlos_Command avlos_cmd = (cmd == AVLOS_CMD_WRITE) ? AVLOS_CMD_WRITE : AVLOS_CMD_READ; |
| The UART port is TTL at 115200 baud. A regular FTDI-style USB to UART adapter should be sufficient. | ||
| - **Baud rate**: 115200 | ||
| - **Data bits**: 8 | ||
| - **Parity**: None |
There was a problem hiding this comment.
[MEDIUM] [Documentation]
Affected files:
docs/interfaces/interfaces.rst:43
The UART section still documents the legacy dot protocol, but this PR replaces UART with Avlos binary framing (sync/len/hash/payload/CRC). As-is, users will follow obsolete instructions.
Recommendation:
Update the UART protocol docs to describe the new frame format, CRC, protocol hash compatibility, and any client tooling expectations. If the dot protocol is intentionally deprecated, call that out explicitly.
📋 Preloop Review SummaryLast Updated: 2026-01-31 10:48:28 UTC Current Status❌ Changes Requested Issue Tracker🔴 Critical Issues
🟠 High Priority Issues
🟡 Medium Priority Issues
🟢 Low Priority / Suggestions
✅ Resolved Issues
Progress: 0 of 3 issues addressed This summary is automatically updated on each review cycle. |
firmware/src/uart/uart_lowlevel.h
Outdated
| char uart_tx_msg[96]; | ||
| uint8_t uart_tx_byte_idx; | ||
| // Receive buffer and state | ||
| extern char uart_rx_msg[UART_RX_BUFFER_SIZE]; |
There was a problem hiding this comment.
[MEDIUM] [Quality]
Affected files:
firmware/src/uart/uart_lowlevel.h:29firmware/src/uart/uart_lowlevel.c:24firmware/src/uart/uart_interface.c:109
The RX/TX buffers are char, but the new binary protocol uses 0xAA sync bytes. On toolchains where char is signed, 0xAA becomes negative and the sync check in UART_process_message can fail (and byte values can be sign-extended).
Recommendation:
Use uint8_t for RX/TX buffers and message arrays, and keep comparisons in unsigned byte space to make the framing deterministic across toolchains.
🔍 Preloop Code ReviewLast Updated: 2026-01-31 11:18:31 UTC 📝 SummaryThis PR upgrades UART handling to the Avlos binary spec and adds CRC/length validation. The core structure looks solid, but there are a few protocol robustness and documentation gaps to address before it’s fully production-safe. ✅ What Looks Good
|
| } | ||
|
|
||
| /** | ||
| * @brief Process received UART message using Avlos protocol |
There was a problem hiding this comment.
[MEDIUM] [Documentation]
Affected files:
firmware/src/uart/uart_interface.c:95docs/protocol/integrating.rst:7
The UART protocol is now full Avlos binary framing (sync byte, length semantics, CRC16-CCITT, payload limits), but the integration docs still describe Avlos mainly in CAN terms and don't document the UART frame layout. That makes it hard for external integrators to generate UART frames correctly.
Recommendation:
Add a short UART framing section in the protocol/integration docs (frame layout, length definition, CRC16-CCITT, max payload 8, and protocol hash behavior), and note that the previous ASCII UART protocol is replaced by the Avlos binary framing.
| * Frame format: | ||
| * | Sync (1) | Length (1) | Hash (1) | EP_ID (2) | CMD (1) | Payload (0-8) | CRC16 (2) | | ||
| */ | ||
| void UART_process_message(void) |
There was a problem hiding this comment.
[MEDIUM] [Testing]
Affected files:
firmware/src/uart/uart_interface.c:100
This introduces new UART framing/CRC validation, but there are no tests to cover key cases (zero-payload read, max-payload write, CRC/length rejection). That leaves the protocol change unverified.
Recommendation:
Add a small UART protocol test (simulation in studio/Python/tests or a firmware/HITL test) that exercises valid frames and rejects invalid CRC/length/payload sizes.
🔍 Preloop Code ReviewLast Updated: 2026-01-31 11:47 UTC 📝 SummaryUpgrades UART to Avlos binary framing with CRC validation and endpoint dispatch. Implementation looks consistent with the CAN path, but docs and tests need updates to make the new UART protocol verifiable and integrator-friendly. ✅ What Looks Good
|
| } | ||
|
|
||
| // Map command to Avlos command type | ||
| Avlos_Command avlos_cmd = (cmd == AVLOS_CMD_WRITE) ? AVLOS_CMD_WRITE : AVLOS_CMD_READ; |
There was a problem hiding this comment.
[MEDIUM] [Quality/BestPractice]
Affected files:
firmware/src/uart/uart_interface.c:171
cmd is coerced to READ unless it is WRITE, which drops AVLOS_CMD_CALL semantics. That means CALL requests are treated as READ and any endpoint that checks AVLOS_CMD_CALL (current or future generator output) won’t execute as intended. It also accepts unknown command values as READ.
Recommendation:
Map READ/WRITE/CALL explicitly and reject unknown command values. For example:
| Avlos_Command avlos_cmd = (cmd == AVLOS_CMD_WRITE) ? AVLOS_CMD_WRITE : AVLOS_CMD_READ; | |
| Avlos_Command avlos_cmd; | |
| switch (cmd) | |
| { | |
| case AVLOS_CMD_READ: | |
| avlos_cmd = AVLOS_CMD_READ; | |
| break; | |
| case AVLOS_CMD_WRITE: | |
| avlos_cmd = AVLOS_CMD_WRITE; | |
| break; | |
| case AVLOS_CMD_CALL: | |
| avlos_cmd = AVLOS_CMD_CALL; | |
| break; | |
| default: | |
| return; // Unknown command | |
| } |
|
|
||
| // Transmit buffer and state | ||
| extern uint8_t uart_tx_msg[UART_TX_BUFFER_SIZE]; | ||
| extern uint8_t uart_tx_byte_idx; |
There was a problem hiding this comment.
[LOW] [Quality]
Affected files:
firmware/src/uart/uart_lowlevel.h:29
uart_rx_msg_len, uart_tx_byte_idx, and the new uart_tx_frame_len are shared between the ISR and the main loop. Without volatile, the compiler can cache or reorder access, which can lead to missed updates in optimized builds.
Recommendation:
Mark these shared variables as volatile in the header and definitions, e.g.:
| extern uint8_t uart_tx_byte_idx; | |
| extern volatile uint8_t uart_rx_msg_len; | |
| extern volatile uint8_t uart_tx_byte_idx; | |
| extern volatile uint8_t uart_tx_frame_len; |
🔍 Preloop Code ReviewLast Updated: 2026-01-31 11:57 UTC 📝 SummaryThis PR upgrades UART framing to the Avlos binary protocol and adds CRC/length validation. The core framing work looks solid, but there are a few protocol-compliance, testing, and documentation gaps to address. ✅ What Looks Good
|
1 participant
Upgrade uart protocol to full avlos spec