Add an autosave feature. State can saved frequently

[Race666]

I also need an autosave feature for mopidy as @kokosowy #4
This PR implements such a feature, but not sure if programming is sufficent for mopidy and your code quality standard :-)

I need this function because mopidy runs on an device which is simply powered off instead of shutting down the operating system cleanly.

[mikiair]

I just thought about writing this kind of "autosave-on-demand" for myself, but came across this conversation at the right time.
Power-down instead of clean system shutdown is also my preferred method. My Raspi is controlled by mobile app, and I experienced not-saved volume and URI changes several times since then.

How about adding some "dirty" state set through event handlers (like mute_changed, playback_state_changed...) and only then doing the store to SD card?

[Race666]

The "dirty" state sounds good. I also thinked about this, but the interval based save was easier to implement :-)
. For me the track_playback_started or playlist_changed event should set the dirty bit and depending on the URI prefix (stream => http(s),:// file = file;// ,,,), I only want to use this feature for Internet streams, so that the last Radio station is played automatically on startup.

Config something like this:

autosave.enabled=True
# Choose the events. Possibe choices: track_playback_started,playlist_changed,mute_changed,playback_state_changed
autosave.events=track_playback_started,playlist_changed,mute_changed,playback_state_changed
# protocols on which only the autosave is triggered, possible choices: all, http, https, file
autosave.for_protocols=http,https

P.S. I used it with armbian on a Orange Pi Zero => great item: small, cheap, WLAN and LAN Intrerface, stable, good audio interface.

[sphh]

@Race666: Thank you for the nicely made PR. Highly appreciated.

My own plan would have been to use the mopidy events to save the state with:

        state = self.store_state()
        self.write_state(state, self.statefile)

This would implement the 'dirty' state approach and at the same time get rid of the Timer thread.

Configuration options would be a list of states, where changes in this state should be tracked and saved, e.g. tracklist, mixer, playback and maybe others.

NB: There should be a 'delay' in the saving the mixer.volume, because if you rapidly press the volume up button, without a delay all intermediate steps would be saved.

I have not found the time to do it, despite its ‘easiness’ to implement …

What do you think about this approach?

[Race666]

@sphh I agree with you, (Configurable) Events are the best approach. I see attaching to events is simple to define such a function with the name of the event:

    def track_playback_started(self, tl_track):
        logger.info('started' + str(tl_track))
...
Jun 08 18:59:29 RadioPlayer mopidy[5474]: INFO     [AutoplayFrontend-9] mopidy_autoplay.frontend startedTlTrack(tlid=5, track=Track(comment='http://www.gong971.de', genre='Rock', name='Gong 97.1', uri='https://addrad.io/44552sv'))

I think it should be possible to set the "dirty state" on tracklist or playback events. A state save on a mixer change, is in my opinion, not necessary.

[Race666]

@sphh Check updated PR :-)

[sphh]

Thanks, @Race666. That's what I had in mind!!

A couple of points:

• I believe, that import threading is a left-over.
• We now have two configuration options doing the same: autosave.enabled and autosave.events. Couldn't we have just one: autosave_events (or just autosave or autosave_at)?
• There is also the on_event available. Wouldn't it be easier to use this? Then the (power) user is free to put any available event into the list of autosave_events and s/he would not be restricted by the events supported explicitly by this extension? (As a positive side-effect the repetitive code would not be needed.)

[Race666]

@sphh Changed to on_event. But not sure where is the best place to store const

self._autosave_min_save_frequency=60

[sphh]

Sorry for being a nuisance!

I do like the feature to limit the autosave events!! This should definitely be configurable!!

• Why are you introducing self._autosave_enabled? Just checking the autosave list with if self.config['autosave_events'] should do it.
• Do you really need to do str(event) in on_event()? The API documentation says, that the event is already a string …
• Do you have to return the event from on_event()?
• Why do you need track_playback_started(), playback_state_changed(), tracklist_changed() and stream_title_changed()? Wouldn't the state be saved twice when these events fire?

I would have thought, something like that is enough:

def on_event(self, event, **kwargs):
        logger.debug("Event %s args: %s",str(event),str(kwargs))
        if event in self.config['autosave_events']:
            now = time.time()
            if (now - self._last_autosave) < self.config['autosave_min_interval']:
                logger.info("Skip autosave")
            else:
                logger.info("Autosave ({0})".format(save_event))
                state = self.store_state()
                self.write_state(state, self.statefile)    
                self._last_autosave = now

[Race666]

@sphh Written compact code and added autosave_min_interval

[sphh]

Thanks, @Race666. That looks good.

Some more questions:

• Why does the autosave_min_interval have a default minimum value of 10? Shouldn't it be 0 to enable immediate saving?
• I just noticed, that autosave_min_interval can be easily read to represent minutes and not seconds. Maybe it should be renamed to use minimum instead?
• Why is the value for autosave_min_interval in ext.conf different to the default value set in the code?

[mikiair]

• I am not sure if I got this right, but for my understanding on_event will only trigger a save/store if the time since the last logged event is greater than the minimum interval. With other words, a sequence of events (like frequent volume change) within that time will not get saved, or luckily at clean shutdown. I guess, this still needs some timer event clocking.
  [I am actually quite interested in getting the volume saved correctly with the last track/stream: radio stations have varying 'raw' loudness levels (depending on their settings, music style etc.); too quiet can be handled, but too loud is a bit more worrying]
• logger.info("Skip autosave") - I would recommend to remove this line, will only blow up log files.
• In __init__.py, the types of the new config entries are defined as AutoValue as the others. Is this required? I do not see the meaning of setting these to "auto" here.

[sphh]

@mikiair: Looks like you know my own code better than I!!

You are absolutely right regarding the log level. After looking through my own code I would very much prefer a debug log level for all three messages.

You are also absolutely right regarding the AutoValue! I introduced the AutoValue(config.ConfigValue) only because I needed a ConfigValue, where 'auto' has a special meaning. As you rightly observe, neither autosave_events nor autosave_min_interval do need this feature (this should also made clear in the comments in ext.conf). So schema['autosave_events'] = config.List(optional=True) and schema['autosave_min_interval'] = config.Integer(minimum=0) should do.

Regarding you most important point: You are absolutely right. Well spotted and shame on me for not having seen it: The state is only saved with the next event.

Solution?

Either do not care about writing too often (the only problem I could see here is the volume_changed event). That would remove a lot of complexity from the code.

Or start a timer thread on the event and save the state whenever it finishes. (Should a new event fired in the interval before the timer has expired, restart the timer from 0?)

I would very much prefer the first option (and write a warning regarding the volume). I could also imagine, that the volume is only autosaved in steps of 10s.

[mikiair]

@sphh: Thanks! Fortunately, it is not too much code, and I like Python for its 'cleanliness' more and more!
I still would care about writing too often - not really as I care about SD card (there's much more write access in the system taking place anyway), but rather for source elegance reasons.
I come back to my initial "dirty state" proposal and combine it with the on_event handler (just a draft outline):

def on_event(self, event, **kwargs):
    if event in self.config['autosave_events']:
        if not self._isdirty:    # no restart of timer on multiple events within minimum interval
            self._isdirty = True
            start_timer(...min_interval...)
        else:
            ....
....
def on_timer():
    store_and_save()
    stop_timer()              # don't know about Python timers, only to prevent that it fires on and on again...
    self._isdirty = False
....

[sphh]

Ok, if there is a universal request for not writing too often, I can see two options:

1. Similar to your approach, but there is no need to have a dirty flag, because if you keep a reference to the threading.Timer() instance, you can check, if the timer has expired (→start a new one) or if it is still running (→do not start a new one or kill it and start a new one).
2. Another approach is to have a ‘timer’ running always in a separate thread and only control it (see below).

The question still remains: How should it behave, if new events are fired and the timer has not expired yet:

1. Should the timer be reset to 0.
2. Should the timer not be touched.

It might be true, that compared to other programming languages, it does not look too complex, but still it adds complexity to the code!

As promised above, here is a controllable timer:

import threading
import queue
import enum

# Could be replaced by strings ...
class TimerCommand(enum.Enum):
    STOP = enum.auto()
    SET = enum.auto()
    CANCEL = enum.auto()

class ResetTimer(threading.Thread):
    def __init__(self, timeout, callback, *args, **kwargs):
        super().__init__()
        self.timeout = timeout
        self.callback = callback
        self.args = args
        self.kwargs = kwargs
        # The queue to control the timer
        self.queue = queue.SimpleQueue()

    def stop(self):
         """Stop and exit the timer."""
        self.queue.put(TimerCommand.STOP)

    def set(self, timeout=None):
        if timeout is not None:
            self.timeout = timeout
        self.queue.put(TimerCommand.SET)

    def cancel(self):
        self.queue.put(TimerCommand.CANCEL)

    def run(self):
        timeout = None
        while True:
            try:
                cmd = self.queue.get(block=True, timeout=timeout)
            except queue.Empty:
                # Timed out
                timeout = None
                self.callback(*self.args, **self.kwargs)
            else:
                if cmd == TimerCommand.STOP:
                    # Exit thread
                    return
                elif cmd == TimerCommand.SET:
                    timeout = self.timeout
                elif cmd == TimerCommand.CANCEL:
                    timeout = None
                else:
                    raise ValueError(
                        f'Unknown command received. '
                        f'Got: {cmd}')

[Race666]

The question still remains: How should it behave, if new events are fired and the timer has not expired yet:

Should the timer be reset to 0.
Should the timer not be touched.

=> On_event
=> Check if timer is active => If not => store immediately => Start timer
=> If timer is active => on each event the last state should stored in a class variable and after the timer expires it should be written to file.

[mikiair]

Yes, resetting the timer to 0 on each event could delay the data storage again and again. Consider the (unlikely) case that some condition leads to an infinite or unusual long sequence of events --> status data will never be written to SD.

• Better to start the timer with the first event,
• then collect all other incoming changes before it expires
• and finally call write_state when timer expires (also stop the timer, so it does only fire once, not repeatedly). Any new event after that shall start from beginning (by re-starting the timer instance if possible, instead of creating a new one).

And maybe it could help to have an optimized or adapted version of the store_state function which only updates the relevant parts of the status dictionary instead of creating a new one each time.

[sphh]

@mikiair: You are absolutely right. I just wanted to know, which behaviour you expect from such a feature. Thanks, @Race666 and @mikiair.

I took the liberty to implement this feature taking bits from @Race666, from this discussion and my own ideas. I hope, you don't mind!

Please have a look at the autosave branch (https://github.com/sphh/mopidy-autoplay/tree/autosave) and test it, if that is what you want. Thanks.

@mikiair: Now that we have a persistent timer, we should also have a persistent state variable. That is not implemented. I might do that next, if no one beats me to it.

[Race666]

@sphh Thats ok 👍 Hope I could try it tommorrow. Thanks for the expedient discussion and implementing this feature!

[sphh]

With the latest commit, the state variable should be persistent.

Please give everything a thorough check! It was not that heavily tested …

[mikiair]

Great! Thanks for the rapid implementation!
Just installed on my Raspi and (so far) works like expected: autoplay.state file gets written with ~10s delay and only once despite multiple events in the log during that time.

[sphh]

Happy to hear, that it works so far. Please observe it a while and please tell me your experience with it. Thanks!

[Race666]

@sphh Working fine and stable:-) . A delay of 10secs before the file is written is ok.

Thank you!

[sphh]

Great to hear that it works. Whenever 10 s does not work for you, you know, that you can change the delay in the configuration file … (option save_interval).

@Race666: Thanks for get this starting and for your PRs, which gave me a good idea what and how to implement it!

[sphh]

Closed, because the final implementation can be found in https://github.com/sphh/mopidy-autoplay/tree/autosave (which is already merged into master).