From a59e822e92ce952d7ce82311817ee27b9a164d74 Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 26 Sep 2026 10:49:21 -0400 Subject: [PATCH] refactor: delete an accessor nothing used and stop repeating the guards sh_temp() had no callers outside the tests written to exercise it, and the bench tool in this very repo reached past it to read packet->temps directly. An accessor its own author bypasses is not pulling its weight. Every real call site indexes with a compile-time constant, where the bound is already known. sh_analog() stays, for the one reason that distinguishes them: CarDisplay passes an index that comes from runtime channel configuration, so the check there is load-bearing. The four digital accessors repeated the same null guard, range guard and shift four times. They now share bit_get and bit_set, so there is one copy of the logic and the guards cannot drift apart. That matters for the mutation job: deleting one range check should not leave three others to pass the tests. Also cut the comments back. Several blocks ran past twenty and one past thirty lines, narrating debugging sessions and repeating what the commit messages and PR descriptions already say. Reference material stays: the frame diagram, the field tables, the return-value tables. The editorialising is gone. No behaviour change. All 1378 checks pass, all nine mutations are still caught, and the mutation targeting the digital bounds check now hits both shared helpers instead of one of four copies. --- .github/workflows/ci.yml | 2 +- src/sensorhub/parser.c | 48 ++++++++---------- src/sensorhub/sensorhub.h | 100 ++++++++++---------------------------- test/test_parser.c | 5 +- 4 files changed, 47 insertions(+), 108 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1380795..2ad8e39 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -127,7 +127,7 @@ jobs: check "dropped checksum-error counting" sed -i 's/else if (byte == SH_HEADER_1) {/else if (0) {/' src/sensorhub/parser.c check "broke repeated-header handling" - sed -i 's/if (channel > SH_DIGITAL_MAX) return SH_E_RANGE;/;/' src/sensorhub/parser.c + sed -i 's/if (channel > SH_DIGITAL_MAX) return SH_E_RANGE;/;/' src/sensorhub/parser.c check "dropped the digital channel bounds check" sed -i 's/if (buffer_size < SH_FRAME_SIZE) return SH_E_SPACE;/;/' src/sensorhub/parser.c check "dropped the encode buffer size check" diff --git a/src/sensorhub/parser.c b/src/sensorhub/parser.c index 5174420..0142ac5 100644 --- a/src/sensorhub/parser.c +++ b/src/sensorhub/parser.c @@ -49,53 +49,43 @@ bool sh_failed(sh_status_t status) #define SH_DIGITAL_MAX 7u -sh_status_t sh_digital_in(const sh_packet_t *packet, unsigned channel, - bool *out) +static sh_status_t bit_get(uint8_t field, unsigned channel, bool *out) { - if (packet == NULL || out == NULL) return SH_E_NULL; - if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; + if (out == NULL) return SH_E_NULL; + if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; - *out = ((packet->digital_in >> channel) & 1u) != 0u; + *out = ((field >> channel) & 1u) != 0u; return SH_OK; } -sh_status_t sh_digital_out(const sh_packet_t *packet, unsigned channel, - bool *out) +static sh_status_t bit_set(uint8_t *field, unsigned channel, bool value) { - if (packet == NULL || out == NULL) return SH_E_NULL; - if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; + if (field == NULL) return SH_E_NULL; + if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; - *out = ((packet->digital_out >> channel) & 1u) != 0u; + if (value) *field |= (uint8_t)(1u << channel); + else *field &= (uint8_t)~(1u << channel); return SH_OK; } -sh_status_t sh_set_digital_in(sh_packet_t *packet, unsigned channel, bool value) +sh_status_t sh_digital_in(const sh_packet_t *p, unsigned channel, bool *out) { - if (packet == NULL) return SH_E_NULL; - if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; - - if (value) packet->digital_in |= (uint8_t)(1u << channel); - else packet->digital_in &= (uint8_t)~(1u << channel); - return SH_OK; + return (p == NULL) ? SH_E_NULL : bit_get(p->digital_in, channel, out); } -sh_status_t sh_set_digital_out(sh_packet_t *packet, unsigned channel, bool value) +sh_status_t sh_digital_out(const sh_packet_t *p, unsigned channel, bool *out) { - if (packet == NULL) return SH_E_NULL; - if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; - - if (value) packet->digital_out |= (uint8_t)(1u << channel); - else packet->digital_out &= (uint8_t)~(1u << channel); - return SH_OK; + return (p == NULL) ? SH_E_NULL : bit_get(p->digital_out, channel, out); } -sh_status_t sh_temp(const sh_packet_t *packet, unsigned index, float *out) +sh_status_t sh_set_digital_in(sh_packet_t *p, unsigned channel, bool value) { - if (packet == NULL || out == NULL) return SH_E_NULL; - if (index >= SH_TEMP_COUNT) return SH_E_RANGE; + return (p == NULL) ? SH_E_NULL : bit_set(&p->digital_in, channel, value); +} - *out = packet->temps[index]; - return SH_OK; +sh_status_t sh_set_digital_out(sh_packet_t *p, unsigned channel, bool value) +{ + return (p == NULL) ? SH_E_NULL : bit_set(&p->digital_out, channel, value); } sh_status_t sh_analog(const sh_packet_t *packet, unsigned index, uint16_t *out) diff --git a/src/sensorhub/sensorhub.h b/src/sensorhub/sensorhub.h index 70d8e3c..4ae9d0b 100644 --- a/src/sensorhub/sensorhub.h +++ b/src/sensorhub/sensorhub.h @@ -12,23 +12,12 @@ * sender and stay framed, instead of desynchronising. * crc16 CRC-16-CCITT over fmt, len, and the payload, little-endian. * - * The CRC is not a byte-sum. An XOR checksum misses two corruptions that a - * car produces for real: two bit flips in the same bit position cancel - * exactly, and swapped bytes are invisible to an order-independent sum. Both - * were measured at 100% undetected. CRC-16 catches both, plus every burst up - * to 16 bits, for 32 bytes of flash on an ATmega328p. + * CRC, not a byte-sum: XOR misses paired same-bit flips and swapped bytes + * entirely. Both were 100% undetected in simulation. * - * The version and length bytes are the difference between "we changed the - * format and everything broke loudly" and "we changed the format and the car - * logged plausible nonsense for a season". They cost two bytes on a link - * running at 7% utilisation. - * - * PARSING IS SEPARATE FROM I/O - * - * sh_parser_feed() takes one byte and never touches a file descriptor, so the - * framing can be tested on any machine with no car attached. The serial - * helpers in are a convenience for callers that do have - * hardware. + * sh_parser_feed() takes one byte and touches no file descriptor, so the + * framing is testable with no car attached. Serial helpers live in + * . */ #ifndef SENSORHUB_H @@ -69,14 +58,6 @@ extern "C" { * The packet * ------------------------------------------------------------------ */ -/* - * Digital channels are bits, not bytes: a switch carries one bit of - * information and sixteen of them fit in the space two bytes used to take. - * - * Outputs are reported as well as inputs. The firmware drives a radiator fan - * and a water pump, and until now their state appeared nowhere in telemetry, - * so you could not display or log what the car was doing to itself. - */ typedef struct __attribute__((packed)) { float speed; /* mph, from the wheel interrupt */ float airspeed; /* mph, pitot, zeroed at startup */ @@ -112,15 +93,8 @@ typedef struct __attribute__((packed)) { * Status * ------------------------------------------------------------------ */ -/* - * Every call that can fail returns one of these. SH_OK is zero, so the - * common shape reads naturally: - * - * if (sh_parser_feed(&parser, byte, &packet) == SH_OK) { ... } - * - * SH_INCOMPLETE is not an error. It is the usual answer, returned for every - * byte that did not happen to complete a packet. - */ +/* SH_OK is zero, so `== SH_OK` reads naturally. SH_INCOMPLETE is not an + error; it is the answer for most bytes. */ typedef enum { SH_OK = 0, /* a packet is ready */ SH_INCOMPLETE, /* byte consumed, nothing complete yet */ @@ -168,9 +142,9 @@ sh_status_t sh_set_digital_in(sh_packet_t *packet, unsigned channel, sh_status_t sh_set_digital_out(sh_packet_t *packet, unsigned channel, bool value); -/* Bounds-checked reads for the array slots, so an out-of-range index is an - error rather than whatever was next in memory. */ -sh_status_t sh_temp(const sh_packet_t *packet, unsigned index, float *out); +/* Bounds-checked, for callers whose index comes from runtime configuration + rather than a constant. Read packet->temps[SH_TEMP_ENGINE] directly when the + index is known at compile time; there is no accessor for that. */ sh_status_t sh_analog(const sh_packet_t *packet, unsigned index, uint16_t *out); @@ -178,23 +152,14 @@ sh_status_t sh_analog(const sh_packet_t *packet, unsigned index, * Link statistics * ------------------------------------------------------------------ * * - * A link that is losing packets shows up here long before it shows up on the - * dashboard. The three counters fail differently and are worth reading - * separately: - * - * checksum_errors rising electrical: noise, a marginal cable, a bad ground - * resyncs rising the sender is being interrupted mid-frame - * dropped rising packets never arrived at all; the receiver is - * not keeping up, or the sender is restarting + * The counters fail differently and are worth reading separately: + * checksum_errors electrical: noise, a marginal cable, a bad ground + * resyncs the sender is being interrupted mid-frame + * dropped packets never arrived; a CRC cannot tell you this * - * Width is configurable because this header also compiles for an ATmega328p, - * where 64-bit counters cost RAM a Nano does not have and every increment is - * a slow multi-word add. The AVR default is 32-bit rather than 16: at roughly - * 40 packets a second, 16 bits wraps in under half an hour, and a diagnostic - * counter that quietly lies is worse than a larger one. - * - * Note this changes sizeof(sh_stats_t), so everything linked together must - * agree on it. + * 32-bit on AVR, not 16: at 40 packets a second 16 bits wraps in under half + * an hour. Changing the width changes sizeof(sh_stats_t), so everything + * linked together must agree on it. */ #ifndef SH_COUNTER_BITS # if defined(__AVR__) || defined(SH_EMBEDDED) @@ -262,9 +227,7 @@ void sh_parser_init(sh_parser_t *parser); * uses the length byte to skip it cleanly and carries on * SH_E_NULL parser or out was NULL * - * Errors are reported, not thrown: the parser stays usable and keeps - * counting. A caller that only wants packets can compare against SH_OK and - * read parser->stats occasionally. + * Errors are reported, not thrown; the parser stays usable and keeps counting. */ sh_status_t sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out); @@ -273,14 +236,8 @@ sh_status_t sh_parser_feed(sh_parser_t *parser, uint8_t byte, * Encoding * ------------------------------------------------------------------ */ -/* - * CRC-16-CCITT, polynomial 0x1021, initial value 0xFFFF, no final xor. - * - * Exposed so tests and any other transmitter need not duplicate it. The - * bitwise form is deliberate: a 256-entry table would be faster and cost 512 - * bytes of flash on a part that has 30KB, to save time this loop does not - * need at 20 packets a second. - */ +/* CRC-16-CCITT, poly 0x1021, init 0xFFFF, no final xor. Bitwise on purpose: + a 256-entry table costs 512 bytes of flash to save time nobody needs. */ uint16_t sh_crc16(const uint8_t *data, size_t length); /* Resume a CRC over a second run of bytes. Lets the receiver cover the @@ -288,17 +245,12 @@ uint16_t sh_crc16(const uint8_t *data, size_t length); one contiguous buffer first. */ uint16_t sh_crc16_continue(uint16_t crc, const uint8_t *data, size_t length); -/* - * Serialise a packet into a complete frame. - * - * buffer must have room for at least SH_FRAME_SIZE bytes; pass its real size - * in buffer_size and the function will refuse rather than overrun. On success - * *written holds the frame length. written may be NULL if you do not care. - * - * SH_OK frame written - * SH_E_SPACE buffer_size was too small - * SH_E_NULL packet or buffer was NULL - */ +/* Serialise into a frame. Pass the real buffer size; too small is refused, + not overrun. written may be NULL. + + SH_OK frame written, *written is its length + SH_E_SPACE buffer_size < SH_FRAME_SIZE + SH_E_NULL packet or buffer was NULL */ sh_status_t sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer, size_t buffer_size, size_t *written); diff --git a/test/test_parser.c b/test/test_parser.c index 6ddeb74..8fe0af2 100644 --- a/test/test_parser.c +++ b/test/test_parser.c @@ -421,17 +421,14 @@ static void test_out_of_range_is_an_error_not_a_read(void) { sh_packet_t p = sample_packet(); bool v; - float f; uint16_t a; CHECK(sh_digital_in(&p, 8, &v) == SH_E_RANGE, "channel 8 is out of range"); CHECK(sh_digital_out(&p, 99, &v) == SH_E_RANGE, "so is 99"); CHECK(sh_set_digital_in(&p, 8, true) == SH_E_RANGE, "and for writes"); - CHECK(sh_temp(&p, SH_TEMP_COUNT, &f) == SH_E_RANGE, "temp index bound"); CHECK(sh_analog(&p, SH_ANALOG_COUNT, &a) == SH_E_RANGE, "analog bound"); - CHECK(sh_temp(&p, SH_TEMP_ENGINE, &f) == SH_OK && f == 180.0f, - "a valid temp index still works"); + CHECK(p.temps[SH_TEMP_ENGINE] == 180.0f, "temps read directly"); CHECK(sh_analog(&p, SH_ANALOG_BATTERY, &a) == SH_OK && a == 812, "a valid analog index still works"); }