Delete an accessor nothing used, stop repeating the guards, cut the comments

.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"

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)

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 <sensorhub/serial.h> 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
+ * <sensorhub/serial.h>.
  */
 
 #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,33 +142,24 @@ 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);
 
 /* ------------------------------------------------------------------ *
  *  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,32 +236,21 @@ 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
    format and length fields and then the payload without copying them into
    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);
 

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");
 }