esp32-arch-review

Perform a structured architecture review of ESP32 firmware code, with emphasis on FastLED driver patterns, DMA safety, RTOS correctness, and LED-specific constraints.

$ARGUMENTS

What This Skill Reviews

1. FastLED Driver Architecture

Review LED driver implementations under src/platforms/esp/:

• Driver inherits from correct base class (ClocklessController, ClockedController, or equivalent)
• show() method is interrupt-safe and atomic from the LED strip's perspective
• Pixel data encoding is performed before DMA transfer starts (no on-the-fly encoding race)
• Color order (RGB/GRB/BGR) is handled at compile time, not runtime switch
• Gamma correction and color scaling applied before encoding, not after DMA
• leds_data / CRGB array not modified during active DMA transfer
• FastLED's show_ordinal() / beginShow() / endShow() lifecycle respected
• Brightness scaling via FastLED.setBrightness() applied before encoding

2. DMA Safety

• DMA buffers allocated with heap_caps_malloc(size, MALLOC_CAP_INTERNAL | MALLOC_CAP_DMA)
• DMA buffers are 4-byte aligned (use __attribute__((aligned(4))) or heap_caps_aligned_alloc)
• DMA buffer lifetime extends beyond transfer completion (no premature free)
• DMA descriptors reside in internal SRAM (not PSRAM/SPIRAM)
• Cache coherence handled: esp_cache_msync() called or memory marked non-cacheable for DMA regions
• Double-buffering used where needed to allow CPU-side encoding to overlap with DMA transmission
• DMA completion callback or semaphore used to signal transfer end (not busy-wait polling)
• Buffer sizes within hardware limits (RMT: 64 items/channel, I2S/SPI: configurable block size)

3. FreeRTOS Safety

• No blocking calls (vTaskDelay, xSemaphoreTake with timeout) inside ISRs
• ISR functions use FromISR API variants only (xSemaphoreGiveFromISR, xQueueSendFromISR)
• ISR handlers marked IRAM_ATTR (ESP32) or __attribute__((section(".iram1"))) (other)
• No malloc, new, fl::vector allocation inside ISRs
• No printf, FL_DBG, ESP_LOGI or logging inside ISRs
• No flash access from ISRs (all ISR code and data must be in IRAM/DRAM)
• Shared state between ISR and task protected with critical section or ISR-safe primitives
• Task stack sizes adequate for FastLED usage (show() + encoding can be stack-heavy)
• No unbounded spin-wait loops without taskYIELD() (causes watchdog reset)
• Watchdog not starved during long show() calls with high LED counts

4. RMT-Specific Checks (WS2812/SK6812/APA106)

• RMT channel count within hardware limits (ESP32: 8, S2: 4, S3/C3/C6/H2: 4)
• RMT resolution set correctly for target LED protocol (typically 10ns for WS2812)
• RMT item encoding matches LED chipset timing spec (T0H, T0L, T1H, T1L, reset)
• RMT encoder (rmt_encoder_t) properly initialized and not shared between channels
• RMT callback or event queue used for completion, not polling
• RMT loop mode disabled (LED output is one-shot, not continuous)
• RMT end-of-frame reset pulse duration meets minimum spec (≥50µs for WS2812)

5. I2S/SPI-Specific Checks (APA102/SK9822/HD107)

• SPI clock rate set correctly for target LED chipset (APA102 max: 20MHz typical)
• SPI word size matches encoding (32-bit for APA102, 8-bit for raw SPI)
• I2S sample rate matches LED bit rate requirement
• SPI/I2S DMA buffer count adequate (≥2 for double buffering)
• SPI/I2S CS/CLK/DATA pins not conflicting with flash (GPIO 6-11 forbidden on ESP32)
• SPI transaction callback used to notify completion (not blocking transmit)

6. PARLIO-Specific Checks (ESP32-S3 Parallel Output)

• PARLIO clock source and divider correctly configured for target bit rate
• Data bus width matches LED strip count (1, 2, 4, 8, or 16 parallel strips)
• PARLIO DMA buffer aligned to cache line boundaries
• Output enable pin wired or bypassed correctly

7. Memory Management

• LED CRGB buffer allocation checked for NULL (large strips can exhaust heap)
• No large stack-allocated arrays in show() path (> 512 bytes on stack is risky)
• PSRAM usage documented when LED buffers are placed in SPIRAM
• Free heap monitored at startup and logged (alert if < 10KB before show())
• No memory leaks in driver initialization error paths
• String/buffer operations have bounds checking

8. Peripheral Pin Safety

• GPIO 6-11 not used for any output (ESP32 internal flash pins)
• GPIO 34-39 not used for output (input-only on original ESP32)
• Strapping pins (0, 2, 12, 15) not used without awareness of boot implications
• ADC2 channels not used alongside Wi-Fi (GPIO 0, 2, 4, 12-15, 25-27)
• Pin assignments match board schematic or documented LED strip wiring

9. Error Handling

• All ESP-IDF API return values checked (esp_err_t, esp_err check macros)
• Driver initialization failures reported with meaningful error messages
• Graceful degradation: driver failure doesn't crash the whole application
• Timeout values defined for all blocking operations
• Error paths clean up allocated resources (channels, DMA buffers, callbacks)

10. Code Quality & Maintainability

• Platform guards (#ifdef FASTLED_ESP32, #ifdef CONFIG_IDF_TARGET_ESP32S3, etc.) correct
• No magic numbers — timing constants named and commented with source (datasheet)
• Driver variant selection at compile time, not runtime (no dynamic dispatch overhead)
• Header includes minimal (no platform headers leaked into public FastLED API)
• Porting guide updated if new platform port added (see PORTING.md)

Output Format

# ESP32 Architecture Review Report

## Scope
- **Files reviewed**: [list]
- **Driver type**: [RMT / I2S / SPI / PARLIO / Other]
- **Target chip**: [ESP32 / ESP32-S2 / ESP32-S3 / ESP32-C3 / ESP32-C6 / ESP32-H2]

## Overall Assessment
[PASS / PASS WITH NOTES / FAIL]

## Critical Issues (must fix before merge)
- [ ] [Issue description + file:line reference]

## Warnings (should fix)
- [ ] [Issue description]

## Recommendations (consider for future)
- [ ] [Suggestion]

## Checklist Results
[Filled checklist from sections above — mark each PASS / FAIL / N/A]

Common FastLED ESP32 Architecture Anti-Patterns

Anti-Pattern	Problem	Fix
show() in ISR	Blocks ISR with long DMA wait	Move to task, use semaphore from ISR
Stack-allocated encode buffer	Stack overflow on high LED counts	Use heap or static allocation
Polling DMA completion	Wastes CPU, may block IDLE	Use DMA completion callback/semaphore
Shared RMT channel for multiple strips	Data corruption	One channel per strip
malloc() without MALLOC_CAP_DMA	DMA transfer fails silently	Always use heap_caps_malloc for DMA buffers
Timing constants without datasheet reference	Hard to audit or adapt	Add comments citing datasheet section
Runtime color order switch	Branch overhead per LED	Use compile-time template parameter