Speed up Frame printing to (finally) solve missing AVCLAN bus frames

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

src/avclan/frame.cc

@@ -6,8 +6,11 @@
 #include <cstdint>
 #include <cstdio>
 #include <cstring>
+#include <type_traits>
 
 #include "frame.hpp"
+#include "hal/stdio.h"
+#include "stdshim.hpp"
 
 #if defined(AVCLAN_FRAME_POOL_N)
   #include <array>
@@ -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 <class T>
+  requires std::is_unsigned_v<T>
+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<uint8_t>(is_unicast);
+    *bptr++ = static_cast<char>(is_unicast);
 
     // Send addresses in big-endian order
-    *bptr++ = static_cast<uint8_t>(controller_addr >> 8);
-    *bptr++ = static_cast<uint8_t>(controller_addr);
-    *bptr++ = static_cast<uint8_t>(peripheral_addr >> 8);
-    *bptr++ = static_cast<uint8_t>(peripheral_addr);
+    *bptr++ = static_cast<char>(controller_addr >> 8);
+    *bptr++ = static_cast<char>(controller_addr);
+    *bptr++ = static_cast<char>(peripheral_addr >> 8);
+    *bptr++ = static_cast<char>(peripheral_addr);
 
-    *bptr++ = control;
-    *bptr++ = length;
-    fwrite(buffer, 1, 8, stdout);
-    fwrite(data, 1, length, stdout);
+    *bptr++ = static_cast<char>(control);
+    *bptr++ = static_cast<char>(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<unsigned>(is_unicast));
-    printf(" 0x%03X", static_cast<unsigned>(controller_addr & 0x0FFF));
-    printf(" 0x%03X", static_cast<unsigned>(peripheral_addr & 0x0FFF));
-    printf(" 0x%X", static_cast<unsigned>(control & 0x0F));
-    printf(" 0x%X", static_cast<unsigned>(length & 0x0F));
 
-    for (uint8_t i = 0; i < length; i++) {
-      printf(" 0x%02X", static_cast<unsigned>(data[i]));
-    }
-    putchar('\n');
+    stdio_write_nonblock(buffer, static_cast<size_t>(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<uint16_t>(controller_addr & 0x0FFF),
+                .width = 3},
+        field_t{.value = static_cast<uint16_t>(peripheral_addr & 0x0FFF),
+                .width = 3},
+        field_t{.value = static_cast<uint16_t>(control & 0x0F), .width = 1},
+        field_t{.value = static_cast<uint16_t>(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<size_t>(bptr - buffer));
 }
 
 Error::Parse Frame::parse(const uint8_t *bytes, uint8_t len) {

src/avclan/hal/stdio.h

@@ -3,6 +3,8 @@
 
 #pragma once
 
+#include <stddef.h>
+
 #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
+//    <stdio.h> 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

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 <cstdint>
+  #include <type_traits>
+
+namespace avclan {
+// No <charconv> (nor the <system_error> 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 <class T>
+  requires std::is_integral_v<T> && std::is_unsigned_v<T>
+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<unsigned>(value % static_cast<T>(base));
+    digits[ndigits++] =
+        static_cast<char>(digit < 10 ? '0' + digit : 'a' + (digit - 10));
+    value = static_cast<T>(value / static_cast<T>(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 <charconv>
+
+namespace avclan {
+using errc = std::errc;
+using std::to_chars;
+using std::to_chars_result;
+} // namespace avclan
+#endif

src/avclan/target/avr-attiny3216/stdio_avr.c

@@ -4,11 +4,27 @@
 #include <avr/io.h>
 #include <stdint.h>
 #include <stdio.h>
+#include <string.h>
+#include <util/atomic.h>
 
 #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
 }