From 417172eef237bc06c336af2f9c50d5ed2c4e8ab2 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 26 Dec 2021 22:53:33 -1000 Subject: [PATCH] Cleanup HomeKit names to avoid unknown error when adding (#62831) --- .../components/homekit/accessories.py | 8 +++- homeassistant/components/homekit/type_fans.py | 6 ++- .../components/homekit/type_media_players.py | 7 ++-- .../components/homekit/type_remotes.py | 21 +++++----- .../components/homekit/type_switches.py | 7 ++-- homeassistant/components/homekit/util.py | 9 ++++- tests/components/homekit/test_type_remote.py | 38 +++++++++++++++++++ tests/components/homekit/test_type_sensors.py | 34 +++++++++++++++++ 8 files changed, 107 insertions(+), 23 deletions(-) diff --git a/homeassistant/components/homekit/accessories.py b/homeassistant/components/homekit/accessories.py index 23f546910f1..75a3a0fe797 100644 --- a/homeassistant/components/homekit/accessories.py +++ b/homeassistant/components/homekit/accessories.py @@ -57,7 +57,6 @@ from .const import ( MANUFACTURER, MAX_MANUFACTURER_LENGTH, MAX_MODEL_LENGTH, - MAX_NAME_LENGTH, MAX_SERIAL_LENGTH, MAX_VERSION_LENGTH, SERV_BATTERY_SERVICE, @@ -73,6 +72,7 @@ from .util import ( accessory_friendly_name, async_dismiss_setup_message, async_show_setup_message, + cleanup_name_for_homekit, convert_to_float, format_sw_version, validate_media_player_features, @@ -239,7 +239,11 @@ class HomeAccessory(Accessory): ): """Initialize a Accessory object.""" super().__init__( - driver=driver, display_name=name[:MAX_NAME_LENGTH], aid=aid, *args, **kwargs + driver=driver, + display_name=cleanup_name_for_homekit(name), + aid=aid, + *args, + **kwargs, ) self.config = config or {} if device_id: diff --git a/homeassistant/components/homekit/type_fans.py b/homeassistant/components/homekit/type_fans.py index d25f197e0ca..54d6424e8d5 100644 --- a/homeassistant/components/homekit/type_fans.py +++ b/homeassistant/components/homekit/type_fans.py @@ -39,11 +39,11 @@ from .const import ( CHAR_ROTATION_DIRECTION, CHAR_ROTATION_SPEED, CHAR_SWING_MODE, - MAX_NAME_LENGTH, PROP_MIN_STEP, SERV_FANV2, SERV_SWITCH, ) +from .util import cleanup_name_for_homekit _LOGGER = logging.getLogger(__name__) @@ -102,7 +102,9 @@ class Fan(HomeAccessory): serv_fan.add_linked_service(preset_serv) preset_serv.configure_char( CHAR_NAME, - value=f"{self.display_name} {preset_mode}"[:MAX_NAME_LENGTH], + value=cleanup_name_for_homekit( + f"{self.display_name} {preset_mode}" + ), ) self.preset_mode_chars[preset_mode] = preset_serv.configure_char( diff --git a/homeassistant/components/homekit/type_media_players.py b/homeassistant/components/homekit/type_media_players.py index 61c043ebcaa..8faa1385e18 100644 --- a/homeassistant/components/homekit/type_media_players.py +++ b/homeassistant/components/homekit/type_media_players.py @@ -55,12 +55,11 @@ from .const import ( FEATURE_PLAY_STOP, FEATURE_TOGGLE_MUTE, KEY_PLAY_PAUSE, - MAX_NAME_LENGTH, SERV_SWITCH, SERV_TELEVISION_SPEAKER, ) from .type_remotes import REMOTE_KEYS, RemoteInputSelectAccessory -from .util import get_media_player_features +from .util import cleanup_name_for_homekit, get_media_player_features _LOGGER = logging.getLogger(__name__) @@ -135,7 +134,9 @@ class MediaPlayer(HomeAccessory): def generate_service_name(self, mode): """Generate name for individual service.""" - return f"{self.display_name} {MODE_FRIENDLY_NAME[mode]}"[:MAX_NAME_LENGTH] + return cleanup_name_for_homekit( + f"{self.display_name} {MODE_FRIENDLY_NAME[mode]}" + ) def set_on_off(self, value): """Move switch state to value if call came from HomeKit.""" diff --git a/homeassistant/components/homekit/type_remotes.py b/homeassistant/components/homekit/type_remotes.py index 69267c733d2..be1ab2a4f94 100644 --- a/homeassistant/components/homekit/type_remotes.py +++ b/homeassistant/components/homekit/type_remotes.py @@ -47,10 +47,10 @@ from .const import ( KEY_PREVIOUS_TRACK, KEY_REWIND, KEY_SELECT, - MAX_NAME_LENGTH, SERV_INPUT_SOURCE, SERV_TELEVISION, ) +from .util import cleanup_name_for_homekit MAXIMUM_SOURCES = ( 90 # Maximum services per accessory is 100. The base acccessory uses 9 @@ -103,7 +103,9 @@ class RemoteInputSelectAccessory(HomeAccessory): self.entity_id, MAXIMUM_SOURCES, ) - self.sources = sources[:MAXIMUM_SOURCES] + self.sources = [ + cleanup_name_for_homekit(source) for source in sources[:MAXIMUM_SOURCES] + ] if self.sources: self.support_select_source = True @@ -132,10 +134,8 @@ class RemoteInputSelectAccessory(HomeAccessory): SERV_INPUT_SOURCE, [CHAR_IDENTIFIER, CHAR_NAME] ) serv_tv.add_linked_service(serv_input) - serv_input.configure_char( - CHAR_CONFIGURED_NAME, value=source[:MAX_NAME_LENGTH] - ) - serv_input.configure_char(CHAR_NAME, value=source[:MAX_NAME_LENGTH]) + serv_input.configure_char(CHAR_CONFIGURED_NAME, value=source) + serv_input.configure_char(CHAR_NAME, value=source) serv_input.configure_char(CHAR_IDENTIFIER, value=index) serv_input.configure_char(CHAR_IS_CONFIGURED, value=True) input_type = 3 if "hdmi" in source.lower() else 0 @@ -161,7 +161,8 @@ class RemoteInputSelectAccessory(HomeAccessory): # Set active input if not self.support_select_source or not self.sources: return - source_name = new_state.attributes.get(self.source_key) + source = new_state.attributes.get(self.source_key) + source_name = cleanup_name_for_homekit(source) _LOGGER.debug("%s: Set current input to %s", self.entity_id, source_name) if source_name in self.sources: index = self.sources.index(source_name) @@ -169,8 +170,8 @@ class RemoteInputSelectAccessory(HomeAccessory): return possible_sources = new_state.attributes.get(self.source_list_key, []) - if source_name in possible_sources: - index = possible_sources.index(source_name) + if source in possible_sources: + index = possible_sources.index(source) if index >= MAXIMUM_SOURCES: _LOGGER.debug( "%s: Source %s and above are not supported", @@ -189,7 +190,7 @@ class RemoteInputSelectAccessory(HomeAccessory): _LOGGER.debug( "%s: Source %s does not exist the source list: %s", self.entity_id, - source_name, + source, possible_sources, ) self.char_input_source.set_value(0) diff --git a/homeassistant/components/homekit/type_switches.py b/homeassistant/components/homekit/type_switches.py index cd0d4243726..7bc529d7e40 100644 --- a/homeassistant/components/homekit/type_switches.py +++ b/homeassistant/components/homekit/type_switches.py @@ -42,7 +42,6 @@ from .const import ( CHAR_ON, CHAR_OUTLET_IN_USE, CHAR_VALVE_TYPE, - MAX_NAME_LENGTH, SERV_OUTLET, SERV_SWITCH, SERV_VALVE, @@ -51,6 +50,7 @@ from .const import ( TYPE_SPRINKLER, TYPE_VALVE, ) +from .util import cleanup_name_for_homekit _LOGGER = logging.getLogger(__name__) @@ -263,8 +263,7 @@ class SelectSwitch(HomeAccessory): SERV_OUTLET, [CHAR_NAME, CHAR_IN_USE] ) serv_option.configure_char( - CHAR_NAME, - value=f"{option}"[:MAX_NAME_LENGTH], + CHAR_NAME, value=cleanup_name_for_homekit(option) ) serv_option.configure_char(CHAR_IN_USE, value=False) self.select_chars[option] = serv_option.configure_char( @@ -286,6 +285,6 @@ class SelectSwitch(HomeAccessory): @callback def async_update_state(self, new_state): """Update switch state after state changed.""" - current_option = new_state.state + current_option = cleanup_name_for_homekit(new_state.state) for option, char in self.select_chars.items(): char.set_value(option == current_option) diff --git a/homeassistant/components/homekit/util.py b/homeassistant/components/homekit/util.py index 894bfcf8985..2906bf97e7e 100644 --- a/homeassistant/components/homekit/util.py +++ b/homeassistant/components/homekit/util.py @@ -1,4 +1,6 @@ """Collection of useful functions for the HomeKit component.""" +from __future__ import annotations + import io import ipaddress import logging @@ -76,6 +78,7 @@ from .const import ( FEATURE_TOGGLE_MUTE, HOMEKIT_PAIRING_QR, HOMEKIT_PAIRING_QR_SECRET, + MAX_NAME_LENGTH, TYPE_FAUCET, TYPE_OUTLET, TYPE_SHOWER, @@ -352,14 +355,16 @@ def convert_to_float(state): return None -def cleanup_name_for_homekit(name): +def cleanup_name_for_homekit(name: str | None) -> str | None: """Ensure the name of the device will not crash homekit.""" # # This is not a security measure. # # UNICODE_EMOJI is also not allowed but that # likely isn't a problem - return name.translate(HOMEKIT_CHAR_TRANSLATIONS) + if name is None: + return None + return name.translate(HOMEKIT_CHAR_TRANSLATIONS)[:MAX_NAME_LENGTH] def temperature_to_homekit(temperature, unit): diff --git a/tests/components/homekit/test_type_remote.py b/tests/components/homekit/test_type_remote.py index 5c5b5ee6cd9..e77dd5de54c 100644 --- a/tests/components/homekit/test_type_remote.py +++ b/tests/components/homekit/test_type_remote.py @@ -167,3 +167,41 @@ async def test_activity_remote(hass, hk_driver, events, caplog): ) await hass.async_block_till_done() assert call_reset_accessory[0].data[ATTR_ENTITY_ID] == entity_id + + +async def test_activity_remote_bad_names(hass, hk_driver, events, caplog): + """Test if remote accessory with invalid names works as expected.""" + entity_id = "remote.harmony" + hass.states.async_set( + entity_id, + None, + { + ATTR_SUPPORTED_FEATURES: SUPPORT_ACTIVITY, + ATTR_CURRENT_ACTIVITY: "Apple TV", + ATTR_ACTIVITY_LIST: ["TV", "Apple TV", "[[[--Special--]]]", "Super"], + }, + ) + await hass.async_block_till_done() + acc = ActivityRemote(hass, hk_driver, "ActivityRemote", entity_id, 2, None) + await acc.run() + await hass.async_block_till_done() + + assert acc.aid == 2 + assert acc.category == 31 # Television + + assert acc.char_active.value == 0 + assert acc.char_remote_key.value == 0 + assert acc.char_input_source.value == 1 + + hass.states.async_set( + entity_id, + STATE_ON, + { + ATTR_SUPPORTED_FEATURES: SUPPORT_ACTIVITY, + ATTR_CURRENT_ACTIVITY: "[[[--Special--]]]", + ATTR_ACTIVITY_LIST: ["TV", "Apple TV", "[[[--Special--]]]", "Super"], + }, + ) + await hass.async_block_till_done() + assert acc.char_active.value == 1 + assert acc.char_input_source.value == 2 diff --git a/tests/components/homekit/test_type_sensors.py b/tests/components/homekit/test_type_sensors.py index 958306e026f..d864a90fe61 100644 --- a/tests/components/homekit/test_type_sensors.py +++ b/tests/components/homekit/test_type_sensors.py @@ -366,3 +366,37 @@ async def test_sensor_restore(hass, hk_driver, events): acc = get_accessory(hass, hk_driver, hass.states.get("sensor.humidity"), 2, {}) assert acc.category == 10 + + +async def test_bad_name(hass, hk_driver): + """Test an entity with a bad name.""" + entity_id = "sensor.humidity" + + hass.states.async_set(entity_id, "20") + await hass.async_block_till_done() + acc = HumiditySensor(hass, hk_driver, "[[Humid]]", entity_id, 2, None) + await acc.run() + await hass.async_block_till_done() + + assert acc.aid == 2 + assert acc.category == 10 # Sensor + + assert acc.char_humidity.value == 20 + assert acc.display_name == "--Humid--" + + +async def test_empty_name(hass, hk_driver): + """Test an entity with a empty name.""" + entity_id = "sensor.humidity" + + hass.states.async_set(entity_id, "20") + await hass.async_block_till_done() + acc = HumiditySensor(hass, hk_driver, None, entity_id, 2, None) + await acc.run() + await hass.async_block_till_done() + + assert acc.aid == 2 + assert acc.category == 10 # Sensor + + assert acc.char_humidity.value == 20 + assert acc.display_name is None