Limit Whirlpool timestamp changes to +/- 60 seconds

[mkmer]

Proposed change

Limit timestamp changes to +/- 60 seconds: Updating timestamp changes for anything less than 60 seconds produces unnecessary state machine updates with no value to the user. For most cycles this reduces the "changes" by 90%.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Deprecation (breaking change to happen in the future)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[home-assistant]

Hey there @abmantis, mind taking a look at this pull request as it has been labeled with an integration (whirlpool) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of whirlpool can trigger bot actions by commenting:

• @home-assistant close Closes the issue.
• @home-assistant rename Awesome new title Change the title of the issue.
• @home-assistant reopen Reopen the issue.
• @home-assistant unassign whirlpool Removes the current integration label and assignees on the issue, add the integration domain after the command.

[elupus]

It looks like something was missed in reviews here. State is not supposed to contain things like remaining time that keeps on changing.

Could you please change this sensor to a completion time instead.

[mkmer]

This timer value is marked as end time for the wash/dry cycle, it updates the end time while the cycle is running (it starts with and estimate which gets better as the load runs).
It's almost a direct copy of the rainmachine integration -

core/homeassistant/components/rainmachine/sensor.py

Line 235 in 38f183a

class TimeRemainingSensor(RainMachineEntity, RestoreSensor):

I'm afraid I don't understand the request.

[elupus]

It should be a timestamp of when the machine ends tte cycle instead of a count down.

[mkmer]

It is a timestamp.
We take the (current time + duration) and set a time stamp. The API updates the remaining time (duration) on it's own with a state change message (every 15 to 30 seconds). If the abs(current_time + duration) > 60 seconds, we update the timestamp (once for each time the API reports > 60 seconds).
What this PR addresses is the 15-30 second message which caused updates to the timestamp <60 seconds - this happens multiple times during a wash cycle and really adds no value to the user - typically the UI only shows "in x minutes".

Maybe I'm still missing your point?

[elupus]

You were not missing my point. I was miss-reading the code. How accurate is the update? Could 30 seconds diff work, could 20?

Im okey with 60. But if we dont need yo be that course it could.be worth being more accurate.

[elupus]

I fail to understand how the added tests verify the 60 second limits on update? Or are they not doing that?

[mkmer]

They are not verifying 60 second update. They are just causing the update to execute because "now" is > than 2022. I suppose I could add a test to verify <60 doesn't update too.

…

On Sat, Jan 7, 2023, 1:52 PM Joakim Plate ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/components/whirlpool/test_sensor.py <#85368 (comment)> : > @@ -90,6 +109,25 @@ async def test_washer_sensor_values( mock_sensor1_api: MagicMock, ): """Test the sensor value callbacks.""" + hass.state = CoreState.not_running + thetimestamp: datetime = datetime(2022, 11, 29, 00, 00, 00, 00, timezone.utc) I fail to understand how the added tests verify the 60 second limits on update? Or are they not doing that? — Reply to this email directly, view it on GitHub <#85368 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3GVBHNS2BN6RLPYNLWP6DWRG3PVANCNFSM6AAAAAATT4ZP2A> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[mkmer]

the estimate changes quite a bit at the beginning, then settles down. I've found 60 seconds to limit unnecessary updates the really don't show on the UI until it's closer to the end of the cycle. Being off by a minute doesn't really affect "knowing when the cycle will finish" from a user perspective. (Wife still has to walk over to the machine :) )

…

On Sat, Jan 7, 2023, 1:46 PM Joakim Plate ***@***.***> wrote: You were not missing my point. I was miss-reading the code. How accurate is the update? Could 30 seconds diff work, could 20? Im okey with 60. But if we dont need yo be that course it could.be worth being more accurate. — Reply to this email directly, view it on GitHub <#85368 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3GVBHXNPFBND5UB7DLQODWRG2YNANCNFSM6AAAAAATT4ZP2A> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[elupus]

Ok for me. A test would be good. I will not merge right away if you want to add a test.

[mkmer]

Added tests. Plan to stick with 60 second threshold as it has the minimum number of updates to HA state machine (only a couple per wash/dry cycle) while continuing to provide similar completion times as the Whirlpool app.