Add a `timeStep` for print progress notifications

[jessemillar]

This PR adds a new setting internally called timeStep. timeStep configures a second condition for when to notify during print progress.

Without timeStep, if I have step set to notify me every 10% and I print a file that takes 10 hours, I'll receive a progress notification every 10% or one notification every hour. For longer prints, I like to be notified more frequently than that but if I set my step to something like 2% instead, I end up getting notified too frequently when I print a smaller item. With timeStep set to 10 minutes and step set to 10%, the 10 hour print will notify me roughly every ten minutes and smaller prints will only notify me every 10%.

Caveat: Octoprint only sends progress event updates in 1% intervals so the frequency of notifications sent via timeStep will never be exact (e.g. 1% of a 10 hour print takes 6 minutes so a timeStep value of 10 minutes would result in a notification at 2% after 12 minutes had passed). This PR could be refactored to instead have an actual timer that doesn't rely on Octoprint events, but the current implementation was enough for my needs.

(Sorry for all the trailing whitespace removal. My IDE deleted those automatically. You can view only non-whitespace changes in the PR diff or I can revert those changes if you'd prefer.)

[jessemillar]

The current code does not care if you just received a timeStep notification when the step notification triggers. This sometimes results in a, for example, 68% notification and then a minute later a 70% notification if you have step set to 10% and timeStep set to 10 minutes. This isn't a bug per se, but it's not a perfect user experience. I'm still mulling over options because I want the step notifications for small prints and the timeStep notifications for long prints but I don't want to overcomplicate the code.

[bchanudet]

Hello and thanks a lot for your work!

I still have to find some time to have a thorough look at the code but that looks like a good workaround before using the big things like a background thread with timers and so on.

One thing I'd ask first is if you could redo your PR but on the dev branch? I did some modifications on the code there but that shouldn't create merge conflicts. But more pragmatically, merging on master will make the new code available for new installs (the plugin in OctoPrint's Plugin Manager is configured to use the master.zip URL on new installs) even before we could catch bugs and act on them. For people that already have the plugin, the plugin manager check for GH releases so it's no problem there. (That reminds me I have to make the distinction between master and dev branches clearer in the readme)

For avoiding being spammed between step and timeStep notifications, maybe a "minimum interval between notifications" value could avoid too much notifications. But in the case of short prints this could be counterproductive. I'll try to think about it, but any idea is welcome. :)

[jessemillar]

I can definitely remake the PR on top of the dev branch. Gimme a bit.

The only thought I had other than a background thread was detecting at print start based on the estimated print time if we should use just step or just timeStep (step for shorter prints and timeStep for longer prints). Something like (pseudocode):

if estimatedPrintTime / step > timeStep
then
    # only use timeStep
else
    # only use step

My initial research into whether or not Octoprint exposes a print estimation wasn't super promising though. I found this GitHub issue that seemed to say that the estimation given by Octoprint is wildly inaccurate.

[jessemillar]

I dug through the Octoprint API docs and found a reliable way to get the rough estimation for print duration. Updated my code to essentially use the pseudocode I provided above. I'm gonna test it on my own for a bit before fixing the merge conflicts with the dev branch.

[jessemillar]

I discovered the RepeatedTimer functionality in the octoprint.util class last night. We could use that instead of my current implementation to guarantee notifications on a set time schedule (e.g. exactly every five minutes) but I kinda like waiting for the next percentage so we don't get multiple "Print is at 3%" notifications for super long prints.

[jessemillar]

I discovered the RepeatedTimer functionality in the octoprint.util class last night. We could use that instead of my current implementation to guarantee notifications on a set time schedule (e.g. exactly every five minutes) but I kinda like waiting for the next percentage so we don't get multiple "Print is at 3%" notifications for super long prints.

I've had nothing but problems working with the RepeatedTimer function. I can't get the timer to shut off reliably.

Merge conflicts are fixed now but I think I broke something with the settings UI. It's only rendering one setting on my end. Lemme fix that before merge.

[jessemillar]

Working now! Ready for an actual review whenever.

[bchanudet]

Alright I finally had some time to do a quick review.

First thing first: Sorry for the new merge conflict, I fired up my development VM and found out there was a commit that was waiting for nearly a year to be pushed. My dev survival reflexes kicked up before I knew it and made me hit the Sync button in vscode. It shouldn't be hard to resolve though.

As a developer, I always try to enforce some kind of ascending compatibility in my devs. I'm tired of updates that break everything, and I want users of my work not to be afraid of this when they update their software. In a way, I'm still trying to find the perfect balance between https://xkcd.com/1172/ and https://xkcd.com/2224/ :)

In our case here, I'm "worried" that users with a big percent threshold (for example 25%) will get "spammed" every 10 minutes on long prints with the default configuration value.

Would you be OK to set the default value at -1 and then adding some logic to deactivate completely the timer alert code in this case? This way existing users won't see any difference in the plugin behavior. We can add some indication in the settings page about this (f.e. setting "time_step_unit" : "minute(s), set "-1" to disable timed updates or something like this).

In the future I'll see if I can divide it in two different messages under the same progress category and change it to a threaded thing (or investigate further on the RepeatedTimer function you found), but I think that for the moment that'll do. :)

Thanks again for your hard work and your involvement in this PR. It reminded me that some people are actually using the plugin and that I should pay more attention to it sometimes.

[jessemillar]

Thanks for the thoughtful review! You get major brownie points for linking to XKCD.

No worries about the merge conflict. I'll take care of it.

Good thinking on disabling the time-based alerts by default. I'm kinda disappointed I didn't think of that part of the user experience. I'm new to Octoprint so I was only thinking about new installations, not automatic updates that'd surprise existing users. I'll definitely implement the -1 with explanation in the settings.

I actually think I've wrapped my head around the RepeatedTimer function thanks to my other PR so I could go ahead and implement exact timers in this PR if you'd prefer. Wouldn't take long. With timers we could change the settings page to say something like "alert every X% or Y minute(s) (whichever comes first)" which might be more understandable.

Thanks for making this plugin by the way! I had nothing but trouble with the DiscordRemote plugin and this one just works out of the box for me. I love the beautiful simplicity of it. :heart:

[bchanudet]

Well if you want to see if you can make RepeatedTimer work, feel free to have a go at it. :)

One thing I thought just before getting to bed: avoiding spam could maybe be as simple as resetting self.lastProgressNotificationTimestamp also when the plugin does a regular "percent-based" alert. This way, quick prints will mostly use the percent-based alerts, but for longer prints, as those will not be triggered frequently enough, the "timer-based" alert will notify the user of the ongoing progress.

But this was just a quick idea, I'd have to make some tests to check if it's actually working. Anyway that's the kind of quirks that can be ironed out later if necessary.

[jessemillar]

That's basically how it works currently (contrary to some of my original comments above).

Right now, when a print starts, the plugin gets the estimated duration of the print from Octoprint and runs some math to figure out if we should alert based only on percentage (step) or only on time elapsed (timeStep). The math looks something like if estimatedPrintTime / step > timeStep then use timeStep else use step. The plugin basically chooses which alert method to use based on which one would alert more frequently, time-based or percentage-based. If it chooses time-based alerting, it'll update the self.lastProgressNotificationTimestamp variable every time there's a notification sent (since we're not currently using RepeatedTimer).

The benefit to moving to RepeatedTimer is that right now, there's only a chance of alerting when the percent progress value of the print increases (e.g. 5% to 6%). If you're running a SUPER LONG print, that 1% increase could take hours and you wouldn't get alerted even though you have your timeStep set to a value less than hours. RepeatedTimer would result in an alert being sent every time your timeStep is hit even if the percent hasn't increased. We wouldn't need the self.lastProgressNotificationTimestamp variable with RepeatedTimer either.

Hopefully that explanation makes sense. Sorry for making a bunch of commits and comments that made the PR confusing!