driver-review

name: driver-review description: Review and implement hardware driver code — DMA safety, interrupt correctness, timing constraints, peripheral register usage, channel drivers, and peripheral mock implementations. Use when writing, modifying, or reviewing LED drivers, SPI/I2S/RMT/UART/PARLIO/LCD_CAM peripherals, GPIO configuration, or peripheral mock code. disable-model-invocation: true

Hardware Driver Review & Implementation Guide

Review and implement hardware driver code changes for embedded-specific safety and correctness issues.

Your Task

Run git diff --cached and git diff to see all changes
Identify files that are hardware driver code (see "What Counts as Driver Code" below)
For reviews: Check ALL driver code changes against the Review Rules below
For implementations: Follow the Implementation Guide below
Fix straightforward violations directly
Report summary of all findings

What Counts as Driver Code

Files matching these patterns:

src/platforms/** — Platform-specific implementations
src/fl/channels/** — LED channel engine and DMA pipeline
**/drivers/** — Hardware driver implementations
Files containing: DMA buffers, SPI/I2S/RMT/UART/PARLIO peripheral access, GPIO configuration, interrupt handlers, timer configuration

Part 1: Review Rules

1. DMA Safety

DMA buffers allocated with MALLOC_CAP_INTERNAL | MALLOC_CAP_DMA
DMA buffers are 4-byte aligned (use __attribute__((aligned(4))) or aligned allocator)
No stack-allocated DMA buffers (must be heap or static)
DMA descriptors in internal SRAM (not PSRAM/SPIRAM)
Cache coherence handled: esp_cache_msync() or non-cacheable memory for DMA
DMA transfer size within hardware limits
Buffer lifetime extends beyond DMA completion (no use-after-free)

2. Interrupt Safety

ISR functions marked with IRAM_ATTR (ESP32) or proper section attributes
No heap allocation (malloc, new, fl::vector) inside ISRs
No mutex/semaphore take with blocking timeout in ISRs (use portMAX_DELAY = 0 only)
No printf, FL_DBG, FL_WARN, or logging in ISRs
No flash access from ISRs (all ISR code and data in IRAM/DRAM)
ISR-safe queue operations only (xQueueSendFromISR, not xQueueSend)
Critical sections use proper primitives (portENTER_CRITICAL_ISR not portENTER_CRITICAL)
ISR handlers return correct value (true if higher-priority task woken)

3. Peripheral Register Access

Registers accessed through volatile pointers or HAL functions
No read-modify-write races on shared registers (use atomic or critical section)
Peripheral clock enabled before register access
Peripheral properly initialized before use and cleaned up on teardown
GPIO matrix/IOMUX configured correctly for peripheral signals

4. Timing Constraints

SPI/I2S/RMT clock calculations match LED protocol requirements
Reset timing meets protocol minimums (WS2812: >280us, SK6812: >80us)
No blocking waits in time-critical paths
Watchdog fed in long-running operations
vTaskDelay(1) or yield() in busy loops to prevent watchdog reset

5. Memory Safety

Buffer sizes checked before DMA transfer setup
No buffer overflows in encoding functions (bounds checking on output buffer)
Encoding output size calculated correctly (e.g., wave8: 8 SPI bits per LED bit)
Chunk sizes aligned to hardware requirements (SPI: 4-byte aligned)

6. Channel Engine Patterns (FastLED-specific)

show() waits for poll() == READY before starting new frame
No branching on intermediate states (DRAINING, STREAMING) in wait loops
Channel released after transmission complete (frees peripheral for next channel)
State machine handles all transitions (no stuck states)
Error recovery path exists (timeout, reset to IDLE)

7. Peripheral Mock Rules (CRITICAL)

NO background threads — mock must be fully synchronous
NO wall-clock timing (fl::micros(), sleep_for) — use mSimulatedTimeUs
NO mutex/condition_variable — single-threaded, no synchronization needed
Synchronous callback pump via pumpDeferredCallbacks() with re-entrancy guard
waitDone() returns instantly — never polls or sleeps
reset() clears ALL state — called between test cases for isolation
Transmitted data captured in history vector for test inspection
Singleton via fl::Singleton<Impl>

8. Power and Reset

Brown-out detection configured if needed
Peripheral reset on initialization (clean state)
GPIO pins set to safe state on driver teardown
Power domains managed correctly (light sleep compatibility)

9. Multi-Platform Considerations

Platform guards (#ifdef ESP32, #ifdef FL_IS_ARM) correct and complete
No platform-specific types leaking into shared headers
Fallback/no-op implementations for unsupported platforms
Integer types match platform expectations (see src/platforms/*/int.h)

Part 2: Implementation Guide

Architecture

FastLED's driver stack has three layers:

IChannelDriver              (driver.h — show/poll state machine)
  └─ ChannelEngine*         (groups channels by timing, iterates chipset groups)
      └─ IPeripheral        (virtual interface — real HW or mock)
          ├─ PeripheralEsp  (real ESP-IDF calls)
          └─ PeripheralMock (synchronous test simulation)

File Structure for New Peripheral `foo`

src/platforms/esp/32/drivers/foo/
  ├─ ifoo_peripheral.h              # Virtual interface (no ESP-IDF types)
  ├─ foo_peripheral_esp.h           # Real hardware implementation
  ├─ foo_peripheral_mock.h          # Mock class declaration
  ├─ foo_peripheral_mock.cpp.hpp    # Mock implementation (synchronous)
  └─ channel_driver_foo.cpp.hpp     # Channel driver using IFooPeripheral

tests/platforms/esp/32/drivers/foo/
  ├─ foo_peripheral_mock.cpp        # Mock peripheral unit tests
  └─ channel_driver_foo.cpp         # Driver integration tests

Reference implementations:

I2S: src/platforms/esp/32/drivers/i2s/
LCD_CAM: src/platforms/esp/32/drivers/lcd_cam/
PARLIO: src/platforms/esp/32/drivers/parlio/

Peripheral Interface

Define the virtual interface in ifoo_peripheral.h:

No ESP-IDF types — use void*, u16*, basic types only
All methods FL_NOEXCEPT override
Buffer management with 64-byte alignment (DMA requirement)
Time simulation: getMicroseconds(), delay(ms)
Callback registration: registerCallback(void* fn, void* ctx)

Peripheral Mock Implementation

Required Members

// Lifecycle
bool mInitialized, mEnabled, mBusy;
size_t mTransmitCount;
FooConfig mConfig;

// ISR callback
void* mCallback;
void* mUserCtx;

// Simulation settings
u32 mTransmitDelayUs;
bool mTransmitDelayForced;
bool mShouldFailTransmit;

// Test inspection
fl::vector<TransmitRecord> mHistory;

// Pending state
size_t mPendingTransmits;

// Simulated time (deterministic — advances only via delay() calls)
u64 mSimulatedTimeUs;

// Synchronous callback pump
bool mFiringCallbacks;            // Re-entrancy guard
size_t mDeferredCallbackCount;    // Pending callbacks to fire

Core Pattern: transmit() → pump → fireCallback()

transmit() — Queue + pump:

bool transmit(const u16* buffer, size_t size_bytes) {
    if (!mInitialized || mShouldFailTransmit) return false;

    // Capture data for test inspection
    TransmitRecord record;
    record.buffer_copy.resize(size_bytes / 2);
    fl::memcpy(record.buffer_copy.data(), buffer, size_bytes);
    record.size_bytes = size_bytes;
    record.timestamp_us = mSimulatedTimeUs;
    mHistory.push_back(fl::move(record));

    // Queue + fire synchronously
    mTransmitCount++;
    mBusy = true;
    mPendingTransmits++;
    mDeferredCallbackCount++;
    pumpDeferredCallbacks();
    return true;
}

waitDone() — Instant check, never polls:

bool waitDone(u32 timeout_ms) {
    if (!mInitialized) return false;
    (void)timeout_ms;  // Not used — synchronous mock
    if (mPendingTransmits == 0) { mBusy = false; return true; }
    return false;
}

pumpDeferredCallbacks() — Re-entrant safe:

void pumpDeferredCallbacks() {
    if (mFiringCallbacks) return;  // Re-entrancy guard
    mFiringCallbacks = true;
    while (mDeferredCallbackCount > 0) {
        mDeferredCallbackCount--;
        fireCallback();
    }
    mFiringCallbacks = false;
}

fireCallback() — One callback at a time:

void fireCallback() {
    if (mPendingTransmits > 0) mPendingTransmits--;
    if (mPendingTransmits == 0) mBusy = false;
    if (mCallback != nullptr) {
        using CallbackType = bool (*)(void*, const void*, void*);
        auto fn = reinterpret_cast<CallbackType>(mCallback);
        fn(nullptr, nullptr, mUserCtx);
    }
}

Time simulation:

u64 getMicroseconds() { return mSimulatedTimeUs; }
void delay(u32 ms) { mSimulatedTimeUs += static_cast<u64>(ms) * 1000; }

reset() — Full state reset:

void reset() {
    mInitialized = mEnabled = mBusy = false;
    mTransmitCount = 0;
    mConfig = FooConfig();
    mCallback = nullptr; mUserCtx = nullptr;
    mTransmitDelayUs = 0; mTransmitDelayForced = false; mShouldFailTransmit = false;
    mHistory.clear(); mPendingTransmits = 0;
    mSimulatedTimeUs = 0; mFiringCallbacks = false; mDeferredCallbackCount = 0;
}

Mock-Specific Test API (required)

void simulateTransmitComplete();           // Manually complete one pending transmit
void setTransmitFailure(bool should_fail); // Force transmit() to return false
void setTransmitDelay(u32 microseconds);   // Set forced delay
const fl::vector<TransmitRecord>& getTransmitHistory() const;
fl::span<const u16> getLastTransmitData() const;
size_t getTransmitCount() const;
bool isEnabled() const;
void clearTransmitHistory();
void reset();

Channel Driver

Implement IChannelDriver. State machine:

READY → (enqueue) → READY → (show) → BUSY → (poll) → DRAINING → (poll) → READY

Constructor pattern:

ChannelDriverFoo();                                          // Production
ChannelDriverFoo(fl::shared_ptr<IFooPeripheral> peripheral); // Testing

Chipset grouping: Channels with different timing (T0H, T1H, T0L, T1L) must be transmitted in separate groups. Sort by transmission time.

Tests

namespace {
void resetFooMockState() {
    auto& mock = FooPeripheralMock::instance();
    mock.reset();  // CRITICAL: reset between every test
}
}

FL_TEST_CASE("FooPeripheralMock - basic transmit") {
    resetFooMockState();
    auto& mock = FooPeripheralMock::instance();
    FooConfig config;
    config.num_lanes = 4;
    config.pclk_hz = 3200000;
    FL_REQUIRE(mock.initialize(config));
    u16* buffer = mock.allocateBuffer(1024);
    FL_REQUIRE(buffer != nullptr);
    FL_CHECK(mock.transmit(buffer, 1024));
    FL_CHECK(mock.waitDone(100));
    FL_CHECK(mock.getTransmitCount() == 1);
    mock.freeBuffer(buffer);
}

Driver Registration Priority

In channel_manager_esp32.cpp.hpp:

PARLIO: 4 (highest)
LCD_RGB: 3
RMT: 2
I2S: 1
SPI: 0
UART: -1

Buffer Alignment (64-byte for DMA)

u16* allocateBuffer(size_t size_bytes) {
    size_t aligned = ((size_bytes + 63) / 64) * 64;
#ifdef FL_IS_WIN
    return static_cast<u16*>(_aligned_malloc(aligned, 64));
#else
    return static_cast<u16*>(aligned_alloc(64, aligned));
#endif
}
void freeBuffer(u16* buffer) {
    if (!buffer) return;
#ifdef FL_IS_WIN
    _aligned_free(buffer);
#else
    fl::free(buffer);
#endif
}

Output Format (for reviews)

## Hardware Driver Review Results

### File-by-file Analysis
- **src/platforms/esp/32/drivers/spi/channel_engine_spi.cpp.hpp**: [findings]

### Findings by Category
- **DMA Safety**: N issues
- **Interrupt Safety**: N issues
- **Mock Rules**: N issues
- **Timing Constraints**: N issues

### Summary
- Files reviewed: N
- Violations found: N
- Violations fixed: N

Instructions

Focus on driver/platform code — skip application-level changes
Be thorough on DMA and interrupt safety (these cause hard-to-debug crashes)
Mock violations are P0 — async mocks with threads/wall-clock = flaky tests
Reference agents/docs/cpp-standards.md for general C++ rules
Make corrections directly when safe
Ask for user confirmation on significant changes