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

[OmniLink] Fix daylight savings when setting date/time #12546

Merged
merged 9 commits into from
Apr 18, 2022

Conversation

ecdye
Copy link
Contributor

@ecdye ecdye commented Mar 30, 2022

Signed-off-by: Ethan Dye [email protected]

@ecdye ecdye added the bug An unexpected problem or unintended behavior of an add-on label Mar 30, 2022
@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/omnilink-binding-controller-thing-missing-system-date-channel-on-oh-3-3-0-m2/134278/16

@ecdye
Copy link
Contributor Author

ecdye commented Mar 30, 2022

Assuming that this goes through before the 3.3 release a breaking change notice is already present for this channel due to unrelated changes made previously see #12246.

@lolodomo
Copy link
Contributor

Why a date should be stored in a string item rather than a date item? I don't understand the logic.
Is there an issue describing the problem you are trying to resolve?

@ecdye
Copy link
Contributor Author

ecdye commented Mar 30, 2022

Because in order to allow calculation of DST we need to preserve the location data but DateTimeType does not do that which causes the DST flag to always be false. This was brought up in the forum discussion that was auto posted here above.

@lsiepel
Copy link
Contributor

lsiepel commented Mar 30, 2022

Because in order to allow calculation of DST we need to preserve the location data but DateTimeType does not do that which causes the DST flag to always be false. This was brought up in the forum discussion that was auto posted here above.

The problem is that the DateTimeType lacks timezone information. As a solution DateTimeType is switched to a StringType item so one can add the timezone and convert that string to a ZonedDateTime, right? Not very user friendly and intuitive.

Not sure how feasible this is, but you might look into another scenario: Get the openhab Time Zone Settings from the i18n services. But not sure if that is always available to the binding.

I also wonder how other bindings solve this problem.

@jlaur
Copy link
Contributor

jlaur commented Mar 30, 2022

The problem is that the DateTimeType lacks timezone information.

DateTimeType internally stores a ZonedDateTime? See also https://www.openhab.org/javadoc/latest/org/openhab/core/library/types/datetimetype.

@lsiepel
Copy link
Contributor

lsiepel commented Mar 30, 2022

Sorry, i meant not only timezone. It is the combination timezone / locale that is needed to determine if DST is active or not.

@ecdye
Copy link
Contributor Author

ecdye commented Mar 31, 2022

Correct, time zone and locale are required for DST calculation. However, DateTimeType drops the locale information when converting to/from ZonedDateTime.

I forgot to update the rules documentation to describe how to send the command with the changes but will do that in the morning.

@jlaur
Copy link
Contributor

jlaur commented Mar 31, 2022

Correct, time zone and locale are required for DST calculation. However, DateTimeType drops the locale information when converting to/from ZonedDateTime.

IMHO ZonedDateTime has even too much information for a DateTimeType. :) A UTC timestamp would have been sufficient for recording an exact unambiguous moment in time. This could then be used with the openHAB/system timezone (including DST) at any moment to produce a local datetime or to calculate if timestamp was generated in DST or not. This will also be resilient to any changes in the openHAB/system timezone after timestamp was generated.

I still don't understand the need, so as also requested by @lolodomo - maybe a description of the exact problem being solved would help?

@jlekhter
Copy link

jlekhter commented Apr 1, 2022

Hi,

The need for the request is the following. The Ommi system requires you to identify if your current location is in DST. In openHAB, DateTimeType stores the Date and Time in ISO8601 format with just a simple timezone offset. This is not sufficient for determining whether you are in DST or not. For instance "-0700" is in DST if you are in California, but it is not in DST if you are in Arizona. You need a location to determine DST. Using the DateTimeType in openHAB doesnt give you the location.

@jlaur @lolodomo What exactly are you recommending so that we can be unambiguous about the location of the UTC offset?

@@ -492,7 +493,7 @@ private void getSystemStatus() throws IOException, OmniNotConnectedException, Om
.append(String.format("%02d", status.getHour())).append(":")
.append(String.format("%02d", status.getMinute())).append(":")
.append(String.format("%02d", status.getSecond())).toString();
updateState(CHANNEL_SYSTEM_DATE, new DateTimeType(dateString));
updateState(CHANNEL_SYSTEM_DATE, new StringType(dateString));
Copy link
Contributor

@jlaur jlaur Apr 1, 2022

Choose a reason for hiding this comment

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

Leaving DateTimeType/StringType discussion aside for a moment, I wonder if something like this would have been a better approach:

        updateState(CHANNEL_SYSTEM_DATE,
                new DateTimeType(
                        ZonedDateTime.ofLocal(
                                LocalDateTime.of(status.getYear() + 2000, status.getMonth(), status.getDay(),
                                        status.getHour(), status.getMinute(), status.getSecond()),
                                ZoneId.systemDefault(), null)));

Also, I can see that SystemStatus returned by the controller contains daylightSavings, but unfortunately that seems useless without also knowing the time zone. Do you need time zone/DST information from the controller, or simply from the openHAB system, so you can assume that system time returned by the controller is local time in openHAB's timezone?

The example is for simplicity using system time zone. Using TimeZoneProvider it's possible to get the time zone configured in openHAB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, but I actually didn't write that code in the first place. I migrated the existing code and got it up to spec for inclusion in the openHAB addons proper repo and never really changed it. At this point it doesn't make a to much sense to have it be a date time type because of the issues discussed above but that has yet to be resolved, so for the moment I plan to leave it.

Comment on lines 183 to 184
ZonedDateTime zdt = ZonedDateTime.parse(((StringType) command).toString(),
DateTimeFormatter.ISO_ZONED_DATE_TIME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding: This seems like logic used for setting the system time on the controller in local time with provided DST flag - correct? Is this needed to be called twice per year in order to set correct DST flag, or what is the actual need? I assume the controller cannot automatically adjust since it's not aware of the time zone it's in?

As to the actual change, is the difference that using ISO_ZONED_DATE_TIME you will be able to provide something like [Europe/Paris] to exactly get DST right?

Would it be good enough to deduct time zone and DST flag from system or openHAB configuration?

@jlaur
Copy link
Contributor

jlaur commented Apr 1, 2022

The Ommi system requires you to identify if your current location is in DST.

When setting the date/time on the controller, or for what purpose?

In openHAB, DateTimeType stores the Date and Time in ISO8601 format with just a simple timezone offset. This is not sufficient for determining whether you are in DST or not. For instance "-0700" is in DST if you are in California, but it is not in DST if you are in Arizona. You need a location to determine DST. Using the DateTimeType in openHAB doesnt give you the location.

Thanks for clarifying this part. I think I'm beginning to understand the issue, but I'm still not entirely there. :-) I guess I still don't understand why and when the Omni system needs the DST flag.

@jlaur @lolodomo What exactly are you recommending so that we can be unambiguous about the location of the UTC offset?

No recommendation yet, but getting there. Please see my code comments - for now raises more questions than answered, but hopefully that will turn around soon. Is this problem inconsistency in the controller wanting DST, but not time zone? What can the controller do when having this information? Is the controller configured with local time (and DST on/off)? How do you experience the current problem as a user - what exactly happens?

@jlekhter
Copy link

jlekhter commented Apr 1, 2022

We are writing to the controller to set its time. The Omni controllers are notoriously bad a keeping a good clock. My clock drifts by 2-3min per day. Therefore at 1am nightly, I set the controller time through openHAB. The set time API to the controller requires you to let the controller know if the user is currently in DST. It has nothing to do with setting DST or ST, it simply part of the API to the controller

Doing this boolean inDaylightSavings = ZoneId.systemDefault().getRules().isDaylightSavings(zdt.toInstant()); fixes the issue too. However @ecdye was concerned that using the systemDefault zoneId broke the API contract with the user. If the user passed in their own zoneId for whatever reason, the code would ignore it, hence the string.

Is this problem inconsistency in the controller wanting DST, but not time zone?

Its not a problem with the controller not wanting time zone, the controller can figure out the correct time if you tell it whether or not you are actively in DST while setting the time.

Is the controller configured with local time (and DST on/off)?

Yes. Thats exactly how you set the time on the controller

What can the controller do when having this information?

The controller figures out what time Sunset, Sunrise, etc is based on the DST flag and your Lat/Long.

How do you experience the current problem as a user - what exactly happens?

Because the DST flag isnt set when you set the controller time, all of the controller rules happen an hour too early. For instance, I lower a blackout shade when it gets dark via the controller. Its lowered an hour earlier. I turn on my landscape lights when it get dark -- they turn on too early. I water my lawn at dawn, it gets watered earlier than when I want. Etc.

I could use openHAB rules for all of that, but the controller is really reliable (except for the time drift) and I lose network more frequently than I lose the controller.

Let me know if I can answer any other questions.

Thanks,

Jerry

@ecdye
Copy link
Contributor Author

ecdye commented Apr 1, 2022

Everything that @jlekhter said is correct, sorry about the silence on my part, I have been extra busy these last two days. Let me know if there are any further questions.

@jlaur
Copy link
Contributor

jlaur commented Apr 2, 2022

Thanks for this additional explanation, I think I finally understand the details at play now.

We are writing to the controller to set its time. The Omni controllers are notoriously bad a keeping a good clock. My clock drifts by 2-3min per day. Therefore at 1am nightly, I set the controller time through openHAB. The set time API to the controller requires you to let the controller know if the user is currently in DST. It has nothing to do with setting DST or ST, it simply part of the API to the controller

Will you need to manually flip the DST flag twice per year when switching between summer time and normal time, or will the controller automatically do that when you are not interfering in any way by using the API to set the clock?

If it can do that automatically, all you would need to do is read the DST flag from the controller and apply this again when setting the new time. That should fix the issue, but it wouldn't give you full control, i.e. the ability to flip the flag by openHAB - will be addressed in next comment.

How do you experience the current problem as a user - what exactly happens?

Because the DST flag isnt set when you set the controller time, all of the controller rules happen an hour too early. For instance, I lower a blackout shade when it gets dark via the controller. Its lowered an hour earlier. I turn on my landscape lights when it get dark -- they turn on too early. I water my lawn at dawn, it gets watered earlier than when I want. Etc.

I could use openHAB rules for all of that, but the controller is really reliable (except for the time drift) and I lose network more frequently than I lose the controller.

Completely understandable. I have the same situation with my shades having automations defined in a Hunter Douglas PowerView Hub. I have winter and summer automations depending on sunrise time (and DST actually), so recently added ability from openHAB to just flip the automations four times per year instead of letting openHAB actually be in charge of them: #11516

Let me know if I can answer any other questions.

Thanks for the insight. I now have some propositions:

  • If you don't need manual control over DST, you could perhaps simply fetch and reapply the DST flag when setting the time?
  • If you need manual control, I would propose to keep the DateTime, but add a dedicated Switch channel for reading/writing the DST flag.
  • Perhaps it could be nice to add a thing action which would simply synchronize the controller with the openHAB time and DST flag. This could be called from a rule without any parameters, so would be really simple to use.

@jlekhter
Copy link

jlekhter commented Apr 2, 2022

The controller should flip the flag when DST occurs. Its not something that I've tested, but the controller does have settings for changing to Daylight Savings. The DST flag could be read using a REQUEST SYSTEM STATUS command to the controller. So that's an option.

Having a separate switch channel would only store data in the binding, not really update the controller. The DST flag cannot be set without setting the controller date and time. It seems more complicated than necessary to use 2 channels to update the date/time of the controller.

My favorite option is the Thing action which would synch the controller time with openHAB time. But that would leave the current channel broken.

I think to fix the current channel, @ecdye's idea of using a String to store the ZonedDateTime value is a good idea. We could always add Thing action later. That's just my opinion though :)

@jlaur
Copy link
Contributor

jlaur commented Apr 3, 2022

The controller should flip the flag when DST occurs. Its not something that I've tested, but the controller does have settings for changing to Daylight Savings. The DST flag could be read using a REQUEST SYSTEM STATUS command to the controller. So that's an option.
[...]
I think to fix the current channel, @ecdye's idea of using a String to store the ZonedDateTime value is a good idea. We could always add Thing action later. That's just my opinion though :)

Why is it better than keeping the DateTime and simply reapplying the current DST flag? The String solution seems like a work-around that could easily be avoided this way:

  • Read system time from controller (with DST flag).
  • Set system time from command's DateTime with DST flag received from controller.
  • Time is now set and DST flag is preserved.

Anything I'm missing?

Having a separate switch channel would only store data in the binding, not really update the controller. The DST flag cannot be set without setting the controller date and time. It seems more complicated than necessary to use 2 channels to update the date/time of the controller.

That channel would only be needed if there is any need/requirement to be able to change the controller's DST flag from the binding. It could work the same way as described above:

  • Read system time from controller.
  • Set system time from command's DST flag with time received from controller.
  • DST flag is now set and system time is preserved.

But if changing DST is not needed, there is no reason to introduce this channel.

My favorite option is the Thing action which would synch the controller time with openHAB time. But that would leave the current channel broken.

We could have both. :-)

@ecdye
Copy link
Contributor Author

ecdye commented Apr 3, 2022

I don't think that it actually does flip it automatically on all models. I can't test but at least in my testing of the models I have only the OmniPro II controller did, the LTE mode didn't.

EDIT:

And besides that, what if the user wants to update the daylight savings setting because for some reason it is wrong? Then your approach won't work.

@jlaur
Copy link
Contributor

jlaur commented Apr 3, 2022

And besides that, what if the user wants to update the daylight savings setting because for some reason it is wrong? Then your approach won't work.

It will when introducing an additional Switch channel for managing the DST flag: #12546 (comment)

@ecdye
Copy link
Contributor Author

ecdye commented Apr 3, 2022

Once again that leaves the issue of in order to change the DST flag on the controller you have to update the time too. I personally think that separating the channels in this case is more complicated and less user friendly. I would prefer to stick with the string approach.

What would be most ideal is if DateTimeType could be fixed to properly preserve the locale information so at some point in the future we could move back to it. Also given the user base of the binding this change is not going to have a large impact as to may knowledge there are only a handful of users who use the binding.

@jlekhter
Copy link

jlekhter commented Apr 3, 2022

Can we also consider the use cases being solved? There is no use case in the HAI controller for flipping the DST flag. The only use case we're considering is to set the time of the controller properly. The controller requires local date/time + DST flag. As @ecdye mentioned, the real fix is to fix DateTimeType. Everything else is a work around. The StringType is a good workaround for now. This allows people to update the time of a remote controller and a local controller through openHAB, which is really the only use case.

@jlaur
Copy link
Contributor

jlaur commented Apr 3, 2022

Once again that leaves the issue of in order to change the DST flag on the controller you have to update the time too.

No, that was addressed here: #12546 (comment)

I personally think that separating the channels in this case is more complicated and less user friendly. I would prefer to stick with the string approach.

Well, IMHO is actually simplifies the logic when separating time and DST flag:

  • System time is contained in the appropriate type (DateTime) like you will see in all other bindings.
  • When you need to flip the DST flag, you simply update the DST channel (without having to consider time).
  • When you need to set the system time, you simply update the time (without having to consider DST).

With the String approach:

  • You need to learn about ISO_ZONED_DATE_TIME for providing something like "2011-12-03T10:15:30+01:00[Europe/Paris]" to set the time/DST.
  • When setting the time, you also have to set DST.
  • When setting DST, you also have to set time.

What would be most ideal is if DateTimeType could be fixed to properly preserve the locale information

I disagree here. DateTimeType should actually internally store an Instant instead of a ZonedDateTime. At any moment this Instant could be converted to ZonedDateTime or LocalDateTime using TimeZoneProvider and/or LocationProvider.

@jlaur
Copy link
Contributor

jlaur commented Apr 3, 2022

Can we also consider the use cases being solved? There is no use case in the HAI controller for flipping the DST flag. The only use case we're considering is to set the time of the controller properly.

If that's the only use case, we are back to: #12546 (comment) without need for a separate channel. So DateTime can be preserved at the same time as solving the issue? @ecdye however brought up this need: #12546 (comment)

@ecdye
Copy link
Contributor Author

ecdye commented Apr 11, 2022

@jlaur I've updated the code to address all of your review comments.

@ecdye
Copy link
Contributor Author

ecdye commented Apr 15, 2022

bump

@jlaur
Copy link
Contributor

jlaur commented Apr 15, 2022

@jlaur I've updated the code to address all of your review comments.

I'm currenty on Easter holiday, will be able to resume review within the next couple of days.

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! Only a few additional minor comments.

Signed-off-by: Ethan Dye <[email protected]>
@ecdye
Copy link
Contributor Author

ecdye commented Apr 18, 2022

@jlaur Concerns addressed, I think we are ready for merge now.

@jlaur
Copy link
Contributor

jlaur commented Apr 18, 2022

@jlaur Concerns addressed, I think we are ready for merge now.

Let's get it merged then - thanks!

Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur merged commit b6fa985 into openhab:main Apr 18, 2022
@jlaur jlaur added this to the 3.3 milestone Apr 18, 2022
@jlaur
Copy link
Contributor

jlaur commented Apr 18, 2022

@ecdye - now it would be nice if you could create separate PR for correcting this line in the upgrade notes:
https://github.com/openhab/openhab-distro/blob/c4a1d03455cff53712d07ef0c86588097f02f1b7/distributions/openhab/src/main/resources/bin/update.lst#L67

@jlekhter
Copy link

@ecdye sorry, for the late testing. It looks like the javascript version of this fix is working fine, but the DSL Rules portion is not working.

I get the following error when I run DSL rules:
2022-04-24 13:38:19.731 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'omnilink-2' failed: 'synchronizeControllerTime' is not a member of 'org.openhab.core.thing.binding.ThingActions'; line 26, column 9, length 60 in omnilink

It seems like the DSL interpreter cant find the action.

@ecdye
Copy link
Contributor Author

ecdye commented Apr 25, 2022

@jlekhter Did you delete and re-add your controller after updating the binding?

@jlekhter
Copy link

@ecdye. Yes I did. I can try doing it again when I get home, but I deleted the controller several times during the upgrade to make sure.

Using JavaScript works, but if I use a rules file or a script from within OpenHAB I get the same error as above.

I never restarted OpenHAB. I could try restarting openhab too later today.

@jlekhter
Copy link

I deleted the controller, re-added it, restarted openHAB and still the same problem with DSL rules. Here is the error from within the script interface in openHAB

2022-04-25 14:27:10.252 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test_setting_time' failed: val omniActions = getActions("omnilink", "omnilink:controller:home")
omniActions.synchronizeControllerTime("America/Los_Angeles")
      
   The method synchronizeControllerTime(String) is undefined for the type ThingActions; line 2, column 81, length 25

@jlaur
Copy link
Contributor

jlaur commented Apr 25, 2022

I deleted the controller, re-added it, restarted openHAB and still the same problem with DSL rules. Here is the error from within the script interface in openHAB

2022-04-25 14:27:10.252 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'test_setting_time' failed: val omniActions = getActions("omnilink", "omnilink:controller:home")
omniActions.synchronizeControllerTime("America/Los_Angeles")
      
   The method synchronizeControllerTime(String) is undefined for the type ThingActions; line 2, column 81, length 25

Here is the problem:

public static void synchronizeSystemTime(ThingActions actions, @Nullable String zone) {
((OmnilinkActions) actions).synchronizeControllerTime(zone);

@jlekhter
Copy link

jlekhter commented Apr 25, 2022

Yes that works. Changing the DSL rule to synchronizeSystemTime works. Javascript still wants synchronizeControllerTime as the method.

The methods should be consistent across both rule sets.

As an aside, is there a way to see what Thing Actions are available in openHAB? VSCode doesn't seem to complete possible options (at least for DSL).

@jlaur
Copy link
Contributor

jlaur commented Apr 25, 2022

The methods should be consistent across both rule sets.

Created #12655 for that.

@ecdye
Copy link
Contributor Author

ecdye commented Apr 26, 2022

Ahh, silly me. Got to love those mistakes in code that are hard to find. Thanks for fixing that @jlaur.

wborn pushed a commit to openhab/openhab-distro that referenced this pull request Apr 26, 2022
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Apr 27, 2022
* Fix daylight savings when setting date/time
* Use an action to set date/time
* Add default time zone detection and i18n

Signed-off-by: Ethan Dye <[email protected]>
Signed-off-by: Nick Waterton <[email protected]>
@jlekhter
Copy link

jlekhter commented May 2, 2022

Just to finish this off, I retested with the fix and it seems to work with DSL rules now. Thank you. But is this also supposed to work when you do not pass an argument into the action?

If its supposed to work, then it doesnt. I was under the impression that you could write this and it would work:

      val omniActions = getActions("omnilink", "omnilink:controller:home")
      omniActions.synchronizeControllerTime()

But I get a scripting error when the parameter is not there.

@ecdye
Copy link
Contributor Author

ecdye commented May 2, 2022

But I get a scripting error when the parameter is not there.

That is because of the JavaScript, it requires a parameter try omniActions.synchronizeControllerTime("") and it should set it to your default time zone.

@jlekhter
Copy link

jlekhter commented May 2, 2022

Thanks -- an empty string works fine.

All good then, Also as an FYI, this does require an openHab restart. Works like a charm after that.

Thanks for all the work.

Jerry

andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
* Fix daylight savings when setting date/time
* Use an action to set date/time
* Add default time zone detection and i18n

Signed-off-by: Ethan Dye <[email protected]>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
* Fix daylight savings when setting date/time
* Use an action to set date/time
* Add default time zone detection and i18n

Signed-off-by: Ethan Dye <[email protected]>
Signed-off-by: Andras Uhrin <[email protected]>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
* Fix daylight savings when setting date/time
* Use an action to set date/time
* Add default time zone detection and i18n

Signed-off-by: Ethan Dye <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on (potentially) not backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants