diff --git a/.clang-tidy b/.clang-tidy index 8ad0bc8..4fdea0b 100644 --- a/.clang-tidy +++ b/.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 diff --git a/scripts/packet-analysis/extcap/avclan-pcap.py b/scripts/packet-analysis/extcap/avclan-pcap.py index bc0bb14..adfc7ec 100755 --- a/scripts/packet-analysis/extcap/avclan-pcap.py +++ b/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 diff --git a/src/avclan/bus.cc b/src/avclan/bus.cc index 12cfeb3..c2c87b0 100644 --- a/src/avclan/bus.cc +++ b/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 Trailer> requires(sizeof(T) < 3 && N < 16 && !std::same_as) - Error::Send send(T bits, Trailer /*tag*/) { + Send send(T bits, Trailer /*tag*/) { const auto parity = sendbits(bits); if constexpr (std::is_same_v) @@ -80,22 +79,20 @@ public: template 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(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 Trailer> requires(sizeof(T) < 3 && N < 16 && !std::same_as) - Error::Read read(T *bits, Trailer /*tag*/) { + Read read(T *bits, Trailer /*tag*/) { const auto calc_parity = readbits(bits); if constexpr (std::is_same_v) { uint8_t read_parity; @@ -108,7 +105,7 @@ public: template 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(bits, with_parity) == Read::BAD_PARITY) return Read::BAD_PARITY; @@ -123,23 +120,17 @@ public: }; template 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(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 Bit sendbits(T bits); template Bit readbits(T *bits); - // Temporary specializations bridging to legacy C API - // Replace with proper (single?) template when phy has been ported template 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(&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( - &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 - [&]() { - shouldACK = !is_muted() && (in->peripheral_addr == address); - return shouldACK; - }); + // 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(&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(&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(&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(out->is_unicast), no_parity); - handle.send(out->controller_addr, with_parity); + handle.send<12>(out->controller_addr, with_parity); - if (auto serr = handle.send(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(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(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(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; diff --git a/src/avclan/cdchanger.cc b/src/avclan/cdchanger.cc index d313f46..496d056 100644 --- a/src/avclan/cdchanger.cc +++ b/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(*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; } diff --git a/src/avclan/frame.cc b/src/avclan/frame.cc index dd0308c..a7a6780 100644 --- a/src/avclan/frame.cc +++ b/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(is_unicast)); printf(" 0x%03X", static_cast(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; } diff --git a/src/avclan/frame.hpp b/src/avclan/frame.hpp index 2d676af..056f45c 100644 --- a/src/avclan/frame.hpp +++ b/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; diff --git a/src/avclan/hal/phy.h b/src/avclan/hal/phy.h index 6bdc8cf..4b634a7 100644 --- a/src/avclan/hal/phy.h +++ b/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); diff --git a/src/avclan/peripheral.hpp b/src/avclan/peripheral.hpp index ffb6b5e..e564569 100644 --- a/src/avclan/peripheral.hpp +++ b/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 + // case Restart_Lan: not handled + default: break; } } else if (in->peripheral_addr == address_ && b0 == 0x00) { ((Devs::id == static_cast(b2) diff --git a/src/avclan/target/avr-attiny3216/phy_avr.c b/src/avclan/target/avr-attiny3216/phy_avr.c index 4a1020a..f15e3e4 100644 --- a/src/avclan/target/avr-attiny3216/phy_avr.c +++ b/src/avclan/target/avr-attiny3216/phy_avr.c @@ -7,12 +7,11 @@ #include #include #include -#include #include -#include "hal/phy.h" -#include "media_avr.h" // media_sync_during_mask (guard) #include "hal/cd_timer.h" // statustimer_enable/disable (guard) +#include "hal/phy.h" +#include "media_avr.h" // media_sync_during_mask (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,26 +238,27 @@ 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(); - READING_BYTE = 0; - READING_PARITY = 0; - READING_NBITS = len; - sei(); + uint8_t parity; + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + READING_BYTE = 0; + READING_PARITY = 0; + READING_NBITS = len; - TCB1.CNT = 0; - while (READING_NBITS) { - // 200% the duration of `len` bits - if (TCB1.CNT > ((uint16_t)AVCLAN_BIT_LENGTH_MAX * 2 * len)) { - READING_BYTE = 0; - READING_PARITY = 0; - break; // Should have finished by now; something's wrong + NONATOMIC_BLOCK(NONATOMIC_RESTORESTATE) { + TCB1.CNT = 0; + while (READING_NBITS) { + // 200% the duration of `len` bits + if (TCB1.CNT > ((uint16_t)AVCLAN_BIT_LENGTH_MAX * 2 * len)) { + READING_BYTE = 0; + READING_PARITY = 0; + break; // Should have finished by now; something's wrong + } + }; } - }; - cli(); - *bits = READING_BYTE; - uint8_t parity = READING_PARITY; - sei(); + *bits = READING_BYTE; + parity = READING_PARITY; + } return (parity & 1); } @@ -283,26 +278,27 @@ 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(); - READING_BYTE = 0; - READING_PARITY = 0; - READING_NBITS = 8; - sei(); + uint8_t parity; + ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { + READING_BYTE = 0; + READING_PARITY = 0; + READING_NBITS = 8; - TCB1.CNT = 0; - while (READING_NBITS) { - // 200% the length of a byte - if (TCB1.CNT > ((uint16_t)AVCLAN_BIT_LENGTH_MAX * 2 * 8)) { - READING_BYTE = 0; - READING_PARITY = 0; - break; // Should have finished by now; something's wrong + NONATOMIC_BLOCK(NONATOMIC_RESTORESTATE) { + TCB1.CNT = 0; + while (READING_NBITS) { + // 200% the length of a byte + if (TCB1.CNT > ((uint16_t)AVCLAN_BIT_LENGTH_MAX * 2 * 8)) { + READING_BYTE = 0; + READING_PARITY = 0; + break; // Should have finished by now; something's wrong + } + }; } - }; - cli(); - *byte = READING_BYTE; - uint8_t parity = READING_PARITY; - sei(); + *byte = READING_BYTE; + parity = READING_PARITY; + } return (parity & 1); } diff --git a/src/sniffer.cc b/src/sniffer.cc index 9986ab7..ea57fff 100644 --- a/src/sniffer.cc +++ b/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 {