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

Feature request (low): adding a "now" keyword for @time_trigger #139

Closed
wsw70 opened this issue Jan 6, 2021 · 9 comments
Closed

Feature request (low): adding a "now" keyword for @time_trigger #139

wsw70 opened this issue Jan 6, 2021 · 9 comments

Comments

@wsw70
Copy link
Contributor

wsw70 commented Jan 6, 2021

In the same vein as to noon or midnight, it would be great to have a now entry that would simplify the use of @time_trigger.

A typical case is would be @time_trigger("period(now, 1m)") or @time_trigger("period(now+1h, 1m)"). This would help to avoid fiddling with datetime.

I had a look at the code (

elif dt_str.startswith("midnight"):
) with the hope to make a PR but I exclusively use arrow (alternatives are pendulum, or delorean) because I never could get the time / date right (this is for me one of the most messed up parts in Python)

@dlashua
Copy link
Contributor

dlashua commented Jan 6, 2021

I agree this would be useful. At the moment, I use a function like this to make it a bit easier:

import datetime

def from_now(**kwargs):
    ref_dt = datetime.datetime.now() + datetime.timedelta(**kwargs)
    return ref_dt.strftime('%Y/%m/%d %H:%M:%S')

And use it like this:

@time_trigger(f'once({from_now(seconds=5)})')
def doit():
    log.info('here!')

There are some caveats to be aware of, though. PyScript will accept a time_trigger(once()) with a datetime in the past, and it'll just never fire. If I use the above to have something happen 1 millisecond from now, chances are, by the time it's fully scheduled and ready to triiger, that 1ms has past and it'll never actually trigger. Depending on the speed of the system you are running Home Assistant on, and how many other things it's doing at that moment this minimum time to trigger will increase. I would imagine anything over 5 seconds is safe in most cases.

An easier alternative uses task.create and a task.sleep inside of that. Something like this (untested):

def do_in(secs, func, *args, **kwargs):
  def inner():
    task.sleep(secs)
    func(*args, **kwargs)

  return task.create(inner)

def thing_to_do():
  log.info('here')


do_in(5, thing_to_do)

do_in will return immediately and then, outside of the current execution path, perform the thing_to_do function 5 seconds later.

@wsw70
Copy link
Contributor Author

wsw70 commented Jan 6, 2021

PyScript will accept a time_trigger(once()) with a datetime in the past, and it'll just never fire

This is also what I was concerned with for periodic() as well. When using AppDaemon, a similar trigger (periodic calls) had to be scheduled a bit in the future because a past time would not trigger anything (it may have been changed since).

When I used datetime.datetime.now() in periodic() (in pyscript) it worked, possibly on the next occurrence (the first one (now()) being effectively in the past, but I did not really pay attention as it was not impacting)

@craigbarratt
Copy link
Member

craigbarratt commented Jan 22, 2021

The @time_trigger decorator is "evaluated" (ie, parsed to determine the next trigger time) on startup and whenever a time trigger occurs. That's so that things like 'once(8:00)' mean a daily trigger, and once(sunset) will get a slightly different sunset time each day.

So adding now is certainly possible, but it will be a little counter-intuitive. For example:

  • once(now + 1m) will be a periodic trigger of duration just over 1m (ie, the re-arming computation time will accumulate), with the first trigger 1m after startup; this won't trigger just once one minute from startup
  • period(now, 1m) will be a periodic trigger of duration just over 1m, with the first trigger 1m after startup
  • period(midnight, 1m) will be a periodic trigger of duration exactly 1m (since the start time is fixed)
  • period(now + 10m, 1m) will be a periodic trigger of duration just over 10m; the interval of 1m will never trigger since the start time always resets to now + 10m
  • once(now) will never trigger

If I made now just after the current time that would cause once(now) and period(now, ...) to go into an infinite loop of triggering, so that's not a good idea.

An alternative is to set now to be the current time at startup (fixed at the first evaluation), or maybe just after the current time at startup so it's guaranteed to trigger if there is no offset. Then the behavior would be:

  • once(now + 1m) will trigger just once one minute from startup
  • period(now, 1m) will be a periodic trigger of duration exactly 1m, with the first trigger at startup
  • period(midnight, 1m) will be a periodic trigger of duration exactly 1m (since the start time is fixed)
  • period(now + 10m, 1m) will be a periodic trigger of duration 1m starting 10m from now
  • once(now) will trigger once at startup.

Now that I wrote all that, the 2nd option definitely looks the simplest and most intuitive. Thoughts?

@dlashua
Copy link
Contributor

dlashua commented Jan 22, 2021

I agree that that 2nd option is more intuitive than the first.

I'm not keen on the idea of setting now to "just after the current time at startup" because it means that in a trigger created well after startup "now" no longer means "now" but, really just means some time in the past.

I think I would prefer a solution where "once(now)" works like @time_trigger('startup'), where it executes as soon as possible, even if that means 5 minutes from now due to a very slow system. The trouble is how to deal with "now + 10m" without using sleep().

@wsw70
Copy link
Contributor Author

wsw70 commented Jan 22, 2021

An alternative is to set now to be the current time at startup

Yes, this is exactly what I had in mind.

My major issue with the situation today (it is the same in AppDaemon BTW) is that the request is not natural. Nobody is going to say "do it in 10 minutes, and start the countdown now". They will say "do it in 10 minutes" and it is obvious that it starts "now". So once(10min) is more natural than once(datetime.something.lambda.shift.delta(鞭), 10 min).

If I want to say "do it now", it is once(now).

If I want to say "do it every 3 hours", I expect periodic(3h) and the obvious start is now. "do it every 3 hours, but start in 2 hours": periodic(now+2h, 3h).

The thing is that "now" in pyscript is the same as "startup", which is understandable, but not immediately obvious when building a script.

If now needs to be "now + 10 ms because otherwise now will be in the past when we get there" - no problem for me. I definitely prefer that to building a datetime thing (which I will use arrow for anyway because I am lazy and lost hours of my life when doing that the battery included python way)

Having a now that matches your 2nd proposal is close to my understanding of natural language.

@craigbarratt
Copy link
Member

craigbarratt commented Jan 23, 2021

I've implemented it as discussed.

I'm not keen on the idea of setting now to "just after the current time at startup" because it means that in a trigger created well after startup "now" no longer means "now" but, really just means some time in the past.

I didn't explain it clearly. Hopefully the docs explain it better. now is set to the time when each trigger is created (more precisely when first evaluated), which will be around the HASS startup time for most triggers, but could be a later time for an inner function / closure. So its value is constant for each trigger, but could be a different value for each trigger. In all cases "once(now)" is the same as "startup", and something like "once(now + 5min)" does what you expect - trigger just once 5 minutes after the trigger is created.

@dlashua
Copy link
Contributor

dlashua commented Jan 24, 2021

excellent! now, a question of garbage collection... I think that, over time, this would run out of memory. But maybe it wouldn't? Obviously, there are easier/better ways to do the below, this is merely a point of curiosity.

RT = []

@state_trigger('binary_sensor.motion == "on"')
def turn_on():
  light.room.turn_on()

@state_trigger('binary_sensor.motion == "off"')
def turn_off():
  @time_trigger('once(now + 5m)')
  def inner_turn_off():
    light.room.turn_off()

  RT.append(inner_turn_off)

@craigbarratt
Copy link
Member

craigbarratt commented Jan 26, 2021

Yes, that should work, although as you point out it's a lot less efficient than just doing a task.sleep. Yes, the RT array will grow on every trigger, and none of the old trigger functions will be garbage collected, which isn't a good thing.

I tried this alternative to clean up as it goes:

RT = set()

@time_trigger('period(now, 3s)')
def every_3sec():
    @time_trigger('once(now + 10sec)')
    def do_something_10sec_later():
        log.info("do_something_10sec_later")
        RT.discard(do_something_10sec_later)
        
    RT.add(do_something_10sec_later)

RT grows to 4 elements, and then stays between 3 and 4 elements.

However, this doesn't cancel (delete) each trigger function since the function body now has a reference to itself, so the __del__ method isn't called by the destructor (that happens when the reference count goes to zero). That means the RT.discard doesn't delete the trigger function. I'm not sure how to solve that problem. One solution could be to make the self-reference a weakref, but there's no easy way I can think of to figure out when that case occurs.

@dlashua
Copy link
Contributor

dlashua commented Jan 27, 2021

That's an interesting way to go about it. I like it. Though, as mentioned, not critical since there are better ways to accomplish these same tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants