RPi-Jukebox-RFID icon indicating copy to clipboard operation
RPi-Jukebox-RFID copied to clipboard

feat: Add Idle Shutdown Timer support

Open hoffie opened this issue 2 months ago • 9 comments

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.

hoffie avatar Apr 11 '24 21:04 hoffie

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 Coverage Status
Change from base Build 8692197280: 0.0%
Covered Lines: 494
Relevant Lines: 1292

💛 - Coveralls

coveralls avatar Apr 11 '24 21:04 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 avatar Apr 12 '24 07:04 pabera

@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. :)

hoffie avatar Apr 12 '24 11:04 hoffie

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.

pabera avatar Apr 12 '24 13:04 pabera

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.

pabera avatar Apr 12 '24 13:04 pabera

In #1970 this feature was already requested.

s-martin avatar Apr 15 '24 10:04 s-martin

Will merge this to test the functionality in a RPI environment.

pabera avatar Apr 21 '24 09:04 pabera

@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.

phoniebox-timer-handling

pabera avatar Apr 24 '24 10:04 pabera

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

s-martin avatar May 02 '24 14:05 s-martin