fix: warn when effect registration exceeds the list limit - #5837
fix: warn when effect registration exceeds the list limit#5837keeltrace wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. Walkthrough
ChangesEffect List Diagnostics
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to When the effect list is full, rejected effects now produce a debug warning with their metadata while retaining existing registration behavior. No current merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@keeltrace you are an AI ? I ;-) |
Lol. Keeltrace is indeed my coding agent. And he wasn't able to test that as stated. He's my pr bot. Just trying to help out the community. Feel free to close unmerged or tell me what changes you want.
|
| return _mode.size() - 1; | ||
| } else { | ||
| DEBUG_PRINT(F("WARNING: effect list full, could not add effect: ")); | ||
| DEBUG_PRINTLN(FPSTR(mode_name)); |
There was a problem hiding this comment.
@coderabbitai please check if FPSTR()is the correct macro here to assure that mode_name stays in PROGMEM on 8266.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- FPSTR definitions/usages ---'
rg -n -C 3 '\bFPSTR\s*\(' . -g '*.{cpp,h,hpp,ino}' | head -n 240
printf '%s\n' '--- debug print macro definitions ---'
rg -n -C 4 '#\s*define\s+DEBUG_PRINT(LN)?\b|DEBUG_PRINTLN\s*\(' . -g '*.{cpp,h,hpp,ino}' | head -n 240
printf '%s\n' '--- PROGMEM metadata declarations and addEffect declaration ---'
rg -n -C 2 '_data_FX_MODE_.*PROGMEM|addEffect\s*\(' wled00 -g '*.{cpp,h,hpp,ino}' | head -n 260Length of output: 41667
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all addEffect call sites outside the built-in registration block ---'
rg -n -C 3 '\baddEffect\s*\(' . -g '*.{cpp,h,hpp,ino}' | head -n 500
printf '%s\n' '--- usermod effect registration APIs and metadata-string examples ---'
rg -n -i -C 4 'addEffect|effect.*(metadata|name)|metadata.*effect|PROGMEM' usermods wled00 -g '*.{cpp,h,hpp,ino}' | head -n 520
printf '%s\n' '--- ESP8266 framework headers, if present in the checkout ---'
fd -HI 'pgmspace\.h|WString\.h|Print\.h' . | head -n 80Length of output: 77966
@softhack007, yes. FPSTR(mode_name) is correct for this path.
FPSTR() does not move data into PROGMEM. It casts the const char* to const __FlashStringHelper*, which selects the ESP8266 Print overload that reads from flash.
The built-in effect metadata strings use PROGMEM. The checked in-tree usermod effect metadata strings also use PROGMEM. Therefore, DEBUG_PRINTLN(FPSTR(mode_name)) reads the dropped effect metadata from program memory on ESP8266.
The addEffect() API accepts const char*. A third-party usermod could pass an SRAM string. That would not be safe with FPSTR(). This is already an implicit API contract because _modeData is used as PROGMEM elsewhere. A brief PROGMEM requirement in the addEffect() declaration would make the contract explicit.
You are interacting with an AI system.
@keeltrace fair. For future PR's, please make sure you add a statement in the description saying that you used a coding agent. Some maintainers here are very allergic to undeclared AI source code, especially when we have to find out ourselves by looking for typical "AI smells". |
softhack007
added
the
AI
Super valid. My goal with keeltrace is to be helpful. It found this repo related to light shows which is a hobby of mine. I will add that disclaimer going forward. Sorry if I upset anyone |
Thanks, no need to worry - this is a small PR, and knowing it was AI generated helps to set reviewer's expectations. Edit "managed by a human" is another of these AI smells 😉 . Sometimes we understand that like "I have no idea of the code, just did some testing and it worked for me". |
|
On further thinking this is my bad. -keeltrace handler |
|
@keeltrace fun fact: while professional developers are good at coding, many of us are bad at creative things like "building an awesome light show". So we definitely need creative hobby devs, who bring in their ideas for new light effects. |
2 participants
AI-generated contribution. I am the human operator responsible for follow-up with maintainers. Please feel free to close this PR or request any changes.
Fixes #5827.
When
WS2812FX::addEffect()reaches the 254-effect capacity, it currently returns255silently. Most usermods discard that return value, so dropped effects are difficult to diagnose.This adds a debug warning only on the full-list failure path and includes the effect metadata string that could not be registered. It intentionally does not change the 8-bit effect ID limit or overlap with the separate larger-effect-ID work.
Validation:
git diff --checkmainI could not run a firmware build locally because PlatformIO is not installed in this environment.
Summary by CodeRabbit