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 start and end time attributes #477

Merged
merged 3 commits into from
Feb 11, 2023
Merged

Add start and end time attributes #477

merged 3 commits into from
Feb 11, 2023

Conversation

rianadon
Copy link
Contributor

The PR adds start_time and end_time attributes to washing machines, which indicates what time of day the washer started running & what time it will end.
It is similar to the same attributes used in the OpenSprinler custom integration.

This attribute is helpful for the Timer Bar Card (disclosure: my own card), which uses the start time to display the time the washer is scheduled to run. The card prefers using timestamps rather than a hh:mm offset because the hh:mm format requires the entity to update its state every minute to give a new hh:mm estimate, whereas the start time timestamp will change much less often.

I don't have an LG washer so have not been able to test whether the code is bug free or whether taking the current time + reserve time - initial time + remaining time is the correct way to calculate the start time.

See also: rianadon/timer-bar-card#70

@ollo69
Copy link
Owner

ollo69 commented Jan 25, 2023

Hi @rianadon,

I know your card, really useful and also referred here by read-me (I'm also using your card). But is this PR really required? I do no like so much the idea to add additional attribute (this integration have really lot) to provide duplicated information. There is really a benefit with this?

@rianadon
Copy link
Contributor Author

rianadon commented Jan 30, 2023

Hey @ollo69. I'd ask the same question if I were you, and you're right to be concerned this is adding extra attributes.

I tried to explain my motivation in the previous comment, but I'll explain in more detail. The main motivation behind the PR is that there are multiple ways of specifying a start time for a timer.

One way is to give a timestamp (date + time). This method is used by official Home Assistant components: BMW Connected Drive, Home Connect, and SmartThings as well as custom ones: the OpenSprinkler custom integration and Alexa media player timers. Aside: There still is a lot of variation in these integrations: some place the start time in the attributes, others as a separate entity, and some make the end time the entity's state. But the format of these start/end times are all timestamps.

The second is to give a time offset, such as a time in the hh:mm like your card does, to show how much time is remaining before the timer begins. I haven't seen this method used anywhere else, but I'm receptive to being shown examples.

Ideally, there would be a single standard for specifying start times. Consistent attributes & formats make it easier to port automations & templates from one integration to another. For me personally that would keep the code for my card simple. And all engineers love standards :) Due to the timestamp method's higher adoption and better technical performance (which I discussed in my original comment), I suggest that the standard be using timestamps for the start time.

Yes, that does add two extra attributes to your integration. I certainly would not want to replace the existing remaintime and reservetime attributes in case someone is using them for automations or templates. However, template writers may find the start_time and end_time timestamps to be easier to work with as they can easily be parsed into datetime objects. And using timetamps for start/end times will create more consistency between this integration and the other integrations out there. Hope that helps!

I know your card, really useful and also referred here by read-me (I'm also using your card).

And I appreciate the praise 😄. Thank you for maintaining such a great-quality integration.

@ollo69 ollo69 marked this pull request as draft February 11, 2023 12:09
@ollo69 ollo69 marked this pull request as ready for review February 11, 2023 12:10
@ollo69 ollo69 merged commit 89d82d7 into ollo69:master Feb 11, 2023
ollo69 added a commit that referenced this pull request Feb 11, 2023
@ollo69
Copy link
Owner

ollo69 commented Feb 11, 2023

@rianadon,

This is merged now. I did a little change post merge, using the date from HomeAssistant dt_util package. This set the correct timezone to the date and allow proper conversion in HA UI.

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

Successfully merging this pull request may close these issues.

2 participants