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 frontier_silicon #64365

Merged
merged 37 commits into from
Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
427a711
Add config_flow to frontier_silicon
wlcrs Jun 29, 2022
1436787
Add missing translation file
wlcrs Jun 29, 2022
7fc104b
Delay unique_id validation until radio_id can be determined
wlcrs Jun 29, 2022
2c3456a
Fix tests
wlcrs Jun 29, 2022
f7bcbff
Improve tests
wlcrs Jun 29, 2022
65be2b6
Use FlowResultType
wlcrs Jun 29, 2022
1bae242
Bump afsapi to 0.2.6
wlcrs Jul 12, 2022
482f0e2
Fix requirements_test_all.txt
wlcrs Jul 26, 2022
44c1a28
Stash ssdp, reauth and unignore flows for now
wlcrs Aug 24, 2022
1fa59e2
Re-introduce SSDP flow
wlcrs Aug 25, 2022
5170de2
hassfest changes
wlcrs Aug 25, 2022
4a17ac8
Merge branch 'dev' into fsapi-0.2.0-update
wlcrs Oct 13, 2022
d4ca822
Merge branch 'dev' into fsapi-0.2.0-update
wlcrs Oct 14, 2022
e527195
Merge remote-tracking branch 'upstream/dev' into fsapi-0.2.0-update
wlcrs Oct 18, 2022
ddf694e
Address review comments
wlcrs Oct 18, 2022
f269111
Small style update
wlcrs Oct 18, 2022
577a6f3
Fix tests
wlcrs Oct 19, 2022
808217b
Merge branch 'dev' into fsapi-0.2.0-update
wlcrs Nov 6, 2022
447edf9
Update integrations.json
wlcrs Nov 6, 2022
12ef02b
Merge branch 'dev' into fsapi-0.2.0-update
wlcrs Nov 6, 2022
956d8b0
Merge branch 'dev' into fsapi-0.2.0-update
wlcrs Dec 16, 2022
62caf1e
Merge branch 'dev' into fsapi-0.2.0-update
wlcrs Mar 2, 2023
842f7e2
fix order in manifest.json
wlcrs Mar 2, 2023
1b8ef30
fix black errors
wlcrs Mar 2, 2023
eb5abb8
Apply suggestions from code review
wlcrs Mar 2, 2023
e087f21
Address review comments
wlcrs Mar 2, 2023
43c715e
fix black errors
wlcrs Mar 2, 2023
da0a7e1
Use async_setup_platform instead of async_setup
wlcrs Mar 6, 2023
e215fdd
Address review comments on tests
wlcrs Mar 6, 2023
1e69338
parameterize tests
wlcrs Mar 6, 2023
6fcb5c5
Remove discovery component changes from this PR
wlcrs Mar 6, 2023
c956220
Address review comments
wlcrs Mar 6, 2023
e174b53
Apply suggestions from code review
wlcrs Mar 6, 2023
382ed4f
Add extra asserts to tests
wlcrs Mar 6, 2023
df07d73
Restructure _async_step_device_config_if_needed
wlcrs Mar 6, 2023
06090ad
Add return statement
wlcrs Mar 6, 2023
08896fd
Update homeassistant/components/frontier_silicon/media_player.py
wlcrs Mar 9, 2023
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
3 changes: 2 additions & 1 deletion .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,8 @@ omit =
homeassistant/components/fritzbox_callmonitor/__init__.py
homeassistant/components/fritzbox_callmonitor/base.py
homeassistant/components/fritzbox_callmonitor/sensor.py
homeassistant/components/frontier_silicon/const.py
homeassistant/components/frontier_silicon/__init__.py
homeassistant/components/frontier_silicon/browse_media.py
homeassistant/components/frontier_silicon/media_player.py
wlcrs marked this conversation as resolved.
Show resolved Hide resolved
homeassistant/components/futurenow/light.py
homeassistant/components/garadget/cover.py
Expand Down
1 change: 1 addition & 0 deletions CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ build.json @home-assistant/supervisor
/homeassistant/components/frontend/ @home-assistant/frontend
/tests/components/frontend/ @home-assistant/frontend
/homeassistant/components/frontier_silicon/ @wlcrs
/tests/components/frontier_silicon/ @wlcrs
/homeassistant/components/fully_kiosk/ @cgarwood
/tests/components/fully_kiosk/ @cgarwood
/homeassistant/components/garages_amsterdam/ @klaasnicolaas
Expand Down
46 changes: 45 additions & 1 deletion homeassistant/components/frontier_silicon/__init__.py
Original file line number Diff line number Diff line change
@@ -1 +1,45 @@
"""The frontier_silicon component."""
"""The Frontier Silicon integration."""
from __future__ import annotations

import logging

from afsapi import AFSAPI, ConnectionError as FSConnectionError

from homeassistant.config_entries import ConfigEntry
from homeassistant.const import Platform
from homeassistant.core import HomeAssistant
from homeassistant.exceptions import PlatformNotReady

from .const import CONF_PIN, CONF_WEBFSAPI_URL, DOMAIN

PLATFORMS = [Platform.MEDIA_PLAYER]

_LOGGER = logging.getLogger(__name__)


async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
"""Set up Frontier Silicon from a config entry."""

webfsapi_url = entry.data[CONF_WEBFSAPI_URL]
pin = entry.data[CONF_PIN]

afsapi = AFSAPI(webfsapi_url, pin)

try:
await afsapi.get_power()
except FSConnectionError as exception:
raise PlatformNotReady from exception
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be ConfigEntryNotReady for it to work as intended


hass.data.setdefault(DOMAIN, {})[entry.entry_id] = afsapi

await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)

return True


async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
"""Unload a config entry."""
if unload_ok := await hass.config_entries.async_unload_platforms(entry, PLATFORMS):
hass.data[DOMAIN].pop(entry.entry_id)

return unload_ok
178 changes: 178 additions & 0 deletions homeassistant/components/frontier_silicon/config_flow.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
"""Config flow for Frontier Silicon Media Player integration."""
from __future__ import annotations

import logging
from typing import Any

from afsapi import AFSAPI, ConnectionError as FSConnectionError, InvalidPinException
import voluptuous as vol

from homeassistant import config_entries
from homeassistant.const import CONF_HOST, CONF_NAME, CONF_PORT
from homeassistant.data_entry_flow import FlowResult

from .const import CONF_PIN, CONF_WEBFSAPI_URL, DEFAULT_PIN, DEFAULT_PORT, DOMAIN

_LOGGER = logging.getLogger(__name__)

STEP_USER_DATA_SCHEMA = vol.Schema(
{
vol.Required(CONF_HOST): str,
vol.Required(CONF_PORT, default=DEFAULT_PORT): int,
}
)

STEP_DEVICE_CONFIG_DATA_SCHEMA = vol.Schema(
{
vol.Required(
CONF_PIN,
default=DEFAULT_PIN,
): str,
}
)


class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
"""Handle a config flow for Frontier Silicon Media Player."""
wlcrs marked this conversation as resolved.
Show resolved Hide resolved

VERSION = 1

def __init__(self) -> None:
"""Initialize flow."""

self._webfsapi_url: str | None = None
self._name: str | None = None
self._unique_id: str | None = None
Comment on lines +40 to +45
Copy link
Member

Choose a reason for hiding this comment

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

These can be set up as class variables instead.


async def async_step_import(self, import_info: dict[str, Any]) -> FlowResult:
"""Handle the import of legacy configuration.yaml entries."""

device_url = f"http://{import_info[CONF_HOST]}:{import_info[CONF_PORT]}/device"
try:
self._webfsapi_url = await AFSAPI.get_webfsapi_endpoint(device_url)
except FSConnectionError:
return self.async_abort(reason="cannot_connect")
except Exception as exception: # pylint: disable=broad-except
_LOGGER.exception(exception)
return self.async_abort(reason="unknown")

try:
afsapi = AFSAPI(self._webfsapi_url, import_info[CONF_PIN])

self._unique_id = await afsapi.get_radio_id()
Copy link
Member

Choose a reason for hiding this comment

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

Why is this assigned to a class variable? It isn't used anywhere later on?

except FSConnectionError:
return self.async_abort(reason="cannot_connect")
except InvalidPinException:
return self.async_abort(reason="invalid_auth")
except Exception as exception: # pylint: disable=broad-except
_LOGGER.exception(exception)
return self.async_abort(reason="unknown")

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

self._name = import_info[CONF_NAME] or "Radio"

return await self._create_entry(pin=import_info[CONF_PIN])

async def async_step_user(
self, user_input: dict[str, Any] | None = None
) -> FlowResult:
"""Handle the initial step of manual configuration."""
errors = {}

if user_input:
device_url = (
f"http://{user_input[CONF_HOST]}:{user_input[CONF_PORT]}/device"
)
try:
self._webfsapi_url = await AFSAPI.get_webfsapi_endpoint(device_url)
except FSConnectionError:
errors["base"] = "cannot_connect"
except Exception as exception: # pylint: disable=broad-except
_LOGGER.exception(exception)
errors["base"] = "unknown"
else:
return await self._async_step_device_config_if_needed()

data_schema = self.add_suggested_values_to_schema(
STEP_USER_DATA_SCHEMA, user_input
)
return self.async_show_form(
step_id="user", data_schema=data_schema, errors=errors
)

async def _async_step_device_config_if_needed(self) -> FlowResult:
"""Most users will not have changed the default PIN on their radio.

We try to use this default PIN, and only if this fails ask for it via `async_step_device_config`
"""

try:
# try to login with default pin
afsapi = AFSAPI(self._webfsapi_url, DEFAULT_PIN)

self._name = await afsapi.get_friendly_name()
except InvalidPinException:
# Ask for a PIN
return await self.async_step_device_config()

self.context["title_placeholders"] = {"name": self._name}

self._unique_id = await afsapi.get_radio_id()
await self.async_set_unique_id(self._unique_id)
self._abort_if_unique_id_configured()

return await self._create_entry()

async def async_step_device_config(
self, user_input: dict[str, Any] | None = None
) -> FlowResult:
"""Handle device configuration step.

We ask for the PIN in this step.
"""
assert self._webfsapi_url is not None

if user_input is None:
return self.async_show_form(
step_id="device_config", data_schema=STEP_DEVICE_CONFIG_DATA_SCHEMA
)

errors = {}

try:
afsapi = AFSAPI(self._webfsapi_url, user_input[CONF_PIN])

self._name = await afsapi.get_friendly_name()

except FSConnectionError:
errors["base"] = "cannot_connect"
except InvalidPinException:
errors["base"] = "invalid_auth"
except Exception as exception: # pylint: disable=broad-except
_LOGGER.exception(exception)
errors["base"] = "unknown"
else:
self._unique_id = await afsapi.get_radio_id()
await self.async_set_unique_id(self._unique_id)
self._abort_if_unique_id_configured()
return await self._create_entry(pin=user_input[CONF_PIN])
wlcrs marked this conversation as resolved.
Show resolved Hide resolved

data_schema = self.add_suggested_values_to_schema(
STEP_DEVICE_CONFIG_DATA_SCHEMA, user_input
)
return self.async_show_form(
step_id="device_config",
data_schema=data_schema,
errors=errors,
)

async def _create_entry(self, pin: str | None = None) -> FlowResult:
Copy link
Member

Choose a reason for hiding this comment

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

IMHO this method adds complexity that is not needed.
Instead of a simple async_create_entry call, we not have additional variables and calls over the place (that are out of standard).

I suggest to just remove this method fully, as the only thing it does is making a single call.

"""Create the entry."""
assert self._name is not None
assert self._webfsapi_url is not None
Comment on lines +173 to +174
Copy link
Member

Choose a reason for hiding this comment

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

These asserts are not needed. It means the typing isn't correct. Please correct that instead and remove these asserts.


data = {CONF_WEBFSAPI_URL: self._webfsapi_url, CONF_PIN: pin or DEFAULT_PIN}

return self.async_create_entry(title=self._name, data=data)
3 changes: 3 additions & 0 deletions homeassistant/components/frontier_silicon/const.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
"""Constants for the Frontier Silicon Media Player integration."""
DOMAIN = "frontier_silicon"

CONF_WEBFSAPI_URL = "webfsapi_url"
CONF_PIN = "pin"

DEFAULT_PIN = "1234"
DEFAULT_PORT = 80

Expand Down
1 change: 1 addition & 0 deletions homeassistant/components/frontier_silicon/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"domain": "frontier_silicon",
"name": "Frontier Silicon",
"codeowners": ["@wlcrs"],
"config_flow": true,
"documentation": "https://www.home-assistant.io/integrations/frontier_silicon",
"iot_class": "local_polling",
"requirements": ["afsapi==0.2.7"]
Expand Down
57 changes: 40 additions & 17 deletions homeassistant/components/frontier_silicon/media_player.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,17 @@
MediaPlayerState,
MediaType,
)
from homeassistant.config_entries import SOURCE_IMPORT, ConfigEntry
from homeassistant.const import CONF_HOST, CONF_NAME, CONF_PASSWORD, CONF_PORT
from homeassistant.core import HomeAssistant
from homeassistant.helpers import issue_registry as ir
import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.entity import DeviceInfo
from homeassistant.helpers.entity_platform import AddEntitiesCallback
from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType

from .browse_media import browse_node, browse_top_level
from .const import DEFAULT_PIN, DEFAULT_PORT, DOMAIN, MEDIA_CONTENT_ID_PRESET
from .const import CONF_PIN, DEFAULT_PIN, DEFAULT_PORT, DOMAIN, MEDIA_CONTENT_ID_PRESET

_LOGGER = logging.getLogger(__name__)

Expand All @@ -49,7 +51,11 @@ async def async_setup_platform(
async_add_entities: AddEntitiesCallback,
discovery_info: DiscoveryInfoType | None = None,
) -> None:
"""Set up the Frontier Silicon platform."""
"""Set up the Frontier Silicon platform.

YAML is deprecated, and imported automatically.
SSDP discovery is temporarily retained - to be refactor subsequently.
"""
if discovery_info is not None:
Copy link
Member

Choose a reason for hiding this comment

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

You should only do the import in the platform setup and add the media player entities only from the setup_entry method. Right now you add the entities twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which case are the entities loaded twice? I fail to see what problem you are mentioning?

In case of an SSDP discovery: they are added here in async_setup_platform and not in setup_entry.
In case of an import of the old YAML: the entity is migrated here but not added. It is subsequently added from setup_entry.
In case of an entity added via the config flow: only added via setup_entry.

Copy link
Member

@Kane610 Kane610 Mar 9, 2023

Choose a reason for hiding this comment

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

In which case are the entities loaded twice? I fail to see what problem you are mentioning?

In case of an SSDP discovery: they are added here in async_setup_platform and not in setup_entry. In case of an import of the old YAML: the entity is migrated here but not added. It is subsequently added from setup_entry. In case of an entity added via the config flow: only added via setup_entry.

Aah sorry, I apparently forgot the nomenclature for the old way of loading entities. Still good I mentioned it as you should also move the SSDP discovery from the old discovery way to the config flow, there shouldn't be any in-between steps, its either old platform or new way. Can't say that I have any other comments apart from this though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah - that was a recommendation that I made, to reduce the size of the PR.
I felt it was easier to review without SSDP, and then migrate SSDP as a follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. And this was the future config entry PR mentioned? If that is the case I can be ok with that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll be creating a PR for the following branch from the moment this one is merged: wlcrs/home-assistant-core@fsapi-0.2.0-update...wlcrs:home-assistant-core:frontier_silicon_ssdp

webfsapi_url = await AFSAPI.get_webfsapi_endpoint(
discovery_info["ssdp_description"]
Expand All @@ -61,24 +67,41 @@ async def async_setup_platform(
[AFSAPIDevice(name, afsapi)],
True,
)

return
wlcrs marked this conversation as resolved.
Show resolved Hide resolved

host = config.get(CONF_HOST)
port = config.get(CONF_PORT)
password = config.get(CONF_PASSWORD)
name = config.get(CONF_NAME)
ir.async_create_issue(
hass,
DOMAIN,
"remove_yaml",
breaks_in_ha_version="2023.6.0",
is_fixable=False,
wlcrs marked this conversation as resolved.
Show resolved Hide resolved
severity=ir.IssueSeverity.WARNING,
translation_key="removed_yaml",
)

try:
webfsapi_url = await AFSAPI.get_webfsapi_endpoint(
f"http://{host}:{port}/device"
)
except FSConnectionError:
_LOGGER.error(
"Could not add the FSAPI device at %s:%s -> %s", host, port, password
)
return
afsapi = AFSAPI(webfsapi_url, password)
async_add_entities([AFSAPIDevice(name, afsapi)], True)
await hass.config_entries.flow.async_init(
DOMAIN,
context={"source": SOURCE_IMPORT},
data={
CONF_NAME: config.get(CONF_NAME),
CONF_HOST: config.get(CONF_HOST),
CONF_PORT: config.get(CONF_PORT, DEFAULT_PORT),
CONF_PIN: config.get(CONF_PASSWORD, DEFAULT_PIN),
},
)


async def async_setup_entry(
hass: HomeAssistant,
config_entry: ConfigEntry,
async_add_entities: AddEntitiesCallback,
) -> None:
"""Set up the Frontier Silicon entity."""

afsapi = hass.data[DOMAIN][config_entry.entry_id] # type: AFSAPI
wlcrs marked this conversation as resolved.
Show resolved Hide resolved

async_add_entities([AFSAPIDevice(config_entry.title, afsapi)], True)


class AFSAPIDevice(MediaPlayerEntity):
Expand Down
35 changes: 35 additions & 0 deletions homeassistant/components/frontier_silicon/strings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
{
"config": {
"flow_title": "{name}",
Copy link
Member

Choose a reason for hiding this comment

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

This is the default flow title and should thus be removed.

"step": {
"user": {
"title": "Frontier Silicon Setup",
Copy link
Member

Choose a reason for hiding this comment

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

This is already implied, please remove title.

"data": {
"host": "[%key:common::config_flow::data::host%]",
"port": "[%key:common::config_flow::data::port%]"
}
},
"device_config": {
"title": "Device Configuration",
"description": "The pin can be found via 'MENU button > Main Menu > System setting > Network > NetRemote PIN setup'",
"data": {
"pin": "[%key:common::config_flow::data::pin%]"
}
}
},
"error": {
"cannot_connect": "[%key:common::config_flow::error::cannot_connect%]",
"invalid_auth": "[%key:common::config_flow::error::invalid_auth%]",
"unknown": "[%key:common::config_flow::error::unknown%]"
},
"abort": {
"already_configured": "[%key:common::config_flow::abort::already_configured_device%]"
Copy link
Member

Choose a reason for hiding this comment

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

We're missing abort reason strings for cannot_connect, invalid_auth and unknown.

}
},
"issues": {
"removed_yaml": {
"title": "The Frontier Silicon YAML configuration has been removed",
"description": "Configuring Frontier Silicon using YAML has been removed.\n\nYour existing YAML configuration is not used by Home Assistant.\n\nRemove the YAML configuration from your configuration.yaml file and restart Home Assistant to fix this issue."
}
}
}
1 change: 1 addition & 0 deletions homeassistant/generated/config_flows.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@
"fritzbox",
"fritzbox_callmonitor",
"fronius",
"frontier_silicon",
"fully_kiosk",
"garages_amsterdam",
"gdacs",
Expand Down
Loading