Config Flow and Entity registry support for Monoprice#30337
Conversation
probot-home-assistant
Bot
added
integration: monoprice
small-pr
|
I'm planning a few changes to this integration to bring it up to date with HA (looks like it hasn't really been touched much in the past couple of years), but this change can stand on its own and is the least controversial of those changes, so even if the rest seems off, I believe this is a good one.
|
There was a problem hiding this comment.
This doesn't work because one could configure two devices via 2 different serial ports. Can you get a unique ID from the device ?
There was a problem hiding this comment.
The device does not have a unique id. I thought about using the serial port address as the prefix, but:
- The current implementation also doesn't seem to support more than one instance (or at least has confusing functionality with snapshots).
- These devices can chain together if you have 2 or 3, so having two unique devices, each connected via serial, should be very rare.
There was a problem hiding this comment.
And of course that would mean that a change to the serial address would create new entities.
There was a problem hiding this comment.
We need a better solution for Home Assistant to deal with these cases. In it's current form we cannot accept this PR, as it means we might have to break it in the future when it will be possible to get unique IDs per device.
There was a problem hiding this comment.
OK, I'm open to other ideas. Here's my thinking:
Some axioms first:
- The device itself does not have any unique ID
- The zones within it do have a unique ID relative to the device
- If you have several of them, up to 3 devices can be chained together so they look like a single device to HA. A deployment with two distinct instances seems like an extremely rare occasion.
- The existing implementation wasn't designed for multiple instances, but might partially work (I don't really have a way to test it).
Here are my humble suggestions:
- Create some sort of mechanism for "singleton" devices that can only have one instance in HA. Some other components can clearly fit this category (e.g. Sun), and I'm pretty sure there are also a lot of integrations that might not support multiple instances (many integrations in the "Hub" category are likely candidates, for example). This will limit functionality, but ensure no errors are caused as a result of multiple instances in integrations that don't support it. This will require core changes though, and can not be implemented just within this PR.
- Add the serial address as a prefix to the unique ID. As mentioned, this will mean new entities if the address changes (but then again, if this was configured through a config entry, a change in address would likely have caused new entities as well).
- Allow the user to define a prefix per instance. In essence, this means the user is controlling the unique id of each device. This is the simplest and cleanest implementation, I believe, and could be generalized as a pattern.
- Once the configuration switches over to a config entry, store a random value in the config entry as a prefix. As long as it's the same config entry, the entities will stay the same.
What do you think?
|
@balloob gave it more thought, and I really like solution 3 - it also has the benefit of being very explicit (after all, the user cares about the "logical uniqueness" of the device, not its "physical uniqueness", so you can replace the device, and keep the same connections, and entity ids will stay the same, yet if you choose a new namespace, it'll create new entities). I also think I know how to fix snapshots in case of multiple instances - will fix this in a separate PR once this is approved. |
|
I like option 3, but we should first decide on that in the architecture repository. We dont' change architecture rules in PRs to the Python repo. |
|
No problem. I thought the arch repo was just when changes to core are required, but I opened this home-assistant/architecture#333, including a potential core implementation. |
|
Is there no serial number for the device? PySerial supports getting the serial number of a connected device. Did you try this? |
|
@MartinHjelmare from what I understand PySerial can only obtain the serial number for USB devices:
https://pyserial.readthedocs.io/en/latest/tools.html In this case, even if I obtain a serial number, it will most likely represent the USB to RS232 adapter, rather than the actual device. |
|
Does it matter? If the adapter is required it could be seen as part of the device. |
|
It's not required (you can use a COM port, RS232 hat, tunnel over the network with ser2net, etc...), but even if it was, I don't think it's the right approach. It will be like using the serial ID of a USB splitter hub for a USB device - these are cheap adapters and replacing them should not create new entities. |
|
@balloob: Any guidance? |
|
It's possible to have YAML configuration that is imported from the component setup. https://github.com/home-assistant/home-assistant/blob/2e5161997f03554639b8d1f270a73be869950f2e/homeassistant/components/hue/__init__.py#L92-L102 I think that the options flow is the right way to go. We have just updated the demo integration to have an options flow, including the new multi-select. Add |
|
@balloob Thanks, I'll check out the demo flow. |
OnFreund
changed the title
OnFreund
force-pushed
the
monoprice_entity_registry
branch
from
4e7adce to
e3f8876
Compare
|
@balloob I added the config flow and modified the tests. I have one failing test: Not sure how to fix this. |
|
As for the options flow, I'll add that in a future PR. |
|
Make sure to remove the |
OnFreund
changed the title
OnFreund
force-pushed
the
monoprice_entity_registry
branch
from
774fc0d to
552d117
Compare
Codecov Report
@@ Coverage Diff @@
## dev #30337 +/- ##
==========================================
+ Coverage 94.74% 94.76% +0.01%
==========================================
Files 772 776 +4
Lines 55911 56220 +309
==========================================
+ Hits 52975 53278 +303
- Misses 2936 2942 +6
Continue to review full report at Codecov.
|
OnFreund
changed the title
OnFreund
changed the title
|
@balloob any chance you can review this soon? There's a follow up options flow PR, and I'm hoping we can get both together into 0.108. Thanks |
OnFreund
changed the title
|
|
||
| try: | ||
| monoprice = get_monoprice(port) | ||
| monoprice = await hass.async_add_executor_job(get_monoprice, port) |
There was a problem hiding this comment.
If we do this in __init__.py we can raise ConfigEntryNotReady if you get a SerialException so it will automatically retry later.
There was a problem hiding this comment.
Maybe use get_async_monoprice instead?
There was a problem hiding this comment.
@MartinHjelmare that's my goal, and the code changes are relatively straightforward. The reason I'm not doing this yet is that pymonoprice is dependent on pyserial-asyncio for its async implementation, which does not seem to be maintained and is using old-style coroutine syntax. I'm afraid it's not going to work with future python versions.
| monoprice = get_monoprice(port) | ||
| monoprice = await hass.async_add_executor_job(get_monoprice, port) | ||
| except SerialException: | ||
| _LOGGER.error("Error connecting to Monoprice controller") |
There was a problem hiding this comment.
You want to add the port in case you have multiple set up.
| """Error to indicate we cannot connect.""" | ||
|
|
||
|
|
||
| class InvalidAuth(exceptions.HomeAssistantError): |
|
|
||
|
|
||
| def setup_platform(hass, config, add_entities, discovery_info=None): | ||
| async def async_setup_entry(hass, config_entry, async_add_devices): |
There was a problem hiding this comment.
Please rename async_add_devices to async_add_entities.
| async def async_setup_entry(hass, config_entry, async_add_devices): | ||
| """Set up the Monoprice 6-zone amplifier platform.""" | ||
| port = config.get(CONF_PORT) | ||
| port = config_entry.data.get(CONF_PORT) |
There was a problem hiding this comment.
Port is required so we can use dict[key].
| } | ||
| sources = _get_sources(config_entry.data.get(CONF_SOURCES)) | ||
|
|
||
| devices = [] |
There was a problem hiding this comment.
Please rename devices to entities.
|
It looks like there's no docs PR linked. Please link a docs PR since the config has changed. |
|
@MartinHjelmare I created a PR for the docs: |
|
@MartinHjelmare @balloob I addressed your comments in a new PR: #33133 |
4 participants
Breaking Change:
Monoprice is now configured through the UI. Please remove the configuration from YAML, and add the Monoprice integration via the Integrations page.
Description:
Monoprice is now configured through the UI, with device and entity registry support.
Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#12480
Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code does not interact with devices: