Fix blind position reporting

niobos commented 1 year ago

This PR fixes some issues with the Blind implementation:

(minor) correct decoding of the status bits for old VMB2BL modules
rename methods of BlindStatusMessage to better reflect their meaning: state refers to the movement of the blind, not the position. I was confused by this as well when I started working on Velbus.
Determine is_closed() based on position (if available), not on movement.

This should fix #44 and fix #31.

Cereal2nd commented 1 year ago

Thanks a lot for the help, this really appreciated.

Can we rebase this pr? So we do not have the changes from the other pr added in this one.

niobos commented 1 year ago

This PR is still a draft, but I wanted to communicate my work in progress. I intend to add "currently moving up/down" reporting into Home Assistant, and that might need some more changes here. Or do you prefer to already go ahead with this?

Cereal2nd commented 1 year ago

Doe the homeassistant covers have this state? I'm not sure

Cereal2nd commented 1 year ago

It seems there are states for this

https://developers.home-assistant.io/docs/core/entity/cover#platform-properties-to-be-implemented-by-deriving-platform-classes

niobos commented 1 year ago

Doe the homeassistant covers have this state? I'm not sure

Yes, in Home Assistant you can use is_opening and is_closing to indicate the cover/blind is currently moving. It then shows an icon with an arrow in the UI.

Cereal2nd commented 1 year ago

we can never do this all at once we need to follow 3 steps: 1- make changes to velbusaio and make a new release 2- upgrade home-assistant dependency to the release created in 1 3- add new feature to the cover platform in the velbus component

niobos commented 1 year ago

we can never do this all at once we need to follow 3 steps: 1- make changes to velbusaio and make a new release 2- upgrade home-assistant dependency to the release created in 1 3- add new feature to the cover platform in the velbus component

Sorry, I wasn't being clear. What I meant was: do you want a single PR on velbusaio to include both the position reporting (currently in this PR) and the movement reporting (still to do)? Or should I finish up this PR as is, and create a new PR for velbusaio with the changes needed for movement reporting?

Cereal2nd commented 1 year ago

you can both in this one

niobos commented 1 year ago

This PR is ready for review & merge. I have 2 follow up questions:

Do you have experience with PRs towards Home Assistant? Can you provide some guidance there?
Where can I find more information on the pre-commit checks and automatic validation you have set up here?

Cereal2nd commented 1 year ago

Yes I usually do this for Hass, certainly the new version. So to get this into home-aseistant we first need to release a new version and then do a dependency upgrade pull request. One that PR is merged, we can open a new PR with the code changes in the cover platform to start using the new functionality.

For the pre-commit checks have a look at .pre-commit.yaml file in the root

niobos commented 1 year ago

If you don't mind, I'd like to do the PRs to Hass myself, but I may shout for help. I'll await the release of velbus-aio first.

KevChief commented 1 year ago

As far as I can see , the new code makes total sense like this. I’m not an expert though on this syntax so I can’t judge on correctness. I’m glad though we are making progress and people with better development skills than me are actively looking into this 🙏

KevChief commented 1 year ago

@Cereal2nd , @niobos : do we have news on this fix for cover positions to release or test? I'm not sure whether this went dead from last post or did this get in production, or?

Thanks a lot!

Cereal2nd commented 1 year ago

This will be included in 2023.2 I just missed the 1023.1 deadline...

KevChief commented 1 year ago

Perfect; I’ll make sure to try to test it out when I see the release. Thanks!

Cereal2nd / velbus-aio

Fix blind position reporting #46