Implement lastPreset tracking for auto-save usermod#5389
Implement lastPreset tracking for auto-save usermod#5389ampledashes wants to merge 1 commit intowled:mainfrom
Conversation
Added lastPreset variable to track the last preset used for auto-saving functionality so that the last preset active will be called upon reboot
WalkthroughAdds preset-recall detection to the auto-save usermod by tracking the last recalled preset and extending the autosave settle window when a new preset is recalled, preventing premature autosave triggers during preset switches. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp (1)
134-144:⚠️ Potential issue | 🟠 MajorPreset-recall tracking is unreachable with default
autoSaveIgnorePresets=trueThe early-return guard at line 134 fires for any
currentPreset > 0whenautoSaveIgnorePresetsistrue(the default since line 56). The new block at lines 141–144 is therefore dead code in the default configuration — it is only reached whencurrentPreset == 0, i.e. no preset is active. The feature only works when users explicitly setautoSaveIgnorePresets = false, which is the non-default, undocumented path.A secondary consequence: because
lastPresetis never updated during the early-return path, it goes stale. When a user recalls preset N (early return fires,lastPresetunchanged) and then makes a manual change that clearscurrentPresetto 0, the comparisonlastPreset (N) != currentPreset (0)fires a spurious "preset change" event. The practical impact is limited — the brightness/speed/etc. change already setsautoSaveAfterindependently — butlastPresetshould still be kept consistent.Suggested fix: move the
lastPresetsynchronisation (and conditionallyautoSaveAfterextension) before the early-return guard, or restructure the guard to allow preset-recall tracking to proceed regardless ofautoSaveIgnorePresets:🐛 Proposed fix
void loop() { static unsigned long lastRun = 0; unsigned long now = millis(); if (!autoSaveAfterSec || !enabled || (strip.isUpdating() && now - lastRun < 240)) return; lastRun = now; uint8_t currentMode = strip.getMainSegment().mode; uint8_t currentPalette = strip.getMainSegment().palette; unsigned long wouldAutoSaveAfter = now + autoSaveAfterSec*1000; - if (!autoSaveAfterSec || !enabled || (autoSaveIgnorePresets && currentPreset>0) || (strip.isUpdating() && now - lastRun < 240)) return; // setting 0 as autosave seconds disables autosave - lastRun = now; - uint8_t currentMode = strip.getMainSegment().mode; - uint8_t currentPalette = strip.getMainSegment().palette; - unsigned long wouldAutoSaveAfter = now + autoSaveAfterSec*1000; - - // Trigger on preset recall - if (lastPreset != currentPreset) { - lastPreset = currentPreset; - autoSaveAfter = wouldAutoSaveAfter; - } - - if (knownBrightness != bri) { + // Track preset recalls before the autoSaveIgnorePresets guard so lastPreset + // is always consistent and the settle window is extended on any preset switch. + if (lastPreset != currentPreset) { + lastPreset = currentPreset; + autoSaveAfter = wouldAutoSaveAfter; + } + + if (autoSaveIgnorePresets && currentPreset > 0) return; // preset active, skip save + + if (knownBrightness != bri) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp` around lines 134 - 144, The preset-recall update is unreachable when autoSaveIgnorePresets is true because the early-return uses (autoSaveIgnorePresets && currentPreset>0); fix by moving the preset-change detection/synchronization before that guard: compute wouldAutoSaveAfter, check if lastPreset != currentPreset, update lastPreset and (only if you want to extend autosave regardless of ignore flag) set autoSaveAfter = wouldAutoSaveAfter as appropriate, then evaluate the existing early-return (which can still skip extending autosave when autoSaveIgnorePresets is true). Ensure you reference and update the same variables: lastPreset, currentPreset, autoSaveAfter, wouldAutoSaveAfter, and keep the strip.isUpdating()/now-lastRun check intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@usermods/usermod_v2_auto_save/usermod_v2_auto_save.cpp`:
- Around line 134-144: The preset-recall update is unreachable when
autoSaveIgnorePresets is true because the early-return uses
(autoSaveIgnorePresets && currentPreset>0); fix by moving the preset-change
detection/synchronization before that guard: compute wouldAutoSaveAfter, check
if lastPreset != currentPreset, update lastPreset and (only if you want to
extend autosave regardless of ignore flag) set autoSaveAfter =
wouldAutoSaveAfter as appropriate, then evaluate the existing early-return
(which can still skip extending autosave when autoSaveIgnorePresets is true).
Ensure you reference and update the same variables: lastPreset, currentPreset,
autoSaveAfter, wouldAutoSaveAfter, and keep the strip.isUpdating()/now-lastRun
check intact.
|
@ampledashes I'm still struggling to understand the use case for this PR. Can you explain a bit more in which scenarios this will be helpful? The usermod already tracks the current effect, palette, effect sliders, colors, brightness. Afaik the purpose is not to create a copy of the last active preset, but to help users who tinker with effects and want the same effect to be active on next start.
This is exactly something that will not happen: If you activate preset 37, then switch WLED off after a minute, WLED will not re-activate preset 37. It will boot into the autosave preset, let's say preset 250. Maybe I missed something? |
|
The specific gap we’re addressing is that recalling a preset and then powering off before making any value changes results in nothing being saved — the autosave never triggered because no tracked values changed. On reboot, the device loads stale autosave state rather than the recently recalled preset’s state. This PR ensures preset recalls also schedule a save, so the autosave snapshot stays current. |
does that not work? |
Ah OK, so technically the issue is that this UM is not multi-segment aware. It tracks changes to the global "currenteffect", "currentPalette" etc. and reacts on changes in the main segment, but changing the effect in the second, third, etc segment will not be recognised. The same happens when changing to a preset that doesn't modify the first segment, but only affects other segments. Adding "currentPreset" to the watched state information will be an improvement, but it's still not a solution when users change the effect in the second segment without using a preset. Plus it will add flash writes (wearing out flash) that are just copying the current preset state into the dedicated autosave preset. |
|
Added lastPreset variable to track the last preset used for auto-saving functionality so that the last preset active will be called upon reboot if the user has those settings configured