Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new denon_rs232 integration to Home Assistant to control Denon receivers over an RS232 serial connection, including config flow, media player entities, translations, and initial test coverage.
Changes:
- Introduces the
denon_rs232integration (config entry setup/unload + media_player platform). - Adds config flow for selecting a detected USB serial port or entering a port manually.
- Adds tests for config flow and media_player behavior, plus generated/requirements/brand/codeowner updates.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/denon_rs232/init.py | Sets up/unloads the receiver and forwards the config entry to platforms. |
| homeassistant/components/denon_rs232/config_flow.py | Implements UI flow for selecting/entering a serial port and validating connectivity. |
| homeassistant/components/denon_rs232/media_player.py | Adds receiver media_player entities (main + zones) with volume/source/power controls. |
| homeassistant/components/denon_rs232/const.py | Defines domain/logger and typed config entry alias. |
| homeassistant/components/denon_rs232/strings.json | Provides translations for config flow and source attribute states. |
| homeassistant/components/denon_rs232/quality_scale.yaml | Declares integration quality scale rule statuses. |
| homeassistant/components/denon_rs232/manifest.json | Declares the new integration metadata and dependency. |
| tests/components/denon_rs232/conftest.py | Provides fixtures and a receiver test double for integration tests. |
| tests/components/denon_rs232/test_config_flow.py | Tests config flow success, error handling, and duplicate-port aborts. |
| tests/components/denon_rs232/test_media_player.py | Tests entity creation, state updates, and service command mapping. |
| tests/components/denon_rs232/init.py | Defines shared constants for tests. |
| requirements_all.txt | Adds denon-rs232==3.0.0 to runtime requirements. |
| requirements_test_all.txt | Adds denon-rs232==3.0.0 to test requirements. |
| homeassistant/generated/integrations.json | Registers the new integration in generated metadata. |
| homeassistant/generated/config_flows.py | Adds the integration to the generated config flow list. |
| homeassistant/brands/denon.json | Adds denon_rs232 to Denon brand integrations list. |
| CODEOWNERS | Adds codeowners for the new integration and its tests. |
| self._attr_device_info = DeviceInfo( | ||
| identifiers={(DOMAIN, config_entry.entry_id)}, | ||
| manufacturer="Denon", | ||
| model=model.name, | ||
| name=config_entry.title, | ||
| ) |
There was a problem hiding this comment.
If they are zones, can they be in different areas? Should we have a different device so you can easily assign it to a different area?
There was a problem hiding this comment.
You can add entities to other areas, do we really want to make a receiver show up as 3 different devices?
There was a problem hiding this comment.
There are integrations that do this, I believe cambridge audio, bluesound
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com> Co-authored-by: Paulus Schoutsen <balloob@gmail.com>
|
|
||
| from .const import DOMAIN, LOGGER | ||
|
|
||
| MODEL_OPTIONS = {key: model.name for key, model in MODELS.items()} |
There was a problem hiding this comment.
Why would we include Other? (Do note again that we can use a selectselector here if we want to keep Other in).
In a way it would be cool if we can discover more models and have people report that to us so we can add them to the list
There was a problem hiding this comment.
The RS232 protocol does not support capability or model discovery. Users need to tell us the models in advance so we can populate the right source list.
Example: https://github.com/home-assistant-libs/denon-rs232/blob/main/src/denon_rs232/models.py#L198-L210
There was a problem hiding this comment.
Yea, I figured, but more like, there's not a real nice way for users to tell us their device upfront (and what its capable off). And I guess you can add this option to the reconfiguration flow in the future so the user can select another one.
There's no nice way to allow people to use it while finding out what device they use without disallowing them to set it up.
One option could be to allow to reconfigure it, log in the logs that users need to create an issue with the functionality it has, the model and that they need to reconfigure it once fixed. But that also depends on people reading logs
There was a problem hiding this comment.
So with Other, at least we give them all possible source options. And then hopefully, some people will open github issues and help fix it for everyone.
Reconfigure flow has been left out of the initial PR, can be added in the future.
| async def test_only_active_zones_are_created( | ||
| hass: HomeAssistant, initial_receiver_state: MockState | ||
| ) -> None: | ||
| """Test setup only creates entities for zones with queried power state.""" |
There was a problem hiding this comment.
If power of a zone is None, the receiver does not have that zone.
| def test_input_source_translation_keys_cover_all_enum_members() -> None: | ||
| """Test all input sources have a declared translation key.""" | ||
| assert set(INPUT_SOURCE_DENON_TO_HA) == set(InputSource) | ||
|
|
||
| strings = load_json(STRINGS_PATH) | ||
| assert set(INPUT_SOURCE_DENON_TO_HA.values()) == set( | ||
| strings["entity"]["media_player"]["receiver"]["state_attributes"]["source"][ | ||
| "state" | ||
| ] | ||
| ) |
There was a problem hiding this comment.
We generally don't have tests for this
There was a problem hiding this comment.
This test is to:
- ensure we map all values from the libraries enum
- we provide a translation for each one
This will help make sure that future library upgrades are not just blindly applied if they need a translation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| try: | ||
| await receiver.connect() | ||
| await receiver.query_state() | ||
| except (ConnectionError, OSError) as err: | ||
| LOGGER.error("Error connecting to Denon receiver at %s: %s", port, err) | ||
| if receiver.connected: | ||
| await receiver.disconnect() | ||
| raise ConfigEntryNotReady from err |
There was a problem hiding this comment.
Catch TimeoutError (and any other connect/query exceptions you already handle in the config flow) during config entry setup so the entry retries instead of failing and leaving an open connection.
| self._attr_device_info = DeviceInfo( | ||
| identifiers={(DOMAIN, config_entry.entry_id)}, | ||
| manufacturer="Denon", | ||
| model_id=model.name, |
There was a problem hiding this comment.
Populate the device registry model field (and reserve model_id for a stable identifier) so the device shows a proper model name in the UI and can be filtered consistently.
| model_id=model.name, | |
| model=model.name, |
Breaking change
Proposed change
This adds a new integration for Denon receivers controlled via RS232 serial protocol.
Waiting for #167023Type 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: