Add config flow to Yamaha #131246

slyoldfox · 2024-11-22T09:29:35Z

Breaking change

Proposed change

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

This PR fixes or closes issue: fixes 130820
This PR is related to issue:
Link to documentation pull request:

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:

Documentation added/updated for www.home-assistant.io

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:

I have reviewed two other open pull requests in this repository.

slyoldfox · 2024-11-22T09:36:44Z

As discussed in #130820 this is a first attempt to add a config flow to the Yamaha integration.

As this was a first attempt at this and for people that are stumbling on this in the future, it's a mix of https://github.com/home-assistant/core/pull/51561/files and the current state of the yahama_musiccast integration.

I've removed references to rxv.find in this integration because the SSDP logic in rxv contains a bug and the library is hardly maintained. It now relies on the internal SSDP logic in HA.

I still have 2 unit tests marked as ignore in which I need your help to understand if they can be removed.

One is about a discovery feature inside the Yamaha integration, which seems to be removed?
One tests zone_names which I omitted since the zones names will nog be able to be managed through the UI (see comment Add unique_id to Yamaha integration #130820 (comment))

Hope this is more or less adequate for a first attempt :-)

joostlek · 2024-11-22T10:36:38Z

The CI is failing

slyoldfox · 2024-11-22T11:06:06Z

The CI is failing

Yes sorry about that, it seems my dev environment is not running the git pre commit hooks, i'll fix this as soon as possible!

joostlek · 2024-11-22T11:08:09Z

I'll convert to draft for now.

I'll also ping @pssc and @Petro31 since they know about the device and might be able to help you.

joostlek · 2024-11-22T11:42:25Z

FYI, you don't have to add the issue to the commit message. If you want to do so, feel free to do so, but you're not required to :)

joostlek

Some initial comments to steer into the right direction

homeassistant/components/yamaha/yamaha_config_info.py

joostlek · 2024-11-22T11:44:22Z

homeassistant/components/yamaha/config_flow.py

+            if serial_number is None:
+                errors["base"] = "no_yamaha_device"


Are there devices without serial number?

I am unsure, I did assume there was going to be. I was doubting to add CONF_HOST as fallback as the unique_id but that would not accepted I read?
How should I move forward with this?

I'm gonna leave this for now, the implementation is the same as the Yamaha Musiccast integration now. I found some SSDP replies through github (https://github.com/foxthefox/yamaha-yxc-nodejs/blob/da9039e3edb1cc338b6f5c19710bbbfb6770b428/lib/data/ssdp_answer_WX30.xml#L17 and https://github.com/foxthefox/yamaha-yxc-nodejs/blob/da9039e3edb1cc338b6f5c19710bbbfb6770b428/lib/data/ssdp_answer_YSP1600.xml#L17).
As well as my RX-A2010

@joostlek @slyoldfox yes from my experience there seem to be ssdp responses without serial in the wild, and the rxv lib doesn't support /cant get the serial post ssdp.

@pssc does that include crawls from http://IP/YamahaRemoteControl/desc.xml ?
@joostlek how should I handle these?

homeassistant/components/yamaha/config_flow.py

joostlek · 2024-11-22T11:46:05Z

homeassistant/components/yamaha/config_flow.py

+        config_entry: ConfigEntry,
+    ) -> OptionsFlow:
+        """Return the options flow."""
+        return YamahaOptionsFlowHandler(config_entry)


Config flow.py is required to be 100% unit tested

joostlek · 2024-11-22T11:48:17Z

homeassistant/components/yamaha/media_player.py

+    if config.get(CONF_HOST):
+        if hass.config_entries.async_entries(DOMAIN) and config[CONF_HOST] not in [
+            entry.data[CONF_HOST] for entry in hass.config_entries.async_entries(DOMAIN)
+        ]:
+            _LOGGER.error(
+                "Configuration in configuration.yaml is not supported anymore. "
+                "Please add this device using the config flow: %s",
+                config[CONF_HOST],


We should raise issues instead. Check the smarty integration for an example. (but keep the YAML entrypoint the same)

Also IMO better to let it go to the config flow and abort there instead of checking here.

joostlek · 2024-11-22T11:48:44Z

homeassistant/components/yamaha/config_flow.py

+                    OPTION_INPUT_SOURCES_IGNORE: user_input.get(
+                        OPTION_INPUT_SOURCES_IGNORE
+                    ),


For the ignore one, I believe there is a way on the device to limit the sources, can explore that option?

I checked my receiver (RX-A2010) but didn't find a way to ignore inputs.
I can rename inputs on the device but even those modified names do not get propagated outside the unit, the output of rxv.inputs() stays the same.

Same here not on my device (RX2020) there is no functionality to support it in the rxv lib.

It also isn't supported by the integration in HACS via the device and that uses a different protocol / method than rxv.

home-assistant · 2024-11-22T11:49:00Z

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.

arturpragacz

The config flow itself seems not quite finished at the moment, so I have not reviewed it.

Instead couple of pointers for some other stuff.

homeassistant/components/yamaha/media_player.py

arturpragacz · 2024-11-22T11:53:22Z

homeassistant/components/yamaha/media_player.py

+    if config.get(CONF_HOST):
+        if hass.config_entries.async_entries(DOMAIN) and config[CONF_HOST] not in [
+            entry.data[CONF_HOST] for entry in hass.config_entries.async_entries(DOMAIN)
+        ]:
+            _LOGGER.error(
+                "Configuration in configuration.yaml is not supported anymore. "
+                "Please add this device using the config flow: %s",
+                config[CONF_HOST],


Also IMO better to let it go to the config flow and abort there instead of checking here.

homeassistant/components/yamaha/media_player.py

arturpragacz · 2024-11-22T11:55:02Z

homeassistant/components/yamaha/media_player.py

+                )
+            )
+    else:
+        _LOGGER.error(


Start a discovery instead and for each discovered receiver launch an import flow.

* Realign code with yamaha_musiccast and store UPNP Description instead of model and name * Reuse rxv.ssdp.rxv_details for querying the rxv data instead of querying from HA code

…add discovery when configuration yaml has no host, add issues (taken from onkyo integration)

slyoldfox · 2024-11-27T17:42:22Z

I hope this is moving in the right direction, so I've requested another review.
Not sure why the code coverage is now failing even though I added tests and seem to have covered all of config_flow.py

home-assistant#130820 Add config flow to Yamaha

ad66426

home-assistant bot added bugfix cla-signed config-flow This integration migrates to the UI by adding a config flow deprecation Indicates a breaking change to happen in the future has-tests integration: yamaha Quality Scale: No score labels Nov 22, 2024

slyoldfox added 2 commits November 22, 2024 10:42

Merge branch 'dev' into dev

fb441fe

home-assistant#130820 Add config flow to Yamaha

02e611b

epenet changed the title ~~#130820 Add config flow to Yamaha~~ Add config flow to Yamaha Nov 22, 2024

epenet added code-quality and removed bugfix labels Nov 22, 2024

home-assistant#130820 Add config flow to Yamaha (format with Prettier)

f852ee3

joostlek marked this pull request as draft November 22, 2024 11:07

home-assistant#130820 Add config flow to Yamaha (format with Prettier)

cd9030e

slyoldfox marked this pull request as ready for review November 22, 2024 11:41

joostlek requested changes Nov 22, 2024

View reviewed changes

home-assistant bot marked this pull request as draft November 22, 2024 11:49

arturpragacz suggested changes Nov 22, 2024

View reviewed changes

slyoldfox added 4 commits November 22, 2024 18:55

home-assistant#130820 Add config flow to Yamaha (format with Prettier)

99bf77b

home-assistant#130820 Add config flow to Yamaha

83f0ee5

* Realign code with yamaha_musiccast and store UPNP Description instead of model and name * Reuse rxv.ssdp.rxv_details for querying the rxv data instead of querying from HA code

home-assistant#130820 Add config flow to Yamaha, cover OptionsFlow

e1edf38

Merge branch 'dev' into dev

dd12dde

slyoldfox added 4 commits November 25, 2024 18:35

home-assistant#130820 Add config flow to Yamaha

00301c3

home-assistant#130820 Add config flow to Yamaha

8370290

home-assistant#130820 Add config flow to Yamaha - use async_abort(), …

7a8f0bd

…add discovery when configuration yaml has no host, add issues (taken from onkyo integration)

Merge branch 'dev' into dev

4899ece

slyoldfox marked this pull request as ready for review November 27, 2024 17:41

home-assistant bot requested review from joostlek and arturpragacz November 27, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config flow to Yamaha #131246

Add config flow to Yamaha #131246

slyoldfox commented Nov 22, 2024 •

edited by epenet

Loading

slyoldfox commented Nov 22, 2024

joostlek commented Nov 22, 2024

slyoldfox commented Nov 22, 2024

joostlek commented Nov 22, 2024

joostlek commented Nov 22, 2024

joostlek left a comment

joostlek Nov 22, 2024

slyoldfox Nov 22, 2024

slyoldfox Nov 25, 2024

pssc Nov 25, 2024

slyoldfox Nov 25, 2024

joostlek Nov 22, 2024

joostlek Nov 22, 2024

arturpragacz Nov 22, 2024

joostlek Nov 22, 2024

slyoldfox Nov 25, 2024

pssc Nov 25, 2024

pssc Nov 25, 2024

home-assistant bot commented Nov 22, 2024

arturpragacz left a comment

arturpragacz Nov 22, 2024

arturpragacz Nov 22, 2024

slyoldfox commented Nov 27, 2024

		if serial_number is None:
		errors["base"] = "no_yamaha_device"

Add config flow to Yamaha #131246

Are you sure you want to change the base?

Add config flow to Yamaha #131246

Conversation

slyoldfox commented Nov 22, 2024 • edited by epenet Loading

Breaking change

Proposed change

Type of change

Additional information

Checklist

slyoldfox commented Nov 22, 2024

joostlek commented Nov 22, 2024

slyoldfox commented Nov 22, 2024

joostlek commented Nov 22, 2024

joostlek commented Nov 22, 2024

joostlek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

home-assistant bot commented Nov 22, 2024

arturpragacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slyoldfox commented Nov 27, 2024

slyoldfox commented Nov 22, 2024 •

edited by epenet

Loading