home-assistant / core

:house_with_garden: Open source home automation that puts local control and privacy first.
https://www.home-assistant.io
Apache License 2.0
70.92k stars 29.58k forks source link

Hounter Douglas Luxaflex Bottom-Up/Top-Down shades cannot update in parallel #122016

Open miracle2k opened 1 month ago

miracle2k commented 1 month ago

The problem

I am using Battery powered Duette shades.

In HomeAssistant, when initiating a change in, say, the top rail, moving it from position 0 to 40, and then while that change is on-going, and before the entity state in HA is refreshed after the move, I ask the bottom rail to move, then the top rail will move back to 0.

This is because any change is communicated to the Hub always as a pair:

http PUT http://192.168.1.25/home/shades/positions\?ids\=23 positions:='{"primary": 0, "secondary": 50, "velocity": 0.0}'

And so the second request will essentially include the old, previous position of the rail affected by the first request.

This can be annoying in the UI, but especially breaks VoiceAssistant integrations, when the request entails changing both rails. Both the PowerView app and the remote do not have this problem.

I would advocate to storing the temporary "future desired" state of the shade in an appropriate location, maybe either on the entity or the coordinator class? I could try to work on this but would need some guidance.

What version of Home Assistant Core has the issue?

core-2024.7.2

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant OS

Integration causing the issue

Hounter Douglas Powerview

Link to integration documentation on our website

https://www.home-assistant.io/integrations/hunterdouglas_powerview/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

home-assistant[bot] commented 1 month ago

Hey there @bdraco, @kingy444, @trullock, mind taking a look at this issue as it has been labeled with an integration (hunterdouglas_powerview) you are listed as a code owner for? Thanks!

Code owner commands Code owners of `hunterdouglas_powerview` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign hunterdouglas_powerview` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

(message by CodeOwnersMention)


hunterdouglas_powerview documentation hunterdouglas_powerview source (message by IssueLinks)

kingy444 commented 1 month ago

Storing the desired/requested state in the coordinator is easy but I believe there was a decision to only take update information from the hub post processing (ie the shade has reported its state back to us) and not make assumptions of where it is.

@bdraco could you provide any opinion or insight?

Setting the move position in the coordinator would only be temporary as the shade's refresh of data would override this anyway? Shades like the top down/ bottom up was the reason for the coordinator to store this data to start with, this is an extension of that

Would just be changing this block I believe (excuse poor formatting, on the phone)


    async def _async_execute_move(self, move: ShadePosition) -> None:
        """Execute a move that can affect multiple positions."""
        _LOGGER.debug("Move request %s: %s", self.name, move)

# Store the requested positions in the coordinator, so subsequent move requests contain the secondary shade positions 

self.data.update_shade_position(self._shade.id, move)

        async with self.coordinator.radio_operation_lock:
            response = await self._shade.move(move)
        _LOGGER.debug("Move response %s: %s", self.name, response)

        # Process the response from the hub (including new positions)
        self.data.update_shade_position(self._shade.id, response)
bdraco commented 1 month ago

Thats probably fine, but we should move the storage of the model state to the library before we do that. Effectively the change would be