Add Bitvis Power Hub integration - #165457
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
Bot
added
cla-needed
has-tests
new-integration
integration: bitvis
labels
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
80148db to
2b9760f
Compare
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
home-assistant
Bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
There was a problem hiding this comment.
Pull request overview
Adds a new Home Assistant core integration for the Bitvis Power Hub, exposing power/energy and diagnostic sensors via a local UDP push data path, with config flow + zeroconf discovery and accompanying tests.
Changes:
- Introduces the
bitvisintegration (config flow, UDP coordinator/protocol, sensors, diagnostics, translations, manifest, quality scale). - Adds a full test suite for config flow, coordinator/protocol, sensors, and diagnostics.
- Wires the integration into HA metadata/infra (requirements, generated discovery/config-flow registries, strict typing, codeowners).
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/components/bitvis/test_sensor.py | Validates sensor entity creation, availability, values, and device info. |
| tests/components/bitvis/test_init.py | Tests setup/unload behavior and retry on startup failure. |
| tests/components/bitvis/test_diagnostics.py | Tests diagnostics payload structure with/without device diagnostics. |
| tests/components/bitvis/test_coordinator.py | Exercises UDP protocol parsing/dispatch + coordinator watchdog and data setters. |
| tests/components/bitvis/test_config_flow.py | Covers user + zeroconf config flow paths, errors, and duplicates. |
| tests/components/bitvis/conftest.py | Provides integration setup fixtures and a default mock config entry. |
| tests/components/bitvis/init.py | Marks the Bitvis tests package. |
| requirements_test_all.txt | Adds bitvis-protobuf for tests. |
| requirements_all.txt | Adds bitvis-protobuf runtime dependency. |
| mypy.ini | Enables strict-ish mypy settings for the new integration package. |
| homeassistant/generated/zeroconf.py | Registers _powerhub._udp.local. zeroconf type to bitvis. |
| homeassistant/generated/integrations.json | Adds bitvis integration metadata (hub/local_push/config_flow). |
| homeassistant/generated/config_flows.py | Registers bitvis as having a config flow. |
| homeassistant/components/bitvis/strings.json | Adds config flow strings + entity translation strings. |
| homeassistant/components/bitvis/sensor.py | Defines measurement + diagnostic sensors backed by the coordinator. |
| homeassistant/components/bitvis/quality_scale.yaml | Declares integration quality scale status (targeting silver). |
| homeassistant/components/bitvis/manifest.json | Declares integration manifest, requirements, and zeroconf type. |
| homeassistant/components/bitvis/diagnostics.py | Adds diagnostics endpoint support for the integration. |
| homeassistant/components/bitvis/coordinator.py | Implements UDP listener/protocol + watchdog for push updates. |
| homeassistant/components/bitvis/const.py | Adds integration constants (domain, defaults, watchdog interval). |
| homeassistant/components/bitvis/config_flow.py | Implements user + zeroconf config flow and UDP port binding test. |
| homeassistant/components/bitvis/init.py | Sets up/unloads the config entry and starts/stops the coordinator. |
| CODEOWNERS | Assigns owners for the integration and its tests. |
| .strict-typing | Adds the integration to the strict-typing list. |
727ea53 to
37a72c5
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new bitvis integration to Home Assistant Core for the Bitvis Power Hub, including UDP push handling, config flow + zeroconf discovery, sensors, and diagnostics, along with generated metadata and a full test suite.
Changes:
- Introduces the Bitvis integration (config flow, coordinator/UDP protocol, sensors, diagnostics, translations, manifest, quality scale).
- Registers zeroconf discovery and generated integration metadata.
- Adds comprehensive tests and updates requirements/typing configuration for the new domain.
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/bitvis/init.py | Integration setup/unload wiring using a coordinator |
| homeassistant/components/bitvis/config_flow.py | User + zeroconf config flow with a UDP port bind check |
| homeassistant/components/bitvis/coordinator.py | UDP listener/protocol, coordinator storage, watchdog for availability |
| homeassistant/components/bitvis/sensor.py | Sample + diagnostic sensors backed by the coordinator |
| homeassistant/components/bitvis/diagnostics.py | Diagnostics output with redaction |
| homeassistant/components/bitvis/strings.json | UI strings for config flow + entity translations |
| homeassistant/components/bitvis/manifest.json | Declares domain metadata, requirements, zeroconf |
| homeassistant/components/bitvis/quality_scale.yaml | Integration quality scale checklist |
| homeassistant/generated/{config_flows.py,integrations.json,zeroconf.py} | Generated registration for config flows/integration/zeroconf |
| tests/components/bitvis/* | New test suite + fixtures for the integration |
| requirements_all.txt / requirements_test_all.txt | Adds bitvis-protobuf==0.1.2 |
| mypy.ini / .strict-typing | Enables stricter typing checks for bitvis |
| CODEOWNERS | Adds code ownership entries for the new integration |
There was a problem hiding this comment.
Pull request overview
Adds a new Home Assistant integration for the Bitvis Power Hub (local UDP push), including config flow + Zeroconf discovery, sensors, diagnostics, and a full test suite.
Changes:
- Introduces the
bitvisintegration (manifest, config flow, coordinator/UDP protocol, sensors, diagnostics, translations). - Adds comprehensive tests for config flow, coordinator behavior, sensors, init/unload, and diagnostics.
- Wires the integration into generated registries, strict typing/mypy config, CODEOWNERS, and requirements.
Reviewed changes
Copilot reviewed 21 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/bitvis/__init__.py |
Sets up/unloads the config entry and forwards platforms. |
homeassistant/components/bitvis/config_flow.py |
User + Zeroconf config flow with UDP port bind pre-check. |
homeassistant/components/bitvis/const.py |
Integration constants (domain, defaults, watchdog interval). |
homeassistant/components/bitvis/coordinator.py |
UDP listener/protocol, data storage, watchdog for staleness. |
homeassistant/components/bitvis/sensor.py |
Sample + diagnostic sensor entities and entity descriptions. |
homeassistant/components/bitvis/diagnostics.py |
Diagnostics payload generation + redaction. |
homeassistant/components/bitvis/manifest.json |
Declares integration metadata, requirements, zeroconf type. |
homeassistant/components/bitvis/strings.json |
UI strings for the config flow and sensors. |
homeassistant/components/bitvis/quality_scale.yaml |
Quality scale self-assessment checklist. |
tests/components/bitvis/conftest.py |
Common fixtures and integration setup helper. |
tests/components/bitvis/test_config_flow.py |
Config flow coverage for user + zeroconf paths. |
tests/components/bitvis/test_coordinator.py |
Coordinator/protocol unit tests + watchdog behavior. |
tests/components/bitvis/test_diagnostics.py |
Diagnostics output + redaction assertions. |
tests/components/bitvis/test_init.py |
Setup/unload behavior and retry on startup failure. |
tests/components/bitvis/test_sensor.py |
Ensures expected entities and state behavior with/without data. |
tests/components/bitvis/__init__.py |
Test package marker. |
homeassistant/generated/config_flows.py |
Registers bitvis as a config-flow integration. |
homeassistant/generated/integrations.json |
Adds integration metadata to generated registry. |
homeassistant/generated/zeroconf.py |
Maps _powerhub._udp.local. to the bitvis domain. |
requirements_all.txt |
Adds runtime dependency bitvis-protobuf==0.1.2. |
requirements_test_all.txt |
Adds test dependency bitvis-protobuf==0.1.2. |
mypy.ini |
Enables strict mypy settings for homeassistant.components.bitvis.*. |
.strict-typing |
Marks homeassistant.components.bitvis.* as strict-typing. |
CODEOWNERS |
Adds codeowners for the integration and its tests. |
| domain_data = hass.data.setdefault(DOMAIN, {}) | ||
| if "listener_registry" not in domain_data: | ||
| domain_data["listener_registry"] = BitvisListenerRegistry() | ||
| registry: BitvisListenerRegistry = domain_data["listener_registry"] |
There was a problem hiding this comment.
- We don't have to put the data in hass.data in a dict, we can just store the listener directly in
hass.data[DOMAIN] - Can we use a HassKey to make
hass.data[DOMAIN]typed?
There was a problem hiding this comment.
Sure thing, that makes sense 👍
| """Coordinator to manage data updates from UDP packets.""" | ||
|
|
||
| def __init__( | ||
| self, hass: HomeAssistant, config_entry: ConfigEntry, host: str, port: int |
There was a problem hiding this comment.
can we use the extended config entry
| async def _async_resolve_host(self) -> set[str]: | ||
| """Resolve the configured host to a set of IP addresses. | ||
|
|
||
| Only IP addresses are registered with the shared listener. If the host is | ||
| not already an IP literal and cannot be resolved, treat this as a setup | ||
| error so Home Assistant can retry the config entry later. | ||
| """ | ||
| ips: set[str] = set() | ||
|
|
||
| # If the configured host is already an IP address, always include it. | ||
| try: | ||
| ipaddress.ip_address(self.host) | ||
| except ValueError: | ||
| pass | ||
| else: | ||
| ips.add(self.host) | ||
|
|
||
| loop = asyncio.get_running_loop() | ||
| try: | ||
| addrinfo = await loop.getaddrinfo(self.host, None) | ||
| except OSError: | ||
| _LOGGER.debug("Could not resolve host %s to IP addresses", self.host) | ||
| else: | ||
| for *_, sockaddr in addrinfo: | ||
| ips.add(sockaddr[0]) | ||
|
|
||
| if not ips: | ||
| raise UpdateFailed( | ||
| f"Could not resolve Bitvis Power Hub host {self.host!r} to an IP address" | ||
| ) | ||
|
|
||
| return ips |
There was a problem hiding this comment.
I think this is something for the library instead
| ) from err | ||
|
|
||
| self._watchdog_task = asyncio.create_task(self._async_watchdog()) | ||
| _LOGGER.info( |
There was a problem hiding this comment.
we don't have to log this on info imo
| if self.coordinator.data.sample is None: | ||
| return None |
There was a problem hiding this comment.
| if self.coordinator.data.sample is None: | |
| return None |
If this was the case, the entity would be unavailable
There was a problem hiding this comment.
You're correct, but mypy doesn't understand this and won't lets me pass CI without the check.
There was a problem hiding this comment.
you can always add if TYPE_CHECKING: with an assert that it's not None
There was a problem hiding this comment.
That's a nice way to do it 👍
| return DeviceInfo( | ||
| identifiers={(DOMAIN, device_identifier)}, | ||
| connections={(CONNECTION_NETWORK_MAC, mac_address)} if mac_address else set(), | ||
| name=title, |
There was a problem hiding this comment.
the entry title is the default for every device
| return ( | ||
| super().available | ||
| and self.coordinator.data.sample is not None | ||
| and self.entity_description.exists_fn(self.coordinator.data.sample.sample) |
There was a problem hiding this comment.
so we're now checking for if an data point exists on runtime, but we still create the entity, is that correct? If we don't get a certain data type, shouldn't we avoid creating that entity at all?
There was a problem hiding this comment.
You are correct. There are a few things that complicate the "create-only-when-available" idea:
- We don't know what entities will be available when we create the device.
- Some meters don't send all of their data points in every message.
This handling didn't reflect the latter case too well either. I have removed the exist_fn entirely. From what I can tell, is should make handling of "sparse" data points better.
| """Initialize the sensor.""" | ||
| super().__init__(coordinator) | ||
| self.entity_description = description | ||
| self._device_identifier = entry.unique_id or entry.entry_id |
There was a problem hiding this comment.
It should never be, removed the fallback and added an assertion instead.
| CoordinatorEntity[BitvisDataUpdateCoordinator], SensorEntity | ||
| ): | ||
| """Representation of a Bitvis diagnostic sensor.""" | ||
|
|
||
| entity_description: BitvisDiagnosticSensorEntityDescription | ||
| _attr_has_entity_name = True |
There was a problem hiding this comment.
Considered creating a base sensor?
|
@joostlek I think I have addressed all of you active comments, please have a look 👍 I also found that boot time was calculated incorrectly, so that has also been fixed. |
| Skips the check when HA already owns a shared listener on this port (a | ||
| second device on the same port is valid — it will share the existing socket). | ||
| Raises OSError if the port is unavailable (e.g. already in use by another | ||
| process) or invalid. | ||
| """ | ||
| if async_get_listener_registry(hass).has_listener(port): | ||
| return | ||
|
|
||
| loop = asyncio.get_running_loop() | ||
| transports: list[asyncio.DatagramTransport] = [] | ||
| bind_errors: list[Exception] = [] | ||
| for family, local_addr in ( | ||
| (socket.AF_INET6, ("::", port)), | ||
| (socket.AF_INET, ("0.0.0.0", port)), | ||
| ): | ||
| try: | ||
| transport, _ = await loop.create_datagram_endpoint( | ||
| asyncio.DatagramProtocol, | ||
| local_addr=local_addr, | ||
| family=family, | ||
| ) | ||
| except (OSError, ValueError) as err: | ||
| bind_errors.append(err) | ||
| else: | ||
| assert isinstance(transport, asyncio.DatagramTransport) | ||
| transports.append(transport) | ||
|
|
||
| if not transports: | ||
| raise OSError("UDP port is unavailable or invalid") from bind_errors[0] | ||
|
|
||
| for transport in transports: | ||
| transport.close() |
There was a problem hiding this comment.
I think
loop = asyncio.get_running_loop()
transports: list[asyncio.DatagramTransport] = []
bind_errors: list[Exception] = []
for family, local_addr in (
(socket.AF_INET6, ("::", port)),
(socket.AF_INET, ("0.0.0.0", port)),
):
try:
transport, _ = await loop.create_datagram_endpoint(
asyncio.DatagramProtocol,
local_addr=local_addr,
family=family,
)Is device specific
| ) | ||
| resolved_host = host | ||
|
|
||
| await self.async_set_unique_id(format_unique_id(resolved_host, port)) |
There was a problem hiding this comment.
this almost looks like you're putting a host and port together as unique id, but that is not acceptable as a unique id
There was a problem hiding this comment.
You're right, you left a comment on this in an earlier review but I would like some input on what's the most reasonable way forwards:
The device does broadcast its MAC address as part of its diagnostics so I could use that instead. However, diagnostics are only published it hourly so waiting for it at this point isn't really feasible. Any other ideas? From what I can tell, entry_id might be a sensible option in this case.
There was a problem hiding this comment.
Basically that's also up to you, to us but also yourself. What does the Bitvis exposes that can be considered unique? An HomeID? ClientID? DeviceID? InstallationID? Think something along that way. :)
There was a problem hiding this comment.
As mentioned, we have a MAC address in the diagnostics message. At the point of adding the device we're unfortunately going in a bit blind as there is no way of polling the device for information.
I suppose it would be possible to use it. But in that case I would first add the device with some generic ID and then somehow migrate it once I receive the MAC? Other options would include serial numbers transmitted by the meter the device is connected to, but we run into the same issue with information not being available during adding the device. In addition, energy meters are not exactly consistent in what information they transmit, some may not send anything at all. So MAC is probably my best bet.
Thoughts?
There was a problem hiding this comment.
So may I ask why we don't have the information yet? Like I don't mind having logic in the config flow that goes further than just checking credentials.
The idea behind this check is that whenever someone has this set up, that they can't set up the same device twice. And if we use a temporary generic ID, then we can't compare an entry that was migrated to that ID to the generic ID
There was a problem hiding this comment.
We only require an IP address and port from the user when adding the device, so we won't have more to go on without further introspection. The device itself knows very little about the facility and mostly acts as a data pipe to a backend. I have a few ideas on how we could get more info, maybe the two of you can chime in? Here are my thoughts:
- Add MAC to telemetry data
- Meaning it is sent every 10 seconds rather than hourly. Is waiting in this function acceptable?
- Query the MAC
- For example using the
getmaclibrary - Should be fairly quick and easy, this is the option I'm leaning towards
- For example using the
- Lean into the non-uniqueness
- Any normal installation would only ever have one of these devices, so having a predictable (essentially hard-coded) unique ID might be favorable as it would prevent adding multiple devices.
- From what I can tell, this is not how the unique ID is intended to be used and it artificially limits multi-device installations so I don't like it...
Thoughts?
There was a problem hiding this comment.
@joostlek This should be addressed now. Instead of using the IP/port combo I look up the MAC address of the device and use that to create the unique ID. Please have a look.
| { | ||
| vol.Required(CONF_HOST): cv.string, | ||
| vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port, | ||
| vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string, |
There was a problem hiding this comment.
the name should not be asked as a parameter, only helpers are allowed to ask for a user defined name
| self.data = BitvisData() | ||
| self.last_update_success = False | ||
| self.async_update_listeners() | ||
|
|
||
| async def _async_update_data(self) -> BitvisData: | ||
| """Return current data (updates are push-based via UDP datagrams).""" | ||
| return self.data |
There was a problem hiding this comment.
The device pushes data every 1-10 seconds (depending on the energy meter its connected to). The idea with the watchdog was to catch cases where, for whatever reason, data is no longer received and make sure the data is marked as unavailable.
| if self.coordinator.data.sample is None: | ||
| return None |
There was a problem hiding this comment.
you can always add if TYPE_CHECKING: with an assert that it's not None
There was a problem hiding this comment.
Yes, we should keep diagnostics for a later PR to keep the initial PR smaller
|
@joostlek @erwindouna checking in here. I've updated the PR and I believe all active threads should be resolved, with the exception of the ongoing UUID discussion that is awaiting your feedback. Please have a look, we are eager to keep this moving :) |
| if unload_ok := await hass.config_entries.async_unload_platforms(entry, _PLATFORMS): | ||
| await entry.runtime_data.async_stop() | ||
| return unload_ok |
There was a problem hiding this comment.
| if unload_ok := await hass.config_entries.async_unload_platforms(entry, _PLATFORMS): | |
| await entry.runtime_data.async_stop() | |
| return unload_ok | |
| return await hass.config_entries.async_unload_platforms(entry, _PLATFORMS) |
| class BitvisConfigFlow(ConfigFlow, domain=DOMAIN): | ||
| """Handle a config flow for Bitvis Power Hub.""" | ||
|
|
||
| VERSION = 1 |
There was a problem hiding this comment.
| VERSION = 1 |
It will default to 1, for a new integration. Can be left out for now, until there are migrations in place. :)
|
|
||
| Skips the check when HA already owns a shared listener on this port (a | ||
| second device on the same port is valid — it will share the existing socket). | ||
| Raises OSError if the port is unavailable (e.g. already in use by another | ||
| process) or invalid. |
There was a problem hiding this comment.
| Skips the check when HA already owns a shared listener on this port (a | |
| second device on the same port is valid — it will share the existing socket). | |
| Raises OSError if the port is unavailable (e.g. already in use by another | |
| process) or invalid. |
| ) | ||
|
|
||
| assert result["type"] == FlowResultType.FORM | ||
| assert result["errors"] == {"base": "cannot_connect"} |
There was a problem hiding this comment.
We should also test that we can recover from failure. So basically when a failure happened, restore it and demonstrate we can successfully create a new entry.
There was a problem hiding this comment.
That makes sense, added tests for this and the zeroconf path.
There was a problem hiding this comment.
Missing these for the general form, still. We need to be thorough. :)
| ) | ||
|
|
||
| assert result["type"] == FlowResultType.ABORT | ||
| assert result["reason"] == "cannot_connect" |
| """Test that confirming a zeroconf discovery creates a config entry.""" | ||
| result = await hass.config_entries.flow.async_init( | ||
| DOMAIN, | ||
| context={"source": config_entries.SOURCE_ZEROCONF}, |
There was a problem hiding this comment.
| context={"source": config_entries.SOURCE_ZEROCONF}, | |
| context={"source": SOURCE_ZEROCONF}, |
| async def test_user_form_cannot_connect(hass: HomeAssistant) -> None: | ||
| """Test that a port binding failure surfaces a cannot_connect error.""" | ||
| result = await hass.config_entries.flow.async_init( | ||
| DOMAIN, context={"source": config_entries.SOURCE_USER} |
There was a problem hiding this comment.
| DOMAIN, context={"source": config_entries.SOURCE_USER} | |
| DOMAIN, context={"source": SOURCE_USER} |
| async def test_user_form_create_entry(hass: HomeAssistant) -> None: | ||
| """Test creating an entry via user flow.""" | ||
| result = await hass.config_entries.flow.async_init( | ||
| DOMAIN, context={"source": config_entries.SOURCE_USER} |
There was a problem hiding this comment.
Use the direct constants, please.
| DOMAIN, context={"source": config_entries.SOURCE_USER} | |
| DOMAIN, context={"source": SOURCE_USER} |
| return dt_util.utcnow().replace(microsecond=0) - timedelta(seconds=uptime_s) | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
Maybe expand this with kw_only and frozen, if possible?
There was a problem hiding this comment.
I can easily do kw_only, but frozen is currently a bit difficult since the data is updated throughout the file.
|
|
||
| self.async_update_listeners() | ||
|
|
||
| async def _async_watchdog(self) -> None: |
There was a problem hiding this comment.
What's the rationale of this and why not rely on what the data coordinator returns? Feel a bit double from what the data update coordinator responsibility is supposed to do, unless I completely misunderstood your intention here. :)
There was a problem hiding this comment.
Since the integration is push-based, with data arriving asynchronously. Data may suddenly stop arriving (the device is rebooting, lost Wi-Fi connection, etc.) so the idea with the watchdog is to detect these cases and mark the data as unavailable until it starts appearing again.
There was a problem hiding this comment.
Then why is the polling not disabled, according to: https://developers.home-assistant.io/docs/integration_fetching_data/#push-vs-poll? :)
There was a problem hiding this comment.
I genuinely have mixed feelings of building a dedicated watchdog for this, although I understand what you're saying. Maybe another reviewer has a different opinion.
There was a problem hiding this comment.
The push/poll error would be rookie error 😅 Quite a bit to get into with all the HA classes and helpers. I think I got it right now, please have a look.
I'm not opposed to removing the watchdog, maybe its a bit overkill and strange. Having the data just go static is fine by me.
|
|
||
| self.async_update_listeners() | ||
|
|
||
| async def _async_watchdog(self) -> None: |
There was a problem hiding this comment.
Then why is the polling not disabled, according to: https://developers.home-assistant.io/docs/integration_fetching_data/#push-vs-poll? :)
|
|
||
| self.async_update_listeners() | ||
|
|
||
| async def _async_watchdog(self) -> None: |
There was a problem hiding this comment.
I genuinely have mixed feelings of building a dedicated watchdog for this, although I understand what you're saying. Maybe another reviewer has a different opinion.
|
|
||
|
|
||
| @pytest.mark.usefixtures("init_integration") | ||
| async def test_sensor_platform_creates_all_entities( |
There was a problem hiding this comment.
Please look into how to use the snapshot platforms, for instance as it can be seen in the Portainer integration. This will drastically save you loads of assert writing and compact the test for a platform.
There was a problem hiding this comment.
Oh, that's really nice. Good point!
| ) | ||
|
|
||
| assert result["type"] == FlowResultType.FORM | ||
| assert result["errors"] == {"base": "cannot_connect"} |
There was a problem hiding this comment.
Missing these for the general form, still. We need to be thorough. :)
| assert result["reason"] == "already_configured" | ||
|
|
||
|
|
||
| async def test_zeroconf_shows_confirm_form(hass: HomeAssistant) -> None: |
There was a problem hiding this comment.
I doubt a bit of the effectiveness of this test, it's fine to test the confirm, but maybe merge it in test that will run through the flow successfully.
| assert result["reason"] == "cannot_connect" | ||
|
|
||
|
|
||
| async def test_zeroconf_confirm_cannot_connect_recovery(hass: HomeAssistant) -> None: |
There was a problem hiding this comment.
Please use the cannot_connect as the base. Now we have two tests, one stopped the desired test flow and one going through the desire test flow. :)
There was a problem hiding this comment.
I think I got what you meant, simplified this (and the user_form one) so that each test does both the normal and recovery path through parametrization.
|
@erwindouna I believe all of your comments are addressed now, please have a look. I tore out the watchdog we were discussing in its entirety as I am more concerned with getting this integrated than having it in place. :) |
Check requirementsChecked at commit
📦 bitvis-protobuf: 0.2.6
|
Check requirementsChecked at commit
📦 bitvis-protobuf: 0.2.6
|
Check requirementsChecked at commit
📦 bitvis-protobuf: 0.2.7
|
|
@joostlek @erwindouna I hope the two of you have had a pleasant summer, I brought this PR up-to-date with the latest dev branch all your comments should have been addressed. Please have a look :) |
| data_schema = vol.Schema( | ||
| { | ||
| vol.Required(CONF_HOST): cv.string, | ||
| vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port, |
There was a problem hiding this comment.
Not on the device itself, it will always broadcast on the same port. I left the option here to allow instances where a user has set up port forwarding between networks and the port may not correspond to what the device uses. A niche case, perhaps. I am fine with removing it if you think it should go.
| async def _async_get_device_unique_id(hass: HomeAssistant, host: str) -> str: | ||
| """Resolve *host* and look up a MAC address for the config entry unique ID.""" | ||
| return await hass.async_add_executor_job(get_mac_address_for_host, host) |
There was a problem hiding this comment.
We now use getmac for fetching the mac address. We should not use getmac. Getmac historically has caused issues with storing the wrong mac address and return the mac address of a switch in between. So we should use a different thing for unique id, and if there's not, we should use self._async_abort_entries_match
There was a problem hiding this comment.
Ooh, that's interesting! I removed it and went with _async_abort_entries_match since we don't really have anything to go with.
Integration bitvis:
- [ERROR] [CONFIG_FLOW] Config flows that are discoverable need to set a unique ID
hassfest wouldn't allow me to leave the ID empty, so I set it to the domain for now. That's probably not ideal but seeing as these devices are usually connected to a home energy meter, a user is unlikely to have more than one device in their installation.
| BitvisSensorEntityDescription( | ||
| key="phase_voltage_l1", | ||
| translation_key="phase_voltage_l1", | ||
| device_class=SensorDeviceClass.VOLTAGE, | ||
| native_unit_of_measurement=UnitOfElectricPotential.VOLT, | ||
| state_class=SensorStateClass.MEASUREMENT, | ||
| suggested_display_precision=1, | ||
| value_fn=lambda data: ( | ||
| data.phase_voltage_l1_v if data.HasField("phase_voltage_l1_v") else None | ||
| ), | ||
| ), | ||
| BitvisSensorEntityDescription( | ||
| key="phase_voltage_l2", | ||
| translation_key="phase_voltage_l2", | ||
| device_class=SensorDeviceClass.VOLTAGE, | ||
| native_unit_of_measurement=UnitOfElectricPotential.VOLT, | ||
| state_class=SensorStateClass.MEASUREMENT, | ||
| suggested_display_precision=1, | ||
| value_fn=lambda data: ( | ||
| data.phase_voltage_l2_v if data.HasField("phase_voltage_l2_v") else None | ||
| ), | ||
| ), | ||
| BitvisSensorEntityDescription( | ||
| key="phase_voltage_l3", | ||
| translation_key="phase_voltage_l3", | ||
| device_class=SensorDeviceClass.VOLTAGE, | ||
| native_unit_of_measurement=UnitOfElectricPotential.VOLT, | ||
| state_class=SensorStateClass.MEASUREMENT, | ||
| suggested_display_precision=1, |
There was a problem hiding this comment.
you can use translation placeholders to avoid having 3 seperate translation keys
There was a problem hiding this comment.
Right, that's clever! I could also use it for a lot of the other keys so the translations file is a bit slimmer now.
|
|
||
| UPTIME_DESCRIPTION = SensorEntityDescription( | ||
| key="uptime", | ||
| translation_key="uptime", |
There was a problem hiding this comment.
I think you can omit the translation key here as it will use the device class transaltion
| if self.coordinator.data.diagnostic is None: | ||
| return None |
There was a problem hiding this comment.
this is also in available and when available is False, native_value won't be queried, so you can remove this check as it will never be False
There was a problem hiding this comment.
Right, that makes sense!
Check requirementsChecked at commit
📦 bitvis-protobuf: 0.2.7
|
Check requirementsChecked at commit All requirements checks passed. ✅
📦 bitvis-protobuf: 0.2.7
|
Check requirementsChecked at commit All requirements checks passed. ✅
📦 bitvis-protobuf: 0.2.7
|
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Feel free to send me a message on Discord btw
| host = discovery_info.host | ||
|
|
||
| await self.async_set_unique_id(_SINGLE_INSTANCE_UNIQUE_ID) | ||
| self._abort_if_unique_id_configured() |
There was a problem hiding this comment.
So instead of setting this, you can set "single_config_entry": true in the manifest. Is the reason we can't have multiple entries because of UDP?
| BitvisSensorEntityDescription( | ||
| key="power_active_l1_delivered_by_client", | ||
| translation_key="power_active_phase", | ||
| translation_placeholders={"direction": "export", "phase": "L1"}, |
There was a problem hiding this comment.
Okay so 2 things:
- I believe different countries have different ways to describe phases other than L, so let's make only the number a placeholder
- Now export is hardcoded, so we can't translate that anymore, while its something we want to translate
| suggested_display_precision=2, | ||
| value_fn=lambda data: ( | ||
| data.energy_active_delivered_by_client_kwh | ||
| if data.HasField("energy_active_delivered_by_client_kwh") |
There was a problem hiding this comment.
Do all devices have all fields? As in, are there devices that just have a subset of fields?
|
@joostlek I'm about to head out but I'll send you a ping on discord after the weekend, I appreciate you taking the time to keep on reviewing this! |
Breaking change
Proposed change
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: