Ensure traits to always reflect the the status of commands #592
Conversation
This changes the contract for traits that have commands that mutate state to always ensure they reflect the latest device state after the command completes.
allenporter
requested review from
Lash-L and
Copilot
and removed request for
Copilot
allenporter
force-pushed
the
trait-updates
branch
from
614b589 to
4fed92b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR establishes a contract for traits with state-mutating commands to ensure they always reflect the latest device state after command completion. The implementation uses a mix of optimistic updates (directly setting local state) and refresh calls (fetching state from the device).
Key Changes:
- Updated trait methods to maintain state consistency after command execution through either optimistic updates or refresh calls
- Enhanced documentation to clarify state update responsibilities and distinguish between trait commands and raw commands
- Added comprehensive test coverage to verify state updates after command execution
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/devices/traits/v1/test_dnd.py |
Added test assertions to verify state updates and multiple command calls after DnD timer operations |
tests/devices/traits/v1/test_consumable.py |
Added test assertions to verify state refresh after consumable reset operations |
roborock/devices/traits/v1/volume.py |
Added optimistic state update (self.volume = volume) after volume change command |
roborock/devices/traits/v1/valley_electricity_timer.py |
Added refresh call to clear_timer() and optimistic updates to enable()/disable() methods |
roborock/devices/traits/v1/led_status.py |
Added optimistic state updates to enable()/disable() methods |
roborock/devices/traits/v1/flow_led_status.py |
Added optimistic state updates to enable()/disable() methods |
roborock/devices/traits/v1/do_not_disturb.py |
Added refresh calls to set_dnd_timer()/clear_dnd_timer() and optimistic updates to enable()/disable() |
roborock/devices/traits/v1/consumeable.py |
Added refresh call after consumable reset operation |
roborock/devices/traits/v1/child_lock.py |
Added optimistic state updates to enable()/disable() methods |
roborock/devices/traits/v1/common.py |
Expanded documentation to clarify trait state update contract and cross-trait synchronization responsibilities |
roborock/devices/traits/v1/command.py |
Added documentation clarifying that raw commands don't auto-update trait state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RoborockCommand.SET_VALLEY_ELECTRICITY_TIMER, | ||
| params=self.as_list(), | ||
| ) | ||
| # Optimistcally update state to avoid an extra refresh |
There was a problem hiding this comment.
Typo in comment: "Optimistcally" should be "Optimistically"
| await self.rpc_channel.send_command(RoborockCommand.SET_LED_STATUS, params=[1]) | ||
| # Optimistcally update state to avoid an extra refresh | ||
| self.status = 1 |
There was a problem hiding this comment.
The optimistic state update occurs after the command is sent, but if the command fails with an exception, the state will remain inconsistent. Consider moving the state update before the command and reverting on exception, or wrapping in a try-except to only update on success.
| # Optimistcally update state to avoid an extra refresh | ||
| self.status = 1 | ||
|
|
||
| async def disable(self) -> None: | ||
| """Disable the LED status.""" | ||
| await self.rpc_channel.send_command(RoborockCommand.SET_LED_STATUS, params=[0]) | ||
| # Optimistcally update state to avoid an extra refresh |
There was a problem hiding this comment.
Typo in comment: "Optimistcally" should be "Optimistically"
| # Optimistcally update state to avoid an extra refresh | |
| self.status = 1 | |
| async def disable(self) -> None: | |
| """Disable the LED status.""" | |
| await self.rpc_channel.send_command(RoborockCommand.SET_LED_STATUS, params=[0]) | |
| # Optimistcally update state to avoid an extra refresh | |
| # Optimistically update state to avoid an extra refresh | |
| self.status = 1 | |
| async def disable(self) -> None: | |
| """Disable the LED status.""" | |
| await self.rpc_channel.send_command(RoborockCommand.SET_LED_STATUS, params=[0]) | |
| # Optimistically update state to avoid an extra refresh |
| # Optimistcally update state to avoid an extra refresh | ||
| self.status = 1 | ||
|
|
||
| async def disable(self) -> None: | ||
| """Disable the LED status.""" | ||
| await self.rpc_channel.send_command(RoborockCommand.SET_LED_STATUS, params=[0]) | ||
| # Optimistcally update state to avoid an extra refresh |
There was a problem hiding this comment.
Typo in comment: "Optimistcally" should be "Optimistically"
| # Optimistcally update state to avoid an extra refresh | |
| self.status = 1 | |
| async def disable(self) -> None: | |
| """Disable the LED status.""" | |
| await self.rpc_channel.send_command(RoborockCommand.SET_LED_STATUS, params=[0]) | |
| # Optimistcally update state to avoid an extra refresh | |
| # Optimistically update state to avoid an extra refresh | |
| self.status = 1 | |
| async def disable(self) -> None: | |
| """Disable the LED status.""" | |
| await self.rpc_channel.send_command(RoborockCommand.SET_LED_STATUS, params=[0]) | |
| # Optimistically update state to avoid an extra refresh |
| # Optimistcally update state to avoid an extra refresh | ||
| self.status = 1 | ||
|
|
||
| async def disable(self) -> None: | ||
| """Disable the Flow LED status.""" | ||
| await self.rpc_channel.send_command(RoborockCommand.SET_FLOW_LED_STATUS, params={_STATUS_PARAM: 0}) | ||
| # Optimistcally update state to avoid an extra refresh |
There was a problem hiding this comment.
Typo in comment: "Optimistcally" should be "Optimistically"
| # Optimistcally update state to avoid an extra refresh | |
| self.status = 1 | |
| async def disable(self) -> None: | |
| """Disable the Flow LED status.""" | |
| await self.rpc_channel.send_command(RoborockCommand.SET_FLOW_LED_STATUS, params={_STATUS_PARAM: 0}) | |
| # Optimistcally update state to avoid an extra refresh | |
| # Optimistically update state to avoid an extra refresh | |
| self.status = 1 | |
| async def disable(self) -> None: | |
| """Disable the Flow LED status.""" | |
| await self.rpc_channel.send_command(RoborockCommand.SET_FLOW_LED_STATUS, params={_STATUS_PARAM: 0}) | |
| # Optimistically update state to avoid an extra refresh |
| await self.rpc_channel.send_command(RoborockCommand.SET_CHILD_LOCK_STATUS, params={_STATUS_PARAM: 1}) | ||
| # Optimistcally update state to avoid an extra refresh | ||
| self.lock_status = 1 | ||
|
|
||
| async def disable(self) -> None: | ||
| """Disable the child lock.""" | ||
| await self.rpc_channel.send_command(RoborockCommand.SET_CHILD_LOCK_STATUS, params={_STATUS_PARAM: 0}) | ||
| # Optimistcally update state to avoid an extra refresh | ||
| self.lock_status = 0 |
There was a problem hiding this comment.
The optimistic state update occurs after the command is sent, but if the command fails with an exception, the state will remain inconsistent. Consider moving the state update before the command and reverting on exception, or wrapping in a try-except to only update on success.
| await self.rpc_channel.send_command(RoborockCommand.SET_CHILD_LOCK_STATUS, params={_STATUS_PARAM: 1}) | |
| # Optimistcally update state to avoid an extra refresh | |
| self.lock_status = 1 | |
| async def disable(self) -> None: | |
| """Disable the child lock.""" | |
| await self.rpc_channel.send_command(RoborockCommand.SET_CHILD_LOCK_STATUS, params={_STATUS_PARAM: 0}) | |
| # Optimistcally update state to avoid an extra refresh | |
| self.lock_status = 0 | |
| try: | |
| await self.rpc_channel.send_command(RoborockCommand.SET_CHILD_LOCK_STATUS, params={_STATUS_PARAM: 1}) | |
| except Exception: | |
| # Optionally, log the exception here | |
| raise | |
| else: | |
| # Optimistically update state to avoid an extra refresh | |
| self.lock_status = 1 | |
| async def disable(self) -> None: | |
| """Disable the child lock.""" | |
| try: | |
| await self.rpc_channel.send_command(RoborockCommand.SET_CHILD_LOCK_STATUS, params={_STATUS_PARAM: 0}) | |
| except Exception: | |
| # Optionally, log the exception here | |
| raise | |
| else: | |
| # Optimistically update state to avoid an extra refresh | |
| self.lock_status = 0 |
| await self.rpc_channel.send_command( | ||
| RoborockCommand.CLOSE_VALLEY_ELECTRICITY_TIMER, | ||
| ) | ||
| # Optimistcally update state to avoid an extra refresh |
There was a problem hiding this comment.
Typo in comment: "Optimistcally" should be "Optimistically"
| async def disable(self) -> None: | ||
| """Disable the Flow LED status.""" | ||
| await self.rpc_channel.send_command(RoborockCommand.SET_FLOW_LED_STATUS, params={_STATUS_PARAM: 0}) | ||
| # Optimistcally update state to avoid an extra refresh |
There was a problem hiding this comment.
Typo in comment: "Optimistcally" should be "Optimistically"
| # Optimistcally update state to avoid an extra refresh | |
| # Optimistically update state to avoid an extra refresh |
| """Set the Do Not Disturb (DND) timer settings of the device.""" | ||
| await self.rpc_channel.send_command( | ||
| RoborockCommand.SET_DND_TIMER, | ||
| params=self.as_list(), | ||
| ) | ||
| # Optimistcally update state to avoid an extra refresh | ||
| self.enabled = 1 |
There was a problem hiding this comment.
The optimistic state update occurs after the command is sent, but if the command fails with an exception, the state will remain inconsistent. Consider moving the state update before the command and reverting on exception, or wrapping in a try-except to only update on success.
| await self.rpc_channel.send_command(RoborockCommand.SET_CHILD_LOCK_STATUS, params={_STATUS_PARAM: 1}) | ||
| # Optimistcally update state to avoid an extra refresh | ||
| self.lock_status = 1 |
There was a problem hiding this comment.
The optimistic state update occurs after the command is sent, but if the command fails with an exception, the state will remain inconsistent. Consider moving the state update before the command and reverting on exception, or wrapping in a try-except to only update on success.
2 participants
This changes the contract for traits that have commands that mutate state to always ensure they reflect the latest device state after the command completes.
Raw commands or commands that impact the state of other traits as a side effect do not have this behavior and need to be managed by the caller. This provides an inconsistency between parts of the API, but I think it is more intuitive than never refreshing state for simple traits.