Skip to content
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

Add config flow to Yamaha #131246

Draft
wants to merge 19 commits into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions homeassistant/components/yamaha/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1,72 @@
"""The yamaha component."""

from __future__ import annotations

from functools import partial
import logging

import rxv

from homeassistant.components import ssdp
from homeassistant.config_entries import ConfigEntry
from homeassistant.const import CONF_HOST, Platform
from homeassistant.core import HomeAssistant

from .const import CONF_SERIAL, CONF_UPNP_DESC, DOMAIN
from .yamaha_config_info import YamahaConfigInfo

PLATFORMS = [Platform.MEDIA_PLAYER]

_LOGGER = logging.getLogger(__name__)


async def get_upnp_desc(hass: HomeAssistant, host: str) -> str:
"""Get the upnp description URL for a given host, using the SSPD scanner."""
ssdp_entries = await ssdp.async_get_discovery_info_by_st(hass, "upnp:rootdevice")
matches = [w for w in ssdp_entries if w.ssdp_headers.get("_host", "") == host]
upnp_desc = None
for match in matches:
if upnp_desc := match.ssdp_location:
break

if not upnp_desc:
_LOGGER.warning(
"The upnp_description was not found automatically, setting a default one"
)
upnp_desc = f"http://{host}:49154/MediaRenderer/desc.xml"
Comment on lines +32 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reason why we should do this in the config flow, who knows if this description works? The user is unable to recover from this

return upnp_desc


async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"""Set up Yamaha from a config entry."""
if entry.data.get(CONF_UPNP_DESC) is None:
hass.config_entries.async_update_entry(

Check warning on line 43 in homeassistant/components/yamaha/__init__.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/yamaha/__init__.py#L43

Added line #L43 was not covered by tests
Comment on lines +42 to +43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work?

Like I am known with the technique to do this in the config flow discovery step, so for example when someone setup the integration using the host and then we find either a zeroconf or DHCP flow that we can link to it (for example if the entry uses the same host and is loaded) and then we update the entry to have that unique identifier. Examples of this are fritzbox and reolink, both do this in the DHCP step I believe.

From my understanding this does the same, but I am wondering how the order of discovery works, because if the integration is setup and after that the SSDP picks up the advertisement, this code is not run anymore.

So I am not really sure what is best here, but I do want to share this info with you to at stir up the discussion.

entry,
data={
CONF_HOST: entry.data[CONF_HOST],
CONF_SERIAL: entry.data[CONF_SERIAL],
Comment on lines +46 to +47
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CONF_HOST: entry.data[CONF_HOST],
CONF_SERIAL: entry.data[CONF_SERIAL],
**entry.data,

CONF_UPNP_DESC: await get_upnp_desc(hass, entry.data[CONF_HOST]),
},
)

hass.data.setdefault(DOMAIN, {})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're storing nothing here so this line can be removed

rxv_details = await YamahaConfigInfo.get_rxv_details(
entry.data[CONF_UPNP_DESC], hass
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So our code relies on having that info, but why do we then only set the upnp description during initialisation and why not during the config (or import) flow?

entry.runtime_data = await hass.async_add_executor_job(
partial(rxv.RXV, **rxv_details._asdict()) # type: ignore[union-attr]
)
await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)

entry.async_on_unload(entry.add_update_listener(async_reload_entry))
return True


async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
"""Unload a config entry."""
return await hass.config_entries.async_unload_platforms(entry, PLATFORMS)


async def async_reload_entry(hass: HomeAssistant, entry: ConfigEntry) -> None:
"""Reload config entry."""
await hass.config_entries.async_reload(entry.entry_id)

Check warning on line 72 in homeassistant/components/yamaha/__init__.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/yamaha/__init__.py#L72

Added line #L72 was not covered by tests
235 changes: 235 additions & 0 deletions homeassistant/components/yamaha/config_flow.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,235 @@
"""Config flow for Yamaha."""

from __future__ import annotations

import logging
from typing import Any
from urllib.parse import urlparse

from requests.exceptions import ConnectionError
import rxv
import voluptuous as vol

from homeassistant.components import ssdp
from homeassistant.config_entries import (
ConfigEntry,
ConfigFlow,
ConfigFlowResult,
OptionsFlow,
)
from homeassistant.const import CONF_HOST
from homeassistant.core import callback
from homeassistant.helpers.selector import (
SelectOptionDict,
Selector,
SelectSelector,
SelectSelectorConfig,
SelectSelectorMode,
TextSelector,
)

from . import get_upnp_desc
from .const import (
CONF_SERIAL,
CONF_UPNP_DESC,
DEFAULT_NAME,
DOMAIN,
OPTION_INPUT_SOURCES,
OPTION_INPUT_SOURCES_IGNORE,
)
from .yamaha_config_info import YamahaConfigInfo

_LOGGER = logging.getLogger(__name__)


class YamahaFlowHandler(ConfigFlow, domain=DOMAIN):
"""Handle a Yamaha config flow."""

VERSION = 1

serial_number: str | None = None
host: str
upnp_description: str | None = None

async def async_step_user(
self, user_input: dict[str, Any] | None = None
) -> ConfigFlowResult:
"""Handle a flow initiated by the user."""
# Request user input, unless we are preparing discovery flow
if user_input is None:
return self._show_setup_form()

host = user_input[CONF_HOST]
serial_number = None

# Check if device is a Yamaha receiver
try:
upnp_desc: str = await get_upnp_desc(self.hass, host)
info = await YamahaConfigInfo.get_rxv_details(upnp_desc, self.hass)
except ConnectionError:
return self.async_abort(reason="cannot_connect")
except Exception:
_LOGGER.exception("Unexpected exception")
return self.async_abort(reason="unknown")
Comment on lines +68 to +71
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not abort in the user flow, rather return errors so the user can correct themselves

else:
if info is None or (serial_number := info.serial_number) is None:
return self.async_abort(reason="cannot_connect")

await self.async_set_unique_id(serial_number, raise_on_progress=False)
self._abort_if_unique_id_configured()

return self.async_create_entry(
title=DEFAULT_NAME,
data={
CONF_HOST: host,
CONF_SERIAL: serial_number,
CONF_UPNP_DESC: await get_upnp_desc(self.hass, host),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already fetched this right?

},
options={
OPTION_INPUT_SOURCES_IGNORE: user_input.get(OPTION_INPUT_SOURCES_IGNORE)
or [],
OPTION_INPUT_SOURCES: user_input.get(OPTION_INPUT_SOURCES) or {},
},
)

def _show_setup_form(self, errors: dict | None = None) -> ConfigFlowResult:
"""Show the setup form to the user."""
return self.async_show_form(
step_id="user",
data_schema=vol.Schema({vol.Required(CONF_HOST): str}),
errors=errors or {},
)

async def async_step_ssdp(
self, discovery_info: ssdp.SsdpServiceInfo
) -> ConfigFlowResult:
"""Handle ssdp discoveries."""
assert discovery_info.ssdp_location is not None
if not await YamahaConfigInfo.check_yamaha_ssdp(
discovery_info.ssdp_location, self.hass
):
return self.async_abort(reason="yxc_control_url_missing")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this happen and what can the user do to fix it?

self.serial_number = discovery_info.upnp[ssdp.ATTR_UPNP_SERIAL]
self.upnp_description = discovery_info.ssdp_location

# ssdp_location and hostname have been checked in check_yamaha_ssdp so it is safe to ignore type assignment
self.host = urlparse(discovery_info.ssdp_location).hostname # type: ignore[assignment]

await self.async_set_unique_id(self.serial_number)
self._abort_if_unique_id_configured(
{
CONF_HOST: self.host,
CONF_UPNP_DESC: self.upnp_description,
}
)
self.context.update(
{
"title_placeholders": {
"name": discovery_info.upnp.get(
ssdp.ATTR_UPNP_FRIENDLY_NAME, self.host
)
}
}
)

return await self.async_step_confirm()

async def async_step_confirm(self, user_input=None) -> ConfigFlowResult:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing typing

"""Allow the user to confirm adding the device."""
if user_input is not None:
return self.async_create_entry(
title=DEFAULT_NAME,
data={
CONF_HOST: self.host,
CONF_SERIAL: self.serial_number,
CONF_UPNP_DESC: self.upnp_description,
},
options={
OPTION_INPUT_SOURCES_IGNORE: user_input.get(
OPTION_INPUT_SOURCES_IGNORE
)
or [],
Comment on lines +146 to +149
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wrap these multiline statements in parenthesis

OPTION_INPUT_SOURCES: user_input.get(OPTION_INPUT_SOURCES) or {},
},
)

return self.async_show_form(step_id="confirm")

async def async_step_import(self, import_data: dict) -> ConfigFlowResult:
"""Import data from configuration.yaml into the config flow."""
return await self.async_step_user(import_data)
Comment on lines +156 to +158
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While for the user flow we want to raise errors, for the import flow we should abort when an error happens.

Also, typing is missing


@staticmethod
@callback
def async_get_options_flow(
config_entry: ConfigEntry,
) -> OptionsFlow:
"""Return the options flow."""
return YamahaOptionsFlowHandler(config_entry)
slyoldfox marked this conversation as resolved.
Show resolved Hide resolved


class YamahaOptionsFlowHandler(OptionsFlow):
"""Handle an options flow for Yamaha."""

def __init__(self, config_entry: ConfigEntry) -> None:
"""Initialize options flow."""
self._input_sources_ignore: list[str] = config_entry.options[
OPTION_INPUT_SOURCES_IGNORE
]
self._input_sources: dict[str, str] = config_entry.options[OPTION_INPUT_SOURCES]

async def async_step_init(
self, user_input: dict[str, Any] | None = None
) -> ConfigFlowResult:
"""Manage the options."""
yamaha: rxv.RXV = self.config_entry.runtime_data
inputs: dict[str, str] = await self.hass.async_add_executor_job(yamaha.inputs)

if user_input is not None:
sources_store: dict[str, str] = {
k: v for k, v in user_input.items() if k in inputs and v != ""
}

return self.async_create_entry(
data={
OPTION_INPUT_SOURCES: sources_store,
OPTION_INPUT_SOURCES_IGNORE: user_input.get(
OPTION_INPUT_SOURCES_IGNORE
),
slyoldfox marked this conversation as resolved.
Show resolved Hide resolved
}
)

schema_dict: dict[Any, Selector] = {}
available_inputs = [
SelectOptionDict(value=k, label=k) for k, v in inputs.items()
]

schema_dict[vol.Optional(OPTION_INPUT_SOURCES_IGNORE)] = SelectSelector(
SelectSelectorConfig(
options=available_inputs,
mode=SelectSelectorMode.DROPDOWN,
multiple=True,
)
)

for source in inputs:
if source not in self._input_sources_ignore:
schema_dict[vol.Optional(source, default="")] = TextSelector()

options = self.config_entry.options.copy()
if OPTION_INPUT_SOURCES_IGNORE in self.config_entry.options:
options[OPTION_INPUT_SOURCES_IGNORE] = self.config_entry.options[
OPTION_INPUT_SOURCES_IGNORE
]
if OPTION_INPUT_SOURCES in self.config_entry.options:
for source, source_name in self.config_entry.options[
OPTION_INPUT_SOURCES
].items():
options[source] = source_name

return self.async_show_form(
step_id="init",
data_schema=self.add_suggested_values_to_schema(
vol.Schema(schema_dict), options
),
)
12 changes: 10 additions & 2 deletions homeassistant/components/yamaha/const.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
"""Constants for the Yamaha component."""

DOMAIN = "yamaha"
DISCOVER_TIMEOUT = 3
KNOWN_ZONES = "known_zones"
BRAND = "Yamaha Corporation"
CONF_SERIAL = "serial"
CONF_UPNP_DESC = "upnp_description"
CONF_SOURCE_IGNORE = "source_ignore"
CONF_SOURCE_NAMES = "source_names"
CONF_ZONE_IGNORE = "zone_ignore"
CONF_ZONE_NAMES = "zone_names"
CURSOR_TYPE_DOWN = "down"
CURSOR_TYPE_LEFT = "left"
CURSOR_TYPE_RETURN = "return"
Expand All @@ -12,3 +17,6 @@
SERVICE_ENABLE_OUTPUT = "enable_output"
SERVICE_MENU_CURSOR = "menu_cursor"
SERVICE_SELECT_SCENE = "select_scene"
OPTION_INPUT_SOURCES = "source_names"
OPTION_INPUT_SOURCES_IGNORE = "source_ignore"
DEFAULT_NAME = "Yamaha Receiver"
13 changes: 11 additions & 2 deletions homeassistant/components/yamaha/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,18 @@
"domain": "yamaha",
"name": "Yamaha Network Receivers",
"codeowners": [],
"config_flow": true,
"dependencies": ["ssdp"],
"documentation": "https://www.home-assistant.io/integrations/yamaha",
"iot_class": "local_polling",
"loggers": ["rxv"],
"quality_scale": "legacy",
"requirements": ["rxv==0.7.0"]
"requirements": ["rxv==0.7.0"],
"ssdp": [
{
"manufacturer": "YAMAHA CORPORATION"
},
{
"manufacturer": "Yamaha Corporation"
}
]
}
Loading