From 0bef565564410a10a2d4edbd34f2e2cc4b40ca7d Mon Sep 17 00:00:00 2001 From: Allen Hill Date: Thu, 13 Aug 2026 17:34:15 -0700 Subject: [PATCH] Speed up Frame printing to (finally) solve missing AVCLAN bus frames Co-Authored-By: Claude Opus 4.8 --- src/avclan/frame.cc | 106 +++++++++++++++---- src/avclan/hal/stdio.h | 21 +++- src/avclan/stdshim.hpp | 47 ++++++++ src/avclan/target/avr-attiny3216/stdio_avr.c | 82 +++++++++++++- 4 files changed, 226 insertions(+), 30 deletions(-) diff --git a/src/avclan/frame.cc b/src/avclan/frame.cc index 0c3e5c9..d076e4b 100644 --- a/src/avclan/frame.cc +++ b/src/avclan/frame.cc @@ -6,8 +6,11 @@ #include #include #include +#include #include "frame.hpp" +#include "hal/stdio.h" +#include "stdshim.hpp" #if defined(AVCLAN_FRAME_POOL_N) #include @@ -65,40 +68,97 @@ void Frame::operator delete(void *ptr) noexcept { } #endif +namespace { +// Emit `value` as at least `width` (lowercase, as to_chars emits) hex digits, +// zero-padded. `width` must be <= 3 (see the padding below); every call site +// reserves its field's worst case in the destination buffer, so this can't +// overflow. +template + requires std::is_unsigned_v +char *put_hex(char *dest, T value, uint8_t width) { + // to_chars emits the minimum number of digits, so work out up front how many + // that will be to know how much zero padding goes in front of them. At most + // two iterations for the field widths used here. + uint8_t ndigits = 1; + for (T rest = value; rest >= 16; rest /= 16) + ndigits++; + + // At most two pad digits (width <= 3). Written out rather than as a counted + // loop because GCC turns that into a memset() call — call overhead an order + // of magnitude above the one or two stores it replaces. + if (ndigits < width) { + *dest++ = '0'; + if (ndigits + 1 < width) + *dest++ = '0'; + } + + return to_chars(dest, dest + ndigits, value, 16).ptr; +} + +// " 0xNNN" for each field, plus the leading unicast digit and trailing newline +constexpr uint8_t LINE_MAX = 1 + (4 * 6) + (Frame::MAXLENGTH * 5) + 1; +// The same buffer serves the binary branch, which is the shorter of the two +static_assert(LINE_MAX >= 8 + Frame::MAXLENGTH + 2); +} // namespace + void Frame::print(Frame::Print print) const { + // The AVC-LAN read loop polls for start bits between calls, so emitting per + // character (~3 µs each through stdio) would blow past a start bit's ~169 µs + // and lose the next frame. + char buffer[LINE_MAX]; + char *bptr = buffer; + if (print.binary) { - uint8_t buffer[8]; - uint8_t *bptr = buffer; *bptr++ = 0x10; // Data Link Escape, signaling binary data forthcoming - *bptr++ = static_cast(is_unicast); + *bptr++ = static_cast(is_unicast); // Send addresses in big-endian order - *bptr++ = static_cast(controller_addr >> 8); - *bptr++ = static_cast(controller_addr); - *bptr++ = static_cast(peripheral_addr >> 8); - *bptr++ = static_cast(peripheral_addr); + *bptr++ = static_cast(controller_addr >> 8); + *bptr++ = static_cast(controller_addr); + *bptr++ = static_cast(peripheral_addr >> 8); + *bptr++ = static_cast(peripheral_addr); - *bptr++ = control; - *bptr++ = length; - fwrite(buffer, 1, 8, stdout); - fwrite(data, 1, length, stdout); + *bptr++ = static_cast(control); + *bptr++ = static_cast(length); + + memcpy(bptr, data, length); + bptr += length; - bptr = buffer; *bptr++ = 0x17; // End of transmission block *bptr++ = 0x0A; // \n - fwrite(buffer, 1, 2, stdout); - } else { - printf("%X", static_cast(is_unicast)); - printf(" 0x%03X", static_cast(controller_addr & 0x0FFF)); - printf(" 0x%03X", static_cast(peripheral_addr & 0x0FFF)); - printf(" 0x%X", static_cast(control & 0x0F)); - printf(" 0x%X", static_cast(length & 0x0F)); - for (uint8_t i = 0; i < length; i++) { - printf(" 0x%02X", static_cast(data[i])); - } - putchar('\n'); + stdio_write_nonblock(buffer, static_cast(bptr - buffer)); + return; } + + struct field_t { + uint16_t value; + uint8_t width; // minimum digits, matching the old %03x / %x formats + }; + + *bptr++ = is_unicast ? '1' : '0'; + for (const field_t field : + {field_t{.value = static_cast(controller_addr & 0x0FFF), + .width = 3}, + field_t{.value = static_cast(peripheral_addr & 0x0FFF), + .width = 3}, + field_t{.value = static_cast(control & 0x0F), .width = 1}, + field_t{.value = static_cast(length & 0x0F), .width = 1}}) { + *bptr++ = ' '; + *bptr++ = '0'; + *bptr++ = 'x'; + bptr = put_hex(bptr, field.value, field.width); + } + + for (uint8_t i = 0; i < length; i++) { + *bptr++ = ' '; + *bptr++ = '0'; + *bptr++ = 'x'; + bptr = put_hex(bptr, data[i], 2); + } + *bptr++ = '\n'; + + stdio_write_nonblock(buffer, static_cast(bptr - buffer)); } Error::Parse Frame::parse(const uint8_t *bytes, uint8_t len) { diff --git a/src/avclan/hal/stdio.h b/src/avclan/hal/stdio.h index 9743392..16b026f 100644 --- a/src/avclan/hal/stdio.h +++ b/src/avclan/hal/stdio.h @@ -3,6 +3,8 @@ #pragma once +#include + #ifdef __cplusplus extern "C" { #endif @@ -11,10 +13,25 @@ extern "C" { // functions. Assumptions/invariants: // - stdin MUST be non-blocking (ie. getchar() returns EOF immediately when // empty). Necessary to avoid stalling the REPL poll loop. -// - stdout is *raw* — no '\n' -> "\r\n" translation: bare LF and binary frame -// payloads (fwrite) are emitted verbatim +// - stdout is *raw*. There is no '\n' -> "\r\n" translation. The port does not +// change a bare LF or a binary frame payload. Writes through +// and writes through stdio_write_nonblock() must reach the same +// stream, in order. void stdio_init(void); +// Queue a buffer onto stdout--if it has space, or return false. +// +// If stdout cannot accept all `len` bytes now, the function queues +// a "\n!\n" indicator onto stdout, overwriting the last 3 chars if stdout is +// full. The caller must not assume that the buffer went out. +// +// A port MUST emit that indicator for every dropped buffer, and MUST be able to +// emit it even if stdout is full. A reader detects the loss as a line that +// holds only '!'. In the overwrite case the buffer in front of that line is +// truncated, so a reader MUST tolerate one malformed buffer there. The return +// value is useful only for diagnostics. +bool stdio_write_nonblock(const void *buf, size_t len); + #ifdef __cplusplus } #endif diff --git a/src/avclan/stdshim.hpp b/src/avclan/stdshim.hpp index 08c0715..e46e7ea 100644 --- a/src/avclan/stdshim.hpp +++ b/src/avclan/stdshim.hpp @@ -23,3 +23,50 @@ using std::unexpect; using std::unexpect_t; } // namespace avclan #endif + +#if !defined(__cpp_lib_to_chars) || __cpp_lib_to_chars < 201611L + #include + #include + +namespace avclan { +// No (nor the its result type needs) in the +// freestanding stdlib, so mirror the std API surface for the unsigned-integral +// overload — the only one used here. Values are those of std::errc, so call +// sites written against either spelling behave the same. +enum class errc : uint8_t { value_too_large = 75 }; + +struct to_chars_result { + char *ptr; + errc ec; +}; + +template + requires std::is_integral_v && std::is_unsigned_v +constexpr to_chars_result to_chars(char *first, char *last, T value, + int base = 10) { + char digits[sizeof(T) * 8]; // worst case: base 2 + uint8_t ndigits = 0; + do { + const auto digit = static_cast(value % static_cast(base)); + digits[ndigits++] = + static_cast(digit < 10 ? '0' + digit : 'a' + (digit - 10)); + value = static_cast(value / static_cast(base)); + } while (value != 0); + + if ((last - first) < ndigits) + return {.ptr = last, .ec = errc::value_too_large}; + + while (ndigits > 0) + *first++ = digits[--ndigits]; + return {.ptr = first, .ec = errc{}}; +} +} // namespace avclan +#else + #include + +namespace avclan { +using errc = std::errc; +using std::to_chars; +using std::to_chars_result; +} // namespace avclan +#endif diff --git a/src/avclan/target/avr-attiny3216/stdio_avr.c b/src/avclan/target/avr-attiny3216/stdio_avr.c index bd9a5d2..a514ade 100644 --- a/src/avclan/target/avr-attiny3216/stdio_avr.c +++ b/src/avclan/target/avr-attiny3216/stdio_avr.c @@ -4,11 +4,27 @@ #include #include #include +#include +#include #include "hal/stdio.h" #include "timing_avr.h" // IWYU pragma: export #include "usart.h" // jnk0le AVR-UART-lib +// TX ring owned by the jnk0le lib. `tx0_Head` is the index of the last byte +// written, so the next write goes to head+1. The DRE ISR reads tail+1 and then +// advances `tx0_Tail`. The ring is empty when the two indexes are equal. The +// ring holds at most TX0_BUFFER_SIZE-1 bytes. +extern char tx0_buffer[TX0_BUFFER_SIZE]; + +// stdio_write_nonblock() is all-or-nothing. If the ring cannot hold the largest +// buffer that the app writes, the function drops every buffer and queues none. +// The largest buffer is a text frame log line (Frame::print). That line is +// ~186 bytes at MAXLENGTH=32. The ring therefore needs headroom above this +// size. +static_assert(TX0_BUFFER_SIZE - 1 >= 192, + "TX ring too small to hold a whole frame log line"); + // Raw stdout backend: emit the byte verbatim, no '\n' -> "\r\n" translation // (unlike the lib's uart_putchar). Keeps binary frame payloads intact. static int stdio_putchar(char data, FILE *stream) { @@ -17,6 +33,62 @@ static int stdio_putchar(char data, FILE *stream) { return 0; } +bool stdio_write_nonblock(const void *buf, size_t len) { + if (len == 0) + return true; + + // A single unguarded read of each index is enough: + // - Only main writes tx0_Head. + // - Only the DRE ISR writes tx0_Tail. + // - 8-bit accesses are atomic on AVR. + // If the ISR advances the tail during this function, the ring has more free + // space than this function measured. That direction is safe. + const uint8_t head = tx0_Head; + const uint8_t space = (uint8_t)((tx0_Tail - head - 1) & TX0_BUFFER_MASK); + if (len > space) { + // The ring cannot take the whole buffer, so drop it and queue the indicator + // instead. A buffer may be refused because it is larger than + // available space or because the ring is full. For the former case, the + // full indicator can be appended without truncating previously queued + // bytes. + if (space >= 3) { + tx0_buffer[(head + 1) & TX0_BUFFER_MASK] = '\n'; + tx0_buffer[(head + 2) & TX0_BUFFER_MASK] = '!'; + tx0_buffer[(head + 3) & TX0_BUFFER_MASK] = '\n'; + tx0_Head = (uint8_t)((head + 3) & TX0_BUFFER_MASK); + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { USART0.CTRLA |= USART_DREIE_bm; } + } else { + // No room for even the indicator, so it goes over the last three queued + // bytes. `space` and the queued byte count always sum to + // TX0_BUFFER_MASK, so space < 3 means at least 253 bytes are queued and + // all three writes land inside them. The ISR already drains the ring, so + // DREIE needs no change. + tx0_buffer[(head - 2) & TX0_BUFFER_MASK] = '\n'; + tx0_buffer[(head - 1) & TX0_BUFFER_MASK] = '!'; + tx0_buffer[head] = '\n'; + } + return false; + } + + const uint8_t start = (uint8_t)((head + 1) & TX0_BUFFER_MASK); + const size_t contiguous = TX0_BUFFER_SIZE - start; + + if (len <= contiguous) { + memcpy(&tx0_buffer[start], buf, len); + } else { // wraps the end of the ring + memcpy(&tx0_buffer[start], buf, contiguous); + memcpy(&tx0_buffer[0], (const char *)buf + contiguous, len - contiguous); + } + + // Publish the bytes only after all of them are in place. This is a single + // store, so the ISR never sees a partly filled buffer. + tx0_Head = (uint8_t)((head + len) & TX0_BUFFER_MASK); + + // The ISR also writes CTRLA (to clear DREIE when it drains the last byte). + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { USART0.CTRLA |= USART_DREIE_bm; } + return true; +} + // Non-blocking stdin backend: next received byte, or _FDEV_EOF when the RX ring // is empty (uart0_getData() returns a negative value when there is no data). static int stdio_getchar(FILE *stream) { @@ -34,14 +106,14 @@ void stdio_init(void) { // The lib's uart0_init configures the USART registers/baud but not the pins; // keep the ATtiny3216 pin-mux the old driver did. PORTMUX.CTRLB = PORTMUX_USART0_ALTERNATE_gc; // TxD/RxD on PA1/PA2 - PORTA.DIRSET = PIN1_bm; // TxD output - PORTA.DIRCLR = PIN2_bm; // RxD input + PORTA.DIRSET = PIN1_bm; // TxD output + PORTA.DIRCLR = PIN2_bm; // RxD input - #ifdef USE_DOUBLE_SPEED +#ifdef USE_DOUBLE_SPEED uart0_init(DOUBLE_BAUD_CALC(1200000)); - #else +#else uart0_init(BAUD_CALC(1200000)); - #endif +#endif stdout = stdin = &stdio_stream; // printf/fputs/fwrite + non-blocking getchar }