Repository navigation
Give every serial bus and every checked line its own buffer - #367
Conversation
Serial::write_checked_line kept the checksum trailer in a static buffer, and SerialBus kept the frame it reads, the frame it writes and the slot frame it sends in static buffers shared by all buses. The communication task of a bus runs next to the main task and next to the tasks of other buses, so with two buses, or a bus next to an expander, one task could send its line with the checksum of another or write another bus's poll on its own wire: on F30 the expander P0 as a second bus received the plexus bus's polls. The checksum trailer now lives on the stack and the three frame buffers in the bus.
The bus's frame buffers move into the header section of the communication task, next to received_config, which stays off the task's stack for the same reason; they go to the end of it, so the members the task reads every cycle keep their short offsets. send_message drops its const instead of writing a mutable buffer, and the buffers are taken by reference, so sizeof still gives their size.
send_message builds at most the header $$255:255$$ and a payload below PAYLOAD_CAPACITY, so the 512 B of the old shared FRAME_BUFFER_SIZE were twice what its frame needs, and since the buffers moved into the bus, every bus paid the surplus from the heap. tx_frame now takes 16 + PAYLOAD_CAPACITY with a static_assert for the longest header; rx_frame keeps its 512 B for any line a peer may send. handle_incoming_message copied an echo into the last static buffer of the file before printing it, although process_uart terminates the payload; it now prints and compares the payload itself. The check for a negative result of csprintf in send_message goes, csprintf throws instead. Cost: heap per bus 1040 B instead of 1280 B, static RAM -256 B. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WgKVNZiHK4ZEsLBzkwycMa
falkoschindler
left a comment
There was a problem hiding this comment.
@JensOgorek the analysis holds: every caller of process_uart, send_message and send_frame_slots sits in the communication task of its own bus, the main task reaches a bus only through its queues, and the remaining static buffers in the repository (expander, echo, get_output, the echo copy in handle_incoming_message) run on the main task only. The two bench tests cover both races. I fixed three small things on the branch in 1e46171; nothing is open for you.
Fixed in 1e46171
tx_framewas twice its largest frame.send_messagebuilds at most the header$$255:255$$(11 bytes) and a payload belowPAYLOAD_CAPACITY, so of the 512 B inherited from the sharedFRAME_BUFFER_SIZEevery bus paid 240 B of heap for nothing once the buffer became a member.tx_framenow takes16 + PAYLOAD_CAPACITYwith astatic_assertfor the longest header;rx_framekeeps its 512 B for any line a peer may send.- The last static buffer of
SerialBus.handle_incoming_messagecopied an echo into astatic char buffer[PAYLOAD_CAPACITY]before printing it, althoughprocess_uartalready terminates the payload. It runs on the main task only, so it was no race, but 256 B of static RAM for nothing; it now prints and compares the payload itself. - Dead check in
send_message. Theheader_len < 0branch aftercsprintfcould never run:csprintfthrows on an encoding error or a too-small buffer, and both land in thecatchofcommunication_loop.
Against main ec5d268 the branch now takes 160 B less flash on the ESP32 and 1552 B less static RAM; each bus takes 1040 B of heap instead of 1280 B. The cost line of the description is updated accordingly.
Retest of
|
Motivation
When two tasks of the same ESP32 write checked lines at the same time, a line can go out with the other task's checksum or with the other task's content, and the receiver loses it.
Serial::write_checked_lineformats the checksum trailer into a static buffer, andSerialBuskeeps the frame it reads, the frame it writes and the frame it copies out of a telemetry slot in static buffers that all buses share. The main task (core 0) writes the lines of an expander, and every bus writes its frames from its own communication task (core 1). So the trailer race hits every ESP32 with an expander next to a serial bus today, and both races hit every ESP32 with two serial buses, as with port expanders as bus peers (#348).On a bench pair of two ESP32s, main loses lines in both setups within 2 minutes: with an expander and a bus 58 of 24 000, with two buses 504 of 12 000 messages of one bus. The receiver drops such a line with a checksum warning or, if the other bus replaced the whole frame, ignores it without any warning. With this PR, none was lost in 10 minutes of each setup. It was found on F30, whose P0 ran as a second bus peer in a test setup and received garbled polls of the other bus every 5 to 10 minutes, and none in 40 minutes with this change.
Implementation
Serial::write_checked_linebuilds the checksum trailer on the stack.SerialBuskeeps its read, write and slot buffers as members, one set per bus; each set is used only by the communication task of its bus. The write buffer is sized by the largest framesend_messagecan build (16 +PAYLOAD_CAPACITY), the read buffer keeps 512 B for any line a peer may send.handle_incoming_messageprints an echo from the payload itself instead of copying it into the last static buffer of the file, and the dead check for a negative result ofcsprintfinsend_messageis gone.Cost: −160 B flash on the ESP32; static RAM −1552 B. Each bus takes 1040 B of heap for its frame buffers, so the RAM use only grows from the second bus on.
Progress
Bench test 1: an expander and a serial bus on the same core
Two ESP32s wired like a Robot Brain's core and P0: core GPIO27 → target RX (GPIO3), target TX (GPIO1) → core GPIO26, and core GPIO25 → target GPIO0, which is free for a second line once the expander runs without boot and enable pins. The target ran main
ec5d268throughout; only the core's firmware changed. Both were built with ESP-IDF 5.3.1.Core:
Target, sent with
p0.run(…):The target's GPIO33 is not connected, so peer 2 never answers: the coordinator's poll times out every 250 ms, and then it sends the queued messages in a burst. Each main loop (10 ms) writes one expander line from the main task and queues one bus message for the bus task.
gowas true for 120 s or 600 s; afterwards the core printednp, nband the targetn, m.ec5d268ba49826ba49826On main, every lost line has a warning. The bus frames came with their own content but with the trailer of the expander line written at the same moment, e.g.
serial bus zbus checksum mismatch: $$1:2$$m = m + 1@27: the frame's checksum is1e, and27is the checksum ofn = n + 1. Once it was@53, the checksum of the expander's pingcore.print('__PONG__').Bench test 2: two serial buses on the same core
Same pair and wiring, no expander. Bus O runs on UART2 to the target's GPIO0; bus X runs on UART1 with its TX on the unconnected GPIO4 and has no peer. The target ran main
ec5d268with this startup and printed its counter every second:Core:
The message on bus O ends with 80 spaces, so that the 25 messages of one burst (100 bytes each) overflow the 2 KB TX ring and bus O's task blocks in the middle of a line, while bus X's task sends its poll or its own burst.
ec5d268ba49826ba4982632b21971e46171343 of the 346 warnings with bus X's content read
$$1:3$$__POLL__1⎵⎵⎵…@27: the first 15 bytes are bus X's poll of its peer 3, the1and the spaces are the rest of bus O's message, and27is the checksum of the poll; in the other three, bus X'sk = k + 1stood in front. The remaining 12 warnings had bus O's content with bus X's trailer (1fof$$1:3$$k = k + 1,27of the poll). The remaining 146 lost messages raised no warning; presumably bus X replaced the whole frame, which then is a valid frame for node 3 that the target ignores.Bench check of 1e46171: echoes and the longest frames
Bench test 2's peer never answers, so it does not reach the echo path. Here the PC played peer 202 on the target's console wires (core GPIO27 and GPIO26) while the target was held in reset, and the core ran
1e46171as coordinator 201, so that every header it built was the longest one,$$201:202$$:bus[202]: Ready.andbus[202]: shortright after the 255-byte one. A payload of 256 bytes was rejected withcould not parse message, and the echo after it printed exactly again.bus.send(202, …)with 254 and 255 bytes arrived complete with a valid checksum; 256 bytes were rejected withpayload is too large for serial bus.core.printof 240 characters sent by the peer came back as a relayed echo of 250 bytes with a valid checksum.※