-
-
Notifications
You must be signed in to change notification settings - Fork 36.5k
Add BMS BLE integration #159556
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
base: dev
Are you sure you want to change the base?
Add BMS BLE integration #159556
Conversation
| await coordinator.async_config_entry_first_refresh() | ||
|
|
||
| # Insert the coordinator in the global registry | ||
| hass.data.setdefault(DOMAIN, {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| hass.data.setdefault(DOMAIN, {}) |
Is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a left-over from the pre-config-entry-runtime-data-era, https://developers.home-assistant.io/blog/2024/04/30/store-runtime-data-inside-config-entry
fixed in 10425b5
| """Set up BT Battery Management System from a config entry.""" | ||
| LOGGER.debug("Setup of %s", repr(entry)) | ||
|
|
||
| if entry.unique_id is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it needed? When can such entries be created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm not sure. In theory it could happen if there is an issue with Bluetooth and the discovery info is incomplete. I have seen strange things happen also with wrong data via proxies, so I would see it as part of defensive programming. Nevertheless, I have no strong feelings about it if you suggest to remove it.
| return unload_ok | ||
|
|
||
|
|
||
| async def async_migrate_entry( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new integration should not have migrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was unsure about this, due to the fact that there are a lot of users of the HACS integration. Will it cause issues if I remove this migration function? I'm not sure about the switch-over, but other than that, I can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion on Discord, removed. Thanks! Fixed in b7c2e4f
| return True | ||
|
|
||
|
|
||
| def migrate_sensor_entities( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new integration should not have migrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. Thanks! Fixed in b7c2e4f
| ) | ||
|
|
||
| if user_input is not None: | ||
| address: str = str(user_input[CONF_ADDRESS]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should normalize the MAC address because the user may enter the address in the wrong format, e.g., lowercase/uppercase, etc. Generally, it is a good idea to do this in any case when saving data, including when the mac address comes from discovery info. See: #157879
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in b18eff2
Proposed change
This is the PR add support for integrating over 30 different battery management systems (even more battery brands/models). The new integration bms_ble Bluetooth Low Energy (BLE) battery management systems (BMS) that is currently available as HACS integration. The sensors follow a consistent schema, while the BMS themself have a big variance in published data points. I oriented my self on the principles that the NUT integration uses, i.e. consistent data over a wide variety of devices.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: