RPi-Jukebox-RFID
RPi-Jukebox-RFID copied to clipboard
feat: Add Idle Shutdown Timer support
This adds an optional idle shutdown timer which can be enabled via timers.idle_shutdown.timeout_sec
in the jukebox.yaml config.
The system will shut down after the given number of seconds if no activity has been detected during that time. Activity is defined as:
- music playing
- active SSH sessions
- changes in configs or audio content.
I know that the definition in documentation/developers/status.md is a bit more ambitious than what this PR does, but it solves the actual need for me and might solve it for others, too. So maybe it can be merged now and extended later.
Pull Request Test Coverage Report for Build 8694662023
Details
- 0 of 0 changed or added relevant lines in 0 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage remained the same at 38.235%
Totals | |
---|---|
Change from base Build 8692197280: | 0.0% |
Covered Lines: | 494 |
Relevant Lines: | 1292 |
💛 - Coveralls
Thanks for the PR. I was wondering, did you consider this code on our development?
- https://github.com/MiczFlor/RPi-Jukebox-RFID/blob/future3/develop/src/jukebox/jukebox/multitimer.py
I am specifically thinking of this cass
https://github.com/MiczFlor/RPi-Jukebox-RFID/blob/acf6ec0b4832db6bea2855a2d0fba4079037ad18/src/jukebox/jukebox/multitimer.py#L90-L93
@pabera Yes, I've seen the existing timer classes. However, I've had a hard time seeing how they could add benefits here. Two use cases come to mind:
- Could the GenericTimerClass handle the real timer (= 900 seconds)?
- We would constantly have to restart that timer, which seems to stop and start a new Thread each time, which looks unnecessarily heavy?
- We would still require another thread to handle the mpd updates, so we would end up with two threads.
- Could the GenericTimerClass handle the mpd update timer?
- Yes, that would be more realistic, but it would still imply regular (once every 10 seconds) starts of new threads for no real reason.
I think the shutdown timer is kind of special in that it is a timer which is expected not to fire most of the time, because some activity would restart it. So, maybe I'm missing some other benefits (plugin integration...? but with what purpose?), but to me it looked like using one of those classes would add more logical overhead (more complex code) and technical overhead (more thread starts).
(I've now added inline documentation to cover the reasoning so far)
Edit: If usage of those classes would be mandatory for the PR to be accepted, I could of course edit the code to use them. I would have more motivation to do so if I see the benefits, though. :)
Edit: If usage of those classes would be mandatory for the PR to be accepted, I could of course edit the code to use them. I would have more motivation to do so if I see the benefits, though. :)
It's definitely not a must. I am aligned with your explanation. Yet I wanted to make sure we don't create competing functionality which in the end makes the system harder to understand because the rational behind it cannot be understood.
For this feature to be complete, it would be nice to have function to get and manipulate the timeout_sec
variable in the jukebox.yaml. This would allow further development, e.g. adding the functionality to the UI.
In #1970 this feature was already requested.
Will merge this to test the functionality in a RPI environment.
@hoffie
I've made several changes to the initial design. Originally, I intended to integrate the Idle Shutdown timer into the UI, but soon discovered that the existing implementation lacked essential functions such as star
, cancel
and status
. In an attempt to implement these, I found myself essentially recreating the GenericTimerClass.
To resolve this, I divided your Timer class into two separate classes: 1) IdleCheck (GenericEndlessTimerClass
) and 2) IdleShutdown (GenericMultiTimerClass
). Additionally, a third class, IdleShutdownTimer, now manages these two timers.
This restructuring has allowed for a seamless integration of the Idle Shutdown Timer into the UI.
However, I encountered an issue with the self._has_changed_files()
check in the IdleShutdown class; it consistently returns true
in my Docker environment, but it worked in your original implementation in the same environment. Could you please review this part?
I'm eager to hear your thoughts on this refactor and would appreciate your help with testing.
The checks work again, however there are two small flake8 errors, see https://github.com/MiczFlor/RPi-Jukebox-RFID/actions/runs/8815089690/job/24513166228?pr=2332