Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Idle Shutdown Timer support #2332

Merged
merged 13 commits into from
Nov 10, 2024

Conversation

hoffie
Copy link
Contributor

@hoffie hoffie commented Apr 11, 2024

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.

@coveralls
Copy link

coveralls commented Apr 11, 2024

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

@pabera
Copy link
Collaborator

pabera commented Apr 12, 2024

Thanks for the PR. I was wondering, did you consider this code on our development?

I am specifically thinking of this cass

class GenericTimerClass:
"""
Interface for plugin / RPC accessibility for a single event timer
"""

@hoffie
Copy link
Contributor Author

hoffie commented Apr 12, 2024

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

@pabera pabera added the future3 Relates to future3 development label Apr 12, 2024
@pabera pabera added this to the v3.6 milestone Apr 12, 2024
@pabera pabera mentioned this pull request Apr 12, 2024
@pabera
Copy link
Collaborator

pabera commented Apr 12, 2024

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
Copy link
Collaborator

pabera commented Apr 12, 2024

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 pabera changed the title Add Idle Shutdown Timer support feat: Add Idle Shutdown Timer support Apr 12, 2024
@s-martin s-martin linked an issue Apr 15, 2024 that may be closed by this pull request
@s-martin
Copy link
Collaborator

In #1970 this feature was already requested.

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.

Fixes: MiczFlor#1970
@pabera
Copy link
Collaborator

pabera commented Apr 21, 2024

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

@pabera
Copy link
Collaborator

pabera commented Apr 24, 2024

@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

@s-martin
Copy link
Collaborator

s-martin commented May 2, 2024

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

@pabera pabera self-assigned this Nov 3, 2024
@s-martin s-martin linked an issue Nov 5, 2024 that may be closed by this pull request
@pabera pabera merged commit 0df1a0c into MiczFlor:future3/develop Nov 10, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement future3 Relates to future3 development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 | Shutdown after [Time] idle 🐛 | GenericMultiTimerClass broken
4 participants