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

Support for Victron BLE devices #128843

Closed
wants to merge 4 commits into from
Closed

Conversation

rajlaud
Copy link
Contributor

@rajlaud rajlaud commented Oct 20, 2024

Proposed change

This adds support for Victron bluetooth low energy sensors, which provide information about various Victron devices including solar controllers, inverters, and lithium batteries.

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 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:

victron_ble remove unnecessary strings.json

victron_ble tests refactor and additions

Test and typing improvements

Test and typing improvements

Additional tests

Additional tests

Handle malformed advertisement victron_ble

Bluetooth discovery for victron_ble

do not cache victron_ble parser

config flow improvements

Add strings for config flow

Add strings for config flow

Improved config flow strings

Improved config flow strings

More config flow improvements

More config flow improvements

Fix description placeholders

Fix description placeholders

Enable rediscovery of removed devices

Filter out non-instant read advertisements

Logging improvements

Use custom sensor-state-data rather than modifying upstream dependency

Bump victron-ble dependency to 0.6.0

Bump victron-ble dependency to 0.6.0

Revert new device class

Properly handle no current_flow in HA device classes

Implement suggestions from code review

Implement suggestions from code review

Move VictronBluetoothDeviceData to separate library

Fix stale reference to victron-ble

Fix stale reference to victron-ble

Rerun CI with dependency available

Fix name of discovered victron_ble device

Bump victron-ble-ha-parser version

Bump victron-ble-ha-parser version

Fix name of title variable for victron-ble config flow

Bump victron-ble-ha-parser for bug fix

Bump victron-ble-ha-parser for bug fix

Complete DC-DC converter support for victron-ble

Add sensor descriptions for smart battery protect

Add sensor descriptions for individual cells

Fix sensor descriptions for individual cells

Retrieve cell key properly

Types

Types
@rajlaud rajlaud marked this pull request as ready for review October 20, 2024 12:17
@rajlaud
Copy link
Contributor Author

rajlaud commented Oct 20, 2024

Regarding the license check for victron-ble library - it is covered by the "unlicense" https://unlicense.org/ which puts the code in the public domain.

assert address is not None
key = entry.data[CONF_ACCESS_TOKEN]
data = VictronBluetoothDeviceData(key)
coordinator = hass.data.setdefault(DOMAIN, {})[entry.entry_id] = (
Copy link
Member

Choose a reason for hiding this comment

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

Can we use entry.runtime_data to store this data? We can then extend the typing of ConfigEntry to also make that typesafe

homeassistant/components/victron_ble/manifest.json Outdated Show resolved Hide resolved
tests/components/victron_ble/test_config_flow.py Outdated Show resolved Hide resolved
tests/components/victron_ble/test_config_flow.py Outdated Show resolved Hide resolved
"""Test discovery via bluetooth with a valid device."""
result = await hass.config_entries.flow.async_init(
DOMAIN,
context={"source": config_entries.SOURCE_BLUETOOTH},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
context={"source": config_entries.SOURCE_BLUETOOTH},
context={"source": SOURCE_BLUETOOTH},

Copy link
Contributor Author

@rajlaud rajlaud Oct 20, 2024

Choose a reason for hiding this comment

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

Happy to do it this way, but I also had to add from homeassistant.config_entries import SOURCE_BLUETOOTH to the imports despite already having import homeassistant.config_entries. Is this a style thing?

tests/components/victron_ble/test_config_flow.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft October 20, 2024 12:51
@home-assistant
Copy link

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

Learn more about our pull request process.

@joostlek
Copy link
Member

They should provide in the metadata of their library that they use the uni license, there's a trove classifier for that

https://pypi.org/classifiers/

@rajlaud
Copy link
Contributor Author

rajlaud commented Oct 20, 2024

They should provide in the metadata of their library that they use the uni license, there's a trove classifier for that

https://pypi.org/classifiers/

Thanks, I'll open a PR

@rajlaud rajlaud marked this pull request as ready for review October 24, 2024 12:47
@home-assistant home-assistant bot requested a review from joostlek October 24, 2024 12:47
@rajlaud
Copy link
Contributor Author

rajlaud commented Oct 24, 2024

For tracking purposes, here is the link to the PR to reflect the victron-ble license on pypi. keshavdv/victron-ble#65

@frenck
Copy link
Member

frenck commented Nov 9, 2024

I've marked this PR a draft, as we are awaiting an upstreaming licensing fix.
Please un-draft it once it is ready for review again by clicking the "Ready for review" button.

Thanks! 👍

../Frenck

Learn more about our pull request process.

@frenck frenck marked this pull request as draft November 9, 2024 09:26
Copy link

github-actions bot commented Jan 8, 2025

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 8, 2025
@github-actions github-actions bot closed this Jan 15, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Jan 16, 2025
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.

3 participants