Add Full support for roborock Zeo washing/drying machines#159575
Add Full support for roborock Zeo washing/drying machines#159575allenporter merged 41 commits intohome-assistant:devfrom
Conversation
|
Hey there @Lash-L, @allenporter, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive support for Roborock washing machines (A01 devices) by introducing new sensors, buttons, select entities, and switches to control various washing machine functions like programs, temperature settings, spin levels, and more.
Key changes:
- Adds 6 new select entities for controlling washing machine settings (program, mode, temperature, drying mode, spin level, rinse times)
- Adds 3 new buttons for controlling the washing machine (start, pause, shutdown)
- Adds 1 new switch for sound settings
- Adds 5 new sensors for monitoring detergent/softener status and type
- Updates coordinator to fetch additional protocol data needed by the new entities
- Adds comprehensive translations for all new entities
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| button.py | Adds A01 button entity class and 3 button descriptions (start, pause, shutdown) |
| select.py | Adds A01 select entity class and 6 select descriptions for washing machine settings |
| switch.py | Adds A01 switch entity class and sound setting switch |
| sensor.py | Adds 5 new sensor descriptions and removes entity_category from some existing sensors |
| coordinator.py | Adds additional protocol fields to request list for washing machine data |
| strings.json | Adds translations for all new entities with proper state mappings |
Comments suppressed due to low confidence (1)
homeassistant/components/roborock/button.py:238
- This PR adds new A01 button entities but doesn't include corresponding test coverage. According to the guidelines, test coverage above 95% is required for all modules. Please add tests for the new RoborockButtonEntityA01 class, covering both successful button presses and error scenarios.
"""A class to define Roborock A01 button entities."""
entity_description: RoborockButtonDescriptionA01
def __init__(
self,
coordinator: RoborockDataUpdateCoordinatorA01,
entity_description: RoborockButtonDescriptionA01,
) -> None:
"""Create an A01 button entity."""
self.entity_description = entity_description
super().__init__(f"{entity_description.key}_{coordinator.duid_slug}", coordinator)
async def async_press(self) -> None:
"""Press the button."""
try:
if self.entity_description.param is not None:
await self.coordinator.api.set_value(
self.entity_description.data_protocol,
self.entity_description.param,
)
else:
await self.coordinator.api.set_value(
self.entity_description.data_protocol,
1, # Default value for button press
)
await self.coordinator.async_request_refresh()
except Exception as err:
raise HomeAssistantError(
translation_domain=DOMAIN,
translation_key="button_press_failed",
) from err
| translation_domain=DOMAIN, | ||
| translation_key="select_option_failed", |
There was a problem hiding this comment.
The translation key "select_option_failed" is used here but is not defined in strings.json. This will result in an untranslated error message being shown to users. Please add the translation key to the "exceptions" section of strings.json.
| translation_domain=DOMAIN, | |
| translation_key="select_option_failed", | |
| "Failed to set selected option; try again." |
| self.entity_description.option_values, | ||
| ) | ||
| # Find the option name that matches the current value | ||
| return current_value |
There was a problem hiding this comment.
The current_option property returns the raw current_value instead of mapping it back to the option name. This will cause the select entity to display the numeric protocol value (e.g., "1", "2") instead of the user-friendly option name (e.g., "wash", "dry"). You need to reverse-lookup the option name from the value, similar to how it's done in the async_select_option method. Consider adding logic like: return next((name for name, val in self.entity_description.option_values.items() if val == current_value), None)
| return current_value | |
| return next( | |
| ( | |
| name | |
| for name, val in self.entity_description.option_values.items() | |
| if val == current_value | |
| ), | |
| None, | |
| ) |
| class RoborockSelectEntityA01(RoborockCoordinatedEntityA01, SelectEntity): | ||
| """A class to let you set options on a Roborock A01 device.""" | ||
|
|
||
| entity_description: RoborockSelectDescriptionA01 | ||
|
|
||
| def __init__( | ||
| self, | ||
| coordinator: RoborockDataUpdateCoordinatorA01, | ||
| entity_description: RoborockSelectDescriptionA01, | ||
| ) -> None: | ||
| """Create an A01 select entity.""" | ||
| self.entity_description = entity_description | ||
| super().__init__( | ||
| f"{entity_description.key}_{coordinator.duid_slug}", | ||
| coordinator, | ||
| ) | ||
| self._attr_options = entity_description.options | ||
|
|
||
| async def async_select_option(self, option: str) -> None: | ||
| """Set the option.""" | ||
| try: | ||
| # Get the protocol value for the selected option | ||
| value = self.entity_description.option_values.get(option) | ||
| if value is not None: | ||
| await self.coordinator.api.set_value( | ||
| self.entity_description.data_protocol, | ||
| value, | ||
| ) | ||
| await self.coordinator.async_request_refresh() | ||
| except Exception as err: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="select_option_failed", | ||
| ) from err | ||
|
|
||
| @property | ||
| def current_option(self) -> str | None: | ||
| """Get the current status of the select entity from coordinator data.""" | ||
| if self.entity_description.data_protocol not in self.coordinator.data: | ||
| return None | ||
|
|
||
| current_value = self.coordinator.data[self.entity_description.data_protocol] | ||
| if current_value is None: | ||
| return None | ||
| _LOGGER.debug( | ||
| "current_value: %s for %s with values %s", | ||
| current_value, | ||
| self.entity_description.key, | ||
| self.entity_description.option_values, | ||
| ) | ||
| # Find the option name that matches the current value | ||
| return current_value |
There was a problem hiding this comment.
This PR adds new A01 select entities but doesn't include corresponding test coverage. According to the guidelines, test coverage above 95% is required for all modules. Please add tests for the new RoborockSelectEntityA01 class, covering option selection, current_option retrieval, and error scenarios.
| except Exception as err: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="button_press_failed", |
There was a problem hiding this comment.
The translation key "button_press_failed" is used here but is not defined in strings.json. This will result in an untranslated error message being shown to users. Please add the translation key to the "exceptions" section of strings.json.
| translation_key="button_press_failed", | |
| translation_key="command_failed", | |
| translation_placeholders={ | |
| "command": "BUTTON_PRESS", | |
| }, |
| value, | ||
| ) | ||
| await self.coordinator.async_request_refresh() | ||
| except Exception as err: |
There was a problem hiding this comment.
This code uses a bare Exception catch, which is too broad. According to the coding guidelines, bare exceptions should only be used in config flows or background tasks. In regular entity code like this, you should catch the specific RoborockException instead. This ensures that unexpected errors are properly surfaced rather than being silently converted to generic HomeAssistantErrors.
| class RoborockSwitchA01(RoborockCoordinatedEntityA01, SwitchEntity): | ||
| """A class to let you turn functionality on Roborock A01 devices on and off.""" | ||
|
|
||
| entity_description: RoborockSwitchDescriptionA01 | ||
|
|
||
| def __init__( | ||
| self, | ||
| coordinator: RoborockDataUpdateCoordinatorA01, | ||
| description: RoborockSwitchDescriptionA01, | ||
| ) -> None: | ||
| """Initialize the entity.""" | ||
| self.entity_description = description | ||
| super().__init__(f"{description.key}_{coordinator.duid_slug}", coordinator) | ||
|
|
||
| async def async_turn_off(self, **kwargs: Any) -> None: | ||
| """Turn off the switch.""" | ||
| try: | ||
| await self.coordinator.api.set_value(self.entity_description.data_protocol, 0) | ||
| await self.coordinator.async_request_refresh() | ||
| except RoborockException as err: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="update_options_failed", | ||
| ) from err | ||
|
|
||
| async def async_turn_on(self, **kwargs: Any) -> None: | ||
| """Turn on the switch.""" | ||
| try: | ||
| await self.coordinator.api.set_value(self.entity_description.data_protocol, 1) | ||
| await self.coordinator.async_request_refresh() | ||
| except RoborockException as err: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="update_options_failed", | ||
| ) from err | ||
|
|
||
| @property | ||
| def is_on(self) -> bool | None: | ||
| """Return True if entity is on.""" | ||
| status = self.coordinator.data.get(self.entity_description.data_protocol) | ||
| if status is None: | ||
| return None | ||
| return bool(status) | ||
|
|
There was a problem hiding this comment.
This PR adds new A01 switch entities but doesn't include corresponding test coverage. According to the guidelines, test coverage above 95% is required for all modules. Please add tests for the new RoborockSwitchA01 class, covering turn on, turn off, and error scenarios.
| key="sound_setting", | ||
| data_protocol=RoborockZeoProtocol.SOUND_SET, | ||
| translation_key="zeo_sound_setting", | ||
| entity_category=EntityCategory.DIAGNOSTIC, |
There was a problem hiding this comment.
The sound_setting switch is assigned EntityCategory.DIAGNOSTIC, but sound settings are typically user-configurable preferences and should use EntityCategory.CONFIG instead. Looking at similar switches in this file (like child_lock, status_indicator, dnd_switch), they all use EntityCategory.CONFIG for user-configurable settings. DIAGNOSTIC is typically reserved for technical information about the device, not user preferences.
| entity_category=EntityCategory.DIAGNOSTIC, | |
| entity_category=EntityCategory.CONFIG, |
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
Thank you very much for this contribution.
| """Describes a Roborock A01 button entity.""" | ||
|
|
||
| data_protocol: RoborockZeoProtocol | ||
| param: Any = None |
There was a problem hiding this comment.
What sets param? How is this ever set to non-None? I think this can be deleted here and below.
| # The protocol that the select entity will send to the api. | ||
| data_protocol: RoborockZeoProtocol | ||
| # Available options for the select entity | ||
| options: list[str] |
There was a problem hiding this comment.
Can you replace options and option_values with a single RoborockEnum field then let the entity call keys or as_dict().items() so every entity description doesn't need to?
| class RoborockSelectEntityA01(RoborockCoordinatedEntityA01, SelectEntity): | ||
| """A class to let you set options on a Roborock A01 device.""" | ||
|
|
||
| entity_description: RoborockSelectDescriptionA01 | ||
|
|
||
| def __init__( | ||
| self, | ||
| coordinator: RoborockDataUpdateCoordinatorA01, | ||
| entity_description: RoborockSelectDescriptionA01, | ||
| ) -> None: | ||
| """Create an A01 select entity.""" | ||
| self.entity_description = entity_description | ||
| super().__init__( | ||
| f"{entity_description.key}_{coordinator.duid_slug}", | ||
| coordinator, | ||
| ) | ||
| self._attr_options = entity_description.options | ||
|
|
||
| async def async_select_option(self, option: str) -> None: | ||
| """Set the option.""" | ||
| try: | ||
| # Get the protocol value for the selected option | ||
| value = self.entity_description.option_values.get(option) | ||
| if value is not None: | ||
| await self.coordinator.api.set_value( | ||
| self.entity_description.data_protocol, | ||
| value, | ||
| ) | ||
| await self.coordinator.async_request_refresh() | ||
| except Exception as err: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="select_option_failed", | ||
| ) from err | ||
|
|
||
| @property | ||
| def current_option(self) -> str | None: | ||
| """Get the current status of the select entity from coordinator data.""" | ||
| if self.entity_description.data_protocol not in self.coordinator.data: | ||
| return None | ||
|
|
||
| current_value = self.coordinator.data[self.entity_description.data_protocol] | ||
| if current_value is None: | ||
| return None | ||
| _LOGGER.debug( | ||
| "current_value: %s for %s with values %s", | ||
| current_value, | ||
| self.entity_description.key, | ||
| self.entity_description.option_values, | ||
| ) | ||
| # Find the option name that matches the current value | ||
| return current_value |
| translation_domain=DOMAIN, | ||
| translation_key="select_option_failed", |
| value, | ||
| ) | ||
| await self.coordinator.async_request_refresh() | ||
| except Exception as err: |
| translation_key="times_after_clean", | ||
| entity_category=EntityCategory.DIAGNOSTIC, | ||
| ), | ||
| RoborockSensorDescriptionA01( |
There was a problem hiding this comment.
This looks like a binary_sensor here and below where there are two values
There was a problem hiding this comment.
This one is still relevant and has not been answered to
There was a problem hiding this comment.
No, it is a sensor recording how many times the washing machine started after the self-clean operation
There was a problem hiding this comment.
I guess you meant the one below. OK.
| "False": "Available" | ||
| } | ||
| }, | ||
| "zeo_detergent_type": { |
There was a problem hiding this comment.
I think it would make sense to drop zeo_ prefix from the translation keys
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This PR makes also a bunch of other small changes to the existing code. Can you send them in logical separate PRs with description that give context/motivation? They should be fast to review. But generally I'm left wondering what is intentional versus accidental AI changes. |
| device_class=SensorDeviceClass.ENUM, | ||
| options=RoborockDyadStateCode.keys(), | ||
| ), | ||
| RoborockSensorDescriptionA01( |
There was a problem hiding this comment.
battery is not used in Zeo, but I guess it may show up in other devices, will revert.
| ), | ||
| RoborockSensorDescriptionA01( | ||
| key="error", | ||
| key="dyad_error", |
There was a problem hiding this comment.
i believe that changing this key will change unique ids which will break existing use cases. can this be reverted?
| "water_level_error": "Water level error" | ||
| } | ||
| }, | ||
| "zeo_state": { |
There was a problem hiding this comment.
I realize i told you to rename to not have "zeo" prefix but the existing key is using it. can you revert this so we don't lose translations?
There was a problem hiding this comment.
I think my comment may have been a bad idea for some of the fields that have unique values to these specific devices, like the mode etc
There was a problem hiding this comment.
OK though I also think how you had it originally before may be needed in the case where there are modes specific to the zeo device (e.g. if we add another device type it may also have a drying mode with different options). For cases where there are device specific things i think it makes sense to keep. Sorry i'm slow to catch this.
| ) | ||
| for coordinator in config_entry.runtime_data.a01 | ||
| for description in A01_SWITCH_DESCRIPTIONS | ||
| if description.data_protocol in coordinator.data |
There was a problem hiding this comment.
I want to avoid adding new dependencies on coordinator.data since we want to make it so it may not be fetched by the time we get here.
If this is not supported on all devices, perhaps we can treat it disabled by default?
There was a problem hiding this comment.
I tested my Zeo device, the switch is added correctly.
Sorry, I did not fully understand your comment. Do you mean to delete this async_add_entities for A01 switches and add "entity_registry_enabled_default=False" to A01_SWITCH_DESCRIPTIONS for "sound_setting"?
Could you make or suggest your change?
There was a problem hiding this comment.
Sorry for not fully explaining, let me elaborate.
The problem with depending on coordinator.data here is that it assumes the coordinator has always been initialized in async_setup_entry here. I just made a short writeup on some of the problems with doing this.
Thinking through solutions our options are either:
(1) Understand what features are supported based on the device type / metadata available from the API
(2) Do additional queries and hide them behind the device API
(3) Make the entities optional and let the user figure it out. Yes, this would be entity_registry_enabled_default=False for new items that we don't think are supported.
There was a problem hiding this comment.
I only have one Zeo(A01) device, so I am not sure if sound_setting is universal or not. I'm now just commenting out the if statement.
| "door_lock_error": "Door lock error", | ||
| "drain_error": "Drain error", | ||
| "drying_error": "Drying error", | ||
| "drying_error_e_12": "Drying error E12", |
There was a problem hiding this comment.
Do you think these are actually valuable to show to the user? It seems like we should just map all these to drying_error
There was a problem hiding this comment.
I encouter a dain error ealier, the debug info could be helpful.
There was a problem hiding this comment.
Does E1* mean the same for every device? If so, can we just call it how they call it?
There was a problem hiding this comment.
Do you still have an answer for this one?
There was a problem hiding this comment.
They are the same for all zeo devices. I removed the duplicated ones. One possibility is to add the error message:
│ drying_error │ Drying error E11: air inlet temperature sensor error │
│ drying_error_e_12 │ Drying error E12: air outlet temperature sensor error │
│ drying_error_e_14 │ Drying error E14: inlet condenser temperature sensor error │
│ drying_error_e_15 │ Drying error E15: heating element or turntable error │
│ drying_error_e_16 │ Drying error E16: drying fan error │
There was a problem hiding this comment.
Can we add the error then? Maybe remove the E part and make sure the state also looks nice
| description, | ||
| ) | ||
| for coordinator in config_entry.runtime_data.a01 | ||
| for description in A01_BUTTON_DESCRIPTIONS |
There was a problem hiding this comment.
I think i realize what needs to be checked for eligibility: it's just is the device type zeo or dyad. Check out how other platforms do this, i think they check the coordinator instance type.
There was a problem hiding this comment.
I see. Could you check the new commit?
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
There's a merge conflict, can you take a look?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
homeassistant/components/roborock/strings.json:445
- The strings.json file adds duplicate sensor translations "error" (lines 368-390) and "state" (lines 431-445) that are not used by any sensors in the code. The code only uses "zeo_error" and "zeo_state" translation keys for the Zeo washing machine sensors. These unused translations should be removed to avoid confusion and maintain consistency.
"error": {
"name": "Error",
"state": {
"communication_error": "Communication error",
"door_lock_error": "Door lock error",
"drain_error": "Drain error",
"drying_error": "Drying error",
"drying_error_e_12": "Drying error E12",
"drying_error_e_13": "Drying error E13",
"drying_error_e_14": "Drying error E14",
"drying_error_e_15": "Drying error E15",
"drying_error_e_16": "Drying error E16",
"drying_error_restart": "Restart the washer",
"drying_error_water_flow": "Check water flow",
"heating_error": "Heating error",
"inverter_error": "Inverter error",
"none": "[%key:component::roborock::entity::sensor::vacuum_error::state::none%]",
"refill_error": "Refill error",
"spin_error": "Re-arrange clothes",
"temperature_error": "Temperature error",
"water_level_error": "Water level error"
}
},
"filter_time_left": {
"name": "Filter time left"
},
"last_clean_end": {
"name": "Last clean end"
},
"last_clean_start": {
"name": "Last clean begin"
},
"main_brush_time_left": {
"name": "Main brush time left"
},
"mop_drying_remaining_time": {
"name": "Mop drying remaining time"
},
"mop_life_time_left": {
"name": "Mop life time left"
},
"q7_status": {
"name": "Status",
"state": {
"charging": "[%key:common::state::charging%]",
"docking": "[%key:component::roborock::entity::sensor::status::state::docking%]",
"mop_airdrying": "Mop air drying",
"mop_cleaning": "Mop cleaning",
"moping": "Mopping",
"paused": "[%key:common::state::paused%]",
"sleeping": "Sleeping",
"sweep_moping": "Sweep mopping",
"sweep_moping_2": "Sweep mopping",
"updating": "[%key:component::roborock::entity::sensor::status::state::updating%]",
"waiting_for_orders": "Waiting for orders"
}
},
"sensor_time_left": {
"name": "Sensor time left"
},
"side_brush_time_left": {
"name": "Side brush time left"
},
"state": {
"name": "State",
"state": {
"cooling": "Cooling",
"done": "Done",
"drying": "Drying",
"rinsing": "Rinsing",
"soaking": "Soaking",
"spinning": "Spinning",
"standby": "[%key:common::state::standby%]",
"under_delay_start": "Delayed start",
"washing": "Washing",
"weighing": "Weighing"
}
},
homeassistant/components/roborock/button.py:235
- The finally block causes the coordinator to refresh even when the button press fails and raises an exception. This is inconsistent with the pattern used in switch and select entities (see switch.py lines 196, 209) where refresh only happens on successful operations. Move the refresh call inside the try block after the set_value call to ensure it only executes on success.
finally:
await self.coordinator.async_request_refresh()
| "zeo_state": { | ||
| "name": "State", | ||
| "state": { | ||
| "aftercare": "Aftercare", |
There was a problem hiding this comment.
Are we sure removing states here is correct?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
homeassistant/components/roborock/sensor.py:541
- Direct dictionary access could raise a KeyError if the protocol is not present in coordinator.data at runtime. Although entities are only created when the protocol is in request_protocols, there's no guarantee the key will always be in the data dictionary during updates. Use coordinator.data.get() with a fallback to None to safely handle missing keys, consistent with Home Assistant best practices for entity state handling.
value = self.coordinator.data[self.entity_description.data_protocol]
if self.entity_description.value_fn is not None:
return self.entity_description.value_fn(value)
return value
homeassistant/components/roborock/binary_sensor.py:219
- Direct dictionary access could raise a KeyError if the protocol is not present in coordinator.data at runtime. Although entities are only created when the protocol is in request_protocols, there's no guarantee the key will always be in the data dictionary during updates. Use coordinator.data.get() with a fallback to return False or None to safely handle missing keys, consistent with Home Assistant best practices for entity state handling.
value = self.coordinator.data[self.entity_description.data_protocol]
return self.entity_description.value_fn(value)
tests/components/roborock/test_select.py:297
- The select platform is missing comprehensive snapshot testing like the other platforms (button, switch, sensor, binary_sensor all have snapshot tests). For consistency with the testing patterns in this integration, add a test_selects function using snapshot_platform similar to the pattern in test_button.py lines 33-41, test_switch.py lines 28-36, etc. This ensures all entity states and registry entries for the new A01 select entities are properly validated.
@pytest.fixture
def zeo_device(fake_devices: list[FakeDevice]) -> FakeDevice:
"""Get the fake Zeo washing machine device."""
return next(device for device in fake_devices if getattr(device, "zeo", None))
homeassistant/components/roborock/button.py:235
- Using finally block here causes async_request_refresh to execute even when the button press fails. This could be problematic because if the set_value operation fails, the coordinator data hasn't changed, but we're still refreshing it. This is inconsistent with the switch and select implementations which only refresh after successful operations. Move async_request_refresh outside the try-finally block, after the exception handling, to only refresh on success.
finally:
await self.coordinator.async_request_refresh()
homeassistant/components/roborock/switch.py:201
- The async_request_refresh call is inside the try block, but according to Home Assistant coding guidelines, the try block should be minimal and only wrap operations that can throw exceptions. If set_value succeeds but async_request_refresh fails, this would incorrectly report that the switch operation failed, even though the device state was successfully changed. Move async_request_refresh outside the try-except block to ensure it always executes after a successful set_value call.
try:
await self.coordinator.api.set_value( # type: ignore[attr-defined]
self.entity_description.data_protocol, 0
)
await self.coordinator.async_request_refresh()
except RoborockException as err:
raise HomeAssistantError(
translation_domain=DOMAIN,
translation_key="update_options_failed",
) from err
homeassistant/components/roborock/switch.py:214
- The async_request_refresh call is inside the try block, but according to Home Assistant coding guidelines, the try block should be minimal and only wrap operations that can throw exceptions. If set_value succeeds but async_request_refresh fails, this would incorrectly report that the switch operation failed, even though the device state was successfully changed. Move async_request_refresh outside the try-except block to ensure it always executes after a successful set_value call.
try:
await self.coordinator.api.set_value( # type: ignore[attr-defined]
self.entity_description.data_protocol, 1
)
await self.coordinator.async_request_refresh()
except RoborockException as err:
raise HomeAssistantError(
translation_domain=DOMAIN,
translation_key="update_options_failed",
) from err
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
homeassistant/components/roborock/sensor.py:538
- The sensor entity directly accesses coordinator.data dictionary without checking if the key exists first. This could cause a KeyError if the protocol is not present in the data. The switch entity (line 219 in switch.py) uses .get() to safely access data, which returns None if the key doesn't exist. You should follow the same pattern here for consistency and to prevent potential errors.
Consider changing:
value = self.coordinator.data[self.entity_description.data_protocol]To:
value = self.coordinator.data.get(self.entity_description.data_protocol)And then handle the None case appropriately.
value = self.coordinator.data[self.entity_description.data_protocol]
homeassistant/components/roborock/binary_sensor.py:218
- The binary sensor entity directly accesses coordinator.data dictionary without checking if the key exists first. This could cause a KeyError if the protocol is not present in the data. The switch entity (line 219 in switch.py) uses .get() to safely access data. You should follow the same pattern here for consistency and to prevent potential errors.
Consider changing:
value = self.coordinator.data[self.entity_description.data_protocol]To:
value = self.coordinator.data.get(self.entity_description.data_protocol)And then handle the None case appropriately (e.g., return False or set the entity as unavailable).
value = self.coordinator.data[self.entity_description.data_protocol]
homeassistant/components/roborock/button.py:235
- The coordinator refresh is called in a
finallyblock, which means it will execute even if the set_value operation fails. This is inconsistent with the switch entity pattern (lines 190-214 in switch.py) where the refresh is only called on success. If the set_value fails, there's no state change to refresh, so calling async_request_refresh would be unnecessary and potentially misleading.
Consider moving the refresh call outside the try/except block (like in select.py line 452) so it only executes on success:
try:
await self.coordinator.api.set_value(
self.entity_description.data_protocol,
1,
)
except RoborockException as err:
raise HomeAssistantError(
translation_domain=DOMAIN,
translation_key="button_press_failed",
) from err
await self.coordinator.async_request_refresh() finally:
await self.coordinator.async_request_refresh()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
homeassistant/components/roborock/button.py:235
- The
async_request_refresh()call in the finally block will execute even when an exception is raised, which may not be the intended behavior. According to Home Assistant guidelines, the try block should only wrap code that can throw exceptions, and data processing (like refresh) should happen after successful execution.
Consider moving the refresh call outside the try-except block and only calling it after successful execution, or ensuring that refresh is only called when the operation succeeds.
finally:
await self.coordinator.async_request_refresh()
homeassistant/components/roborock/switch.py:196
- According to Home Assistant coding guidelines, the try block should only wrap code that can throw exceptions, and data processing should happen outside the try block. The
async_request_refresh()call should be moved outside the try-except block to follow the recommended pattern.
The guideline states: "Only wrap code that can throw exceptions" and "Keep try blocks minimal - process data after the try/catch".
try:
await self.coordinator.api.set_value( # type: ignore[attr-defined]
self.entity_description.data_protocol, 0
)
await self.coordinator.async_request_refresh()
homeassistant/components/roborock/switch.py:209
- According to Home Assistant coding guidelines, the try block should only wrap code that can throw exceptions, and data processing should happen outside the try block. The
async_request_refresh()call should be moved outside the try-except block to follow the recommended pattern.
The guideline states: "Only wrap code that can throw exceptions" and "Keep try blocks minimal - process data after the try/catch".
try:
await self.coordinator.api.set_value( # type: ignore[attr-defined]
self.entity_description.data_protocol, 1
)
await self.coordinator.async_request_refresh()
homeassistant/components/roborock/binary_sensor.py:219
- The
is_onproperty directly accessesself.coordinator.data[self.entity_description.data_protocol]without checking if the key exists. This could raise a KeyError if the protocol is not present in the data dictionary. Consider using.get()with a default value or adding a None check, similar to how the switch'sis_onproperty handles this case.
@property
def is_on(self) -> bool:
"""Return the value reported by the sensor."""
value = self.coordinator.data[self.entity_description.data_protocol]
return self.entity_description.value_fn(value)
joostlek
added
the
noteworthy
Breaking change
Proposed change
The original roborock washing machine only exposes four sesnsors.
Here, this patch adds a bunch more. Not just sensors, but also buttons, selects, and switches.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: