Fix miscellaneous bugs identified by Claude

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

.clang-tidy

@@ -1,17 +1,21 @@
 ---
 Checks: |
   -*,
+  bugprone-*,
+  cppcoreguidelines-
   clang-diagnostic-*,
   clang-analyzer-*,
-  -clang-analyzer-core.NullDereference,
-  bugprone-*,
   misc-*,
   modernize-*,
   performance-*,
   readability-*,
+  -clang-analyzer-core.NullDereference,
   -modernize-use-nodiscard,
   -modernize-use-trailing-return-type,
   -readability-braces-around-statements,
+  -modernize-avoid-c-arrays,
+  -modernize-use-std-print,
+  -readability-magic-numbers,
 WarningsAsErrors: ""
 ExcludeHeaderFilterRegex: 'out/build'
 HeaderFilterRegex: '^src/.*'
@@ -26,7 +30,7 @@ CheckOptions:
   - key: readability-identifier-length.IgnoredVariableNames
     value: "(in|to|id)"
   - key: readability-identifier-length.IgnoredParameterNames
-    value: "(in|to)"
+    value: "(in|to|x)"
   - key: bugprone-argument-comment.CommentBoolLiterals
     value: "0"
   - key: bugprone-argument-comment.CommentCharacterLiterals

scripts/packet-analysis/extcap/avclan-pcap.py

@@ -61,7 +61,7 @@ PCAP_GLOBAL_HEADER = struct.pack(
 
 # Serial framing bytes emitted by AVCLAN_printframe(..., binary=1).
 DLE = 0x10  # start of a binary frame
-ETB = 0x17  # end of a binary frame (followed by \r\n)
+ETB = 0x17  # end of a binary frame (followed by \n; CR tolerated, see below)
 MAX_DATA_LEN = 32  # AVCLAN payload cap; longer "length" => bad framing
 
 BAUD = 1_200_000

src/avclan/bus.cc

@@ -34,12 +34,12 @@
 #include "avclan.h"
 #include "bus.hpp"
 #include "frame.hpp"
-#include "hal/phy.h" // bridge until phy has been ported
+#include "hal/phy.h"
 
 namespace {
-constexpr int ADDR_WIDTH = 12;
-constexpr int CONTROL_WIDTH = 4;
-constexpr int BYTE_WIDTH = 8;
+using Read = avclan::detail::Error::Read;
+using Send = avclan::detail::Error::Send;
+using Bit = avclan::detail::Bit;
 
 struct trailer_bits_t {};
 struct no_parity_t : trailer_bits_t {};   // raw bits (the broadcast bit)
@@ -61,15 +61,14 @@ public:
   ~Handle() { phy_guard_leave(); };
   Handle(const Handle &) = delete;
   Handle(Handle &&) = delete;
-  using Error = detail::Error;
 
   bool sendstartbit() { return phy_send_startbit(); };
-  auto readstartbit() -> Read { return phy_read_startbit(); };
+  Read readstartbit() { return phy_read_startbit(); };
 
   template <auto N, std::unsigned_integral T,
             std::derived_from<trailer_bits_t> Trailer>
     requires(sizeof(T) < 3 && N < 16 && !std::same_as<Trailer, with_ack_t>)
-  Error::Send send(T bits, Trailer /*tag*/) {
+  Send send(T bits, Trailer /*tag*/) {
     const auto parity = sendbits<N>(bits);
 
     if constexpr (std::is_same_v<Trailer, with_parity_t>)
@@ -80,22 +79,20 @@ public:
 
   template <auto N, std::unsigned_integral T>
     requires(sizeof(T) < 3 && N < 16)
-  Error::Send send(T bits, with_ack_t /*tag*/, bool expect_ack) {
+  Send send(T bits, with_ack_t /*tag*/, bool expect_ack) {
     send<N>(bits, with_parity);
 
-    if (expect_ack) {
-      if (!read_ACK())
-        return Send::NAK;
-    } else
-      sendbits<1>(1U); // still needs to fill the ack bit slot
+    if (expect_ack)
+      return read_ACK();
 
+    sendbits<1>(1U); // still need to fill the ack bit slot
     return Send{0};
   };
 
   template <auto N, std::unsigned_integral T,
             std::derived_from<trailer_bits_t> Trailer>
     requires(sizeof(T) < 3 && N < 16 && !std::same_as<Trailer, with_ack_t>)
-  Error::Read read(T *bits, Trailer /*tag*/) {
+  Read read(T *bits, Trailer /*tag*/) {
     const auto calc_parity = readbits<N>(bits);
     if constexpr (std::is_same_v<Trailer, with_parity_t>) {
       uint8_t read_parity;
@@ -108,7 +105,7 @@ public:
 
   template <auto N, std::unsigned_integral T, class F>
     requires(sizeof(T) < 3 && N < 16)
-  Error::Read read(T *bits, with_ack_t /*tag*/, F &&ack) {
+  Read read(T *bits, with_ack_t /*tag*/, F &&ack) {
     if (read<N>(bits, with_parity) == Read::BAD_PARITY)
       return Read::BAD_PARITY;
 
@@ -123,23 +120,17 @@ public:
   };
   template <auto N, std::unsigned_integral T>
     requires(sizeof(T) < 3 && N < 16)
-  Error::Read read(T *bits, with_ack_t /*tag*/, bool ack) {
+  Read read(T *bits, with_ack_t /*tag*/, bool ack) {
     return read<N>(bits, with_ack, [=]() { return ack; });
   }
 
 private:
-  using Read = Error::Read;
-  using Send = Error::Send;
-  using Bit = detail::Bit;
-
   static void send_ACK() { phy_send_ack(); };
-  static uint8_t read_ACK() { return phy_read_ack(); };
+  static Send read_ACK() { return phy_read_ack(); };
 
   template <auto N, class T> Bit sendbits(T bits);
   template <auto N, class T> Bit readbits(T *bits);
 
-  // Temporary specializations bridging to legacy C API
-  // Replace with proper (single?) template when phy has been ported
   template <auto N>
     requires(N > 1 && N < 8)
   Bit sendbits(uint8_t bits) {
@@ -179,13 +170,13 @@ bool Bus::is_active() const { return phy_active(); };
 void Bus::mute(bool mute) { phy_mute(mute); };
 bool Bus::is_muted() const { return phy_is_muted(); };
 
-auto Bus::read(uint16_t address, Frame *in, Frame::Print print) -> Error::Read {
+auto Bus::read(uint16_t address, Frame *in, Frame::Print print) -> Read {
   struct errtype {
-    Error::Read errno;
+    Read errno;
     uint16_t val;
   } err = {};
 
-  using enum detail::Error::Read;
+  using enum Read;
 
   { // bound handle lifetime
     auto handle = get();
@@ -194,53 +185,52 @@ auto Bus::read(uint16_t address, Frame *in, Frame::Print print) -> Error::Read {
     uint8_t tmp = 0;
 
     err.errno = handle.readstartbit();
-    if (err.errno != Error::Read{0})
+    if (err.errno != Read{0})
       goto handle_err;
 
     handle.read<1>(&tmp, no_parity);
     in->is_unicast = (tmp != 0U);
 
-    if (auto rerr = handle.read<ADDR_WIDTH>(&in->controller_addr, with_parity);
+    if (auto rerr = handle.read<12>(&in->controller_addr, with_parity);
         rerr == BAD_PARITY) {
       err.errno = BAD_CONTROLLER_PARITY;
-      if (print.verbose) {
+      if (print.verbose)
         err.val = in->controller_addr;
-      }
+
       goto handle_err;
     }
 
-    if (auto rerr = handle.read<ADDR_WIDTH>(
-            &in->peripheral_addr, with_ack,
     // Using lambda for delayed evaluation of peripheral_addr field
     // deref, which will be written by the time the lambda is evaluated
-            [&]() {
+    auto should_ack_lambda = [&]() {
       shouldACK = !is_muted() && (in->peripheral_addr == address);
       return shouldACK;
-            });
+    };
+    if (auto rerr =
+            handle.read<12>(&in->peripheral_addr, with_ack, should_ack_lambda);
         rerr == BAD_PARITY) {
       err.errno = BAD_PERIPHERAL_PARITY;
-      if (print.verbose) {
+      if (print.verbose)
         err.val = in->peripheral_addr;
-      }
+
       goto handle_err;
     }
 
-    if (auto rerr =
-            handle.read<CONTROL_WIDTH>(&in->control, with_ack, shouldACK);
+    if (auto rerr = handle.read<4>(&in->control, with_ack, shouldACK);
         rerr == BAD_PARITY) {
       err.errno = BAD_CONTROL_PARITY;
-      if (print.verbose) {
+      if (print.verbose)
         err.val = in->control;
-      }
+
       goto handle_err;
     }
 
-    if (auto rerr = handle.read<BYTE_WIDTH>(&in->length, with_ack, shouldACK);
+    if (auto rerr = handle.read<8>(&in->length, with_ack, shouldACK);
         rerr == BAD_PARITY) {
       err.errno = BAD_LENGTH_PARITY;
-      if (print.verbose) {
+      if (print.verbose)
         err.val = in->length;
-      }
+
       goto handle_err;
     }
 
@@ -251,13 +241,12 @@ auto Bus::read(uint16_t address, Frame *in, Frame::Print print) -> Error::Read {
     }
 
     for (uint8_t i = 0; i < in->length; i++) {
-      if (auto rerr =
-              handle.read<BYTE_WIDTH>(&in->data[i], with_ack, shouldACK);
+      if (auto rerr = handle.read<8>(&in->data[i], with_ack, shouldACK);
           rerr == BAD_PARITY) {
         err.errno = BAD_DATA_PARITY;
-        if (print.verbose) {
+        if (print.verbose)
           err.val = in->data[i];
-        }
+
         goto handle_err;
       }
     }
@@ -278,13 +267,13 @@ auto Bus::read(uint16_t address, Frame *in, Frame::Print print) -> Error::Read {
         goto VERBOSE;
       case BAD_CONTROL_PARITY: fputs("reading control", stdout); goto VERBOSE;
       case BAD_LENGTH_PARITY: fputs("reading length", stdout); goto VERBOSE;
-      case BAD_LENGTH_RANGE: printf("bad length 0x%X", err.val & 0x0F); break;
+      case BAD_LENGTH_RANGE: printf("bad length 0x%02X", err.val); break;
       case BAD_DATA_PARITY: fputs("reading data", stdout); goto VERBOSE;
       case BAD_PARITY:
         __builtin_unreachable();
       VERBOSE:
         if (print.verbose) {
-          printf("; read 0x%X", err.val);
+          printf("; read 0x%02X", err.val);
         }
     }
     putchar('\n');
@@ -300,15 +289,15 @@ auto Bus::read(uint16_t address, Frame *in, Frame::Print print) -> Error::Read {
   return err.errno;
 }
 
-auto Bus::send(const Frame *out, Frame::Print print) -> Error::Send {
+auto Bus::send(const Frame *out, Frame::Print print) -> Send {
   struct errtype {
     // Error enum is ordered such that a lower numeric value corresponds to
     // more success
-    Error::Send errno;
+    Send errno;
     uint8_t val;
   } err = {};
 
-  using enum detail::Error::Send;
+  using enum Send;
 
   if (is_muted()) {
     err.errno = MUTED;
@@ -326,24 +315,22 @@ auto Bus::send(const Frame *out, Frame::Print print) -> Error::Send {
 
     handle.send<1>(static_cast<uint8_t>(out->is_unicast), no_parity);
 
-    handle.send<ADDR_WIDTH>(out->controller_addr, with_parity);
+    handle.send<12>(out->controller_addr, with_parity);
 
-    if (auto serr = handle.send<ADDR_WIDTH>(out->peripheral_addr, with_ack,
-                                            out->is_unicast);
+    if (auto serr =
+            handle.send<12>(out->peripheral_addr, with_ack, out->is_unicast);
         serr == NAK) {
       err.errno = NAK_ADDRESS;
       goto handle_err;
     }
 
-    if (auto serr =
-            handle.send<CONTROL_WIDTH>(out->control, with_ack, out->is_unicast);
+    if (auto serr = handle.send<4>(out->control, with_ack, out->is_unicast);
         serr == NAK) {
       err.errno = NAK_CONTROL;
       goto handle_err;
     }
 
-    if (auto serr =
-            handle.send<BYTE_WIDTH>(out->length, with_ack, out->is_unicast);
+    if (auto serr = handle.send<8>(out->length, with_ack, out->is_unicast);
         serr == NAK) {
       err.errno = NAK_MESSAGE_LENGTH;
       goto handle_err;
@@ -354,8 +341,7 @@ auto Bus::send(const Frame *out, Frame::Print print) -> Error::Send {
       // necessary (i.e. This deviates from the previous broadcast specific
       // function that sent an extra `1` bit after each byte/parity)
       // Explanation for why audio-group broadcast state report isn't working?
-      if (auto serr =
-              handle.send<BYTE_WIDTH>(out->data[i], with_ack, out->is_unicast);
+      if (auto serr = handle.send<8>(out->data[i], with_ack, out->is_unicast);
           serr == NAK) {
         err.errno = NAK_DATA;
         err.val = i;

src/avclan/cdchanger.cc

@@ -57,6 +57,9 @@ void CDChanger::init() {
 }
 
 void CDChanger::handle(const Frame *in, Frame *out) {
+  if (in->length < 4)
+    return; // [Currently known] valid CDChanger frames have at least 4 bytes
+
   const uint8_t *data = &in->data[1];
   const auto from = static_cast<Device>(*data++);
   /* const auto to = */ data++;
@@ -184,7 +187,7 @@ void CDChanger::handle(const Frame *in, Frame *out) {
     case Track_Fast_Forward: {
       state |= SEEKING;
       secs += TIME_SKIP;
-      if (secs > 60) {
+      if (secs > 59) {
         secs -= 60;
         ++mins;
       }

src/avclan/frame.cc

@@ -35,9 +35,8 @@ void Frame::print(Frame::Print print) const {
 
     bptr = buffer;
     *bptr++ = 0x17; // End of transmission block
-    *bptr++ = 0x0D; // \r
     *bptr++ = 0x0A; // \n
-    fwrite(buffer, 1, 3, stdout);
+    fwrite(buffer, 1, 2, stdout);
   } else {
     printf("%X", static_cast<unsigned>(is_unicast));
     printf(" 0x%03X", static_cast<unsigned>(controller_addr & 0x0FFF));
@@ -60,7 +59,7 @@ Error::Parse Frame::parse(const uint8_t *bytes, uint8_t len) {
 
   const uint8_t *last = bytes + len;
 
-  if (len < sizeof(avclan::Frame)) {
+  if (len < Frame::MIN_SIZE) {
     err.errno = TOO_SHORT;
     goto handle_err;
   }

src/avclan/frame.hpp

@@ -20,6 +20,7 @@ struct Frame {
   };
   using Error = detail::Error;
   static constexpr int MAXLENGTH = 32;
+  static constexpr int MIN_SIZE = 7;
 
   Error::Parse parse(const uint8_t *bytes, uint8_t len);
   void print(Print print) const;

src/avclan/hal/phy.h

@@ -11,10 +11,12 @@
 
 #ifdef __cplusplus
 using Read = avclan::detail::Error::Read;
+using Send = avclan::detail::Error::Send;
 using Bit = avclan::detail::Bit;
 extern "C" {
 #else
 typedef enum Read Read;
+typedef enum Send Send;
 typedef enum Bit Bit;
 #endif
 
@@ -49,7 +51,13 @@ bool phy_send_startbit(void);
 // suffixes name the source-operand width; `len` is how many bits (<= width).
 void phy_send_bit(Bit bit);
 void phy_send_ack(void);
-uint8_t phy_read_ack(void);
+
+/* Returns 0 (`(Send)0`) if the peripheral sent an ACK bit, otherwise returns
+  NAK. An ACK bit is a cooperative bit, where the sender starts (drives the bus)
+  for the sync period, and allows the receiver to drive the bus (or not) to
+  finish a "1" bit.
+*/
+Send phy_read_ack(void);
 
 Bit phy_send_bits_u8(const uint8_t *bits, int8_t len);
 Bit phy_send_bits_u16(const uint16_t *bits, int8_t len);

src/avclan/peripheral.hpp

@@ -39,8 +39,8 @@ public:
   Error::Read read(Frame *in, Frame::Print print) {
     return bus.read(address_, in, print);
   };
-  // To "forge" a controller_addr, instantiate a new/different Peripheral
   Error::Send send(Frame *out, Frame::Print print) {
+    // To "forge" a controller_addr, instantiate a new/different Peripheral
     postmark(out);
     return bus.send(out, print);
   };
@@ -71,9 +71,12 @@ public:
       b3 = *data++;
 
     if (!in->is_unicast) {
+      const auto from = b0;
+      const auto to = b1;
+      const auto action = b2;
       // Broadcast: bytes are (from, to, action, [extra...]).
       // peripheral_addr unchecked — always 0xFFF or 0x1FF in known traffic.
-      switch (PACK3(b0, b1, b2)) {
+      switch (PACK3(from, to, action)) {
         case PACK3(LAN, COMM_CTRL, to_underlying(Lancheck_Scan_Req)):
           out->length = sizeof(lancheck_resp);
           out->is_unicast = true;
@@ -112,12 +115,9 @@ public:
         case PACK3(COMMUNICATION_V1, COMM_CTRL, to_underlying(Ping_Req)):
         case PACK3(COMMUNICATION_V2, COMM_CTRL, to_underlying(Ping_Req)): {
           out->is_unicast = true;
-          const uint8_t ping_resp[] = {0x00,
-                                       to_underlying(COMM_CTRL),
-                                       to_underlying(COMMUNICATION_V1),
-                                       to_underlying(Ping_Resp),
-                                       0xFF,
-                                       b3};
+          const uint8_t ping_resp[] = {0x00, to_underlying(COMM_CTRL),
+                                       from, to_underlying(Ping_Resp),
+                                       0xFF, b3};
           out->length = sizeof(ping_resp);
           memcpy(out->data, ping_resp, sizeof(ping_resp));
           out->reaction = 1;
@@ -131,14 +131,15 @@ public:
           out->peripheral_addr = controller_;
           out->is_unicast = true;
           const uint8_t list_functions_resp[] = {
-              0x00, to_underlying(COMM_CTRL), to_underlying(COMMUNICATION_V1),
-              to_underlying(List_Functions_Resp), to_underlying(CD_CHANGER)};
+              0x00, to_underlying(COMM_CTRL), from,
+              to_underlying(List_Functions_Resp), to_underlying(Devs::id)...};
           out->length = sizeof(list_functions_resp);
           memcpy(out->data, list_functions_resp, sizeof(list_functions_resp));
           out->reaction = 1;
           break;
         }
         // case Restart_Lan: not handled
+        default: break;
       }
     } else if (in->peripheral_addr == address_ && b0 == 0x00) {
       ((Devs::id == static_cast<Device>(b2)

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

@@ -7,12 +7,11 @@
 #include <avr/io.h>
 #include <avr/sfr_defs.h>
 #include <stdint.h>
-#include <stdio.h>
 #include <util/atomic.h>
 
+#include "hal/cd_timer.h" // statustimer_enable/disable (guard)
 #include "hal/phy.h"
 #include "media_avr.h" // media_sync_during_mask (guard)
-#include "hal/cd_timer.h" // statustimer_enable/disable (guard)
 
 // F_CPU + TICK_US (timing.h) defined here; F_CPU potentially needed by
 // avr-libc.
@@ -152,12 +151,7 @@ void phy_send_ack() {
   phy_send_bit(bit_zero);
 }
 
-/* Returns true if the peripheral sent an ACK bit.
-  An ACK bit is a cooperative bit, where the sender starts (drives the bus) a
-  sync period, and allows the receiver to drive the bus (or not) to finish a "1"
-  bit.
-*/
-uint8_t phy_read_ack() {
+Send phy_read_ack() {
   TCB1.CNT = 0;                              // Double reset of TCB1.CNT: here
   set_AVC_logic_for(0, AVCLAN_BIT1_LOGIC_0); // And here (within)
   AVCLAN_setBusIdle();                       // Stop driving bus
@@ -166,15 +160,15 @@ uint8_t phy_read_ack() {
     if (!BUS_IS_IDLE && (TCB1.CNT > AVCLAN_READBIT_THRESHOLD))
       break; // ACK
     if (TCB1.CNT > AVCLAN_BIT_LENGTH_MAX)
-      return 0; // NAK
+      return NAK;
   }
 
   // Check/wait in case we get here before peripheral finishes ACK bit
   while (!BUS_IS_IDLE) {
     if (TCB1.CNT > AVCLAN_BIT_LENGTH_MAX)
-      return 0; // NAK
+      return NAK;
   }
-  return 1;
+  return (Send)0;
 }
 
 // Send `len` bits on the AVCLAN bus; returns the even parity
@@ -244,12 +238,13 @@ ISR(TCB0_INT_vect) {
 
 // Read `len` bits on the AVCLAN bus; returns the even parity
 uint8_t phy_read_bits_u8(uint8_t *bits, uint8_t len) {
-  cli();
+  uint8_t parity;
+  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
     READING_BYTE = 0;
     READING_PARITY = 0;
     READING_NBITS = len;
-  sei();
 
+    NONATOMIC_BLOCK(NONATOMIC_RESTORESTATE) {
       TCB1.CNT = 0;
       while (READING_NBITS) {
         // 200% the duration of `len` bits
@@ -259,11 +254,11 @@ uint8_t phy_read_bits_u8(uint8_t *bits, uint8_t len) {
           break; // Should have finished by now; something's wrong
         }
       };
+    }
 
-  cli();
     *bits = READING_BYTE;
-  uint8_t parity = READING_PARITY;
-  sei();
+    parity = READING_PARITY;
+  }
 
   return (parity & 1);
 }
@@ -283,12 +278,13 @@ uint8_t phy_read_bits_u16(uint16_t *bits, int8_t len) {
 
 // Read a byte on the AVCLAN bus
 uint8_t phy_read_byte(uint8_t *byte) {
-  cli();
+  uint8_t parity;
+  ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
     READING_BYTE = 0;
     READING_PARITY = 0;
     READING_NBITS = 8;
-  sei();
 
+    NONATOMIC_BLOCK(NONATOMIC_RESTORESTATE) {
       TCB1.CNT = 0;
       while (READING_NBITS) {
         // 200% the length of a byte
@@ -298,11 +294,11 @@ uint8_t phy_read_byte(uint8_t *byte) {
           break; // Should have finished by now; something's wrong
         }
       };
+    }
 
-  cli();
     *byte = READING_BYTE;
-  uint8_t parity = READING_PARITY;
-  sei();
+    parity = READING_PARITY;
+  }
 
   return (parity & 1);
 }

src/sniffer.cc

@@ -219,9 +219,9 @@ int main() {
           seqIsUnicast = false;
           break;
         case '\n':
-          if (readSeq) {
+          if (readSeq && seqIdx > 0) {
             if (readBinary) {
-              if (data_tmp[seqIdx] == 0x17) {
+              if (data_tmp[seqIdx - 1] == 0x17) {
                 if (auto out = cache.pop()) {
                   if (out->parse(data_tmp, --seqIdx) ==
                       Frame::Error::Parse{0}) {
@@ -233,7 +233,7 @@ int main() {
               } else
                 goto DEFAULT; // reading binary and this is a real data byte;
                               // fall through to default
-            } else {
+            } else if (seqIdx <= Frame::MAXLENGTH) {
               if (auto out = cache.pop()) {
                 out->is_unicast = seqIsUnicast;
                 out->peripheral_addr =
@@ -249,7 +249,7 @@ int main() {
           }
         DEFAULT:
         default:
-          if (readSeq) {
+          if (readSeq && seqIdx < (Frame::MAXLENGTH + sizeof(Frame))) {
             if (readBinary) {
               data_tmp[seqIdx++] = readkey;
             } else {