Skip to content

Switch velbus from python-velbus to velbusaio#54032

Merged
MartinHjelmare merged 35 commits intohome-assistant:devfrom
cereal2nd:velbusaio
Sep 13, 2021
Merged

Switch velbus from python-velbus to velbusaio#54032
MartinHjelmare merged 35 commits intohome-assistant:devfrom
cereal2nd:velbusaio

Conversation

@cereal2nd
Copy link
Copy Markdown
Contributor

@cereal2nd cereal2nd commented Aug 5, 2021

Breaking change

  • The configuration.yaml config option is deprecated
  • The velbus services call will now need an interface parameter, to allow support for multiple velbus connections.

Proposed change

Switch the velbus component from python-velbus to velbusaio lib. Both modules are compatible and support the same features. velbusaio is just an asyncio implementation for python-velbus.
https://github.com/Cereal2nd/velbus-aio

The velbusaio lib has has the following advantages

  • asyncio
  • uses a json protocol description generated from the velbus protocol desciption. This will result in more supported modules and les maintenance
  • add caching, the velbus scan can take up to 10 minuts, this is verry long and until its finished HASS does not know the velbus entities. The velbusaio has a caching mechanisme, so the startup only takes about 20 seconds.
  • fix the power sensors so they are useable in the energy dashboard
  • Deprecate the platform setup via the config file
  • Fix some errors in the velbus packet parsing (made the component break in some cases)

Additional information

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

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][dev-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][docs-repository]

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

  • The [manifest file][manifest-docs] 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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following [Integration Quality Scale][quality-scale]:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

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

@brefra
Copy link
Copy Markdown
Contributor

brefra commented Aug 5, 2021

That's Looks great!
I'll give it a try asap and let you know the result.

@cereal2nd
Copy link
Copy Markdown
Contributor Author

That's Looks great!
I'll give it a try asap and let you know the result.

i'm sure there will be items that ar not correctly working yet, thats why its a WIP pull request :)

@cereal2nd cereal2nd marked this pull request as ready for review August 11, 2021 08:44
@cereal2nd
Copy link
Copy Markdown
Contributor Author

THIS PR is now ready, can someone have a look at give some suggestions on how to improve it?

@MartinHjelmare
Copy link
Copy Markdown
Member

Please add a link to the new library on github in the PR description.

@cereal2nd
Copy link
Copy Markdown
Contributor Author

Link added, the only thing that needs to be done is the service change.
i reallyw ant this to be in this PR

@cereal2nd
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare any idea why the python 3.9 runs fail and the other runs pass?

@ludeeus
Copy link
Copy Markdown
Member

ludeeus commented Aug 16, 2021

Both 3.8 and 3.9 tails on tests/components/velbus/test_config_flow.py::test_user_fail

@cereal2nd
Copy link
Copy Markdown
Contributor Author

Both 3.8 and 3.9 tails on tests/components/velbus/test_config_flow.py::test_user_fail

Thanks, looked at the results wrongly. Fixed in the meantime ....

@cereal2nd cereal2nd marked this pull request as draft August 17, 2021 14:13
@cereal2nd
Copy link
Copy Markdown
Contributor Author

cereal2nd commented Aug 17, 2021

Converting to draft because @brefra fond some potential problems with the new lib and we want to investigate before merging

@cereal2nd
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare one point that i'm not sure about:

  • During the intial loading of the component (when finishing the config flow) the async_setup_entry will take a long time, how should we handle this correctly?

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Aug 20, 2021

  • During the intial loading of the component (when finishing the config flow) the async_setup_entry will take a long time, how should we handle this correctly?

What is taking a long time, do you know?

@cereal2nd
Copy link
Copy Markdown
Contributor Author

  • During the intial loading of the component (when finishing the config flow) the async_setup_entry will take a long time, how should we handle this correctly?

What is taking a long time, do you know?

The actually reading of the bus. To read all the module data we need to scan the bus and send a lot of messages to the bus. Depending on the number of modules this can take a long time. For example in my install this takes 5 minute.

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Aug 20, 2021

We could create a non tracked asyncio task that finishes the connect and discovery and have a callback that forwards the entry to the platforms on demand and adds the discovered devices as entities. We could wrap the controller connect method inside a coroutine function that triggers the callback when the connect and discovery is done.

Something like zwave_js does:

if platform not in platform_setup_tasks:
platform_setup_tasks[platform] = hass.async_create_task(
hass.config_entries.async_forward_entry_setup(entry, platform)
)
await platform_setup_tasks[platform]
LOGGER.debug("Discovered entity: %s", disc_info)
async_dispatcher_send(
hass, f"{DOMAIN}_{entry.entry_id}_add_{platform}", disc_info
)

The last part with the dispatch helper signal is only needed if we need to add entities dynamically, ie more than once.

@MartinHjelmare
Copy link
Copy Markdown
Member

Are we ready for a final review? The PR is still draft.

@cereal2nd
Copy link
Copy Markdown
Contributor Author

cereal2nd commented Sep 6, 2021

Are we ready for a final review? The PR is still draft.

I'm waiting for @brefra to releas one new version of the lib, and then i need to bump the version here.

But thats it nothing else will change

@cereal2nd
Copy link
Copy Markdown
Contributor Author

@MartinHjelmare by when does this need to be merged to be included in the 2021.10 release?

@MartinHjelmare
Copy link
Copy Markdown
Member

The beta cut is September 29. You can see the dev calendar here:
https://developers.home-assistant.io/

@cereal2nd cereal2nd marked this pull request as ready for review September 7, 2021 19:43
Co-authored-by: brefra <frank_van_breugel@hotmail.com>
Copy link
Copy Markdown
Contributor

@brefra brefra left a comment

Choose a reason for hiding this comment

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

No issues discovered while running this PR locally on my system.

@cereal2nd
Copy link
Copy Markdown
Contributor Author

cereal2nd commented Sep 9, 2021

@MartinHjelmare everything is ready
would love to have this merged before the beta cut, as we have some people waiting for it

@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Sep 10, 2021

Please add a sentence about the deprecation of configuration.yaml section, in the PR description breaking change paragraph.

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@MartinHjelmare
Copy link
Copy Markdown
Member

Please add a link to a docs PR where we document the change to the services.

@cereal2nd
Copy link
Copy Markdown
Contributor Author

Please add a link to a docs PR where we document the change to the services.

Done

@MartinHjelmare MartinHjelmare changed the title Switch the velbus component from python-velbus to velbusaio Switch velbus from python-velbus to velbusaio Sep 13, 2021
@MartinHjelmare MartinHjelmare merged commit 7472fb2 into home-assistant:dev Sep 13, 2021
@cereal2nd cereal2nd deleted the velbusaio branch September 13, 2021 06:55
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants