Conversation
Allows grouping of related effects presets in 'banks' as the user likes, and provides navigation functions to traverse between or within banks.
obar
force-pushed
the
usermod-preset-array
branch
from
9b2cea5 to
a868e63
Compare
|
Ninja edit: Preset Navigation is a better name for this. Updated accordingly but the usermod is the same as described. |
|
IMO this is why playlists exist. |
|
Thanks for mentioning it. Can you say more about that? It wouldn't be the first time my mental model about WLED would be wrong ;) |
|
@softhack007 Not a revival of anything, it's a feature to let a user create a menu of many presets. There were concerns about textareas in the usermods page initially voiced but they were all addressed, and nobody has suggested any further issues since. |
|
Handling presets is not a thing usermod would need to be invented for IMO. |
|
I could adapt most of this code for a core feature. If WLED were desktop-first, I'd think about a drag-and-drop reordering interface for the preset cards themselves... but that demands a lot of screen real estate, and drag-to-reorder implementations are only barely good enough for a single list on mobile—this is much more complicated as a list of lists . A text area was the simplest idea that I could think up. An enhancement might be a JS parser to check for properly formed input/no undefined preset IDs. Should a text area like this live in a heading under Macros, or would it be a better fit under User Interface? |
|
I think the utility is there for the users who change their WLED effects with buttons, a remote control, an encoder, or other interesting interfaces that aren't web. Not every user but also not the tiniest minority either. Fortunately it won't add much to the binary. For now, how about I do this: text area for a preset array under Macros (because it's fundamentally a macro functionality), no JS input checking but still use the C++ input checking shown in my branch, and write it up for the docs. In the future this feature can get fancier as needed, or even move places. Sounds good? |
|
I like the idea, I just fail see the difference between this and a playlist. can you elaborate? |
|
Sorry team, I do try to look at internet messages generously but I'm struggling to view this discussion and the related one on textareas as something other than animosity. I've answered challenge questions to no response, and I've asked questions that sometimes don't get a response either. I feel like I've caught some snark, but it might just be cultural differences in communication. In the defense of you all, obviously this isn't the only issue or feature being discussed or worked on, and I know you all do a lot to support complete strangers with projects: often with the very basics of how to wire things up. That's incredibly generous. Just noting how this specific discussion feels to me. I did ask how this feature is the same as playlists when it was brought up. The reply I eventually got to that question was that preset handling should be a core feature rather than a usermod, not that it already exists as a core feature. That's why I went forward with asking how this could best fit in. WLED is an open source project of course, and nobody owes anyone anything in open source, but perhaps if someone looks like they're willing to be a new contributor it's worth meeting them with extra kindness and patience, understanding that they aren't as knowledgeable about the code base as seasoned contributors. Anyway, that's a side suggestion and I'd be excited to put it behind, start fresh. @DedeHai that's a great way to put it in a question. My understanding of the distinctions follow. A playlist is a 1D list, and you can navigate it in one direction: A playlist is composed in a largely linear way: it's possible to delete or change single entries anywhere in the list, but there isn't rearranging and you can only insert at the end, not at the beginning or middle. This proposed interface makes it very easy to rearrange the banks and the presets within the banks. I believe most people use a playlist to automatically cycle their LEDs through a set of effects. This interface lets you build a menu of effects where the input is likely tactile and the graphical feedback of where you are in the menu is the LED state. To me they aren't the same feature. But as I said before: I'd love to hear more, my understanding could be flawed. Thanks for your time! |
|
Hi @obar, I think the key contentions are:
The interesting point to me is that each one of those features would be a valuable addition on its own -- and then together they combine to make something even greater. I'm sorry it's been a challenge to get a clear direction - we're all working on this in our spare time. Does this provide more clarity as to a path forward? |
|
Thanks for clarifying some things @willmmiles. I think the biggest point that hadn't been conveyed to me was this:
So it's not so much that what I was proposing === playlists, but rather that because it contains multiple presets it should be a playlist. You've outlined a great list of changes that need to happen to generalize playlists further, and would allow this interface proposal to be reproduced in the playlists model. |
|
@obar your contribution is much appreciated, maybe there was some misunderstanding/communication breakdown. |
I already made that. The problem, as you already mentioned, is how long does a nested playlist run? Until it is finished or until parent timer runs out? Currently implemented as a 1st option. |
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
bot
added
the
stale
WalkthroughThis update introduces a new usermod called "Preset Navigation" for the WLED project. The core logic is implemented in a new header file that defines a class for managing navigation through user-defined banks of effect presets, with support for JSON API commands and configuration persistence. Documentation for the usermod is provided in a new README file, detailing setup, usage, and configuration instructions. Integration with the WLED usermod system is achieved by conditionally including and registering the new usermod in the usermods list source file, based on a build flag. Changes
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
usermods/preset_navigation/preset_navigation.h (9)
13-18: Consider potential overflow withuint8_tfor preset arrays.Using
uint8_tfor the preset arrays limits preset IDs to 0-255. While this is likely sufficient for most use cases, it might be worth documenting this limitation in the comments or README.
22-24: Improve variable naming for better readability.The variables
x,y, andxMaxwould benefit from more descriptive names that indicate their purpose, such ascurrentBankIndex,currentPresetIndex, andmaxBankIndex.- int x = 0; // x is current bank - int y = 0; // y is current sub-bank preset - int xMax = 0; + int currentBankIndex = 0; // Index of the current bank + int currentPresetIndex = 0; // Index of the current preset within the bank + int maxBankIndex = 0; // Maximum bank index
19-19: Unused variable detected.The variable
bootupPresetNavigationis declared but never used in the code. Consider removing it if not needed or implementing its intended functionality.- String bootupPresetNavigation;
42-45: Remove unnecessary return statement and consider applying initial preset.The
setup()method contains an unnecessaryreturnstatement (as it's a void function) and doesn't apply the initial preset.- void setup() override { - x = y = 0; - return; - } + void setup() override { + x = y = 0; + if (enabled && readArray() && xMax >= 0) { + applyPresetNavigation(); + } + }
47-49: Remove unnecessary return statement.The
loop()method contains an unnecessaryreturnstatement.- void loop() override { - return; - } + void loop() override { + // No operation needed in loop + }
101-148: Add validation for preset IDs and improve error handling.The
readArray()method could benefit from validating that the parsed preset IDs correspond to valid presets in the system. Additionally, the function always returnstrueeven if there might be errors during parsing.Consider modifying the return value to indicate success or failure and adding preset validation:
bool UsermodPresetNavigation::readArray() { int readX = 0; int i = 0; int cursor = 0; int currentNewline = presetArray.indexOf("\n"); int currentComma; bool finished = false; bool bankFinished; + bool success = true; while (!finished) { // Iterate over each bank (line of text) in presetArray (string representation of array) if (currentNewline == -1){ finished = true; currentNewline = presetArray.length(); } if (readX >= USERMOD_PRESET_NAVIGATION_MAX_BANKS) { DEBUG_PRINTLN("Preset navigation: exceeded max banks"); + success = false; break; } currentComma = presetArray.indexOf(",", cursor); bankFinished = false; bankOffset[readX]=i; while (!bankFinished) { // Iterate through each PRESET (comma separated value) in this bank if (i >= USERMOD_PRESET_NAVIGATION_MAX_PRESETS) { DEBUG_PRINTLN("Preset navigation: exceeded max presets"); + success = false; break; } if (currentComma == -1 || currentComma > currentNewline) { bankFinished = true; currentComma = currentNewline; } String preset = presetArray.substring(cursor,currentComma); presets[i] = preset.toInt(); + // Optional: Validate that the preset ID exists in the system + // if (!presetExists(presets[i])) { + // DEBUG_PRINTF("Preset navigation: preset %d does not exist\n", presets[i]); + // } i++; cursor = currentComma + 1; currentComma = presetArray.indexOf(",", cursor); } // end PRESET loop readX++; // Find our next newline cursor = currentNewline + 1; currentNewline = presetArray.indexOf("\n", cursor); } // end bank loop xMax=readX-1; bankOffset[readX]=i; - return true; + return success; }
150-152: Implement or remove empty method.The
printArray()method is declared but has an empty implementation. Either implement it for debugging purposes or remove it if not needed.-void UsermodPresetNavigation::printArray() { - -} +// If needed for debugging +void UsermodPresetNavigation::printArray() { + DEBUG_PRINTLN("Preset Navigation Banks:"); + for (int i = 0; i <= xMax; i++) { + DEBUG_PRINTF("Bank %d (%d presets): ", i, bankOffset[i+1] - bankOffset[i]); + for (int j = bankOffset[i]; j < bankOffset[i+1]; j++) { + DEBUG_PRINTF("%d ", presets[j]); + } + DEBUG_PRINTLN(); + } +}
154-172: Consider returning a success indicator for navigation methods.The
incBankanddecBankmethods don't indicate whether the navigation was successful. Consider returning a boolean to allow callers to know if the operation had an effect.-void UsermodPresetNavigation::incBank(bool wrap){ +bool UsermodPresetNavigation::incBank(bool wrap){ if (x == xMax && wrap) { x = 0; y = 0; applyPresetNavigation(); + return true; } else if (x < xMax) { x++; y = 0; applyPresetNavigation(); + return true; } + return false; } -void UsermodPresetNavigation::decBank(bool wrap){ +bool UsermodPresetNavigation::decBank(bool wrap){ if (x == 0 && wrap) { x = xMax; y = 0; applyPresetNavigation(); + return true; } else if (x > 0) { x--; y = 0; applyPresetNavigation(); + return true; } + return false; }
174-198: Add feedback for "no cycle" conditions in preset navigation.The
incPresetanddecPresetmethods silently do nothing when there's nowhere to cycle. Consider providing feedback or returning a success indicator.-void UsermodPresetNavigation::incPreset(bool wrap){ +bool UsermodPresetNavigation::incPreset(bool wrap){ int yMax = bankOffset[x+1] - bankOffset[x] - 1; if (yMax == 0) { // We don't have anywhere to cycle + DEBUG_PRINTLN("Preset navigation: bank has only one preset"); + return false; } else if (y == yMax && wrap) { y=0; applyPresetNavigation(); + return true; } else if (y < yMax) { y++; applyPresetNavigation(); + return true; } + return false; } -void UsermodPresetNavigation::decPreset(bool wrap){ +bool UsermodPresetNavigation::decPreset(bool wrap){ int yMax = bankOffset[x+1] - bankOffset[x] - 1; if (yMax == 0) { // We don't have anywhere to cycle + DEBUG_PRINTLN("Preset navigation: bank has only one preset"); + return false; } else if (y == 0 && wrap) { y = yMax; applyPresetNavigation(); + return true; } else if (y > 0) { y--; applyPresetNavigation(); + return true; } + return false; }usermods/preset_navigation/readme.md (6)
11-21: Document the preset ID limitation.The preset notation explanation is clear, but it doesn't mention the limitation of preset IDs to 0-255 due to the use of
uint8_tin the implementation.Add a note about the preset ID limitation:
This is assuming you've defined presets for given effects with the ID numbers 10, 11, 12, 21... etc. The preset IDs do not have to be sequential or follow any pattern like this, but it's up to you to carefully input this information: the usermod currently doesn't do any checks here for undefined preset IDs. **Do not include trailing commas after banks, just a newline**. You can include a space after commas, but that just makes the stored string take up a little more space in memory. + +Note that preset IDs must be between 0 and 255 due to implementation constraints.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
14-18: Improve code block formatting.The fenced code block should have a language specified for proper syntax highlighting.
-``` +```text 10,11,12 20,21 30,31,32,33<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 14-14: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `22-24`: **Clarify initial state on startup.** The example states that "After starting up WLED, the state will be Preset 10", but this might not be the case if the user configures a different first preset in their first bank. Consider clarifying this. ```diff -After starting up WLED, the state will be Preset 10---you can set this preset to be applied at boot when creating it. One option is to cycle through banks. Cycling up will apply Preset 20, and if cycling up again, Preset 30. If a wrapping command is used, then cycling up a third time will cycle back to Preset 10. From Preset 10, the other option is to cycle within a bank. Cycling up will apply Preset 11 next. Cycling down with a preset wrapping command from 10 will apply Preset 12. +After starting up WLED, the state will begin with the first preset in your first bank (Preset 10 in our example). You can set this preset to be applied at boot when creating it. One option is to cycle through banks. Cycling up will apply the first preset in the next bank (Preset 20), and if cycling up again, the first preset in the third bank (Preset 30). If a wrapping command is used, then cycling up a third time will cycle back to the first preset in the first bank (Preset 10). From Preset 10, the other option is to cycle within a bank. Cycling up will apply Preset 11 next. Cycling down with a preset wrapping command from 10 will apply Preset 12.🧰 Tools
🪛 LanguageTool
[grammar] ~23-~23: Before the countable noun ‘at’ an article or a possessive pronoun is necessary.
Context: ...ou can set this preset to be applied at boot when creating it. One option is to cycl...(IN_NN_CC_VBG)
26-35: Add examples of programmatic API command usage.The API commands are clearly explained, but examples of how to send these commands programmatically (e.g., via HTTP requests) would be helpful.
Add an example of sending commands programmatically:
| `p~-` | Decrement preset with wrap around | | `p-` | Decrement preset without wrapping | +### Example of sending commands via HTTP + +You can send these commands via an HTTP request to the JSON API endpoint: + +``` +http://[WLED-IP]/json?pn=b~ +``` + +Or using a POST request with JSON payload: + +```json +{ + "pn": "b~" +} +```
43-45: Clearer explanation of state tracking limitations.The explanation of state tracking limitations could be clearer with an explicit example.
-This usermod naively keeps track of the current position in the navigation array. If you use the navigation commands to get to Preset 20 in our example above, and then use the web interface to apply Preset 10, sending an Cycle Bank Up command will still go to Preset 30 as if the previous state was still 20. +This usermod independently keeps track of its own navigation state. This means it's unaware of preset changes made through other interfaces. For example, if you: +1. Use the navigation commands to go to Preset 20 (first preset in bank 2) +2. Then use the web interface to manually apply Preset 10 (first preset in bank 1) +3. Then send a "Cycle Bank Up" command + +The usermod will apply Preset 30 (first preset in bank 3), not Preset 20, because it still thinks you're in bank 2. The usermod doesn't automatically sync its state with manual preset changes.🧰 Tools
🪛 LanguageTool
[misspelling] ~44-~44: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...b interface to apply Preset 10, sending an Cycle Bank Up command will still go to ...(EN_A_VS_AN)
[grammar] ~44-~44: You can use the subjunctive mood in this context.
Context: ...o to Preset 30 as if the previous state was still 20. I like to save presets with ...(SUBJUNCTIVE_MOOD)
46-46: Make personal preference section clearer.The note about personal preference regarding brightness settings could be formatted more clearly as a separate section or tip.
-I like to save presets with "include brightness" unchecked, so that changing the brightness is independent to the changing of effects. In fact, on my own WLED builds I change the default of that checkbox to unchecked, by editing the file `wled00/data/index.js` and rebuilding the web interface with npm before compiling. +## Tip: Handling Brightness + +I personally prefer to save presets with "include brightness" unchecked, so that changing the brightness is independent of changing effects. This allows you to adjust brightness without affecting your preset navigation. + +Note: On my own WLED builds, I change the default of that checkbox to unchecked by editing `wled00/data/index.js` and rebuilding the web interface with npm before compiling.🧰 Tools
🪛 LanguageTool
[uncategorized] ~46-~46: The preposition “of” seems more likely in this position.
Context: ... changing the brightness is independent to the changing of effects. In fact, on my...(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
usermods/preset_navigation/PresetNavigationDiagram.pngis excluded by!**/*.pngusermods/preset_navigation/ScreenshotBankUp.pngis excluded by!**/*.png
📒 Files selected for processing (3)
usermods/preset_navigation/preset_navigation.h(1 hunks)usermods/preset_navigation/readme.md(1 hunks)wled00/usermods_list.cpp(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/usermods_list.cpp (1)
wled00/um_manager.cpp (2)
add(65-70)add(65-65)
🪛 LanguageTool
usermods/preset_navigation/readme.md
[style] ~2-~2: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ... usermod This usermod lets you navigate a large number of effect presets more easily. You can gro...
(LARGE_NUMBER_OF)
[uncategorized] ~2-~2: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...* that you can navigate either between (eg bank 1 to bank 2) or within (switch bet...
(E_G)
[grammar] ~23-~23: Before the countable noun ‘at’ an article or a possessive pronoun is necessary.
Context: ...ou can set this preset to be applied at boot when creating it. One option is to cycl...
(IN_NN_CC_VBG)
[uncategorized] ~41-~41: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...at you added this usermod to the build, eg `-D USERMOD_PRESET_NAVIGATION_MAX_PRESE...
(E_G)
[misspelling] ~44-~44: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...b interface to apply Preset 10, sending an Cycle Bank Up command will still go to ...
(EN_A_VS_AN)
[grammar] ~44-~44: You can use the subjunctive mood in this context.
Context: ...o to Preset 30 as if the previous state was still 20. I like to save presets with ...
(SUBJUNCTIVE_MOOD)
[uncategorized] ~46-~46: The preposition “of” seems more likely in this position.
Context: ... changing the brightness is independent to the changing of effects. In fact, on my...
(AI_EN_LECTOR_REPLACEMENT_PREPOSITION)
🪛 markdownlint-cli2 (0.17.2)
usermods/preset_navigation/readme.md
14-14: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (8)
usermods/preset_navigation/preset_navigation.h (4)
5-11: Good use of configurable constants with sensible defaults.The use of preprocessor directives to define configurable constants with reasonable defaults is a good practice. This allows users to override these values as needed without modifying the source code.
51-65: Command handling looks good with clear structure.The command handling in
readFromJsonStateis well-implemented with clear structure and comprehensive command options.
73-93: The config parsing and carriage return handling is thorough.Good implementation of configuration handling with attention to detail in removing carriage returns for platform independence.
97-99: Clear naming and helpful comment about textarea.The static constants for naming have clear names, and the comment about the trailing ">" for textarea is helpful for understanding the UI integration.
wled00/usermods_list.cpp (2)
245-247: Proper conditional include for the new usermod.The include directive for the new usermod follows the established pattern in the file, correctly using the
USERMOD_PRESET_NAVIGATIONflag.
478-480: Correct registration of the usermod in the manager.The registration of the
UsermodPresetNavigationclass in theregisterUsermods()function is properly implemented following the pattern used for other usermods.usermods/preset_navigation/readme.md (2)
1-7: Clear introduction with helpful visual.The introduction clearly explains the purpose and benefits of the usermod. The visual diagram is particularly helpful for understanding the concept of banks and presets.
🧰 Tools
🪛 LanguageTool
[style] ~2-~2: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ... usermod This usermod lets you navigate a large number of effect presets more easily. You can gro...(LARGE_NUMBER_OF)
[uncategorized] ~2-~2: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...* that you can navigate either between (eg bank 1 to bank 2) or within (switch bet...(E_G)
8-10: Concise setup instructions with link to documentation.The setup instructions are clear and concise, with a helpful link to the WLED compilation documentation for users who need more information.
| // One day might return false on a parsing error | ||
| bool readArray(); | ||
| void printArray(); | ||
| inline void applyPresetNavigation(){ | ||
| applyPreset(presets[bankOffset[x]+y]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking to prevent potential out-of-bounds access.
The applyPresetNavigation() method lacks bounds checking before accessing the arrays. This could lead to out-of-bounds access if navigation state becomes invalid.
- inline void applyPresetNavigation(){
- applyPreset(presets[bankOffset[x]+y]);
- }
+ inline void applyPresetNavigation(){
+ if (x >= 0 && x <= xMax && bankOffset[x] + y < USERMOD_PRESET_NAVIGATION_MAX_PRESETS) {
+ applyPreset(presets[bankOffset[x]+y]);
+ } else {
+ DEBUG_PRINTLN("Preset navigation: invalid indices");
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // One day might return false on a parsing error | |
| bool readArray(); | |
| void printArray(); | |
| inline void applyPresetNavigation(){ | |
| applyPreset(presets[bankOffset[x]+y]); | |
| } | |
| // One day might return false on a parsing error | |
| bool readArray(); | |
| void printArray(); | |
| inline void applyPresetNavigation(){ | |
| if (x >= 0 && x <= xMax && bankOffset[x] + y < USERMOD_PRESET_NAVIGATION_MAX_PRESETS) { | |
| applyPreset(presets[bankOffset[x] + y]); | |
| } else { | |
| DEBUG_PRINTLN("Preset navigation: invalid indices"); | |
| } | |
| } |
github-actions
bot
removed
the
stale
|
Hey! This pull request has been open for quite some time without any new comments now. It will be closed automatically in a week if no further activity occurs. |
github-actions
bot
added
the
stale
5 participants
A simple usermod called
Preset ArrayPreset Navigation (edit: it occurred to me since making the request, this name could be better! And this is my only chance to fix it! Sorry if anyone was in the midst of dealing with this while I made the change). It lets you group effect presets together however you see fit, and cycle between the groups ("banks") without going through every single effect preset you've saved. When you've gotten to the bank you want, you can then cycle through just the presets of that bank.This is primarily focused for controlling WLED in ways other than the web UI.
Preset banks are one one line of preset IDs, comma separated:
Cycling between banks in this example takes you through preset 10, 20, 30, 40, then back to 10. If you are at 30, cycling within the bank takes you to 31, 32, 33, and then back to 30.
For a specific example: presets in a bank can have any logical connection you want, in this video demo of the usermod I show how the third bank (presets
30,31,32,33) are all different color variations of a flame effect. When cycling between banks you don't have to cycle through every color variation if you aren't going for flame. When you do get to that flame effect, you can cycle just within that bank through the color variations.Another benefit of this usermod over normal preset cycling is that it isn't sequence-sensitive. I've used a numerical pattern above but a new preset can be squeezed in anywhere in the cycle, whereas the typical preset cycling API call needs preset IDs to be in order without gaps.
This usermod uses a textarea in the config for multiline input, so it depends on #4217
Summary by CodeRabbit