From 4de30ca2ce5c32295054ff36f648f87412e01884 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 2 May 2020 16:15:44 -0500 Subject: [PATCH] Improve stability of homekit media players (#35080) --- .../components/homekit/accessories.py | 7 +- .../components/homekit/type_media_players.py | 53 +++++++-------- homeassistant/components/homekit/util.py | 66 ++++++++++++++++++- .../homekit/test_type_media_players.py | 20 ++++++ tests/components/homekit/test_util.py | 14 ++++ 5 files changed, 125 insertions(+), 35 deletions(-) diff --git a/homeassistant/components/homekit/accessories.py b/homeassistant/components/homekit/accessories.py index ddafbd8fa66..e7724631717 100644 --- a/homeassistant/components/homekit/accessories.py +++ b/homeassistant/components/homekit/accessories.py @@ -164,13 +164,12 @@ def get_accessory(hass, driver, state, aid, config): elif state.domain == "media_player": device_class = state.attributes.get(ATTR_DEVICE_CLASS) - feature_list = config.get(CONF_FEATURE_LIST) + feature_list = config.get(CONF_FEATURE_LIST, []) if device_class == DEVICE_CLASS_TV: a_type = "TelevisionMediaPlayer" - else: - if feature_list and validate_media_player_features(state, feature_list): - a_type = "MediaPlayer" + elif validate_media_player_features(state, feature_list): + a_type = "MediaPlayer" elif state.domain == "sensor": device_class = state.attributes.get(ATTR_DEVICE_CLASS) diff --git a/homeassistant/components/homekit/type_media_players.py b/homeassistant/components/homekit/type_media_players.py index 154355a0da3..209baad125e 100644 --- a/homeassistant/components/homekit/type_media_players.py +++ b/homeassistant/components/homekit/type_media_players.py @@ -64,6 +64,7 @@ from .const import ( SERV_TELEVISION, SERV_TELEVISION_SPEAKER, ) +from .util import get_media_player_features _LOGGER = logging.getLogger(__name__) @@ -83,10 +84,12 @@ MEDIA_PLAYER_KEYS = { # 15: "Information", } +# Names may not contain special characters +# or emjoi (/ is a special character for Apple) MODE_FRIENDLY_NAME = { FEATURE_ON_OFF: "Power", - FEATURE_PLAY_PAUSE: "Play/Pause", - FEATURE_PLAY_STOP: "Play/Stop", + FEATURE_PLAY_PAUSE: "Play-Pause", + FEATURE_PLAY_STOP: "Play-Stop", FEATURE_TOGGLE_MUTE: "Mute", } @@ -105,7 +108,9 @@ class MediaPlayer(HomeAccessory): FEATURE_PLAY_STOP: None, FEATURE_TOGGLE_MUTE: None, } - feature_list = self.config[CONF_FEATURE_LIST] + feature_list = self.config.get( + CONF_FEATURE_LIST, get_media_player_features(state) + ) if FEATURE_ON_OFF in feature_list: name = self.generate_service_name(FEATURE_ON_OFF) @@ -213,7 +218,7 @@ class MediaPlayer(HomeAccessory): self.chars[FEATURE_PLAY_STOP].set_value(hk_state) if self.chars[FEATURE_TOGGLE_MUTE]: - current_state = new_state.attributes.get(ATTR_MEDIA_VOLUME_MUTED) + current_state = bool(new_state.attributes.get(ATTR_MEDIA_VOLUME_MUTED)) _LOGGER.debug( '%s: Set current state for "toggle_mute" to %s', self.entity_id, @@ -239,9 +244,7 @@ class TelevisionMediaPlayer(HomeAccessory): # Add additional characteristics if volume or input selection supported self.chars_tv = [] self.chars_speaker = [] - features = self.hass.states.get(self.entity_id).attributes.get( - ATTR_SUPPORTED_FEATURES, 0 - ) + features = state.attributes.get(ATTR_SUPPORTED_FEATURES, 0) if features & (SUPPORT_PLAY | SUPPORT_PAUSE): self.chars_tv.append(CHAR_REMOTE_KEY) @@ -252,7 +255,8 @@ class TelevisionMediaPlayer(HomeAccessory): if features & SUPPORT_VOLUME_SET: self.chars_speaker.append(CHAR_VOLUME) - if features & SUPPORT_SELECT_SOURCE: + source_list = state.attributes.get(ATTR_INPUT_SOURCE_LIST, []) + if source_list and features & SUPPORT_SELECT_SOURCE: self.support_select_source = True serv_tv = self.add_preload_service(SERV_TELEVISION, self.chars_tv) @@ -297,9 +301,7 @@ class TelevisionMediaPlayer(HomeAccessory): ) if self.support_select_source: - self.sources = self.hass.states.get(self.entity_id).attributes.get( - ATTR_INPUT_SOURCE_LIST, [] - ) + self.sources = source_list self.char_input_source = serv_tv.configure_char( CHAR_ACTIVE_IDENTIFIER, setter_callback=self.set_input_source ) @@ -379,14 +381,13 @@ class TelevisionMediaPlayer(HomeAccessory): hk_state = 0 if current_state not in ("None", STATE_OFF, STATE_UNKNOWN): hk_state = 1 - _LOGGER.debug("%s: Set current active state to %s", self.entity_id, hk_state) if self.char_active.value != hk_state: self.char_active.set_value(hk_state) # Set mute state if CHAR_VOLUME_SELECTOR in self.chars_speaker: - current_mute_state = new_state.attributes.get(ATTR_MEDIA_VOLUME_MUTED) + current_mute_state = bool(new_state.attributes.get(ATTR_MEDIA_VOLUME_MUTED)) _LOGGER.debug( "%s: Set current mute state to %s", self.entity_id, current_mute_state, ) @@ -394,20 +395,16 @@ class TelevisionMediaPlayer(HomeAccessory): self.char_mute.set_value(current_mute_state) # Set active input - if self.support_select_source: + if self.support_select_source and self.sources: source_name = new_state.attributes.get(ATTR_INPUT_SOURCE) - if self.sources: - _LOGGER.debug( - "%s: Set current input to %s", self.entity_id, source_name + _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) + if self.char_input_source.value != index: + self.char_input_source.set_value(index) + else: + _LOGGER.warning( + "%s: Sources out of sync. Restart Home Assistant", self.entity_id, ) - if source_name in self.sources: - index = self.sources.index(source_name) - if self.char_input_source.value != index: - self.char_input_source.set_value(index) - else: - _LOGGER.warning( - "%s: Sources out of sync. Restart Home Assistant", - self.entity_id, - ) - if self.char_input_source.value != 0: - self.char_input_source.set_value(0) + if self.char_input_source.value != 0: + self.char_input_source.set_value(0) diff --git a/homeassistant/components/homekit/util.py b/homeassistant/components/homekit/util.py index 3ccf73d3925..5a8d1f98841 100644 --- a/homeassistant/components/homekit/util.py +++ b/homeassistant/components/homekit/util.py @@ -101,6 +101,40 @@ SWITCH_TYPE_SCHEMA = BASIC_INFO_SCHEMA.extend( ) +HOMEKIT_CHAR_TRANSLATIONS = { + 0: " ", # nul + 10: " ", # nl + 13: " ", # cr + 33: "-", # ! + 34: " ", # " + 36: "-", # $ + 37: "-", # % + 40: "-", # ( + 41: "-", # ) + 42: "-", # * + 43: "-", # + + 47: "-", # / + 58: "-", # : + 59: "-", # ; + 60: "-", # < + 61: "-", # = + 62: "-", # > + 63: "-", # ? + 64: "-", # @ + 91: "-", # [ + 92: "-", # \ + 93: "-", # ] + 94: "-", # ^ + 95: " ", # _ + 96: "-", # ` + 123: "-", # { + 124: "-", # | + 125: "-", # } + 126: "-", # ~ + 127: "-", # del +} + + def validate_entity_config(values): """Validate config entry for CONF_ENTITY.""" if not isinstance(values, dict): @@ -138,8 +172,8 @@ def validate_entity_config(values): return entities -def validate_media_player_features(state, feature_list): - """Validate features for media players.""" +def get_media_player_features(state): + """Determine features for media players.""" features = state.attributes.get(ATTR_SUPPORTED_FEATURES, 0) supported_modes = [] @@ -153,6 +187,20 @@ def validate_media_player_features(state, feature_list): supported_modes.append(FEATURE_PLAY_STOP) if features & media_player.const.SUPPORT_VOLUME_MUTE: supported_modes.append(FEATURE_TOGGLE_MUTE) + return supported_modes + + +def validate_media_player_features(state, feature_list): + """Validate features for media players.""" + supported_modes = get_media_player_features(state) + + if not supported_modes: + _LOGGER.error("%s does not support any media_player features", state.entity_id) + return False + + if not feature_list: + # Auto detected + return True error_list = [] for feature in feature_list: @@ -160,7 +208,9 @@ def validate_media_player_features(state, feature_list): error_list.append(feature) if error_list: - _LOGGER.error("%s does not support features: %s", state.entity_id, error_list) + _LOGGER.error( + "%s does not support media_player features: %s", state.entity_id, error_list + ) return False return True @@ -252,6 +302,16 @@ def convert_to_float(state): return None +def cleanup_name_for_homekit(name): + """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) + + def temperature_to_homekit(temperature, unit): """Convert temperature to Celsius for HomeKit.""" return round(temp_util.convert(temperature, unit, TEMP_CELSIUS), 1) diff --git a/tests/components/homekit/test_type_media_players.py b/tests/components/homekit/test_type_media_players.py index cb2de7264a8..9b8cf074d57 100644 --- a/tests/components/homekit/test_type_media_players.py +++ b/tests/components/homekit/test_type_media_players.py @@ -369,6 +369,26 @@ async def test_media_player_television_basic(hass, hk_driver, events, caplog): assert not caplog.messages or "Error" not in caplog.messages[-1] +async def test_media_player_television_supports_source_select_no_sources( + hass, hk_driver, events, caplog +): + """Test if basic tv that supports source select but is missing a source list.""" + entity_id = "media_player.television" + + # Supports turn_on', 'turn_off' + hass.states.async_set( + entity_id, + None, + {ATTR_DEVICE_CLASS: DEVICE_CLASS_TV, ATTR_SUPPORTED_FEATURES: 3469}, + ) + await hass.async_block_till_done() + acc = TelevisionMediaPlayer(hass, hk_driver, "MediaPlayer", entity_id, 2, None) + await acc.run_handler() + await hass.async_block_till_done() + + assert acc.support_select_source is False + + async def test_tv_restore(hass, hk_driver, events): """Test setting up an entity from state in the event registry.""" hass.state = CoreState.not_running diff --git a/tests/components/homekit/test_util.py b/tests/components/homekit/test_util.py index 2c8c93cee4c..d5ff923270b 100644 --- a/tests/components/homekit/test_util.py +++ b/tests/components/homekit/test_util.py @@ -23,6 +23,7 @@ from homeassistant.components.homekit.const import ( from homeassistant.components.homekit.util import ( HomeKitSpeedMapping, SpeedRange, + cleanup_name_for_homekit, convert_to_float, density_to_air_quality, dismiss_setup_message, @@ -177,6 +178,19 @@ def test_convert_to_float(): assert convert_to_float(None) is None +def test_cleanup_name_for_homekit(): + """Ensure name sanitize works as expected.""" + + assert cleanup_name_for_homekit("abc") == "abc" + assert cleanup_name_for_homekit("a b c") == "a b c" + assert cleanup_name_for_homekit("ab_c") == "ab c" + assert ( + cleanup_name_for_homekit('ab!@#$%^&*()-=":.,>