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

Periodically refresh overkiz states #99499

Closed

Conversation

fetzerch
Copy link
Contributor

@fetzerch fetzerch commented Sep 2, 2023

Proposed change

Somfy gates & garage doors will not automatically send state updates. They have to be explicitly requested. The TaHoma app/web interface therefore asks all devices to send their state updates on start.

This change adds a new OverkizDeviceRefreshCoordinator for impacted devices and will periodically (5min) ask the specific device to send state updates.

Note that even though these devices typically also provide commands such as refreshPedestrianPosition/refreshPartialPosition but they do not cause the device to send its actual state to the gateway.

See Somfy-Developer/Somfy-TaHoma-Developer-Mode#26 and iMicknl/ha-tahoma#167.

This PR still requires iMicknl/python-overkiz-api#956 and a version bump of python-overkiz-api.

Note that this is my first contribution to Home Assistant and so far I have been able to test this only with a Somfy Dexxo Smart io garage door motor. It would certainly be helpful to get this tested by others impacted by iMicknl/ha-tahoma#167.

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

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
  • I have followed the perfect PR recommendations
  • 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:

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:

@home-assistant
Copy link

home-assistant bot commented Sep 2, 2023

Hi @fetzerch

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

home-assistant bot commented Sep 2, 2023

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

Code owner commands

Code owners of overkiz can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign overkiz Removes the current integration label and assignees on the pull request, add the integration domain after the command.

Somfy gates & garage doors will not automatically send state updates.
They have to be explicitly requested. The TaHoma app/web interface
therefore asks all devices to send their state updates on start.

This change adds a new OverkizDeviceRefreshCoordinator for impacted
devices and will periodically (5min) ask the specific device to send
state updates.

Note that even though these devices typically also provide commands
such as refreshPedestrianPosition/refreshPartialPosition but they do
not cause the device to send its actual state to the gateway.

See Somfy-Developer/Somfy-TaHoma-Developer-Mode#26
and iMicknl/ha-tahoma#167.
@fetzerch fetzerch marked this pull request as draft September 4, 2023 12:16

async def _async_update_data(self) -> None:
"""Trigger device state refresh on Overkiz platform."""
await self.client.refresh_device_states(self.device_url)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really a use-case where the data update coordinator is required imo, since you don't receive any data here.

See iMicknl/ha-tahoma#271 for my initial try 3 years ago. Code is very outdated, but shows you another approach. You could use async_track_time_interval to call a function every x minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allright, I'll migrate it to async_track_time_interval.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, let me know if you need help! This issue (#103183) looks related.

Comment on lines +64 to +67
OVERKIZ_REFRESH_DEVICE_STATES_DEVICES: list[UIClass | UIWidget] = [
UIClass.GARAGE_DOOR,
UIClass.GATE,
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this more specific? I am not sure if an UIClass (or widget) is specific enough. There are many many Overkiz devices and perhaps we should scope this on the controllable_name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, makes sense.

@TLongstride
Copy link

TLongstride commented Oct 1, 2023

Hi,

I've done some test with the gate and the result is interesting.

The conlusion is that the gate doesn't send is position when you use the remote, BUT, the state is update to Unknown. So we can detect when state need to be update.

I read that @iMicknl dont like idea to refresh the state of all the device every 5 minutes.

So I think we can use it better :

  • The state could be open, closed, pedestrian or unknown
  • Maybe we could send refresh command until state became other than unknown only every 10 seconds for exemple
  • So that the refresh command are send just like it's necessary

I joind the test I have done. Hope it could help

Test Gate Tahoma API.docx

@iMicknl
Copy link
Contributor

iMicknl commented Oct 1, 2023

I read that @iMicknl dont like idea to refresh the state of all the device every 5 minutes.

No? I just mentioned that there are better ways in Home Assistant to call a function every x minutes. DataUpdateCoordinator is not the best mechanism for this.

Thanks for your test document, this could help to investigate other ways as well.

@TLongstride
Copy link

I read that @iMicknl dont like idea to refresh the state of all the device every 5 minutes.

No? I just mentioned that there are better ways in Home Assistant to call a function every x minutes. DataUpdateCoordinator is not the best mechanism for this.

Not in this PR, you wrote on an other topic that is not the best way to do this according somfy, and that it could surchage the server

@fetzerch
Copy link
Contributor Author

fetzerch commented Oct 2, 2023

I've done some test with the gate and the result is interesting.

Thanks for the additional testing! I can double check if the behavior is the same that I get.
Can you help me to understand the sections in your document (my french is a little weak): The first tests you did was with homeassistant (pedestrian, close), then with a keygo(?) io remote and then with the tahoma app? It would also be interesting to hear how it behaves if you fully open the gate.

In general, your tests were all done without this patch, correct?

The conlusion is that the gate doesn't send is position when you use the remote, BUT, the state is update to Unknown. So we can detect when state need to be update.

From your document that is only the case when you close the gate. Setting to pedestrian seemed to work fine. Or am I overlooking something?

I read that @iMicknl dont like idea to refresh the state of all the device every 5 minutes.

I fully agree that polling is bad, but I fear at this point in time, there's no way around. The problem that I'm trying to additionally solve is getting my physically wired buttons to work. And for these I get no updates at all - it's not even going to 'unknown' unfortunately. For reference, I also link my discussion with Somfy here: Somfy-Developer/Somfy-TaHoma-Developer-Mode#26 (comment)

There is btw a difference between the old PR iMicknl/ha-tahoma#271 where all devices are asked for updates, and this PR where only the specific 'bad' devices are polled for state updates.

@TLongstride
Copy link

I had formating my test to be more readeable and did another test.

I don't know why, but yes, the pedrestrian state is pick up by the motor. And the open an closed state are udpate to the unknown state.

I think we couldn't say anymore that the state is note udpate, in fact, it's update but not with the value we want. But, we could do something.

I made a automation that send a stop command when state of select is unknown for 20 seconds. With this I don't have my gate report open hour before automatic update.

If I could have the possibility to call the refresh command you've done in the python tahoma v1.11 directly in home assitant, I could call it instead of stop command. (When the gate is open and closed with the remote, sometimes the stop command stop the gate ... Not the best way to do it...)

Test Gate Tahoma API v2.docx

@TLongstride
Copy link

Hi,

Did you encounter any problems with rejected requests during your tests?

I got data: {'errorCode': 'TOO_MANY_OPERATIONS_IN_PROGRESS', 'error': 'Too many asynchronous jobs for GATEWAY:XXXX-XXXX-XXXX, try again later'}

Any idea ?

@fetzerch
Copy link
Contributor Author

fetzerch commented Oct 9, 2023

I had formating my test to be more readeable and did another test.

Thanks. This is much clearer now, and I can reproduce the exact same behavior.

Please forget the cover entity for the moment. For any other value than 'closed' it will show as 'open'. I think this is caused by this code part but it's unrelated to this PR.

I don't know why, but yes, the pedrestrian state is pick up by the motor. And the open an closed state are udpate to the unknown state.

Same here. If I use the 'partial button' on the 'keygo io remote' it is announced in the API as 'Partial'. When closing it again it's set to 'Unknown'. That looks like a bug to me either in the motors or in the backend.

This leaves us with 2 somewhat independent problems:

  1. We never get a status update when using wired push buttons.
  2. We get an incorrect status when using the keygo io remote.

For (1) I don't see an alternative to periodically polling these devices. (2) would also be solved by polling periodically, but it could indeed be improved by requesting the state once if it's 'unknown' for a some period (like 20seconds).

I got an email that you commented also because you see the same issue with RS100 motors for covers - did you delete the comment again? There seems to be a hybrid variant, that supports io homecontrol and wired buttons. It could surely be that they suffer from the same issue. Question is if we can detect them reliably. Do you have the repsonse to /setup for one of these?

I made a automation that send a stop command when state of select is unknown for 20 seconds. With this I don't have my gate report open hour before automatic update.

If I could have the possibility to call the refresh command you've done in the python tahoma v1.11 directly in home assistant, I could call it instead of stop command. (When the gate is open and closed with the remote, sometimes the stop command stop the gate ... Not the best way to do it...)

@iMicknl I'd like to hear your opinion on it. I can try to implement also (2) or a service, but I perfere to do it in a way that has a chance to be accepted in HA core.

Did you encounter any problems with rejected requests during your tests?
I got data: {'errorCode': 'TOO_MANY_OPERATIONS_IN_PROGRESS', 'error': 'Too many asynchronous jobs for GATEWAY:XXXX-XXXX-XXXX, try again later'}

What exactly did you do to get this? I assume this is just 'normal' rate limiting on the backend side and can happen on extensive testing / usage.

@TLongstride
Copy link

For (1) I don't see an alternative to periodically polling these devices. (2) would also be solved by polling periodically, but it could indeed be improved by requesting the state once if it's 'unknown' for a some period (like 20seconds).

It's look go to me. Maybe 5 second until state is not 'unknown' anymore be better.

I got an email that you commented also because you see the same issue with RS100 motors for covers - did you delete the comment again? There seems to be a hybrid variant, that supports io homecontrol and wired buttons. It could surely be that they suffer from the same issue. Question is if we can detect them reliably. Do you have the repsonse to /setup for one of these?

That's right, go RS100 Io Hybrid with wired button. Don't see anything that could detect them...
response_1696962792973.json

Did you encounter any problems with rejected requests during your tests?
I got data: {'errorCode': 'TOO_MANY_OPERATIONS_IN_PROGRESS', 'error': 'Too many asynchronous jobs for GATEWAY:XXXX-XXXX-XXXX, try again later'}

What exactly did you do to get this? I assume this is just 'normal' rate limiting on the backend side and can happen on extensive testing / usage.

After some reserch, it seems there are a limit of ten command send. But this is another subject.

@nyroDev
Copy link
Contributor

nyroDev commented Nov 8, 2023

I'm coming maybe a little bit late here, but I have an other use case where a "refresh" request should be made.
In my case, it's a water heater.
When setting to performance/boost (in order to heat the water at a specific temperature), the water heater automatically leave the boost mode when the temperature is reached.
Somfy/tahoma state is not updated automatically, thus Home Assistant still show it in performance mode.

On my device, the "refresh" API endpoint doesn't return an error.
I'll test if it does what is expected (updating current operation mode state).
If that's the case, this device might came in the list too.

But:
As far as I understand the code here, it's basically do:
For every devices listed, we will do a state refresh every 5 minutes, no matter what.
In case of the garage door, I understand why it's needed and you don't have the choice.
Bu in my case, the refresh is only needed when the water heater is in performance mode.
All other time, I don't need to refresh the state, it won't change.

Which leads to my question:
Would it be possible to call a function need_state_refresh_call (or similar) on the device to know if the state is really needed at this point?

And maybe we can go a little further by also implementing a function need_state_refresh on the device that will basically say : "Hey, I need to refresh my state on a regular basis, please come to me every 5 minutes to do so"

And finally, if the refresh needs to be done using something else?
My Water heater do have a REFRESH_PASS_APC_HEATING_MODE and REFRESH_PASS_APC_HEATING_PROFILE commands, maybe it will be better than calling the standard refresh API endpoint.
That might also be a good idea (if possible) to let the device do it's own refresh call(s).
If we can call a function on device that implements a specific function, do it every 5 minutes and the device is responsible of everything else (call, do nothing, etc...), and that will be even better than the 2 functions suggested before.

@iMicknl
Copy link
Contributor

iMicknl commented Nov 21, 2023

@fetzerch @nyroDev apologies for my late reply. Hereby my view on this issue:

As shared earlier, I don't think a DataUpdateCoordinator is the right way. This is way too complex, and it only needs an API endpoint to call. Best would be to leverage async_track_time_interval to schedule a x minute refresh, for every device that requires it. During init you can schedule these refresh requests, based on the controllable_name.

State refreshes that are required after a command execution could be added right after the execution in the entity code, if this wouldn't be solved by the scheduled state refresh.

We cannot support all devices in an optimal way, because some device just don't support real-time states.

Would it be possible to call a function need_state_refresh_call (or similar) on the device to know if the state is really needed at this point?

How would this work? For the devices where this feature has been developed for, we don't know when a state update is required. Regarding your other comment that it needs this update every 5 minutes, that could work for these devices, but might complicate the code a lot? Compared a simple task that is scheduled every x minutes.

And finally, if the refresh needs to be done using something else?

This would be good to check. In theory this endpoint should call all these commands automatically, same as the official app does.

Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Jan 20, 2024
@MartinHjelmare MartinHjelmare changed the title overkiz: Periodically refresh states Periodically refresh overkiz states Jan 21, 2024
@github-actions github-actions bot removed the stale label Jan 21, 2024
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Mar 21, 2024
@github-actions github-actions bot closed this Mar 28, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.