Merge pull request 'Fix wake-from-sleep, RAM overrun, and peripheral hangs in AVR firmware' (#1) from fix/avr-wake-memory-safety into main

Reviewed-on: #1

avr_code/adc.c

@@ -1,6 +1,15 @@
 #include "adc.h"
 
-int8_t adc_Initialize()
+#define ADC_CONVERSION_TIMEOUT 1000U
+
+// The ADC needs a 50-200 kHz clock. At F_CPU = 8 MHz that is a /64 prescaler
+// (125 kHz); the old /2 ran it at 4 MHz, far out of spec.
+#define ADC_PRESCALER_64 ((1 << ADPS2) | (1 << ADPS1))
+
+// The 1.1 V bandgap reference needs time to settle after the mux is switched.
+#define ADC_SETTLE_US 200
+
+int8_t adc_Initialize(void)
 {
     //REFS VAL_0x01; ADLAR disabled; MUX adc0; 
     ADMUX = 0x40;
@@ -8,18 +17,17 @@ int8_t adc_Initialize()
     //ACME disabled; ADTS VAL_0x00; 
     ADCSRB = 0x00;
 
-    //ADEN enabled; ADSC disabled; ADATE disabled; ADIF disabled; ADIE disabled; ADPS VAL_0x01; 
+    ADCSRA = (1 << ADEN) | ADC_PRESCALER_64;
-    ADCSRA = 0x81;
 
     return 0;
 }
 
-void adc_Disable()
+void adc_Disable(void)
 {
 	ADCSRA &= ~(1 << ADEN);
 }
 
-void adc_Enable()
+void adc_Enable(void)
 {
 	ADCSRA |= (1 << ADEN);
 }
@@ -42,25 +50,37 @@ void adc_StartConversion(uint8_t channel)
 	ADMUX &= ~0x0f;
 	ADMUX |= channel;
     }
+	_delay_us(ADC_SETTLE_US);
 	ADCSRA |= (1 << ADSC);
 }
 
-bool adc_IsConversionDone()
+bool adc_IsConversionDone(void)
 {
 	return ((ADCSRA & (1 << ADIF)));
 }
 
 uint16_t adc_GetConversionResult(void)
 {
-	return (ADCL | ADCH << 8);
+	// ADC reads ADCL then ADCH in the right order. Reading the two volatile
+	// registers in one expression leaves the order unspecified, and taking ADCH
+	// first breaks the data-register lock and corrupts the result.
+	return ADC;
 }
 
 uint16_t adc_GetConversion(uint8_t channel)
 {
-
-
 	adc_StartConversion(channel);
-	while (!adc_IsConversionDone());
+
+	// A conversion is 13 ADC clocks (~104 us at 125 kHz); bail out rather than
+	// hang if the ADC is disabled or its clock is gated off.
+	uint16_t attempts = 0;
+	while (!adc_IsConversionDone()) {
+		if (++attempts > ADC_CONVERSION_TIMEOUT) {
+			return 0;
+		}
+		_delay_us(10);
+	}
+
 	uint16_t res = adc_GetConversionResult();
 	ADCSRA |= (1 << ADIF);
 	return res;

avr_code/adc.h

@@ -7,20 +7,22 @@
 
 #ifndef ADC_H
 #define	ADC_H
+#include "defines.h"
 #include <avr/io.h>
-#include <stdint.h>
 #include <stdbool.h>
+#include <stdint.h>
+#include <util/delay.h>
 
 #ifdef	__cplusplus
 extern "C" {
 #endif
 
 
-int8_t adc_Initialize();
+int8_t adc_Initialize(void);
-void adc_Enable();
+void adc_Enable(void);
-void adc_Disable();
+void adc_Disable(void);
 void adc_StartConversion(uint8_t channel);
-bool adc_IsConversionDone();
+bool adc_IsConversionDone(void);
 uint16_t adc_GetConversionResult(void);
 uint16_t adc_GetConversion(uint8_t channel);
 

avr_code/defines.c

@@ -1,6 +1,6 @@
 #include "defines.h"
 
- unsigned char DATA_BUFFER_65[64];
+ unsigned char DATA_BUFFER_65[65];
  uint8_t DATA_BUFFER_7[7];
 // uint8_t DATA_BUFFER_254[255];
  unsigned char DATA_BUFFER_20[20];

avr_code/defines.h

@@ -17,7 +17,9 @@ extern "C" {
 #include <stdbool.h>
 #include <stdlib.h>
 #include <stdio.h>
-#define F_CPU 8000000UL  // 16 MHz clock speed
+#ifndef F_CPU
+#define F_CPU 8000000UL // 8 MHz clock speed; prefer -DF_CPU=8000000UL in the build flags
+#endif
 #define BAUD 38400
 #define F_SCL 200000UL
 
@@ -27,14 +29,14 @@ extern "C" {
 #define MIN(a,b) (((a)<(b))?(a):(b))
 #define MAX(a,b) (((a)>(b))?(a):(b))
 
-extern unsigned char DATA_BUFFER_65[64];
+extern unsigned char DATA_BUFFER_65[65];
 extern uint8_t DATA_BUFFER_7[7];
 //extern uint8_t DATA_BUFFER_254[255];
 extern unsigned char DATA_BUFFER_20[20];
 
 typedef struct {
     uint8_t payload_len;
-    char payload[255];
+    char payload[48];
     uint8_t success;
 } ndef_message;
 extern ndef_message NDEF_MSG;
@@ -47,8 +49,8 @@ extern trimmed_string_struct TRIMMED_STRING;
 
 typedef struct {
     uint8_t name_len;
-    char name_str[128];
+    char name_str[16];
-    char diameter_str[128];
+    char diameter_str[16];
     uint8_t diameter_len;
     uint8_t hashed;
 } identifier_results;
@@ -103,10 +105,6 @@ typedef enum // Goes into flags
 
   
 
-#define WHILE_BREAK(counter, attempts)      \
-    counter+=1;                             \
-    if ((counter+1) > attempts) {  break;};     
-
 #ifdef	__cplusplus
 }
 #endif

avr_code/i2c.c

@@ -1,7 +1,20 @@
 #include "i2c.h"
 #include <util/twi.h>
 
-void i2c_init()
+// Every one of these loops used to spin forever. The peripheral rail is cut
+// before sleeping, so a device that is slow or absent on wake would otherwise
+// hang the firmware with no watchdog reset armed.
+static bool i2c_wait_twint(void)
+{
+    for (uint16_t attempts = 0; attempts < I2C_TIMEOUT_LOOPS; attempts++) {
+        if (TWCR & (1 << TWINT)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+void i2c_init(void)
 {
     // Set SCL and SDA as inputs (automatically done by TWI hardware)
     TWSR = 0; // Prescaler = 1
@@ -13,13 +26,13 @@ void i2c_init()
 uint8_t i2c_start(uint8_t address)
 {
     TWCR = (1 << TWSTA) | (1 << TWINT) | (1 << TWEN); // Send START condition
-    while (!(TWCR & (1 << TWINT)))
+    if (!i2c_wait_twint())
-        ; // Wait for TWINT flag to be set
+        return 1;
     TWDR = address; // Load address into data register
 
     TWCR = (1 << TWINT) | (1 << TWEN); // Send address
-    while (!(TWCR & (1 << TWINT)))
+    if (!i2c_wait_twint())
-        ; // Wait for TWINT flag to be set
+        return 1;
     uint8_t status = TWSR & 0xF8;
     if (status != 0x18 && status != 0x40)
         return 1;
@@ -36,9 +49,15 @@ uint8_t write_one_byte(uint8_t device_addr, uint8_t register_addr, uint8_t data)
 uint8_t write_n_bytes(uint8_t device_addr, uint8_t register_addr, uint8_t* data, uint8_t n_bytes)
 {
     I2C_START_WRITE(device_addr);
-    i2c_write(register_addr);
+    if (i2c_write(register_addr)) {
+        i2c_stop();
+        return 1;
+    }
     for (uint8_t i = 0; i < n_bytes; i++) {
-        i2c_write(data[i]);
+        if (i2c_write(data[i])) {
+            i2c_stop();
+            return 1;
+        }
     }
     i2c_stop();
     return 0;
@@ -106,18 +125,21 @@ read_n_bytes_16bit_addr(uint8_t device_addr, uint16_t register_addr, uint8_t* da
 }
 
 // Stop i2c communication
-void i2c_stop()
+void i2c_stop(void)
 {
     TWCR = (1 << TWSTO) | (1 << TWINT) | (1 << TWEN); // Send STOP condition
-    while (!(TWCR & (1 << TWSTO)))
+    for (uint16_t attempts = 0; attempts < I2C_TIMEOUT_LOOPS; attempts++) {
-        ; // Wait for STOP to complete
+        if (!(TWCR & (1 << TWSTO))) {
+            return; // STOP complete
+        }
+    }
 }
 
-uint8_t i2c_read_ack()
+uint8_t i2c_read_ack(void)
 {
     TWCR = (1 << TWEN) | (1 << TWINT) | (1 << TWEA);
-    while (!(TWCR & (1 << TWINT)))
+    if (!i2c_wait_twint())
-        ; // Wait for TWINT flag to be set
+        return 0xFF;
     return TWDR;
 }
 
@@ -126,17 +148,17 @@ uint8_t i2c_write(uint8_t data)
     // Load data into TWDR
     TWDR = data;
     TWCR = (1 << TWEN) | (1 << TWINT);
-    while (!(TWCR & (1 << TWINT)))
+    if (!i2c_wait_twint())
-        ; // Wait for TWINT flag set
+        return 1;
     if ((TWSR & 0xF8) != TW_MT_DATA_ACK)
         return 1; // Check ACK
     return 0;
 }
 
-uint8_t i2c_read_nack()
+uint8_t i2c_read_nack(void)
 {
     TWCR = (1 << TWEN) | (1 << TWINT);
-    while (!(TWCR & (1 << TWINT)))
+    if (!i2c_wait_twint())
-        ; // Wait for TWINT flag to be set
+        return 0xFF;
     return TWDR;
 }

avr_code/i2c.h

@@ -2,6 +2,14 @@
 #include "uart.h"
 #include <avr/io.h>
 
+#ifndef i2c_H
+#define i2c_H
+
+#define TWSR TWSR0
+#define TWDR TWDR0
+#define TWBR TWBR0
+#define TWCR TWCR0
+
 #define I2C_START_WRITE(device_addr)                                           \
     {                                                                          \
         if (i2c_start((device_addr << 1) | 0x00)) {                            \
@@ -16,15 +24,11 @@
         }                                                                      \
     }
 
-#ifndef i2c_H
+// A byte at F_SCL takes well under 100 us; anything past this means the bus is
-#define i2c_H
+// stuck (peripheral unpowered, SDA held low) and we must not spin forever.
+#define I2C_TIMEOUT_LOOPS 20000U
 
-#define TWSR TWSR0
+void i2c_init(void);
-#define TWDR TWDR0
-#define TWBR TWBR0
-#define TWCR TWCR0
-
-void i2c_init();
 uint8_t i2c_start(uint8_t address);
 
 uint8_t write_one_byte(uint8_t device_addr, uint8_t register_addr,
@@ -45,10 +49,9 @@ uint8_t read_one_byte(uint8_t device_addr, uint8_t register_addr,
 uint8_t read_n_bytes(uint8_t device_addr, uint8_t register_addr, uint8_t *data,
                      uint8_t n_bytes);
 
-void i2c_stop();
+void i2c_stop(void);
-void i2c_scan();
+uint8_t i2c_read_ack(void);
-uint8_t i2c_read_ack();
+uint8_t i2c_read_nack(void);
-uint8_t i2c_read_nack();
 uint8_t i2c_write(uint8_t data);
 
 #endif

avr_code/interrupts.c

@@ -1,54 +1,70 @@
 
 #include "interrupts.h"
 
-void init_pins() {
+void init_pins(void) {
 
-    // Set reed switch interrupt pin
+    // The reed switch (PD3/INT1) switches to ground and the RTC alarm output
+    // (PD2/INT0) is open-drain, so both need the internal pull-up. Leaving them
+    // floating makes the inputs self-trigger.
     SET_PIN_IN(DDRD, DDD3);
-    SET_PIN_LOW(PORTD, PD3);
+    SET_PIN_HIGH(PORTD, PD3);
+
+    SET_PIN_IN(DDRD, DDD2);
+    SET_PIN_HIGH(PORTD, PD2);
 }
 
-void set_up_reed_interrupt() {
+// Both external interrupts are configured low-level triggered (ISCn1:0 = 00).
-    // Falling edge interrupt
+// Edge detection needs the I/O clock, which SLEEP_MODE_PWR_DOWN stops, so a
-    EICRA |= (1 << ISC11);
+// falling-edge INT0/INT1 can never wake the MCU. Only level detection is
-    EICRA &= ~(1 << ISC10);
+// asynchronous. Each handler masks its own interrupt while the source is still
+// asserted, so the low level does not retrigger in a loop.
 
-    // Enable INT1 interrupt
+void set_up_reed_interrupt(void) {
+    EICRA &= ~((1 << ISC11) | (1 << ISC10));
     EIMSK |= (1 << INT1);
 }
 
-void set_up_minute_interrupt() {
+void set_up_minute_interrupt(void) {
-    EICRA |= (1 << ISC01);
+    EICRA &= ~((1 << ISC01) | (1 << ISC00));
-    EICRA &= ~(1 << ISC00);
-
-    // Enable INT1 interrupt
     EIMSK |= (1 << INT0);
 }
 
-void wdt_isr_enable() {
+void reed_interrupt_enable(void) {
+    EIFR = (1 << INTF1); // Drop anything latched while we were masked
+    EIMSK |= (1 << INT1);
+}
+
+void minute_interrupt_enable(void) {
+    EIFR = (1 << INTF0);
+    EIMSK |= (1 << INT0);
+}
+
+void wdt_isr_enable(void) {
+    uint8_t sreg = SREG;
     cli();
     wdt_reset();
 
-    WDTCSR |= (1 << WDCE) | (1 << WDE);
+    // WDRF keeps WDE set, which would block the write below, so it must go
+    // first. The unlock is a single assignment: a read-modify-write does not
+    // open the 4-cycle change window.
+    MCUSR &= ~(1 << WDRF);
+    WDTCSR = (1 << WDCE) | (1 << WDE);
 
-    WDTCSR = (1 << WDP2) | (1 << WDP0); // WDP[3:0] = 0b101 (2 seconds)
+    // WDP[3:0] = 0b011 -> 0.125 s. Interrupt mode only (WDE clear), so an
-    WDTCSR |= (1 << WDIE); // Enable WDT Interrupt mode
+    // expiry wakes us to clear the debounce instead of resetting the part.
-    sei();
+    WDTCSR = (1 << WDIE) | (1 << WDP1) | (1 << WDP0);
+
+    SREG = sreg; // Restore, never blanket-sei(): these run inside an ISR
 }
 
-void wdt_isr_disable() {
+void wdt_isr_disable(void) {
-
+    uint8_t sreg = SREG;
     cli();
+    wdt_reset();
 
-    WDTCSR |= (1 << WDCE) | (1 << WDE);
+    MCUSR &= ~(1 << WDRF);
-    WDTCSR = 0x00; 
+    WDTCSR = (1 << WDCE) | (1 << WDE);
+    WDTCSR = 0x00;
 
-    sei(); 
+    SREG = sreg;
 }
-//
-//void set_debounce_timer_interrupt() {
-//    TCCR0A = 0;
-//    TCCR0B = (1 << CS01) | (1 << CS00);
-//    TIMSK0 = (1 << TOIE0);
-//    TCNT0 = 0;
-//}

avr_code/interrupts.h

@@ -20,12 +20,13 @@ extern "C" {
 
 
 
-    void init_pins();
+    void init_pins(void);
-    void set_up_reed_interrupt();
+    void set_up_reed_interrupt(void);
-//    void set_debounce_timer_interrupt();
+    void set_up_minute_interrupt(void);
-    void set_up_minute_interrupt();
+    void reed_interrupt_enable(void);
-    void wdt_isr_disable();
+    void minute_interrupt_enable(void);
-    void wdt_isr_enable();
+    void wdt_isr_disable(void);
+    void wdt_isr_enable(void);
 
 #ifdef	__cplusplus
 }

avr_code/m95128.c

@@ -35,15 +35,18 @@ void eeprom_write(uint8_t page, unsigned const char* msg, uint8_t msg_len)
     spi_write(EEPROM_WRDI);
     spi_eeprom_select(false);
 
-    while (1) {
+    // Poll the write-in-progress bit, but give up rather than spin forever if
+    // the EEPROM is unpowered or absent.
+    for (uint16_t attempts = 0; attempts < EEPROM_POLL_TIMEOUT_MS; attempts++) {
         spi_eeprom_select(true);
         spi_write(EEPROM_RDSR);
         read_value = spi_read();
         spi_eeprom_select(false);
-        if (read_value == 0x00) {
+        if ((read_value & EEPROM_STATUS_WIP) == 0) {
             return;
         }
-    };
+        _delay_ms(1);
+    }
 }
 
 void eeprom_read(uint8_t page, unsigned char* msg, uint8_t msg_len)
@@ -63,7 +66,7 @@ void eeprom_read(uint8_t page, unsigned char* msg, uint8_t msg_len)
     spi_eeprom_select(false);
 }
 
-void delete_last_page()
+void delete_last_page(void)
 {
     old_last_page = get_last_page();
     if (old_last_page == 0) // If we have nothing, no need to delete anything
@@ -75,7 +78,7 @@ void delete_last_page()
     eeprom_clear_page(old_last_page);
 }
 
-uint8_t get_last_page()
+uint8_t get_last_page(void)
 {
     memset(DATA_BUFFER_65, 0, 1);
     eeprom_read(0, DATA_BUFFER_65, 1);
@@ -127,7 +130,7 @@ tx_rx_data_struct eeprom_read_tx_data(uint8_t page)
     return TX_DATA;
 }
 
-tx_rx_data_struct read_struct_last_page()
+tx_rx_data_struct read_struct_last_page(void)
 {
     uint8_t page_num = get_last_page();
     return eeprom_read_tx_data(page_num);

avr_code/m95128.h

@@ -26,9 +26,12 @@
 #define EEPROM_RDLS 0b10000011 // 0x83
 #define EEPROM_LID 0b10000010 // 0x82
 #define PAGE_SIZE 64
+#define EEPROM_STATUS_WIP 0x01
+
+// A page write takes ~5 ms; past this the device is not responding.
+#define EEPROM_POLL_TIMEOUT_MS 100U
 #ifdef __cplusplus
 extern "C" {
-w
 #endif
 
     void
@@ -37,16 +40,16 @@ void eeprom_write(uint8_t page, unsigned const char* msg, uint8_t msg_len);
 void eeprom_read(uint8_t page, unsigned char* msg, uint8_t msg_len);
 void eeprom_write_tx_data(uint8_t page, tx_rx_data_struct tx_data);
 void write_page_address(uint8_t page);
-uint8_t get_last_page();
+uint8_t get_last_page(void);
 tx_rx_data_struct eeprom_read_tx_data(uint8_t page);
-void delete_last_page();
+void delete_last_page(void);
-uint8_t get_last_page();
+uint8_t get_last_page(void);
 void write_last_page_value(uint8_t page);
 void eeprom_clear_page(uint8_t page);
 void write_page_address(uint8_t page);
 void eeprom_write_tx_data(uint8_t page, tx_rx_data_struct tx_data);
 void write_struct_to_last_page(tx_rx_data_struct tx_data);
-tx_rx_data_struct read_struct_last_page();
+tx_rx_data_struct read_struct_last_page(void);
 #ifdef __cplusplus
 }
 #endif

avr_code/main.c

@@ -19,80 +19,96 @@
 #include <avr/sleep.h>
 #include <stdbool.h>
 #include <stdio.h>
+#include <util/atomic.h>
 #include <util/delay.h>
-#define WAIT_FOREVER                                                                               \
-    while (1) {                                                                                    \
-        _delay_ms(100);                                                                            \
-    };
-
 
 #if ITERATING
 #define SEND_INTERVAL 1
 #else
 #define SEND_INTERVAL 15
 #endif
+
+#define WHEEL_COUNT_SLOTS 15
+
+// Erased EEPROM reads back as 0xFF; anything else is a real page count.
+#define EEPROM_LAST_PAGE_UNINIT 0xFF
+
 uint16_t self_value;
-tx_rx_data_struct CRAP;
-uint16_t main_counter;
 volatile uint8_t is_debouncing = 0;
 volatile bool increment_minute_index = false;
 volatile bool increment_wheel_count = false;
 volatile uint8_t index_wheel_count = 0;
-volatile uint16_t total_wheel_counts[15];
+volatile uint16_t total_wheel_counts[WHEEL_COUNT_SLOTS];
 
 RTC_RFM69_STATUS rtc_rfm69_status;
 
 ISR(INT0_vect) {
-        cli();
+    // The RTC holds INTB low until its flag registers are read, and this is a
+    // level-triggered interrupt, so mask it here and let main re-arm it once
+    // the RTC has released the line.
+    EIMSK &= ~(1 << INT0);
 #if DO_UART
     uart_sendString("\t\t\t\tMINUTE INTERRUPT\n");
 #endif
     increment_minute_index = true;
-//    sei();
 }
 
 ISR(INT1_vect) {
-    cli();
 
 #if ITERATING
     increment_minute_index = true;
 #endif
-    
+
 #if DO_UART
     uart_sendString("\t\t\t\tREED INTERRUPT\n");
 #endif
 
-
     if (!is_debouncing) {
-        total_wheel_counts[index_wheel_count]++;
+        if (index_wheel_count < WHEEL_COUNT_SLOTS) {
+            total_wheel_counts[index_wheel_count]++;
+        }
         is_debouncing = 1;
+        // Mask INT1 for the debounce window: the magnet holds the reed closed
+        // (and the pin low) for far longer than one revolution's worth of
+        // bounce, and a level-triggered interrupt would retrigger continuously.
+        EIMSK &= ~(1 << INT1);
         wdt_isr_enable();
     }
-//    sei();
 }
 
 ISR(WDT_vect) {
-    cli();
     is_debouncing = 0;
     wdt_isr_disable();
-//    sei();
+    reed_interrupt_enable();
 }
 
-void start_sleeping() {
+void start_sleeping(void) {
     spi_eeprom_select(false);
     spi_rfm69_select(false);
     rfid_set_low_power_down(true);
     rfid_set_i2c_power(false);
     ldo_set_state(false);
     _delay_ms(10);
-    sleep_bod_disable();
+
     set_sleep_mode(SLEEP_MODE_PWR_DOWN);
-    sleep_enable();
+
+    cli();
+    // Don't sleep through work that arrived while we were dropping the rails.
+    // Testing the flag with interrupts off, then sei() immediately before
+    // sleep_cpu(), is the avr-libc idiom that closes that race -- and
+    // sleep_bod_disable() is a timed sequence, so it belongs here and not
+    // before sleep_enable() where it had no effect at all.
+    if (!increment_minute_index) {
+        sleep_enable();
+        sleep_bod_disable();
+        sei();
+        sleep_cpu();
+        sleep_disable();
+    }
     sei();
-    sleep_cpu();
 }
 
-uint16_t get_battery_reading() {
+uint16_t get_battery_reading(void) {
     adc_Enable();
     adc_GetConversion(14);
     adc_GetConversion(14);
@@ -149,12 +165,17 @@ int main(void) {
     uart_sendString("Initialized RFM69\n");
 #endif
 
-    write_last_page_value(0);
+    // Only initialise the spool pointer when it has never been written --
+    // clearing it unconditionally would discard every unsent message across a
+    // reset.
+    if (get_last_page() == EEPROM_LAST_PAGE_UNINIT) {
+        write_last_page_value(0);
+    }
 #if DO_UART
     uart_sendString("Set up last page value for SPI flash\n");
 #endif
 
-    for (uint8_t c = 0; c < 15; c++) {
+    for (uint8_t c = 0; c < WHEEL_COUNT_SLOTS; c++) {
         total_wheel_counts[c] = 0;
     }
 
@@ -203,27 +224,43 @@ int main(void) {
         spi_eeprom_select(false);
 
         start_sleeping();
-        cli();
 
-        if (increment_minute_index) {
+        // Short critical sections around the shared variables only. The old
+        // blanket cli() stayed in force through the whole radio/EEPROM
+        // sequence, so every reed pulse in that multi-second window was lost.
+        bool minute_elapsed;
+        ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
+            minute_elapsed = increment_minute_index;
+            increment_minute_index = false;
+        }
+
+        if (minute_elapsed) {
 #if DO_UART
             uart_sendString("In minute index\n");
 #endif
-            increment_minute_index = false;
+            uint8_t slot;
-            is_debouncing = 0;
+            ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
-            index_wheel_count += 1;
+                if (index_wheel_count < WHEEL_COUNT_SLOTS) {
+                    index_wheel_count += 1;
+                }
+                slot = index_wheel_count;
+            }
+
+            // The I2C rail has to be back up before we touch the RTC: sleeping
+            // dropped both the LDO and the tag's supply.
+            ldo_set_state(true);
+            rfid_set_i2c_power(true);
+            _delay_ms(1);
 
             rtc_read_interrupt_register();
             rtc_read_status_register();
 
-            rtc_read_interrupt_register();
+            // Reading the flags releases INTB, so INT0 can safely be re-armed.
-            rtc_read_status_register();
+            minute_interrupt_enable();
 
-            if (index_wheel_count >= SEND_INTERVAL) {
+            if (slot >= SEND_INTERVAL) {
-                index_wheel_count = 0;
+                ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { index_wheel_count = 0; }
-                ldo_set_state(true);
                 rfm69_init();
-                rfid_set_i2c_power(true);
                 rfid_set_low_power_down(false);
                 _delay_ms(1);
                 IDENTIFIER = get_nugget_data();
@@ -233,11 +270,21 @@ int main(void) {
                     rtc_rfm69_status = set_time_from_rfm69(IDENTIFIER);
                 }
 
+                // Snapshot and clear the counters in one critical section so
+                // a reed pulse landing mid-packet is neither lost nor double
+                // counted.
+                uint16_t counts_snapshot[WHEEL_COUNT_SLOTS];
+                ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
+                    for (uint8_t c = 0; c < WHEEL_COUNT_SLOTS; c++) {
+                        counts_snapshot[c] = total_wheel_counts[c];
+                        total_wheel_counts[c] = 0;
+                    }
+                }
+
                 // Generate wheel counts message
                 reset_txrx_struct(&TX_DATA);
-                get_battery_reading();
                 TX_DATA = generate_wheel_counts_message(
-                        IDENTIFIER, rtc_read_time(), get_battery_reading(), total_wheel_counts);
+                        IDENTIFIER, rtc_read_time(), get_battery_reading(), counts_snapshot);
 
 #if DO_UART
                 uart_sendString("TX DATA Sent\n");
@@ -253,28 +300,26 @@ int main(void) {
                     write_struct_to_last_page(TX_DATA);
                 }
 
-                for (uint8_t c = 0; c < 15; c++) {
-                    total_wheel_counts[c] = 0;
-                }
                 if ((result == DATA_SEND_SUCCESS) && (get_last_page() > 0)) {
                     reset_txrx_struct(&TX_DATA);
                     TX_DATA = read_struct_last_page();
                     TX_DATA.flags = MSG_RESENT_COUNTS;
                     _delay_ms(250);
                     result = send_message(TX_DATA);
-#if DO_UART
-                    if (result == DATA_NOT_SENT) {
-                        uart_sendString("       SPI not sent\n");
-                    }
-#endif
-
-                    send_message(TX_DATA);
-                    delete_last_page();
 #if DO_UART
                     uart_sendString("TX DATA From SPI Memory\n");
                     uart_print_tx_rx_data(TX_DATA);
 #endif
-
+                    // Only drop the spooled page once it is actually
+                    // acknowledged, otherwise a failed retry loses the data.
+                    if (result == DATA_SEND_SUCCESS) {
+                        delete_last_page();
+                    }
+#if DO_UART
+                    else {
+                        uart_sendString("       SPI not sent\n");
+                    }
+#endif
                 }
             }
         }

avr_code/max31329.c

@@ -39,7 +39,7 @@ RTC_RFM69_STATUS set_time_from_rfm69(identifier_results id_data)
     return RTC_RFM69_SET_TIME_FAILED;
 }
 
-uint8_t rtc_set_per_minute_alarm()
+uint8_t rtc_set_per_minute_alarm(void)
 {
 
     DATA_BUFFER_7[0] = 0x80;
@@ -61,17 +61,17 @@ void uart_print_rtc_time(time_struct td)
 
 uint8_t rtc_read_register(uint8_t addr) { return read_one_byte_no_err_register(I2C_ADDR, addr); }
 
-uint8_t rtc_read_status_register() { return rtc_read_register(0x00); }
+uint8_t rtc_read_status_register(void) { return rtc_read_register(0x00); }
 
-uint8_t rtc_read_interrupt_register() { return rtc_read_register(0x01); }
+uint8_t rtc_read_interrupt_register(void) { return rtc_read_register(0x01); }
 
-uint8_t rtc_set_alarm_config() { return write_one_byte(I2C_ADDR, 0x04, 0b00001010); }
+uint8_t rtc_set_alarm_config(void) { return write_one_byte(I2C_ADDR, 0x04, 0b00001010); }
 
-uint8_t rtc_enable_interrupts() { return write_one_byte(I2C_ADDR, 0x01, 0b00000010); }
+uint8_t rtc_enable_interrupts(void) { return write_one_byte(I2C_ADDR, 0x01, 0b00000010); }
 
 uint8_t rtc_read_time_array(uint8_t* data) { return read_n_bytes(I2C_ADDR, 0x06, data, 7); }
 
-time_struct rtc_read_time()
+time_struct rtc_read_time(void)
 {
 
     rtc_read_time_array(DATA_BUFFER_7);
@@ -94,15 +94,16 @@ uint8_t rtc_write_time(time_struct tm)
     if (i2c_start((I2C_ADDR << 1) | 0x00))
         return 1;
 
-    i2c_write(0x06);
+    uint8_t err = 0;
-    i2c_write(DEC2BCD(tm.Second));
+    err |= i2c_write(0x06);
-    i2c_write(DEC2BCD(tm.Minute));
+    err |= i2c_write(DEC2BCD(tm.Second));
-    i2c_write(DEC2BCD(tm.Hour));
+    err |= i2c_write(DEC2BCD(tm.Minute));
-    i2c_write(tm.Wday);
+    err |= i2c_write(DEC2BCD(tm.Hour));
-    i2c_write(DEC2BCD(tm.Day));
+    err |= i2c_write(tm.Wday);
-    i2c_write(DEC2BCD(tm.Month));
+    err |= i2c_write(DEC2BCD(tm.Day));
-    i2c_write(DEC2BCD(y2kYearToTm(tm.Year)));
+    err |= i2c_write(DEC2BCD(tm.Month));
+    err |= i2c_write(DEC2BCD(y2kYearToTm(tm.Year)));
 
     i2c_stop();
-    return 0;
+    return err ? 1 : 0;
 }

avr_code/max31329.h

@@ -10,7 +10,6 @@
 #include "rfm69.h"
 #include "st25dv.h"
 #include "uart.h"
-#define I2C_ADDR 0x68
 
 #ifndef MAX31329_H
 #define MAX31329_H
@@ -19,20 +18,25 @@
 extern "C" {
 #endif
 
-#define DEC2BCD(n) (n + (6 * (n / 10)))
+#define I2C_ADDR 0x68
-#define BCD2DEC(n) (n - (6 * (n >> 4)))
 
-#define tmYearToY2k(Y) ((Y) - 30) // offset is from 2000
+#define DEC2BCD(n) ((n) + (6 * ((n) / 10)))
-#define y2kYearToTm(Y) ((Y) + 30)
+#define BCD2DEC(n) ((n) - (6 * ((n) >> 4)))
 
-uint8_t rtc_enable_interrupts();
+// time_struct.Year is years since 2000, which is exactly what the RTC's 2-digit
-uint8_t rtc_set_per_minute_alarm();
+// year register holds -- no offset. (The old +/-30 round-tripped but stored the
+// wrong year in the RTC and overflowed BCD above 2069.)
+#define tmYearToY2k(Y) (Y)
+#define y2kYearToTm(Y) (Y)
+
+uint8_t rtc_enable_interrupts(void);
+uint8_t rtc_set_per_minute_alarm(void);
 uint8_t rtc_read_time_array(uint8_t* data);
-time_struct rtc_read_time();
+time_struct rtc_read_time(void);
 uint8_t rtc_write_time(time_struct tm);
-uint8_t rtc_set_alarm_config();
+uint8_t rtc_set_alarm_config(void);
-uint8_t rtc_read_status_register();
+uint8_t rtc_read_status_register(void);
-uint8_t rtc_read_interrupt_register();
+uint8_t rtc_read_interrupt_register(void);
 uint8_t rtc_read_register(uint8_t addr);
 void uart_print_rtc_time(time_struct td);
 RTC_RFM69_STATUS set_time_from_rfm69(identifier_results id_data);

avr_code/ndef.c

@@ -1,10 +1,23 @@
 
 
 #include "ndef.h"
-ndef_message readNDEFText(unsigned char *buf) {
+// Everything read here comes off an NFC tag that anyone can write, so every
-    int addr = 0;
+// length taken from the buffer is bounds-checked before it is used.
-    NDEF_MSG.success = 0;
+#define NDEF_NEED(n)                                                                               \
+    {                                                                                              \
+        if ((addr + (uint16_t)(n)) > buf_len) {                                                    \
+            NDEF_MSG.success = NDEF_ERR_TRUNCATED;                                                 \
+            return NDEF_MSG;                                                                       \
+        }                                                                                          \
+    }
 
+ndef_message readNDEFText(unsigned char *buf, uint8_t buf_len) {
+    uint16_t addr = 0;
+    NDEF_MSG.success = 0;
+    NDEF_MSG.payload_len = 0;
+    NDEF_MSG.payload[0] = '\0';
+
+    NDEF_NEED(2);
     if (buf[0] != NDEF_TLV) {
         NDEF_MSG.success = 1;
         return NDEF_MSG;
@@ -18,6 +31,7 @@ ndef_message readNDEFText(unsigned char *buf) {
     // int len_field = buf[1];
     addr = 2;
 
+    NDEF_NEED(3);
     // bool is_short_record = (buf[addr] & NDEF_SHORT_RECORD) == NDEF_SHORT_RECORD;
     bool has_id_length = (buf[addr] & NDEF_ID_LEN) == NDEF_ID_LEN;
     uint8_t tnf = buf[addr] & 0x7;
@@ -31,18 +45,18 @@ ndef_message readNDEFText(unsigned char *buf) {
 
     uint8_t id_length = 0;
     if (has_id_length) {
+        NDEF_NEED(1);
         id_length = buf[addr];
         addr += 1;
     }
 
-    uint8_t type_value[type_length + 1];
+    // Only the first type byte is ever inspected, so skip the rest rather than
-    for (uint8_t i = 0; i < type_length; i++) {
+    // copying them into a tag-sized VLA.
-        type_value[i] = buf[addr];
+    NDEF_NEED(type_length);
-        addr += 1; // 6
+    uint8_t type_value_0 = (type_length > 0) ? buf[addr] : 0;
-    }
+    addr += type_length;
 
-    type_value[type_length] = 0;
+    if (type_value_0 != NDEF_TEXT_RECORD) {
-    if (type_value[0] != NDEF_TEXT_RECORD) {
         NDEF_MSG.success = 11;
         return NDEF_MSG;
     };
@@ -52,21 +66,39 @@ ndef_message readNDEFText(unsigned char *buf) {
     };
 
     if (has_id_length && (id_length > 0)) {
+        NDEF_NEED(id_length);
         addr += id_length;
     }
 
+    NDEF_NEED(1);
     uint8_t lang_str_len = buf[addr];
+
+    // payload_length covers the language-length byte plus the language code
+    // plus the text. Subtracting without this check wraps a uint8_t to ~250.
+    if (payload_length < ((uint16_t)lang_str_len + 1)) {
+        NDEF_MSG.success = NDEF_ERR_BAD_LENGTH;
+        return NDEF_MSG;
+    }
+    payload_length -= lang_str_len; // Language string
+    payload_length -= 1;            // The byte that says how long the language string is
+
+    NDEF_NEED((uint16_t)lang_str_len + 1);
     addr += lang_str_len;
     addr += 1;
-    payload_length -= lang_str_len;     // Language string
+
-    payload_length -= 1;                //  The byte that says how long the language string is
+    // Leave room for the terminator the UART print and strchr() both rely on.
+    if (payload_length > (sizeof(NDEF_MSG.payload) - 1)) {
+        payload_length = sizeof(NDEF_MSG.payload) - 1;
+    }
+    NDEF_NEED(payload_length);
+
     for (uint8_t i = 0; i < (payload_length); i++) {
         NDEF_MSG.payload[i] = buf[addr];
         addr += 1;
     }
-
+    NDEF_MSG.payload[payload_length] = '\0';
-    // NDEF_MSG.payload = payload;
     NDEF_MSG.payload_len = payload_length;
+
     #if DO_UART
     uart_sendString(NDEF_MSG.payload);
     uart_sendString("\n");

avr_code/ndef.h

@@ -24,7 +24,11 @@ extern "C" {
 #define NDEF_TEXT_RECORD 0x54
 #define TNF_KNOWN 0x01
 
-ndef_message readNDEFText(unsigned char *buf) ;
+// readNDEFText failure codes reported through ndef_message.success
+#define NDEF_ERR_TRUNCATED 13
+#define NDEF_ERR_BAD_LENGTH 14
+
+ndef_message readNDEFText(unsigned char *buf, uint8_t buf_len);
 
 #ifdef __cplusplus
 }

avr_code/power_mgmt.c

@@ -1,6 +1,6 @@
 #include "power_mgmt.h"
 
-void shutdown_all_peripherals() {
+void shutdown_all_peripherals(void) {
 
     power_adc_disable();
     power_timer0_disable();

avr_code/power_mgmt.h

@@ -4,6 +4,7 @@
  *
  * Created on December 20, 2024, 3:54 PM
  */
+#include "defines.h" // for DO_UART, which shutdown_all_peripherals() tests
 #include <avr/power.h>
 #ifndef POWER_MGMT_H
 #define	POWER_MGMT_H
@@ -13,7 +14,7 @@ extern "C" {
 #endif
 
 
-    void shutdown_all_peripherals();
+    void shutdown_all_peripherals(void);
 
 #ifdef	__cplusplus
 }

avr_code/rfm69.c

@@ -1,8 +1,5 @@
 #include "rfm69.h"
 
-int8_t rssi;
-uint8_t i;
-uint8_t len_payload;
 uint32_t msg_hash;
 uint8_t p_hash_1;
 uint8_t p_hash_2;
@@ -15,8 +12,6 @@ uint8_t c_hash_3;
 bool cond_1;
 bool cond_2;
 bool cond_3;
-bool cond_4;
-bool cond_5;
 
 DATA_SEND_STATUS send_message(tx_rx_data_struct tx_data)
 {
@@ -46,19 +41,19 @@ DATA_SEND_STATUS send_message(tx_rx_data_struct tx_data)
             cond_3 = cond_1 && cond_2;
 
             if (cond_3 && (RX_DATA.flags == MSG_RECV_COUNTS_SUCCESS) && (RX_DATA.msg[3] == 0xFF)) {
-                                    #if DO_UART
+#if DO_UART
-                    uart_sendString("       RX DATA SUCCESS\n");
+                uart_sendString("       RX DATA SUCCESS\n");
 #endif
                 return DATA_SEND_SUCCESS;
-                break;
+            }
-            } else if (
+#if DO_UART
+            else if (
                 cond_3 && (RX_DATA.flags == MSG_RECV_COUNTS_FAIL) && (RX_DATA.msg[3] == 0x00)) {
-uart_sendString("       RX DATA FAILED\n");
+                uart_sendString("       RX DATA FAILED\n");
             } else {
                 uart_sendString("       RX DATA ANOTHER ERROR\n");
             }
-
+#endif
-        } else {
         }
     }
     return DATA_NOT_SENT;
@@ -88,8 +83,11 @@ void uart_print_tx_rx_data(tx_rx_data_struct tx_rx_print)
 
 void rfm69_write_msg(tx_rx_data_struct txrxd)
 {
-    set_rfm69_rx_mode();
+    // TxStart is configured as FifoNotEmpty, so the radio begins transmitting
-    set_rfm69_tx_mode();
+    // the moment the first byte lands. Fill the FIFO from standby and only then
+    // switch to TX, otherwise the packet goes out ahead of its own payload.
+    set_rfm69_standby();
+
     spi_rfm69_select(true);
     spi_write(REG_FIFO | RFM69_SPI_WRITE);
     if (txrxd.len > (60)) {
@@ -106,22 +104,30 @@ void rfm69_write_msg(tx_rx_data_struct txrxd)
     }
     spi_rfm69_select(false);
 
+    set_rfm69_tx_mode();
     wait_tx_sent();
     set_rfm69_rx_mode();
 }
 
-tx_rx_data_struct rfm69_read_msg()
+tx_rx_data_struct rfm69_read_msg(void)
 {
     memset(RX_DATA.msg, ' ', sizeof(RX_DATA.msg));
     spi_rfm69_select(true);
     spi_write(REG_FIFO);
 
-    RX_DATA.len = spi_read() - 4;
+    uint8_t raw_len = spi_read();
     RX_DATA.to = spi_read();
     RX_DATA.from = spi_read();
     RX_DATA.dtype = spi_read();
     RX_DATA.flags = spi_read();
-    uint8_t len_f = RX_DATA.len;
+
+    // The length byte comes off the air and is not trustworthy: below 4 it
+    // underflows to ~252, above 60 it walks off the end of msg[].
+    uint8_t len_f = (raw_len < 4) ? 0 : (uint8_t)(raw_len - 4);
+    if (len_f > sizeof(RX_DATA.msg)) {
+        len_f = sizeof(RX_DATA.msg);
+    }
+    RX_DATA.len = len_f;
 
     for (uint8_t idx_f = 0; idx_f < len_f; idx_f++) {
         RX_DATA.msg[idx_f] = spi_read();
@@ -132,21 +138,11 @@ tx_rx_data_struct rfm69_read_msg()
     return RX_DATA;
 }
 
-void rfm69_set_state(bool state)
-{
-    SET_PIN_OUT(DDRC, DDC2);
-    if (!state) {
-        SET_PIN_HIGH(PORTC, PC2);
-    } else {
-        SET_PIN_LOW(PORTC, PC2);
-    }
-}
-
 uint8_t spi_read_rfm69_rt(uint8_t reg)
 {
     spi_rfm69_select(true);
     spi_write(reg);
-    uint8_t data_read = spi_read(0xFF);
+    uint8_t data_read = spi_read();
     spi_rfm69_select(false);
     return data_read;
 }
@@ -221,23 +217,25 @@ uint32_t hash_3bytes(unsigned const char* str, uint8_t str_len)
 {
     uint32_t hash = 0;
 
-    for (i = 0; i < str_len; i++) {
+    for (uint8_t i = 0; i < str_len; i++) {
         hash = (hash * 31 + str[i]) % 0xFFFFFF;
     }
     return hash;
 }
 
-void set_rfm69_power_amp_boost()
+void set_rfm69_power_amp_boost(void)
 {
+    spi_write_rfm69_rt(REG_OCP, VAL_OCP_OFF);
     spi_write_rfm69_rt(REG_TEST_PA1, VAL_TEST_PA1_BOOST);
     spi_write_rfm69_rt(REG_TEST_PA2, VAL_TEST_PA2_BOOST);
 }
 
-void set_rfm69_power_amp_normal()
+void set_rfm69_power_amp_normal(void)
 {
     spi_write_rfm69_rt(REG_TEST_PA1, VAL_TEST_PA1_NORMAL);
     spi_write_rfm69_rt(REG_TEST_PA2, VAL_TEST_PA2_NORMAL);
-};
+    spi_write_rfm69_rt(REG_OCP, VAL_OCP_ON);
+}
 
 void reset_txrx_struct(tx_rx_data_struct* s)
 {
@@ -258,10 +256,15 @@ void set_rfm69_mode(uint8_t target_mode)
     spi_write_rfm69_rt(REG_OP_MODE, mode);
 }
 
-void wait_tx_sent()
+bool wait_tx_sent(void)
 {
-    while (TX_NOT_SENT)
+    for (uint16_t attempts = 0; attempts < RFM69_TIMEOUT_MS; attempts++) {
-        ;
+        if (TX_SENT) {
+            return true;
+        }
+        _delay_ms(1);
+    }
+    return false;
 }
 
 uint8_t hash(const char* str, uint8_t min, uint8_t max)
@@ -279,68 +282,69 @@ uint8_t hash(const char* str, uint8_t min, uint8_t max)
 bool wait_rx_payload_ready_timeout(uint16_t attempts)
 {
     set_rfm69_rx_mode();
-    uint16_t counter = 0;
+    // Test the flag before spending the tick, so a payload that arrives on the
-    while (1) {
+    // last attempt is not thrown away.
-        _delay_ms(1);
+    for (uint16_t counter = 0; counter < attempts; counter++) {
-        WHILE_BREAK(counter, attempts);
         if (RX_PAYLOAD_READY) {
             return true;
-            break;
         }
+        _delay_ms(1);
+    }
+    return RX_PAYLOAD_READY != 0;
+}
+
+bool wait_rx_payload_ready(void)
+{
+    return wait_rx_payload_ready_timeout(RFM69_TIMEOUT_MS);
+}
+
+bool wait_rfm69_mode_ready(void)
+{
+    for (uint16_t attempts = 0; attempts < RFM69_TIMEOUT_MS; attempts++) {
+        if (MODE_READY) {
+            return true;
+        }
+        _delay_ms(1);
     }
     return false;
-
-    ;
 }
 
-void wait_rx_payload_ready()
+void set_rfm69_tx_mode(void)
-{
-
-    while (RX_PAYLOAD_NOT_READY) { };
-}
-
-void wait_rfm69_mode_ready()
-{
-    while (MODE_NOT_READY)
-        ;
-}
-
-void set_rfm69_tx_mode()
 {
     set_rfm69_power_amp_boost();
     set_rfm69_mode(VAL_OPMODE_TX);
     wait_rfm69_mode_ready();
-};
+}
 
-void set_rfm69_rx_mode()
+void set_rfm69_rx_mode(void)
 {
     set_rfm69_power_amp_normal();
     set_rfm69_mode(VAL_OPMODE_RX);
     wait_rfm69_mode_ready();
-};
+}
 
-void set_rfm69_standby()
+void set_rfm69_standby(void)
 {
     set_rfm69_power_amp_normal();
     set_rfm69_mode(VAL_OPMODE_STDBY);
     wait_rfm69_mode_ready();
 }
 
-void set_rfm69_sleep()
+void set_rfm69_sleep(void)
 {
     set_rfm69_power_amp_normal();
     set_rfm69_mode(VAL_OPMODE_SLEEP);
     wait_rfm69_mode_ready();
 }
 
-void set_rfm69_idle()
+void set_rfm69_idle(void)
 {
     set_rfm69_power_amp_normal();
     set_rfm69_mode(VAL_OPMODE_STDBY);
     wait_rfm69_mode_ready();
 }
 
-void reset_rfm69()
+void reset_rfm69(void)
 {
     rfm69_reset_state(true);
     _delay_ms(10);
@@ -348,14 +352,7 @@ void reset_rfm69()
     _delay_ms(10);
 }
 
-void set_rfm69_tx_power()
+void rfm69_init(void)
-{
-    uint8_t PA_LEVEL_SET
-        = VAL_PALEVEL_PA1_ON | VAL_PALEVEL_PA2_ON | ((20 + 14) & VAL_PALEVEL_PA1_OUTPUTPOWER);
-    spi_write_rfm69_rt(REG_PA_LEVEL, PA_LEVEL_SET);
-}
-
-void rfm69_init()
 {
     reset_rfm69();
     _delay_ms(100);
@@ -364,8 +361,6 @@ void rfm69_init()
     spi_write_rfm69_rt(REG_FREQ_MIDDLE_SB, VAL_FREQ_433MHz_MID_SB);
     spi_write_rfm69_rt(REG_FREQ_LSB, VAL_FREQ_433MHz_LSB);
 
-    spi_write_rfm69_rt(REG_FREQ_DEV_MSB, VAL_FREQ_DEV_MSB);
-
     spi_write_rfm69_rt(
         REG_FIFO_THRESH,
         VAL_TX_START_FIFO_NOT_EMPTY | VAL_FIFO_LEVEL_INTERRUPT); // TX condition
@@ -377,14 +372,15 @@ void rfm69_init()
     spi_write_rfm69_rt(REG_SYNC_CONFIG, VAL_SYNCWORDS_ON | VAL_SYNCWORDS_SIZE_2_BYTES);
 
     spi_write_rfm69_rt(REG_DATA_MODUL,
-                       VAL_DATA_PACKET_MODE | VAL_DATA_MODUL_OOK); // RegDataModul
+                       VAL_DATA_PACKET_MODE | VAL_DATA_MODUL_FSK
+                           | VAL_MODUL_SHAPING_GAUSS_BT_1_0); // RegDataModul
 
     spi_write_rfm69_rt(REG_BITRATE_MSB,
                        VAL_BITRATE_250kbps_MSB); // RegBitrateMSB
     spi_write_rfm69_rt(REG_BITRATE_LSB,
                        VAL_BITRATE_250kbps_LSB); // RegbBitrateLSB
-    spi_write_rfm69_rt(REG_FDEV_MSB, 0x10); // RegFdevMSB
+    spi_write_rfm69_rt(REG_FDEV_MSB, VAL_FDEV_MSB); // RegFdevMSB (0x05)
-    spi_write_rfm69_rt(REG_FDEV_LSB, 0x00); // RegFdevLSB
+    spi_write_rfm69_rt(REG_FDEV_LSB, VAL_FDEV_LSB); // RegFdevLSB (0x06)
 
     spi_write_rfm69_rt(REG_RX_BW, 0xE0); // RegRxBw
     spi_write_rfm69_rt(REG_AFC_BW, 0xE0); // RegAfcBw

avr_code/rfm69.h

@@ -15,14 +15,21 @@
 #ifndef RFM69_H
 #define RFM69_H
 
-#define MODE_NOT_READY !(spi_read_rfm69_rt(REG_IRQ_FLAGS1) & VAL_IRQ_FLAGS1_MODEREADY)
-#define RX_PAYLOAD_READY spi_read_rfm69_rt(REG_IRQ_FLAGS2) & VAL_IRQ_FLAGS2_RX_PAYLOADREADY
-#define RX_PAYLOAD_NOT_READY !(RX_PAYLOAD_READY)
-#define TX_NOT_SENT !(spi_read_rfm69_rt(REG_IRQ_FLAGS2) & VAL_IRQ_FLAGS2_TX_SENT)
 #ifdef __cplusplus
 extern "C" {
 #endif
 
+#define MODE_READY (spi_read_rfm69_rt(REG_IRQ_FLAGS1) & VAL_IRQ_FLAGS1_MODEREADY)
+#define MODE_NOT_READY (!MODE_READY)
+#define RX_PAYLOAD_READY (spi_read_rfm69_rt(REG_IRQ_FLAGS2) & VAL_IRQ_FLAGS2_RX_PAYLOADREADY)
+#define RX_PAYLOAD_NOT_READY (!RX_PAYLOAD_READY)
+#define TX_SENT (spi_read_rfm69_rt(REG_IRQ_FLAGS2) & VAL_IRQ_FLAGS2_TX_SENT)
+#define TX_NOT_SENT (!TX_SENT)
+
+// Bail-out for every RFM69 poll loop: an absent or unpowered radio must not
+// hang the firmware, since no watchdog reset is armed.
+#define RFM69_TIMEOUT_MS 100U
+
 #define REG_FIFO 0x00
 #define REG_FREQ_MSB 0x07
 #define REG_FREQ_MIDDLE_SB 0x08
@@ -35,7 +42,7 @@ extern "C" {
 #define REG_DATA_MODUL 0x02
 #define REG_BITRATE_MSB 0x03
 #define REG_BITRATE_LSB 0x04
-#define REG_FDEV_MSB 0x06
+#define REG_FDEV_MSB 0x05
 #define REG_FDEV_LSB 0x06
 #define REG_RX_BW 0x19
 #define REG_AFC_BW 0x1A
@@ -48,9 +55,15 @@ extern "C" {
 #define REG_IRQ_FLAGS1 0x27
 #define REG_IRQ_FLAGS2 0x28
 #define REG_RSSI_VALUE 0x24
+#define REG_OCP 0x13
 
-#define REG_FREQ_DEV_MSB 0x05
+// Over-current protection must be off while the PA boost registers are set,
-#define VAL_FREQ_DEV_MSB 0x10
+// per the datasheet's high-power (+20 dBm) sequence.
+#define VAL_OCP_OFF 0x0F
+#define VAL_OCP_ON 0x1A
+
+#define VAL_FDEV_MSB 0x10
+#define VAL_FDEV_LSB 0x00
 
 #define VAL_TEST_DAGC_DEFAULT 0x30
 #define VAL_DATA_PACKET_MODE 0x00
@@ -58,7 +71,8 @@ extern "C" {
 #define VAL_BITRATE_250kbps_MSB 0x00
 #define VAL_BITRATE_250kbps_LSB 0x80
 
-#define VAL_DATA_MODUL_OOK 0x01
+#define VAL_DATA_MODUL_FSK 0x00 // RegDataModul ModulationType is bits 4:3
+#define VAL_MODUL_SHAPING_GAUSS_BT_1_0 0x01
 #define VAL_TX_START_FIFO_NOT_EMPTY 0x80
 #define VAL_FIFO_LEVEL_INTERRUPT 0x0f
 
@@ -104,28 +118,26 @@ extern "C" {
 #define VAL_FREQ_433MHz_LSB 0x00
 
 DATA_SEND_STATUS send_message(tx_rx_data_struct tx_data);
-void rfm69_set_state(bool state);
 
 uint8_t spi_read_rfm69_rt(uint8_t reg);
 uint8_t spi_write_rfm69_rt(uint8_t reg, uint8_t val);
 uint8_t spi_write_rfm69_multiple_rt(uint8_t reg, const char* vals, uint8_t len);
-void set_rfm69_power_amp_boost();
+void set_rfm69_power_amp_boost(void);
-void set_rfm69_power_amp_normal();
+void set_rfm69_power_amp_normal(void);
-tx_rx_data_struct rfm69_read_msg();
+tx_rx_data_struct rfm69_read_msg(void);
 void reset_txrx_struct(tx_rx_data_struct* s);
 void rfm69_write_msg(tx_rx_data_struct txrxd);
 void set_rfm69_mode(uint8_t mode);
-void wait_rfm69_mode_ready();
+bool wait_rfm69_mode_ready(void);
-void set_rfm69_tx_mode();
+void set_rfm69_tx_mode(void);
-void wait_tx_sent();
+bool wait_tx_sent(void);
-void wait_rx_payload_ready();
+bool wait_rx_payload_ready(void);
-void reset_rfm69();
+void reset_rfm69(void);
-void set_rfm69_rx_mode();
+void set_rfm69_rx_mode(void);
-void set_rfm69_standby();
+void set_rfm69_standby(void);
-void set_rfm69_sleep();
+void set_rfm69_sleep(void);
-void set_rfm69_idle();
+void set_rfm69_idle(void);
-void set_rfm69_tx_power();
+void rfm69_init(void);
-void rfm69_init();
 bool wait_rx_payload_ready_timeout(uint16_t attempts);
 uint8_t hash(const char* str, uint8_t min, uint8_t max);
 uint32_t hash_3bytes(unsigned const char* str, uint8_t str_len);

avr_code/spi.c

@@ -21,6 +21,6 @@ void spi_rfm69_select(bool state)
         return SPDR1; // Return received data
     }
 
-    uint8_t spi_read() {
+    uint8_t spi_read(void) {
         return spi_write(0xFF);
     }

avr_code/spi.h

@@ -7,7 +7,7 @@
 
 
 uint8_t spi_write(uint8_t data);
-uint8_t spi_read();
+uint8_t spi_read(void);
 void spi_rfm69_select(bool state);
 
 #endif

avr_code/st25dv.c

@@ -1,43 +1,70 @@
 #include "st25dv.h"
 
-identifier_results get_nugget_data() {
+#define IDENT_NAME_MAX (sizeof(IDENTIFIER.name_str) - 1)
+#define IDENT_DIAM_MAX (sizeof(IDENTIFIER.diameter_str) - 1)
+
+static void set_identifier(const char* name, uint8_t name_len, const char* diam, uint8_t diam_len) {
+    if (name_len > IDENT_NAME_MAX) {
+        name_len = IDENT_NAME_MAX;
+    }
+    if (diam_len > IDENT_DIAM_MAX) {
+        diam_len = IDENT_DIAM_MAX;
+    }
+    memcpy(IDENTIFIER.name_str, name, name_len);
+    IDENTIFIER.name_str[name_len] = '\0';
+    IDENTIFIER.name_len = name_len;
+
+    memcpy(IDENTIFIER.diameter_str, diam, diam_len);
+    IDENTIFIER.diameter_str[diam_len] = '\0';
+    IDENTIFIER.diameter_len = diam_len;
+
+    IDENTIFIER.hashed = hash(IDENTIFIER.name_str, 0, 59);
+}
+
+identifier_results get_nugget_data(void) {
 
     NDEF_MSG = rfid_read_first_ndef_entry();
 
+    // readNDEFText reports parse failures through success; without this check a
+    // missing or malformed tag leaves stale/uninitialised bytes in payload and
+    // we transmit them as the node identity.
+    if (NDEF_MSG.success != 0) {
+#if DO_UART
+        uart_print_uint8(NDEF_MSG.success, "NDEF parse failed, code ");
+#endif
+        set_identifier("UNKNOWN", 7, "N/A", 3);
+        return IDENTIFIER;
+    }
+
     TRIMMED_STRING = remove_spaces(NDEF_MSG.payload, NDEF_MSG.payload_len);
     char* delim_ptr = strchr(TRIMMED_STRING.str, ',');
     if (delim_ptr != NULL) {
-        uint8_t index_comma = delim_ptr - TRIMMED_STRING.str;
+        uint8_t index_comma = (uint8_t)(delim_ptr - TRIMMED_STRING.str);
-        memcpy(IDENTIFIER.name_str, TRIMMED_STRING.str, index_comma);
+        // The diameter is what follows the comma, so its length is the
-        memcpy(
+        // remainder of the string -- not the whole string's length, which read
-                IDENTIFIER.diameter_str, TRIMMED_STRING.str + index_comma + 1, TRIMMED_STRING.length);
+        // off the end of the 21-byte buffer.
-        IDENTIFIER.name_len = index_comma;
+        uint8_t diam_len = (uint8_t)(TRIMMED_STRING.length - index_comma - 1);
-        IDENTIFIER.diameter_len = TRIMMED_STRING.length - index_comma;
+        set_identifier(TRIMMED_STRING.str, index_comma, delim_ptr + 1, diam_len);
     } else {
-        memcpy(IDENTIFIER.name_str, TRIMMED_STRING.str, TRIMMED_STRING.length);
+        set_identifier(TRIMMED_STRING.str, (uint8_t)TRIMMED_STRING.length, "N/A", 3);
-        IDENTIFIER.name_len = TRIMMED_STRING.length;
-        memcpy(IDENTIFIER.diameter_str, "N/A", 3);
-        IDENTIFIER.diameter_len = 3;
     }
 
-    IDENTIFIER.hashed = hash(IDENTIFIER.name_str, 0, 59);
-
     return IDENTIFIER;
 }
 
 trimmed_string_struct remove_spaces(char* str, uint8_t len_str) {
-    uint8_t i = 0, j = 0;
+    const uint8_t max_len = sizeof(TRIMMED_STRING.str) - 1;
-    memset(TRIMMED_STRING.str, ' ', 20);
+    uint8_t j = 0;
-    while (str[i]) {
+
-        if (str[i] != ' ') {
+    memset(TRIMMED_STRING.str, 0, sizeof(TRIMMED_STRING.str));
+    for (uint8_t i = 0; (i < len_str) && str[i]; i++) {
+        if ((str[i] != ' ') && (j < max_len)) {
             TRIMMED_STRING.str[j++] = str[i];
         }
-        i++;
-        if (i >= len_str) {
-            break;
-        }
     }
 
+    // Callers run strchr() over this, so it has to be terminated.
+    TRIMMED_STRING.str[j] = '\0';
     TRIMMED_STRING.length = j;
     return TRIMMED_STRING;
 }
@@ -51,25 +78,26 @@ void rfid_set_low_power_down(bool state) {
     }
 }
 
-ndef_message rfid_read_first_ndef_entry() {
+ndef_message rfid_read_first_ndef_entry(void) {
     rfid_set_low_power_down(false);
     rfid_set_i2c_power(true);
     _delay_ms(1);
 
-    memset(DATA_BUFFER_65, ' ', 64);
+    // static: a 65-byte frame here sat on top of an already deep call chain and
+    // was a large part of the stack overrun.
+    static unsigned char DATA_BUFFER_INTERNAL[NDEF_READ_LEN];
+    memset(DATA_BUFFER_INTERNAL, 0, sizeof(DATA_BUFFER_INTERNAL));
+    rfid_read_memory(DATA_BUFFER_INTERNAL, NDEF_READ_LEN, 0x0000 + 4);
 
-    char DATA_BUFFER_INTERNAL[65];
+    NDEF_MSG = readNDEFText(DATA_BUFFER_INTERNAL, NDEF_READ_LEN);
-    rfid_read_memory(DATA_BUFFER_INTERNAL, 64, 0x0000 + 4);
 
-    NDEF_MSG = readNDEFText(DATA_BUFFER_INTERNAL);
-    
 
     rfid_set_low_power_down(true);
     rfid_set_i2c_power(false);
     return NDEF_MSG;
 }
 
-uint8_t rfid_read_system_register() {
+uint8_t rfid_read_system_register(void) {
     return read_one_byte_16bit_addr_no_err_register(I2C_SYSTEM_ADDR, 0x0000);
 }
 

avr_code/st25dv.h

@@ -13,12 +13,15 @@
 #define I2C_SYSTEM_ADDR 0x57
 #define I2C_USER_ADDR 0x53
 
-identifier_results get_nugget_data();
+// Bytes of tag memory pulled in one go to look for the first NDEF record.
-ndef_message rfid_read_first_ndef_entry();
+#define NDEF_READ_LEN 64
+
+identifier_results get_nugget_data(void);
+ndef_message rfid_read_first_ndef_entry(void);
 void rfid_set_low_power_down(bool state);
 void rfid_set_i2c_power(bool state);
 uint8_t rfid_read_memory(uint8_t* data, uint8_t num_bytes, uint16_t address);
-uint8_t rfid_read_system_register();
+uint8_t rfid_read_system_register(void);
 
 trimmed_string_struct remove_spaces(char* str, uint8_t len_str);
 

avr_code/states.c

@@ -1,11 +1,17 @@
 #include "states.h"
 
-    void init_spi() {
+    void init_spi(void) {
-        SET_PIN_OUT(DDRC, DDC1);  // SCK
+        SET_PIN_OUT(DDRC, DDC1);  // SCK1
-        SET_PIN_OUT(DDRE, DDE3);  // MOSI
+        SET_PIN_OUT(DDRE, DDE3);  // MOSI1
-        SET_PIN_IN(DDRC, DDC0); // MISO_RFM69
+        SET_PIN_IN(DDRC, DDC0);   // MISO1 (driven by the slave; no pull-up)
-        SET_PIN_HIGH(PORTC, PC0);
+
-        SPCR1= (1<<SPE1) | (1<<MSTR1);  // Enable, Master, f_osc/16
+        // SS1 must be an output before SPE is set. If it is left as an input and
+        // reads low, the hardware clears MSTR and the port silently stops being
+        // a master.
+        SET_PIN_OUT(DDRE, DDE2);
+        SET_PIN_HIGH(PORTE, PE2);
+
+        SPCR1 = (1 << SPE1) | (1 << MSTR1);  // Enable, Master, SPR1:0 = 00 -> f_osc/4
     }
     
     

avr_code/states.h

@@ -24,7 +24,7 @@
 extern "C" {
 #endif
 
-    void init_spi();
+    void init_spi(void);
         void rfm69_reset_state(bool state) ;
     void led_1_set_state(bool state);
 

avr_code/uart.h

@@ -6,7 +6,6 @@
  */
 
 #include "defines.h"
-#define UBRR_BAUD F_CPU / 16 / BAUD - 1
 
 #include <avr/io.h>
 #include <stdbool.h>
@@ -22,7 +21,9 @@
 extern "C" {
 #endif
 
-void uart_init();
+#define UBRR_BAUD ((F_CPU) / 16 / (BAUD) - 1)
+
+void uart_init(void);
 void uart_sendChar(char c);
 void uart_sendString(const char* str);
 void uart_sendStringArray(unsigned char str[], uint8_t len);
@@ -34,7 +35,7 @@ void uart_print_float(float meas, const char* buf);
 void uart_print_binary(unsigned char vin, const char* buf);
 void uart_print_uint8(uint8_t vin, const char* buf);
 void uart_print_uint8_array(uint8_t* array, size_t length, const char* buf);
-void uart_wait_until_sent();
+void uart_wait_until_sent(void);
 
 #ifdef __cplusplus
 }