Sorry, I changed my mind and swapped the core haha#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a major architectural refactoring that swaps the AI provider from OpenRouter to Anthropic Claude, extracts network-related code into modular components, and migrates the web UI from PROGMEM to a LittleFS filesystem. The changes modernize the codebase structure while maintaining the core LED controller functionality.
Key Changes:
- AI provider switched from OpenRouter to Anthropic Claude with direct API integration
- Network code modularized into separate
network/directory (wifi.cpp, ota.cpp, server.cpp) - API handlers extracted into
api/directory with clear separation of concerns - Configuration field names updated:
apiKey→aiApiKey,openRouterModel→aiModel - Web UI moved from embedded PROGMEM to LittleFS filesystem
- Added comprehensive test scripts for LED effects
- Updated documentation to reflect Anthropic integration
Reviewed changes
Copilot reviewed 62 out of 65 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage.h, src/storage.cpp | Updated config field names for AI settings; potential migration issue with NVS keys |
| src/secrets.h.example | Updated example configuration for Anthropic API |
| src/main.cpp | Major cleanup with network code extracted to modules; reduced from ~1600 to ~240 lines |
| src/network/*.cpp | New modular network components for WiFi, OTA, and web server |
| src/api/*.cpp | New API handlers for segments, config, status, pixels, prompt, and nightlight |
| src/effects/*.cpp | Effect registration macros updated with usesPrimaryColor parameter |
| src/core/controller.cpp | Added nightlight feature; changed segment ID allocation to reuse deleted IDs |
| src/core/effect_registry.h | Added usesPrimaryColor field to effect metadata |
| platformio.ini | Added LittleFS filesystem configuration |
| test/*.sh | New shell scripts for testing effects and connectivity |
| docs/*.md | Comprehensive documentation updates for new architecture |
| docs/archive/* | Archived old web UI and OpenRouter client code |
| // Find lowest available ID (reuse deleted IDs) | ||
| bool usedIds[MAX_SEGMENTS] = {false}; | ||
| for (uint8_t i = 0; i < segmentCount; i++) { | ||
| uint8_t id = segments[i].getId(); | ||
| if (id < MAX_SEGMENTS) { | ||
| usedIds[id] = true; | ||
| } | ||
| } | ||
|
|
||
| uint8_t newId = 0; | ||
| for (uint8_t i = 0; i < MAX_SEGMENTS; i++) { | ||
| if (!usedIds[i]) { | ||
| newId = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Add segment to next available slot | ||
| Segment* seg = &segments[segmentCount]; | ||
| seg->setRange(leds, start, actualLength, reversed); | ||
| seg->id = nextSegmentId++; | ||
| seg->id = newId; |
There was a problem hiding this comment.
The segment ID allocation logic has been changed to reuse deleted IDs by tracking which IDs are in use. However, this creates a discrepancy with the comment on line 262 which says "Find lowest available ID (reuse deleted IDs)" but the actual implementation finds the lowest available ID from 0 to MAX_SEGMENTS. The old implementation used nextSegmentId++ which could exceed MAX_SEGMENTS. While the new implementation is more correct, consider whether the segment ID should be stable across deletions (always incrementing) or recycled (reusing lowest available). The current implementation chooses recycling, which may cause confusion if users expect segment IDs to remain stable.
| config.aiApiKey = prefs.getString("ai_apikey", ""); | ||
| config.aiModel = prefs.getString("ai_model", "claude-3-5-sonnet-20241022"); |
There was a problem hiding this comment.
The NVS storage keys have been changed from "apikey" and "model" to "ai_apikey" and "ai_model", but there is no migration logic to transfer existing user configurations from the old keys to the new keys. This will cause users to lose their AI API key and model settings after upgrading to this version, requiring manual reconfiguration.
| // API credentials (Anthropic Claude) | ||
| #define DEV_AI_API_KEY "" // Your Anthropic API key (get from https://console.anthropic.com/) |
There was a problem hiding this comment.
The comment says "Anthropic API key" but the define is named "DEV_AI_API_KEY", which is more generic. While this is functional, consider updating the comment to match the more generic naming (e.g., "AI API key (Anthropic)") to maintain consistency between the comment and the define name.
| @@ -57,6 +69,9 @@ bool wifiConnected = false; | |||
| unsigned long lastWifiAttempt = 0; | |||
| const unsigned long WIFI_RETRY_INTERVAL = 30000; | |||
There was a problem hiding this comment.
The WiFi retry interval constant is defined as 30000ms in the global scope but there's also a constant WIFI_RETRY_INTERVAL_MS that should be used from constants.h according to the project coding guidelines. This creates duplicate definitions. The code should use the constant from constants.h instead of defining WIFI_RETRY_INTERVAL locally.
| // Speed controls chase rate | ||
| uint16_t scaledFrame = (frame * params.speed) >> 6; |
There was a problem hiding this comment.
The theater chase effect now uses a scaled frame counter based on speed, but the scaling is done with a right shift by 6 bits (>> 6). This means the speed parameter is divided by 64 before being applied. This scaling factor should be documented or made into a named constant to explain why 6 bits specifically, and to allow for easier adjustment if the speed range needs tuning.
| static int16_t scannerPos = 0; | ||
| static int8_t scannerDir = 1; | ||
|
|
||
| // Static for frame skip timing | ||
| static uint8_t scannerFrameCount = 0; |
There was a problem hiding this comment.
The scanner effect uses static variables (scannerPos, scannerDir, scannerFrameCount) which violates the project's best practice of avoiding global/static state in effects. According to the coding guidelines and the ADDING_EFFECTS.md document, effects should use the segment scratchpad for state instead of static variables. This can cause issues when multiple segments use the same effect or when switching between effects.
| const targetBrightness = parseInt(document.getElementById('nightlightTarget').value); | ||
|
|
||
| try { | ||
| const result = await api('/nightlight', 'POST', { |
There was a problem hiding this comment.
Unused variable result.
| const result = await api('/nightlight', 'POST', { | |
| await api('/nightlight', 'POST', { |
No description provided.