Fixes An Issue With Every Hour Cron Schedules

[NeilBetham]

I noticed an issue where if I have a schedule set to run every hour it will get overridden in favor of a schedule that only gets run at a specific hour that is before the current time.

The logic in schedule.py uses the start of the day as the starting point for the croniter. As such events that run hourly appear to happen before events that run at a specific hour of the day. To fix this I added a helper which indexes the croniter backwards from the current time instead of forwards from the start of the day. In my local testing this seems to have resolved the issue.

[maxbbraun]

Thanks for looking into this, Neil! Could you give an example of cron expressions together with the expected and actual behavior for those?

[NeilBetham]

Sure! So for example say we have two crons in the schedule:

1. 20 * * * *
2. 10 17 * * *

In this setup the way the schedule is evaluated currently is by setting the croniter start time to 00:00 and then calling next. This will yield the following times for each schedule respectively:

1. 00:20
2. 17:10

Now if the current time is before 17:10 then the first schedule is the only time to make it through the filter for events in the past. However say for example the time is currently 17:25 and the client is calling the API for a new image then both schedules pass that filter. Then once they are run through the max filter here then the second schedule is selected because the first schedule is only evaluated at 00:20 which puts it before 17:10 in the max filter.

My thinking is that the correct behavior would be to step back in time from the current instant and see when each of the schedules would have most recently executed in the past and then run that same max filters on those instances so that the most recently passed event will be selected. With this PR the times that would evaluate from the example schedule would be the following:

1. 17:20
2. 17:10

After the max function the first schedule would be correctly selected.

Let me know if anything is still not clear!

[NeilBetham]

Thanks for the review. I've address the comments and suggestions you made. I have updated sun.py to more align with how it worked previously. Basically I moved the munging of the reference time to midnight from the image() method to the rewrite_cron method. This seems to replicate the original behavior; though I'm not 100% clear on what the expected behavior for rewrite_cron is so please correct me if I'm wrong here. I have tested this locally the following logs are with the home address set to San Francisco, CA

/gif

INFO:root:Rewrote cron: (sunrise * * *) -> (10 6 * * *), reference Tuesday May 04 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (2 20 * * *), reference Tuesday May 04 2021 00:00:00 PDT 
INFO:root:Using image from schedule entry: City Sunset (sunset * * *, Tuesday May 04 2021 20:02:00 PDT)

Schedule Render

INFO:root:Rewrote cron: (sunrise * * *) -> (11 6 * * *), reference Monday May 03 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (1 20 * * *), reference Monday May 03 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (11 6 * * *), reference Monday May 03 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (1 20 * * *), reference Monday May 03 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (11 6 * * *), reference Monday May 03 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (1 20 * * *), reference Monday May 03 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (10 6 * * *), reference Tuesday May 04 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (2 20 * * *), reference Tuesday May 04 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (10 6 * * *), reference Tuesday May 04 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (2 20 * * *), reference Tuesday May 04 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (9 6 * * *), reference Wednesday May 05 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (3 20 * * *), reference Wednesday May 05 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (9 6 * * *), reference Wednesday May 05 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (3 20 * * *), reference Wednesday May 05 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (8 6 * * *), reference Thursday May 06 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (4 20 * * *), reference Thursday May 06 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (8 6 * * *), reference Thursday May 06 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (4 20 * * *), reference Thursday May 06 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (7 6 * * *), reference Friday May 07 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (5 20 * * *), reference Friday May 07 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (7 6 * * *), reference Friday May 07 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (5 20 * * *), reference Friday May 07 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (6 6 * * *), reference Saturday May 08 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (6 20 * * *), reference Saturday May 08 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (6 6 * * *), reference Saturday May 08 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (6 20 * * *), reference Saturday May 08 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (5 6 * * *), reference Sunday May 09 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (7 20 * * *), reference Sunday May 09 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunrise * * *) -> (5 6 * * *), reference Sunday May 09 2021 00:00:00 PDT 
INFO:root:Rewrote cron: (sunset * * *) -> (7 20 * * *), reference Sunday May 09 2021 00:00:00 PDT 

[NeilBetham]

@knyar Is there something fixed in croniter that is broken in the current version that is used?

[NeilBetham]

Okay, updated based on pydocstyle feedback as well as feedback here. Please let me know if there is anything else you'd like changed!

[knyar]

@knyar Is there something fixed in croniter that is broken in the current version that is used?

I am not aware of anything specific, I just noticed that the version listed in requirements.txt is pretty old.

[knyar]

Just as a data point, I've been running this on my personal accent server for a couple of weeks, and it seems to work fine. Unlike the current version it correctly handles my very simple schedule: cycling between two images every hour.

Hopefully this feedback might help to make a decision to get it merged.

[NeilBetham]

Yeah, no worries on the time front! These are side projects so I understand IRL takes precedence. Most of the changes to docs here were just to make pydocstyle happy. I don't have a preference one way or another on them. I can revert the changes I made and then make fixes based on your config if you prefer.

[maxbbraun]

Yeah, let's keep it to the functional changes for now. The pycodestyle I ran doesn't seem to do docstring and just helps with stuff like this.

[NeilBetham]

Weird, I can't repro the extra critiques it was giving me anymore. Oh well. I'll revert those and fix the issue with sun.py / delay(). Thanks for the feedback!

[NeilBetham]

Alright, sorry for the delay. This time around I have reverted all the doc changes and have updated sun.py to support the same forward and backward semantics as schedule.py. This should make sure that sunrise / sunset rewriting works as expected. I have done some testing locally and everything seems to be working as expected.