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

Add new module DateTimeTrigger for triggering on a DateTime state #2923

Merged
merged 2 commits into from
Apr 24, 2022

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Apr 21, 2022

Closes #2726

Signed-off-by: Jan N. Klug github@klug.nrw

This adds a new module that triggers at the time given by the state of an item.

Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K requested a review from a team as a code owner April 21, 2022 18:01
@wborn wborn added the enhancement An enhancement or new feature of the Core label Apr 22, 2022
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, lgtm. Just one very small comment below.


private final CronScheduler scheduler;
private final String itemName;
private String cronExpression = "@reboot";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This magic string @reboot looks a bit weird to me. How about simply setting it to null, if no cron expression is set yet? Or if you really prefer to set a magic string in order to avoid null values, please at least make it a constant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make it a constant. I used this string because it is properly handled in the CronAdjuster, so probably we should make it a constant there. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't even know that this value was defined in the CronAdjuster. So yeah, it makes a lot of sense to declare it as a constant in there and just use it.

Signed-off-by: Jan N. Klug <github@klug.nrw>
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Flole998
Copy link
Member

I am thinking about extending this so it can handle a "time only match", something like
Time is time of myDateTimeItem
in which case the CronExpression simply needs to be modified to be a s m H * * * * instead so it ignores the date. This will be very helpful when trying to implement rules that run daily at a user-configurable time. This option could be named timeOnly and for DSL Rules there could be a new TimeTrigger that's basically the DateTimeTrigger with that timeOnly option set.

Also the documentation/helptext for this feature can be misunderstood easily: It mentions that this triggers at a time specified in an item. More accurately it should say something like "at a time and date..." and when my idea is added "at a time and optionally a date...".

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/timers-and-datetime/141590/15

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
…enhab#2923)

* Add DateTimeTrigger

This adds a new module that triggers at the time given by the state of an item.

Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: ca94fd5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DateTime as rule trigger
5 participants