MCU Embedded Firmware Code Review Skill

This skill provides comprehensive code review for embedded Rust firmware targeting RP2350 and ESP32-S3 microcontrollers used in the BVR rover system.

Overview

The BVR uses two MCU platforms for peripheral control:

1. RP2350 (Pico 2 W): LED controllers, USB peripherals
2. ESP32-S3 (Heltec): Tool attachments, LoRa communication, OLED display

Both run Rust in no_std environments with hardware-specific runtimes and memory constraints.

Architecture:

mcu/
├── bins/
│   ├── rp2350/         # Pico 2 W binaries (LED controller, etc.)
│   │   └── src/main.rs
│   └── esp32s3/        # Heltec binaries (attachments, SLCAN)
│       └── src/main.rs
├── crates/
│   ├── mcu-core/       # Shared protocol and types
│   ├── mcu-leds/       # LED controller library
│   └── ...

Key Differences:

Feature	RP2350 (Pico 2 W)	ESP32-S3 (Heltec)
Runtime	Embassy async (#[embassy_executor::main])	Polling loop (#[main])
Memory	520 KB SRAM, static allocation	512 KB SRAM, heap allocation
Edition	Rust 2021	Rust 2024
LED Control	PIO state machine (WS2812)	RMT peripheral (WS2811/WS2812)
USB	USB CDC serial (device mode)	USB serial (via UART bridge)
Display	None	OLED SSD1306 (I2C)
Wireless	WiFi (Pico W chip)	WiFi, BLE, LoRa
CAN	External MCP2515 (SPI)	Built-in TWAI (CAN 2.0)

RP2350 (Pico 2 W) Patterns

Embassy Async Runtime

Location: mcu/bins/rp2350/src/main.rs

Key Points to Review:

• #[embassy_executor::main] on main function
• Async main signature: async fn main(spawner: Spawner)
• Tasks spawned with spawner.spawn(task_name()).unwrap()
• All peripherals initialized before spawning tasks
• Interrupt bindings with bind_interrupts! macro

Example Pattern:

// Good: Embassy async main
#[embassy_executor::main]
async fn main(spawner: Spawner) {
    // 1. Initialize peripherals
    let p = embassy_rp::init(Default::default());

    // 2. Bind interrupts
    bind_interrupts!(struct Irqs {
        USBCTRL_IRQ => InterruptHandler<USB>;
    });

    // 3. Create drivers
    let driver = Driver::new(p.USB, Irqs);

    // 4. Spawn tasks
    spawner.spawn(usb_task(driver)).unwrap();
    spawner.spawn(led_controller_task(p.PIO0)).unwrap();

    // 5. Main loop (or empty if all work in tasks)
    loop {
        Timer::after_millis(1000).await;
    }
}

Red Flags:

• Blocking calls in async context (std::thread::sleep)
• Missing .await on async operations
• Unwrapping spawner (should handle spawn errors)
• Peripherals not moved into tasks (ownership violation)

Embassy Tasks

Key Points to Review:

• Task functions marked with #[embassy_executor::task]
• Async signature
• Infinite loop or explicit return
• Error handling (log errors, don't panic)

Example Pattern:

// Good: Embassy task
#[embassy_executor::task]
async fn led_controller_task(mut pio: PIO0) {
    loop {
        // Wait for command
        let cmd = LED_COMMAND_CHANNEL.receive().await;

        // Update LEDs
        match update_leds(&mut pio, cmd) {
            Ok(_) => {}
            Err(e) => error!("LED update failed: {:?}", e),
        }
    }
}

Static Memory Allocation

Purpose: No heap allocator in RP2350, all memory must be static.

Key Points to Review:

• StaticCell used for static mut data
• Global statics wrapped in static with StaticCell
• No Box, Vec, String (heap types)
• Arrays with fixed sizes

Example Pattern:

use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex;
use embassy_sync::channel::{Channel, Sender, Receiver};
use static_cell::StaticCell;

// Good: Static channel for inter-task communication
static LED_CHANNEL: StaticCell<Channel<CriticalSectionRawMutex, LedCommand, 8>> = StaticCell::new();

fn init_channel() -> &'static Channel<CriticalSectionRawMutex, LedCommand, 8> {
    LED_CHANNEL.init(Channel::new())
}

// Good: Static USB buffers
static EP_MEMORY: StaticCell<[u8; 1024]> = StaticCell::new();
static CONFIG_DESC: StaticCell<[u8; 256]> = StaticCell::new();

// Bad: Heap allocation (won't compile in no_std without alloc)
// let vec = vec![1, 2, 3];  // ❌ No Vec
// let string = String::from("hello");  // ❌ No String

Red Flags:

• Heap types (Vec, String, Box)
• Missing StaticCell wrapper for static mut
• Unbounded buffers (use fixed-size arrays)

See: rp2350-patterns.md for detailed patterns.

PIO for WS2812 LEDs

Purpose: Programmable I/O state machine generates precise timing for WS2812 protocol.

Key Points to Review:

• PIO program loaded with pio.load_program()
• State machine configured with correct frequency
• DMA used for efficient data transfer
• Color data in GRB format (not RGB)

WS2812 Protocol:

• Bit 0: 400ns high, 850ns low
• Bit 1: 800ns high, 450ns low
• Reset: >50µs low

Example Pattern:

// Good: PIO-based WS2812 driver
use embassy_rp::pio::{Pio, Common, StateMachine};

async fn init_ws2812(pio: PIO0, pin: PIN_25) -> Ws2812<'static> {
    let Pio { mut common, sm0, .. } = Pio::new(pio);

    // Load PIO program (bit-bangs WS2812 protocol)
    let prg = pio_proc::pio_asm!(
        ".side_set 1 opt",
        ".wrap_target",
        "bitloop:",
        "  out x, 1       side 0 [1]",  // Shift out 1 bit
        "  jmp !x do_zero side 1 [2]",  // High if bit 1
        "do_one:",
        "  jmp bitloop    side 1 [4]",  // Stay high (800ns)
        "do_zero:",
        "  nop            side 0 [4]",  // Go low (400ns)
        ".wrap"
    );

    let program = common.load_program(&prg.program);

    // Configure state machine
    let mut cfg = Config::default();
    cfg.set_out_pins(&[&pin]);
    cfg.set_clock_divider(125);  // 800kHz for WS2812 timing

    let ws2812 = Ws2812::new(sm0, pin, program, cfg);
    ws2812
}

// Send color data (GRB format!)
async fn set_led_color(ws2812: &mut Ws2812<'_>, index: usize, r: u8, g: u8, b: u8) {
    let color = ((g as u32) << 16) | ((r as u32) << 8) | (b as u32);  // GRB
    ws2812.write(&[color]).await;
}

Red Flags:

• RGB byte order (should be GRB for WS2812)
• Incorrect clock divider (breaks timing)
• Blocking write (should use DMA + await)
• Missing reset delay (>50µs)

USB CDC Serial

Purpose: USB serial for debugging and SLCAN bridge.

Key Points to Review:

• USB driver created with Driver::new()
• CDC-ACM class instantiated
• Read/write methods awaited
• Line coding handled (baud rate, etc.)

Example Pattern:

use embassy_usb::class::cdc_acm::{CdcAcmClass, State};

#[embassy_executor::task]
async fn usb_task(driver: Driver<'static, USB>) {
    let mut config = embassy_usb::Config::new(0x1234, 0x5678);
    config.manufacturer = Some("Muni Robotics");
    config.product = Some("BVR LED Controller");

    let mut builder = Builder::new(driver, config, /* ... */);

    // Create CDC-ACM class
    let mut state = State::new();
    let mut cdc = CdcAcmClass::new(&mut builder, &mut state, 64);

    let usb = builder.build();

    loop {
        // Read from USB
        let mut buf = [0u8; 64];
        let n = cdc.read_packet(&mut buf).await.unwrap();

        // Echo back (for testing)
        cdc.write_packet(&buf[..n]).await.unwrap();
    }
}

ESP32-S3 (Heltec) Patterns

Polling Loop (No Async)

Location: mcu/bins/esp32s3/src/main.rs

Key Points to Review:

• #[main] entry point (not #[embassy_executor::main])
• Infinite loop with non-blocking checks
• delay.delay_millis() for timing
• UART read_ready() for non-blocking reads

Example Pattern:

// Good: Polling loop
#[main]
fn main() -> ! {
    // 1. Initialize peripherals
    let peripherals = Peripherals::take();
    let mut delay = Delay::new();

    // 2. Initialize UART
    let mut uart = Uart::new(peripherals.UART0, /* ... */);

    // 3. Main loop
    loop {
        // Non-blocking UART read
        if uart.read_ready() {
            let mut buf = [0u8; 64];
            if let Ok(n) = uart.read(&mut buf) {
                process_command(&buf[..n]);
            }
        }

        // Update LEDs
        update_leds();

        // Small delay (don't busy-wait)
        delay.delay_millis(1);
    }
}

Red Flags:

• Blocking reads (uart.read() without read_ready())
• Busy-wait loop (no delay)
• Async/await syntax (ESP32 uses blocking)

See: esp32s3-patterns.md for detailed patterns.

Heap Allocation (esp-alloc)

Purpose: ESP32-S3 has a heap allocator, allows Vec, String, etc.

Key Points to Review:

• esp_alloc::heap_allocator!() called in main
• Heap types used sparingly (still embedded)
• Fixed-size buffers preferred for critical paths
• No unbounded growth (memory leaks)

Example Pattern:

use esp_alloc as _;

#[main]
fn main() -> ! {
    // Initialize heap allocator
    esp_alloc::heap_allocator!();

    // Now can use heap types (use sparingly!)
    let mut buffer = Vec::with_capacity(256);  // Pre-allocate

    loop {
        // Heap allocation okay for non-critical paths
        let message = String::from("Status: OK");
        send_message(&message);

        buffer.clear();  // Reuse buffer (don't re-allocate)
    }
}

Best Practice: Use heap for initialization and infrequent operations, not in hot loops.

RMT for WS2811/WS2812 LEDs

Purpose: Remote Control Transceiver generates precise pulse sequences.

Key Points to Review:

• RMT channel configured with correct clock
• Pulse sequences match WS2812 timing
• Blocking transmit or interrupt-driven
• Memory limits (8 RMT channels, 64 items each)

Example Pattern:

use esp_hal::rmt::{Rmt, TxChannel};

fn init_rmt_leds(rmt: RMT, pin: GPIO48) -> TxChannel<'static, 0> {
    let rmt = Rmt::new(rmt, 80u32.MHz()).unwrap();  // 80MHz clock

    let tx_config = TxChannelConfig {
        clk_divider: 1,
        idle_output_level: false,
        carrier_modulation: false,
        idle_output: true,
    };

    let mut channel = rmt.channel0.configure(pin, tx_config).unwrap();

    // WS2812 pulse timings (in RMT ticks)
    // Bit 0: 400ns high, 850ns low
    // Bit 1: 800ns high, 450ns low
    // Each tick = 12.5ns at 80MHz

    channel
}

fn send_pixel(channel: &mut TxChannel, r: u8, g: u8, b: u8) {
    let grb = ((g as u32) << 16) | ((r as u32) << 8) | (b as u32);  // GRB order

    for bit in (0..24).rev() {
        let is_one = (grb >> bit) & 1 == 1;
        let (high, low) = if is_one {
            (64, 32)  // 800ns, 400ns at 80MHz
        } else {
            (32, 64)  // 400ns, 800ns
        };

        channel.transmit(&[(high, true, low, false)]).unwrap();
    }
}

Red Flags:

• RGB byte order (should be GRB)
• Incorrect timing calculations
• RMT buffer overflow (>64 items)

OLED Display (SSD1306)

Purpose: Status display for standalone operation.

Key Points to Review:

• I2C initialized with correct frequency
• SSD1306 driver initialized
• Display cleared on startup
• Updates not too frequent (I2C is slow)

Example Pattern:

use ssd1306::{Ssd1306, mode::BufferedGraphicsMode, prelude::*, I2CDisplayInterface};

fn init_display(i2c: I2C0, sda: GPIO17, scl: GPIO18) -> Ssd1306<I2CInterface<I2C0>, ...> {
    let i2c = I2c::new(i2c, sda, scl, 400u32.kHz()).unwrap();
    let interface = I2CDisplayInterface::new(i2c);

    let mut display = Ssd1306::new(interface, DisplaySize128x64, DisplayRotation::Rotate0)
        .into_buffered_graphics_mode();

    display.init().unwrap();
    display.clear();
    display.flush().unwrap();

    display
}

fn update_status(display: &mut Ssd1306<...>, status: &str) {
    display.clear();
    Text::new(status, Point::new(0, 0), MonoTextStyle::new(&FONT_6X10, BinaryColor::On))
        .draw(display)
        .unwrap();
    display.flush().unwrap();
}

LED Controller Library

Location: mcu/crates/mcu-leds/src/lib.rs

LED Mode State Machine

Purpose: Manage LED patterns for rover state indication.

Key Points to Review:

• LedMode enum with all patterns (Off, Solid, Pulse, Chase, Flash)
• State machine updates mode based on CAN messages
• Smooth transitions with wipe animation
• Convenience methods for rover states (idle(), teleop(), etc.)

LED Modes:

• Off: All LEDs off
• Solid(r, g, b, brightness): Fixed color
• Pulse(r, g, b, period_ms): Breathing effect (sine wave)
• Flash(r, g, b, period_ms): On/off strobe
• Chase(r, g, b, speed): Moving pattern

Rover State Convenience Methods:

impl LedMode {
    pub fn idle() -> Self {
        Self::Solid(0, 0, 255, 200)  // Blue solid
    }

    pub fn teleop() -> Self {
        Self::Pulse(0, 255, 0, 2000)  // Green pulse, 2s period
    }

    pub fn autonomous() -> Self {
        Self::Pulse(0, 255, 255, 1500)  // Cyan pulse, 1.5s period
    }

    pub fn estop() -> Self {
        Self::Flash(255, 0, 0, 200)  // Red flash, 200ms (urgent!)
    }

    pub fn fault() -> Self {
        Self::Flash(255, 165, 0, 500)  // Orange flash, 500ms
    }
}

Key Points to Review:

• E-stop flash is fast (200ms) for maximum visibility
• Pulse periods reasonable (1-3 seconds)
• Brightness clamped to [0, 255]
• Color values clamped to [0, 255]

Animation System

Purpose: Smooth transitions and effects.

Key Points to Review:

• Wipe transition with soft edge (gradient blend)
• Pulse uses sine wave (smooth breathing)
• Chase wraps around LED strip
• Frame rate consistent (30-60 Hz)

Pulse Animation:

// Good: Sine wave pulse
fn pulse_brightness(base: u8, time_ms: u32, period_ms: u32) -> u8 {
    let phase = (time_ms % period_ms) as f32 / period_ms as f32;
    let sine = ((phase * 2.0 * PI).sin() + 1.0) / 2.0;  // 0.0 to 1.0
    let min_brightness = base / 2;
    let max_brightness = base;

    (min_brightness as f32 + (max_brightness - min_brightness) as f32 * sine) as u8
}

CAN Attachment Protocol

Location: mcu/crates/mcu-core/src/protocol.rs

ID Scheme

CAN ID Range: 0x200-0x2FF (16 attachment slots, 0x10 offset each)

Per-Attachment Messages (8 message types per attachment):

• +0x00: Heartbeat (A → H, periodic beacon)
• +0x01: Identify request (H → A, query identity)
• +0x02: Identity response (A → H, device info)
• +0x03: Command (H → A, control attachment)
• +0x04: Acknowledgment (A → H, command ACK)
• +0x05: Sensor data (A → H, periodic readings)
• +0x06: Configuration (H → A, set parameters)
• +0x07: Error report (A → H, fault indication)

Example: Attachment 0 (base 0x200)

• Heartbeat: 0x200
• Command: 0x203
• Sensor: 0x205

Key Points to Review:

• CAN IDs calculated as BASE + SLOT * 0x10 + OFFSET
• Extended CAN IDs used (29-bit)
• Heartbeat sent at 1 Hz
• Commands acknowledged within 100ms

See: Firmware skill can-protocol.md for full protocol.

SLCAN Protocol

Purpose: Serial Line CAN bridge for debugging via USB.

Location: mcu/bins/esp32s3/src/slcan.rs

Frame Format

Standard Frame:

t<ID><LEN><DATA>\r

Extended Frame:

T<ID><LEN><DATA>\r

Example:

T00000B0010A\r       # Extended ID 0x0B00, length 1, data 0x0A
t10348AABBCCDD\r     # Standard ID 0x103, length 4, data 0xAABBCCDD

Commands

Command	Description	Response
O\r	Open CAN bus	\r (OK)
C\r	Close CAN bus	\r
S4\r	Set bitrate to 500kbps	\r
V\r	Get version	V1234\r
N\r	Get serial number	NABCD\r

Key Points to Review:

• Frame parsing validates ID length (3 or 8 hex digits)
• Data length checked (0-8 bytes)
• Invalid commands return error (e.g., \x07)
• Carriage return (\r) terminates every message

Example Pattern:

// Good: SLCAN command parsing
fn parse_slcan(cmd: &[u8]) -> Result<SlcanCommand, SlcanError> {
    if cmd.is_empty() || cmd[cmd.len() - 1] != b'\r' {
        return Err(SlcanError::MissingCR);
    }

    match cmd[0] {
        b'O' => Ok(SlcanCommand::Open),
        b'C' => Ok(SlcanCommand::Close),
        b'S' => {
            // Parse bitrate (S0-S8)
            let rate = cmd[1] - b'0';
            if rate > 8 {
                return Err(SlcanError::InvalidBitrate);
            }
            Ok(SlcanCommand::SetBitrate(rate))
        }
        b't' => parse_standard_frame(&cmd[1..]),
        b'T' => parse_extended_frame(&cmd[1..]),
        _ => Err(SlcanError::UnknownCommand),
    }
}

See: esp32s3-patterns.md for full SLCAN implementation.

Memory Constraints

RP2350 (Pico 2 W)

SRAM: 520 KB total

• Stack: ~16 KB
• Static data: ~50 KB (USB buffers, channels, etc.)
• Available: ~450 KB

Flash: 4 MB

• Program: ~100 KB
• Data: Plenty of space

Key Points to Review:

• Stack size reasonable (<16 KB)
• No large arrays on stack (use static)
• StaticCell used for static mut data
• No unbounded recursion

ESP32-S3 (Heltec)

SRAM: 512 KB total

• Heap: ~400 KB (configurable)
• Stack: ~8 KB per task
• DMA: ~50 KB

Flash: 8 MB

• Program: ~500 KB
• SPIFFS/LittleFS: ~7 MB

Key Points to Review:

• Heap allocations tracked (no leaks)
• Large buffers allocated once (not per loop)
• Stack size per task reasonable

Testing Embedded Code

Unit Tests (no_std)

Key Points to Review:

• #[cfg(test)] module for unit tests
• Tests use #[test] attribute
• Mock hardware with traits
• No std imports in tests

Example Pattern:

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn test_led_mode_pulse() {
        let mode = LedMode::Pulse(255, 0, 0, 1000);
        match mode {
            LedMode::Pulse(r, g, b, period) => {
                assert_eq!(r, 255);
                assert_eq!(period, 1000);
            }
            _ => panic!("Wrong mode"),
        }
    }
}

Integration Tests (on-device)

Pattern: Use defmt logging and probe-rs for debugging.

# Flash and run with logging
cargo run --release
# or
probe-rs run --chip RP2350

Common Mistakes

❌ Async on ESP32-S3

// WRONG: ESP32 doesn't use Embassy
#[embassy_executor::main]
async fn main(spawner: Spawner) {  // ❌ Won't compile
    // ...
}

✅ Correct ESP32-S3 Entry

// CORRECT: Blocking main
#[main]
fn main() -> ! {
    loop {
        // ...
    }
}

❌ Heap Types on RP2350

// WRONG: No heap allocator
let vec = vec![1, 2, 3];  // ❌ Won't compile (no alloc)

✅ Correct RP2350 Storage

// CORRECT: Static array
static DATA: StaticCell<[u8; 16]> = StaticCell::new();

❌ RGB Instead of GRB

// WRONG: WS2812 expects GRB
let color = (r as u32) << 16 | (g as u32) << 8 | b as u32;  // ❌ RGB

✅ Correct GRB Order

// CORRECT: GRB for WS2812
let color = (g as u32) << 16 | (r as u32) << 8 | b as u32;  // ✅ GRB

References and Additional Resources

For more detailed information, see:

• rp2350-patterns.md - Embassy async, StaticCell, PIO usage
• esp32s3-patterns.md - Polling loop, RMT, esp-alloc, SLCAN
• CLAUDE.md - Project-wide conventions
• Firmware skill can-protocol.md - CAN attachment protocol

Quick Review Commands

# Build RP2350
cd mcu
cargo build --release -p rover-leds

# Build ESP32-S3
cd mcu/bins/esp32s3
cargo build --release

# Flash RP2350 (requires picotool)
picotool load target/thumbv8m.main-none-eabihf/release/rover-leds -t elf -f

# Flash ESP32-S3 (requires espflash)
espflash flash --monitor target/xtensa-esp32s3-none-elf/release/heltec-attachment