driver-review

Hardware Driver Review & Implementation Guide

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

Your Task

1. Run git diff --cached and git diff to see all changes
2. Identify files that are hardware driver code (see "What Counts as Driver Code" below)
3. For reviews: Check ALL driver code changes against the Review Rules below
4. For implementations: Follow the Implementation Guide below
5. Fix straightforward violations directly
6. 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