Screensavers #462
Screensavers #462
Conversation
Adds screensavers in addition to the backlight idle off. More info in screensavers.md
📝 WalkthroughWalkthroughAdds screensaver support: a new 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🧹 Nitpick comments (11)
Tactility/Source/service/displayidle/BouncingBallsScreensaver.h (1)
9-13: Consider adding explicit special member function declarations for consistency.Unlike
MystifyScreensaverandMatrixRainScreensaver, this class relies on implicit defaults for constructors, destructor, and copy/move operations. While functionally correct, adding explicit declarations would maintain consistency across all screensaver implementations.♻️ Suggested addition for consistency
class BouncingBallsScreensaver final : public Screensaver { public: + BouncingBallsScreensaver() = default; + ~BouncingBallsScreensaver() override = default; + BouncingBallsScreensaver(const BouncingBallsScreensaver&) = delete; + BouncingBallsScreensaver& operator=(const BouncingBallsScreensaver&) = delete; + BouncingBallsScreensaver(BouncingBallsScreensaver&&) = delete; + BouncingBallsScreensaver& operator=(BouncingBallsScreensaver&&) = delete; + void start(lv_obj_t* overlay, lv_coord_t screenW, lv_coord_t screenH) override; void stop() override; void update(lv_coord_t screenW, lv_coord_t screenH) override;Tactility/Source/service/displayidle/BouncingBallsScreensaver.cpp (1)
56-85: Add null check afterlv_obj_createfor consistency and safety.Unlike
MystifyScreensaver::initPolygon()(line 179), this function doesn't check iflv_obj_createreturnsnullptrbefore usingball.obj. While LVGL allocation failures are rare, the other screensaver implementations handle this case.♻️ Suggested fix
void BouncingBallsScreensaver::initBall(Ball& ball, lv_obj_t* parent, lv_coord_t screenW, lv_coord_t screenH, int index) { ball.obj = lv_obj_create(parent); + if (ball.obj == nullptr) { + return; + } lv_obj_remove_style_all(ball.obj); lv_obj_set_size(ball.obj, BALL_SIZE, BALL_SIZE);Tactility/Source/service/displayidle/MatrixRainScreensaver.cpp (3)
16-19: Use ofrand()is not thread-safe and has limited randomness quality.While
rand()works for visual effects, consider using<random>facilities for better distribution. Also,rand()is seeded once inonStart()usingsrand(), which is fine for this use case.
37-44: Potential inefficiency when erasing from vector middle.Erasing from the middle of
availableColumns_is O(n). For a screensaver this is likely fine given the small number of columns, but consider usingstd::swapwith the last element beforepop_back()for O(1) removal if performance becomes a concern.♻️ Optional O(1) removal optimization
int MatrixRainScreensaver::getRandomAvailableColumn() { if (availableColumns_.empty()) { return -1; } int idx = rand() % availableColumns_.size(); int column = availableColumns_[idx]; - availableColumns_.erase(availableColumns_.begin() + idx); + std::swap(availableColumns_[idx], availableColumns_.back()); + availableColumns_.pop_back(); return column; }
245-284:screenWandscreenHparameters are unused inupdate().The parameters are marked with
LV_UNUSEDbut passed in. If screen dimensions could change during screensaver runtime (e.g., rotation), the screensaver wouldn't adapt. Consider whether this is intentional.Tactility/Source/settings/DisplaySettings.cpp (2)
68-82:toStringdoes not handleScreensaverType::Countexplicitly.If
Countis ever passed (which would be a bug), this hitsstd::unreachable(). This is acceptable sinceCountis a sentinel and shouldn't be used as an actual value, but consider adding a case that returns an error string for defensive programming.
141-145: Return value offromStringis ignored.The return value indicating parse success is not checked. While this defaults gracefully to
BouncingBalls, logging a warning on parse failure could help diagnose corrupted settings files.♻️ Optional: Log parse failures
auto screensaver_entry = map.find(SETTINGS_KEY_SCREENSAVER_TYPE); ScreensaverType screensaver_type = ScreensaverType::BouncingBalls; if (screensaver_entry != map.end()) { - fromString(screensaver_entry->second, screensaver_type); + if (!fromString(screensaver_entry->second, screensaver_type)) { + // Log warning about invalid screensaver type in settings + } }Tactility/Source/app/display/Display.cpp (1)
259-280: Dropdown order dependency is documented but fragile.The comment on line 271 notes that dropdown order must match enum order. Consider generating options programmatically or adding a compile-time check to catch mismatches.
Tactility/Source/service/displayidle/DisplayIdle.cpp (2)
117-183: Complex tick logic with multiple lock acquisitions.The
tick()function acquireslvgl::locktwice in some code paths (lines 123 and 168). While each individual lock/unlock pair is correct, this increases complexity. Consider restructuring to minimize lock scope changes.Also, if the lock at line 168 fails, the function returns without restoring state, which is correct as it will retry on next tick.
197-216: Retry logic inonStophas a potential issue.The retry loop calls
stopScreensaver()which may fail to acquire the LVGL lock and return early without clearingscreensaverOverlay. The checkscreensaverOverlayafterstopScreensaver()(line 207) correctly handles this. However,kernel::delayMillis(50)blocks the service shutdown thread.Consider if blocking during shutdown is acceptable for this use case.
Tactility/Private/Tactility/service/displayidle/DisplayIdleService.h (1)
30-33: Avoid hard‑coding auto‑off ticks to a fixed 50ms interval.
SCREENSAVER_AUTO_OFF_TICKSembeds the timer period assumption. If the tick interval changes, the auto‑off duration drifts. Consider storing the duration (e.g.,std::chrono::minutes(5)) and computing ticks from the actual timer period.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Tactility/Source/service/displayidle/MatrixRainScreensaver.cpp (1)
246-251: Stagger logic overwrites initial head position.The stagger on line 249 overwrites the
headRowthat was set ininitRaindrop()(line 100) to-(drop.trailLength). This results in drops starting at-(rand() % staggerRange)instead of the intended-(trailLength), meaning some drops may appear on screen sooner than expected since the random offset is likely smaller than the trail length.If the intent is to stagger while keeping drops fully off-screen initially, consider adjusting the calculation.
♻️ Optional: Stagger while keeping drops off-screen
if (col >= 0) { initRaindrop(drops_[i], col); // Stagger start positions (guard against tiny screens) int staggerRange = numRows_ / 2; if (staggerRange > 0) { - drops_[i].headRow = -(rand() % staggerRange); + drops_[i].headRow = -(drops_[i].trailLength) - (rand() % staggerRange); } }Tactility/Source/service/displayidle/DisplayIdle.cpp (3)
66-109: Document LVGL lock precondition foractivateScreensaver().This method calls LVGL APIs (
lv_layer_top(),lv_disp_get_hor_res(),lv_obj_create(), etc.) without acquiring the lock itself. It relies on callers to hold the lock. While the current call sites intick()(line 178) andstartScreensaver()(line 233) correctly acquire the lock first, this precondition should be documented to prevent future misuse.📝 Suggested documentation
+/** + * Activate the screensaver overlay and start the animation. + * `@pre` Caller must hold the LVGL lock. + */ void DisplayIdleService::activateScreensaver() {
192-202: Consider using<random>instead ofsrand().
srand()modifies global state which could affect other code usingrand(). For an embedded system this is likely fine, but modern C++<random>provides better isolation and distribution quality.
218-220: Log level should bewarningorerrorfor resource leak.A potential resource leak during shutdown is more severe than informational. Using
LOGGER.warn()orLOGGER.error()would make this more visible in logs.♻️ Proposed fix
if (screensaverOverlay) { - LOGGER.info("Failed to stop screensaver during shutdown - potential resource leak"); + LOGGER.warn("Failed to stop screensaver during shutdown - potential resource leak"); }Tactility/Private/Tactility/service/displayidle/DisplayIdleService.h (1)
25-27: Consider makingstopScreensaverRequestedatomic.
stopScreensaverRequestedis set instopScreensaverCb()(LVGL event callback, potentially from input thread) and read/cleared intick()andstopScreensaver()(timer thread). While the current usage pattern may be safe due to LVGL's event handling model, making itstd::atomic<bool>would eliminate any potential data race and be consistent withsettingsReloadRequested.♻️ Proposed fix
lv_obj_t* screensaverOverlay = nullptr; - bool stopScreensaverRequested = false; + std::atomic<bool> stopScreensaverRequested{false}; std::atomic<bool> settingsReloadRequested{false};
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Tactility/Source/service/displayidle/BouncingBallsScreensaver.cpp (1)
100-121: Potential integer truncation in color interpolation.The step calculation uses integer division which can truncate small differences. When
colorStepis large and the color difference is small,stepR/G/Bmay be 0, causing the color to not reach the target until the final snap at line 112-114. This is functionally correct due to the final assignment, but the fade may appear to "jump" at the end.This is a minor visual artifact and the final snap ensures correctness. For smoother fades, consider using fixed-point arithmetic or storing the original color for linear interpolation.
Tactility/Source/app/display/Display.cpp (1)
261-282: Dropdown options match enum order, but consider adding compile-time verification.The enum order and dropdown strings (lines 273-275) are currently synchronized. However, there's no compile-time check to prevent them from drifting if the enum order changes. While bounds checking at line 109 prevents out-of-bounds access via the
Countsentinel, it doesn't verify the string sequence matches enum order.Consider extracting dropdown options into a string array with a static assertion to ensure array size matches
ScreensaverType::Count, or documenting this coupling requirement more explicitly in the code to flag it during code review.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tactility/Source/service/displayidle/DisplayIdle.cpp (1)
119-131: Perform file I/O outside the LVGL lock to avoid UI stalls.
loadOrGetDefault()performs file I/O at/data/settings/display.properties(viafile::loadPropertiesFile()) while the display lock is held in bothtick()(line 130) andstartScreensaver()(line 233). File access latency can unexpectedly block the display thread. Move the load outside the critical section and apply the result under the lock.♻️ Proposed refactor
void DisplayIdleService::tick() { + bool reload = settingsReloadRequested.exchange(false, std::memory_order_acquire); + settings::display::DisplaySettings newSettings{}; + if (reload) { + newSettings = settings::display::loadOrGetDefault(); + } + if (!lvgl::lock(100)) { return; } if (lv_display_get_default() == nullptr) { lvgl::unlock(); return; } - // Check for settings reload request (thread-safe) - if (settingsReloadRequested.exchange(false, std::memory_order_acquire)) { - cachedDisplaySettings = settings::display::loadOrGetDefault(); + if (reload) { + cachedDisplaySettings = newSettings; }void DisplayIdleService::startScreensaver() { + auto newSettings = settings::display::loadOrGetDefault(); if (!lvgl::lock(100)) { return; } // Reload settings to get current screensaver type - cachedDisplaySettings = settings::display::loadOrGetDefault(); + cachedDisplaySettings = newSettings;
|
Thanks a lot! Another great feature! |
2 participants
Adds screensavers in addition to the backlight idle off.
More info in screensavers.md
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.