core icon indicating copy to clipboard operation
core copied to clipboard

Shelly cover position update when moving

Open flonou opened this issue 10 months ago • 14 comments

Proposed change

This change forces Shelly cover integration to regularly update the cover position while it is moving.

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [ ] New integration (thank you!)
  • [x] 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:
  • Link to developer documentation pull request:
  • Link to frontend pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] I have followed the perfect PR recommendations
  • [x] The code has been formatted using Ruff (ruff format 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.

To help with the load of incoming pull requests:

flonou avatar Feb 21 '25 18:02 flonou

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Feb 21 '25 18:02 home-assistant[bot]

Hey there @balloob, @bieniu, @thecode, @chemelli74, @bdraco, mind taking a look at this pull request as it has been labeled with an integration (shelly) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of shelly 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 shelly Removes the current integration label and assignees on the pull request, 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 pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Feb 21 '25 18:02 home-assistant[bot]

While the idea is nice and I would personally love to have a status update for covers while they are moving, I have asked Shelly developers in the past why they don't send status updates while moving and they explained that this put too much pressure on the device. I don't like adding more to it by constantly polling the device. I will ask Shelly developers again if this may be changed in the device as it would be the correct solution.

thecode avatar Feb 21 '25 18:02 thecode

I did try to contact them through their forum here : https://community.shelly.cloud/topic/2554-shelly-plus2pm-doesnt-send-its-position-to-mqtt-or-rpc-while-moving/ Note that the polling only occurs once per second, I believe it won't be too much work for the device ?

flonou avatar Feb 21 '25 19:02 flonou

I was wondering if maybe newer devices (mine is the Plus2PM) would be more capable of frequent position update ? Anyway thanks for asking the shelly devs, if they can do it on their side, this PR would not be useful anymore but that would make more sense to me

flonou avatar Feb 21 '25 19:02 flonou

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[bot] avatar Apr 22 '25 21:04 github-actions[bot]

@thecode Hi, sorry for the ping, do you have any news from the shelly developpers ?

flonou avatar Apr 23 '25 07:04 flonou

@thecode Hi, sorry for the ping, do you have any news from the shelly developpers ?

Shelly replied that they can't send the status constantly as currently any status update from the device is sent to all channels which will spam their cloud.

They are working on a way to separate update channels but no timeline for this.

thecode avatar Apr 23 '25 08:04 thecode

Oh I see, my use case doesn't rely on the servers at all but I can understand that it represent a lot of additional data for them

flonou avatar Apr 23 '25 16:04 flonou

I think since Shelly team doesn't know when they will implement another solution we can continue with this PR.

Tests are needed.

bieniu avatar Apr 24 '25 05:04 bieniu

I think since Shelly team doesn't know when they will implement another solution we can continue with this PR.

Tests are needed.

I updated the PR according to the latest commit on dev. What kind of tests would you expect? I have to admit I am not used to python testing so any help would be welcome

flonou avatar May 09 '25 12:05 flonou

What kind of tests would you expect?

We need a test that proves the new code works. You can extend this test https://github.com/home-assistant/core/blob/d0fe7de501620a5cd5c2e8b134adb097b7f6287b/tests/components/shelly/test_cover.py#L112 by changing the position when closing or opening, like here for slat_pos https://github.com/home-assistant/core/blob/d0fe7de501620a5cd5c2e8b134adb097b7f6287b/tests/components/shelly/test_cover.py#L259

bieniu avatar May 11 '25 11:05 bieniu

What kind of tests would you expect?

We need a test that proves the new code works. You can extend this test

https://github.com/home-assistant/core/blob/d0fe7de501620a5cd5c2e8b134adb097b7f6287b/tests/components/shelly/test_cover.py#L112 by changing the position when closing or opening, like here for slat_pos

https://github.com/home-assistant/core/blob/d0fe7de501620a5cd5c2e8b134adb097b7f6287b/tests/components/shelly/test_cover.py#L259

I ended up creating separate tests. Coverage seems ok with the new code.

flonou avatar May 11 '25 15:05 flonou

Please mark PR as "Ready for review" if ready.

bieniu avatar May 12 '25 07:05 bieniu

Any chance will be merged soon?

jarzebski avatar Jul 17 '25 18:07 jarzebski

Sorry for the copilot noise, I wasn't expected it to show on the PR directly when asking it to review my code on visual studio code. That said it had a valid point in adding more information to the update task.

Tests should be good now !

flonou avatar Aug 31 '25 00:08 flonou