Added ability to query the length of a given timer via keyName.

[rakkur]

Possible use cases:

1. allows user to re-queue the timer at the current or a different time frame with the same or different instruction set
2. allows user to make logical decisions regarding how much longer it will take until the timer executes.

Example: An external garage door "close" timer is set for 30 minutes, but an internal garage door opens and interrupts this. The timer can be inspected and if the time to execution is less than 10 minutes, assume the car is going to leave shortly and extend the timer to another 10 minutes from there.

[rkoshak]

I wonder if this is a feature that should extend beyond just TimerMgr.

If we create a Timer class that consists of the OH timer and keeps the when that gets created from helpers.createTimer then we can get the ability to get the when for any timer created and managed by OHRT, not just in TimerMgr.

Would you be up for that?

[rkoshak]

For consistency, this should return a time.Duration instead of number of seconds.

    return time.Duration.between(this.timers[key]['when'], time.toZDT());
...
    return time.Duration.ofSeconds(0);

[rkoshak]

I hate to do this but it just occurred to me, does it make sense to return 0 if the timer doesn't exist? Shouldn't it return null? I can go either way. Is there some use case where getting a 0 length duration makes sense for a Timer that doesn't exist?

As for the change to between, your approach is better. I wasn't thinking. now should clearly be the first arguments. If the Timer has already passed we'll still get a negative duration but I think that's desireable. "How long since this timer ran?"

[rakkur]

Hi Rich! Thanks for reviewing this change. I made the alterations you requested with one modification: I return a positive value as the Duration object so that the user can re-queue the timer with the current exact duration remaining. The example you gave would return a negative number and would then require a modification or abs() to re-queue.

As for including the timer on the helper function, I think it's a great idea! However, since the openhab timer is returned by this function, we would have to wrap this return value in an object in order to return all the info correctly, eg:

  return {
    'timer': actions.ScriptExecution.createTimer(name, timeout, func),
    'when': timeout,
  }

I'll have to look at the scope at which this changes your other code, as I'm not super familiar with it yet. Would be happy to give it a try if you give this solution your blessing, however.

[rkoshak]

As for including the timer on the helper function, I think it's a great idea! However, since the openhab timer is returned by this function, we would have to wrap this return value in an object in order to return all the info correctly, eg:

Actually, what I was thinking was to moneky patch the timeout to the Timer class similarly to how ZonedDateTime is monkey patched in openhab-js. However, I don't know how easy that is to do with a Java class imported using Java.type. I'm hoping it's the same.

If we can monkey patch then the only immediate changes would need to be in helpers. Then over time I can expose the when though methods added to LoopingTimer et.al. as part of #112. That Issue has grown in my mind to make sure that all the timer classes expose all the usual Timer methods so that the Block library can use one of my timers as if it were a native OH timer.

Code wise I think it would look something like:

const createTimer = (when, func, name, key) => {
  const timeout = time.toZDT(when);
  if (name === null || name === undefined) {
    if (global.ruleUID !== undefined) { // Use UI ruleUID and key if available
      name = 'ui.' + global.ruleUID + ((key !== undefined) ? '.' + key : '');
    } else if (global['javax.script.filename'] !== undefined) { // Use filename and key if available
      name = 'file.' + global['javax.script.filename'].replace(/^.*[\\/]/, '') + ((key !== undefined) ? '.' + key : '');
    }
  }
  //***** Changes go here***
  const timer = actions.ScriptExecution.createTimer(name, timeout, func);
  timer.when = when;
  return timer;
  //****** end changes***
};

Assuming that works, as far as the down stream classes are concerned they are still getting an OH Timer Object and they won't need to change immediately. And TimerMgr won't need to handle storing the when itself and can just pull it as a property.

It might be worth while going upsteam and seeing if openhab-js or even openHAB itself would be willing to implement exposing the when. @florian-h05 WDT?

In the short term, I absolutely will be accepting this PR. I'm just hoping to make it as universally applicable as possible without a lot of rework later. If you aren't willing to go down that path, that's cool and I'll merge and create an issue to remember to come back and make it more generic.

[florian-h05]

I could imagine exposing this upstream in core, which however would create the problem, that we need to convert the Java ZDT to a JS ZDT.
Unfortunately we should not wrap the Java timer in a JS class, otherwise we will run into multi threading problems.

[rakkur]

@rkoshak - Sure, I wasn't sure how deep into the framework you imagined this going. Just a note, if it is feasible to implement in either openhab-js or even OH, you're actually passing when as timeout:

const timeout = time.toZDT(when);
to
actions.ScriptExecution.createTimer(name, timeout, func);

So no need to set the when property in the post processing of the Timer object?

Let me know if you go down the more deeply nested route, otherwise I can wrap it in a local object for use in these tools if you foresee any use for this approach there. It's been handy so far in my scripts!

[rkoshak]

~~@florian-h05 Do you think core would be amenable to adding this to the Java class in the first place? It'd be a pretty small change over all with minimal impacts down stream? The further up stream this can be implemented I think the more who would benefit and the easier it would be to adopt. ~~ It's already there. :-) Though it's almost certainly a Java ZDT. How does openhab-js usually handle that sort of thing if wrapping the class isn't really an option?

So no need to set the when property in the post processing of the Timer object?

The Timer class doesn't expose it. So even though we pass it to create the object, it's not made available to us in the object. But now that I look at the code, there is a getExecutionTime() method on the org.openhab.core.model.script.actions.Timer Class already. Hmmmm.

That's not in the docs.

I just tested this and it worked!

So scratch my earlier suggestion. Remove storing when and instead change your function to:

  /**
   * @param {*} key name of the timer
   * @returns {Duration} of time left in the timer function
   */
  getTimerDuration(key) {
    if (key in this.timers) {
      return time.Duration.between(time.toZDT(), time.toZDT(this.timers[key]).getExecutionTime());
    }

    return time.Duration.ofSeconds(0);
  }

For consistency I think "get" makes more sense than "current" and since we are returning a Duration we should say that in the name too. "Length" is a little ambiguous.

I'll add getExecutionTime methods to the Timers when I add all the other missing methods for the Issue I posted above (unless you want to add it now). My intent is to make it so all the Timer classes in this library can mostly be treated like a standard OH timer where possible so the Blockly blocks can do everything needed.

[rakkur]

I love finding undocumented methods! Haha, great, all set on those changes. Thanks @rkoshak!

I also added a commit (7798f3d and e816362) to add this method to the LoopingTimer, is this what you meant?

For gatekeeper, would you expect it to return the currently executed timer's duration? Or don't add it there?

[rkoshak]

For Gatekeeper I think this method is pretty useful too. But I think there it'd be useful to have two methods:

1. getNextDuration: returns how long until the next task will run; if the queue is empty the timer will be null and we should return null
2. getLastDuration: returns how long before the next task added to the queue will run; if the queue is empty this should be null

However, long term I wonder if it makes sense to hold on to a timestamp for when the last task in the queue was executed so we can see when it last ran also.

I'm OK if this makes it into a separate PR so we don't have to delay merging this one. I think Gatekeeper needs a bit more thought.

---

As I've been thinking about it though I really think the methods should return null if there is no timer at all. It should return a negative duration if the timer exists but has already run. That lets us tell the difference between it never having run and the timer having run already.

---

LoopingTimer is complicated. I think we'll need to be sure to document the behavior.

1. if it returns null LoopingTimer hasn't been started yet
2. if it returns a positive duration the next iteration of the loop is scheduled
3. if it returns a negative duration the loop has exited and the duration is how long ago it exited the loop.

There is a brief period where a new timer is being created at the same time this method is called that we need to watch out for.

[rkoshak]

The more I think on it, we really should return null if there is no timer.

[rkoshak]

Remove the !hasTerminated test. If the timer has terminated it means the loop has exited and this will return a negative duration telling us when the loop exited.

[rkoshak]

return null instead of 0 seconds.

[rakkur]

@rkoshak - I was thinking earlier today that null would be more useful; 0 second duration could be misleading. Great feedback, thanks! I added some more documentation, too, based on what you outlined for loopingtimer.

[rakkur]

Also, not sure how you want to handle:

There is a brief period where a new timer is being created at the same time this method is called that we need to watch out for.

I'm not as familiar with loopingtimer as I don't currently use it in my code, let me know what you'd like to see for that?

[rkoshak]

For LoopingTimer, I think we just run with a straight forward implementation and if it causes problems I'll try to come up with something more clever. I don't necessarily want to go out of our way to fix a problem we don't even know is a problem.

But to avoid the problem, unlike in the other classes, we won't be able to rely on timer.getExecutionTime(). Instead we'll have to track the when manually. However, because the when can change each time through the loop, we will need to get it from the initial call to loop as well as from the return of the timer function.

[florian-h05]

The further up stream this can be implemented I think the more who would benefit and the easier it would be to adopt. ~~ It's already there. :-) Though it's almost certainly a Java ZDT. How does openhab-js usually handle that sort of thing if wrapping the class isn't really an option?

Hmm, that's super difficult, I actually don't have an idea how to handle that without wrapping the class ... I think we need to advise the user to use time.toZDT(timer.getExecutionTime()) instead of timer.getExecutionTime().

[rakkur]

Hey @rkoshak - I've lost track of where we are on this PR. Would you like me to take any additional steps before a merge? It sounds like the additional changes proposed are a bit out of scope of this iteration and this change as it currently stands could add a lot of value for people currently. Let me know!

Should I also iterate the project version? Thanks!

[rkoshak]

I thought you were still working this.

The discussion on LoopingTimer can be handled later (please open an issue so it's not forgotten). But the implementation is pretty simple.

1. convert the when data member to a ZDT and store the ZDT in the when property (this.when = time.toZDT(when))
2. in expire save nextRun to this.when before rescheduling the timer
3. in getDuration instead of returning the time until/since the time in the Timer, use the time between now and this.when

This avoids the timing problem most of the time.

I want to get the cancel stuff added before I cut a new version of the library through so it might be a little bit before this makes it to npm.