Add option to save unmodified presets to autosave usermod #5175
Conversation
WalkthroughAdds a new boolean flag Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (7)📚 Learning: 2025-11-27T06:33:11.436ZApplied to files:
📚 Learning: 2025-11-14T13:37:11.994ZApplied to files:
📚 Learning: 2025-12-01T07:01:16.949ZApplied to files:
📚 Learning: 2025-08-26T11:51:21.817ZApplied to files:
📚 Learning: 2025-11-14T13:37:30.955ZApplied to files:
📚 Learning: 2025-09-02T19:42:57.475ZApplied to files:
📚 Learning: 2025-04-27T09:37:28.415ZApplied to files:
🪛 Cppcheck (2.18.0)usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp[error] 281-281: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it. (unknownMacro) 🔇 Additional comments (6)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (1)
136-136: Logic is correct; consider extracting guard conditions for readability.The implementation correctly integrates the new flag: autosave is now skipped when
autoSaveIgnorePresetsis true AND a preset is active. This preserves backward compatibility while enabling the new use case.The guard condition is getting quite long. If desired, you could extract it to improve readability:
+ bool shouldSkipAutoSave() { + return !autoSaveAfterSec || !enabled || + (autoSaveIgnorePresets && currentPreset>0) || + (strip.isUpdating() && now - lastRun < 240); + } + void loop() { static unsigned long lastRun = 0; unsigned long now = millis(); - if (!autoSaveAfterSec || !enabled || (autoSaveIgnorePresets && currentPreset>0) || (strip.isUpdating() && now - lastRun < 240)) return; + if (shouldSkipAutoSave()) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp
📚 Learning: 2025-04-27T09:37:28.415Z
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Applied to files:
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp
🪛 Cppcheck (2.18.0)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp
[error] 284-284: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (6)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (6)
19-22: Documentation is clear and helpful.The comments accurately describe the new feature and its default behavior, making it easy for users to understand the configuration option.
56-60: Configuration pattern looks good.The implementation follows the established pattern for configurable parameters and correctly defaults to
trueto preserve backward compatibility.
81-81: Static string declaration follows established pattern.The declaration is consistent with other PROGMEM strings in the class.
233-237: Configuration persistence implemented correctly.The new field is properly added to the config JSON using the same pattern as existing fields.
266-266: Config reading implemented correctly.The boolean value is read using the same pattern as other boolean config fields, with appropriate default fallback.
289-289: Static string definition is correct.The PROGMEM string is properly defined. Note: The Cppcheck warning about the PROGMEM macro is a false positive—PROGMEM is a standard Arduino macro for storing data in flash memory.
willmmiles
left a comment
willmmiles
left a comment
There was a problem hiding this comment.
Apart from the build time change, I think this is a reasonable feature to add.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (1)
230-234: LGTM: Configuration persistence properly implemented.The new field is correctly added to the JSON configuration output, following the same pattern as other configuration fields.
Note: As mentioned in the inline comments (lines 222-223), you'll need to separately update the HTML UI, xml.cpp, and set.cpp to expose this configuration option in the web interface. Currently, users would need to manually edit cfg.json to change this setting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR #5134, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
Repo: wled/WLED PR: 4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp
📚 Learning: 2025-04-27T09:37:28.415Z
Learnt from: KrX3D
Repo: wled/WLED PR: 4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.h:121-136
Timestamp: 2025-04-27T09:37:28.415Z
Learning: Using PROGMEM for the seven-segment font array (umSSDRNumbers) in the WLED SSDR usermod causes compilation problems, so it should be left as a regular array.
Applied to files:
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp
🪛 Cppcheck (2.18.0)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp
[error] 281-281: There is an unknown macro here somewhere. Configuration is required. If PROGMEM is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (5)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (5)
56-56: LGTM: Default preserves backward compatibility.The member variable is properly declared with a default value of
true, which preserves the original behavior of not auto-saving when an unmodified preset is active.
77-77: LGTM: Static string declaration follows established pattern.The static string declaration is consistent with the other configuration field names in the class.
263-263: LGTM: Configuration reading properly implemented.The field is correctly read from the configuration with a proper default fallback using the
|operator, ensuring backward compatibility when the field is missing from existing configurations.
281-286: LGTM: PROGMEM string definitions are correct.The flash string definitions follow the established pattern used by other configuration field names in this file. The static analysis warning about the
PROGMEMmacro is a false positive—it's a standard Arduino macro used throughout the WLED codebase for storing strings in flash memory to save RAM.
132-132: Verify thatcurrentPresetis cleared when a preset is modified.The new guard
(autoSaveIgnorePresets && currentPreset>0)assumes that when a user modifies a preset, WLED clearscurrentPresetto 0. If this assumption is incorrect, the default behavior might prevent autosaving even when a preset is modified, which would be a regression from the original behavior.Run the following script to verify how
currentPresetis managed when state changes occur:#!/bin/bash # Description: Search for assignments to currentPreset to understand when it's cleared # Search for writes to currentPreset rg -n -C3 'currentPreset\s*=' --type=cpp # Also search for the variable declaration ast-grep --pattern 'currentPreset'
| // By default it will not save the state if an unmodified preset | ||
| // is selected (to not duplicate it). You can change this behaviour | ||
| // by setting AUTOSAVE_IGNORE_PRESETS=false |
There was a problem hiding this comment.
Update the comment to reference the JSON configuration field.
The comment references AUTOSAVE_IGNORE_PRESETS=false, which suggests a compile-time define, but the actual implementation uses a JSON configuration field autoSaveIgnorePresets. This inconsistency could confuse users trying to configure this option.
Apply this diff to correct the documentation:
-// By default it will not save the state if an unmodified preset
-// is selected (to not duplicate it). You can change this behaviour
-// by setting AUTOSAVE_IGNORE_PRESETS=false
+// By default it will not save the state if an unmodified preset
+// is selected (to not duplicate it). You can change this behavior
+// by setting "autoSaveIgnorePresets": false in the usermod configuration.🤖 Prompt for AI Agents
In usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp around lines 19 to 21,
the comment incorrectly references a compile-time define
"AUTOSAVE_IGNORE_PRESETS=false" while the implementation reads the JSON
configuration field "autoSaveIgnorePresets"; update the comment to mention the
correct JSON config key (autoSaveIgnorePresets) and show the expected JSON usage
(e.g., "set autoSaveIgnorePresets to false in the config JSON") so users are
guided to change the runtime configuration rather than a non-existent
compile-time define.
| unsigned long now = millis(); | ||
| if (!autoSaveAfterSec || !enabled || currentPreset>0 || (strip.isUpdating() && now - lastRun < 240)) return; // setting 0 as autosave seconds disables autosave | ||
| if (!autoSaveAfterSec || !enabled || (autoSaveIgnorePresets && currentPreset>0) || (strip.isUpdating() && now - lastRun < 240)) return; // setting 0 as autosave seconds disables autosave | ||
| lastRun = now; |
There was a problem hiding this comment.
While reviewing this again, I stumbled over lastRun never being set!? I think the intention probably was this; fine to add this in this MR as well? This got introduced in 0ff4016#diff-e57385253fd04c5e758584aeb14c1398a2ec9febfbb230c376b406b4aaa93ffc
And, sorry, rusty cpp: should lastRun be a static scoped variable here? why not a private member variable?
There was a problem hiding this comment.
@mtin I think your fix is correct, "lastRun" implements the standard "be nice but not too nice" logic that enforces running the loop() even under high load (strip.isUdating())
A static variable in a function is different from a private class attribute, I think it was chosen here because we needed a persistent value without polluting global namespace.
So not necessary to make it a private member, because we will never have more than one instance of this usermod.
3 participants
The
usermod_v2_auto_savedeliberately will not save the current state if it is a preset without modifications. I think this totally depends on the use case and should be configurable.My use case:
So I added an option
AUTOSAVE_IGNORE_PRESETSwhich defaults totruewhich is the previous behavior.I'd like to argue that a default of
false(meaning: also autosave if it is an already saved preset) might make more sense for the majority, but ofc that is an opinionated guessI personally was confused why it would not save when cycling through my presets, could not find documentation on it - and ended up being educated by @softhack007 at #3771, thanks for that!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.