Conversation
… with OTA support - Adopted refactored echo relay mechanism from serial_bus (echo_set_target, echo_relay_handler) - Preserved OTA functionality (ota::bus_tick, ota::bus_handle_frame) - Replaced BusFrame with IncomingMessage for consistency - Removed duplicate includes - Merged handle_frame() and handle_message() to support both OTA and command processing
Resolved conflicts keeping main's updated serial bus implementation while preserving OTA functionality: - Kept ota.h include and ota_session member - Added ota::bus_handle_frame call to handle_incoming_message - Added ota::bus_tick call to step() - Used main's callback-based echo system - Used main's updated naming (make_coordinator, enqueue_outgoing_message, etc.) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
falkoschindler
left a comment
There was a problem hiding this comment.
@JensOgorek Can you please have a look at the following issues found by prompting Opus 4.6 with "/review 182". Especially MAJOR-1 needs your judgement since you're more familiar with the code's intention. The remaining issues are probably easy to fix, some of them only need a comment to please the reader (either AI or future-us).
MAJOR
1. Timeout error response sent to wrong node
In serial_bus.cpp:60-67, when bus_tick detects a timeout, fail() writes the error response and then calls bus_reset_session() which zeroes session.sender. When step() then tries to dispatch the response, it sends to sender=0 instead of the actual peer:
// step()
if (this->otb_session.handle != 0) {
otb::bus_tick(this->otb_session, millis()); // fail() resets sender to 0
if (this->otb_session.response_length > 0) {
this->enqueue_outgoing_message(
this->otb_session.sender, ...); // sender is now 0!Fix: save the sender before bus_reset_session, or have bus_reset_session preserve sender/response when there's a pending response. Alternatively, dispatch the response inside bus_tick itself (or pass a send callback).
2. KeyboardInterrupt doesn't abort the firmware session
In otb_update.py:89-91, KeyboardInterrupt just prints and exits. The firmware session stays alive until the 10s timeout. By contrast, the OtbError handler does send __OTB_ABORT__. The interrupt handler should also attempt an abort:
except KeyboardInterrupt:
print('\nInterrupted')
transact('__OTB_ABORT__')
sys.exit(1)3. --reset-partition uses hardcoded flash addresses
In espresso.py:208-209, reset_partition() erases 0xf000 / 0x2000 which is the default otadata location. This will silently break if the partition table ever changes. A comment documenting the assumption (and which partition table it matches) would reduce the risk.
CLEANUP
4. Duplicate response dispatch pattern
The "check response_length, enqueue, clear" pattern appears identically in serial_bus.cpp:62-66 (for timeout) and serial_bus.cpp:227-232 (for immediate responses). Extracting a small helper like send_otb_response_if_pending(sender) would deduplicate this and also fix issue #1 above:
void SerialBus::send_otb_response(uint8_t to) {
if (this->otb_session.response_length > 0) {
this->enqueue_outgoing_message(to, this->otb_session.response,
this->otb_session.response_length);
this->otb_session.response_length = 0;
}
}5. CHUNK_SIZE / WINDOW not linked to firmware constants
otb_update.py:10-11 defines CHUNK_SIZE = 174 and WINDOW = 8 which must stay in sync with BUS_OTB_CHUNK_SIZE in otb.h:21. A comment noting the dependency would help.
6. Protocol flow diagram has a missing __
In docs/tools.md line 164 of the diff, the flow diagram shows __OTB_ACK_COMMIT instead of __OTB_ACK_COMMIT__ (missing trailing underscores). Minor but inconsistent with the rest of the diagram.
|
Updated the code with the suggestions by the review. Also did some AI based reviews myself and tested the code afterwards to ensure functionality. |
falkoschindler
left a comment
There was a problem hiding this comment.
@JensOgorek I finally finished this review. Please have a look into my recent 13 commits with some smaller and some larger findings and check if you agree. It still compiles, but you should perform a final hardware test.
Besides some architectural improvements, I'm particularly glad that I could remove the custom implementation of a base64 decoder. I wonder why the AI didn't spot it.
Opus still finds issues when asked for a review, but nothing substantial as far as I can tell.
|
@falkoschindler perfect. I will take a look and test it tomorrow 👍 |
|
The handling of OTB response messages (ACK/ERROR) back to the coordinator had no explicit handlers in |
Motivation
Enable firmware updates for devices connected via SerialBus. Since we have serial access to the bus coordinator, we can use it to relay firmware chunks to remote nodes without requiring direct USB connection to each device.
Implementation
Python tool (
otb_update.py):bus.send()commands--expanderflag to pause broadcasts during transferwait_ack,transact)Lizard firmware (
main/utils/otb.cpp):__OTB_BEGIN__,__OTB_CHUNK_<seq>__,__OTB_COMMIT__, etc.)__OTB_ACK_BEGIN__,__OTB_ACK_CHUNK_<seq>__,__OTB_ACK_COMMIT__)Broadcast pausing (
core.pause_broadcasts()/core.resume_broadcasts()):broadcast_pausedflag inmodule.cppExpander buffer increase:
Removed
core.ota(url, md5, sha256)methodmain.cppstartupProgress