Add XML command "SetFanSpeed" #560
Conversation
WalkthroughThe changes introduce new classes and methods to enhance command handling within the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (2)
deebot_client/commands/xml/common.py (2)
71-71: Add docstring for the class.The class
ExecuteCommandis missing a docstring.Apply this diff to add a docstring for the class:
class ExecuteCommand(XmlCommandWithMessageHandling, ABC): + """Command, which is executing something (ex. Charge)."""
88-92: Add docstring for the class.The class
XmlSetCommandis missing a docstring.Apply this diff to add a docstring for the class:
class XmlSetCommand(ExecuteCommand, SetCommand, ABC): + """Xml base set command. + + Command needs to be linked to the "get" command, for handling (updating) the sensors. + """
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- deebot_client/commands/xml/common.py (3 hunks)
- deebot_client/commands/xml/fan_speed.py (2 hunks)
- tests/commands/xml/test_fan_speed.py (2 hunks)
Additional context used
Ruff
tests/commands/xml/test_fan_speed.py
15-15:
..json.assert_set_commandimported but unusedRemove unused import:
..json.assert_set_command(F401)
50-50: f-string without any placeholders
Remove extraneous
fprefix(F541)
deebot_client/commands/xml/common.py
38-38: Use
Falseinstead ofFalse and ...Replace with
False(SIM223)
Additional comments not posted (1)
deebot_client/commands/xml/fan_speed.py (1)
5-5: Remove unused import.The import
TYPE_CHECKINGis not used in the file.Apply this diff to remove the unused import:
-from typing import TYPE_CHECKINGLikely invalid or redundant comment.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## dev #560 +/- ##
==========================================
- Coverage 93.02% 93.01% -0.01%
==========================================
Files 199 199
Lines 6608 6629 +21
Branches 292 293 +1
==========================================
+ Hits 6147 6166 +19
- Misses 396 398 +2
Partials 65 65 ☔ View full report in Codecov by Sentry. |
flubshi
force-pushed
the
xml_set_fanspeed
branch
2 times, most recently
from
70ae3c9 to
b968b6c
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- deebot_client/commands/xml/common.py (3 hunks)
- deebot_client/commands/xml/fan_speed.py (2 hunks)
- tests/commands/xml/test_fan_speed.py (2 hunks)
Additional context used
Ruff
tests/commands/xml/test_fan_speed.py
15-15:
..json.assert_set_commandimported but unusedRemove unused import:
..json.assert_set_command(F401)
deebot_client/commands/xml/common.py
38-38: Use
Falseinstead ofFalse and ...Replace with
False(SIM223)
Additional comments not posted (8)
deebot_client/commands/xml/fan_speed.py (2)
Line range hint
12-46: LGTM!The
_handle_xmlmethod correctly processes the XML response and notifies the event bus for different fan speed levels.The code changes are approved.
49-59: Improve readability and maintainability.The constructor logic can be simplified for better readability and maintainability.
Apply this diff to simplify the constructor logic:
def __init__(self, speed: FanSpeedLevel | str) -> None: - if isinstance(speed, int): - speed = "strong" if speed in [1, 2] else "normal" + if isinstance(speed, int) and speed in [1, 2]: + speed = "strong" + elif isinstance(speed, int): + speed = "normal" super().__init__({"speed": speed})Likely invalid or redundant comment.
tests/commands/xml/test_fan_speed.py (4)
Line range hint
17-31: LGTM!The function correctly tests the
GetFanSpeedcommand for different fan speed levels using parameterized tests.The code changes are approved.
Tools
Ruff
15-15:
..json.assert_set_commandimported but unusedRemove unused import:
..json.assert_set_command(F401)
Line range hint
34-46: LGTM!The function correctly tests the
GetFanSpeedcommand for different error scenarios using parameterized tests.The code changes are approved.
48-51: LGTM!The function correctly tests the
SetFanSpeedcommand for the maximum fan speed level.The code changes are approved.
53-56: LGTM!The function correctly tests the
SetFanSpeedcommand for an invalid fan speed value.The code changes are approved.
deebot_client/commands/xml/common.py (2)
71-85: LGTM!The
_handle_xmlmethod correctly handles the success and failure of XML commands.The code changes are approved.
88-92: LGTM!The class correctly serves as a base for XML set commands and links to "get" commands for sensor updates.
The code changes are approved.
Comments failed to post (1)
deebot_client/commands/xml/common.py (1)
37-38: Fix the TypeError in
_get_payloadmethod.The FIXME comment indicates a potential issue with the
has_sub_elementmethod being a class method. Address this to prevent the TypeError.Apply this diff to fix the issue:
- # FIXME TypeError: 'classmethod' object is not callable - if False and self.has_sub_element: + if self.has_sub_element():Committable suggestion was skipped due to low confidence.
Tools
Ruff
38-38: Use
Falseinstead ofFalse and ...Replace with
False(SIM223)
flubshi
force-pushed
the
xml_set_fanspeed
branch
from
b968b6c to
87b1512
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- deebot_client/commands/xml/common.py (3 hunks)
- deebot_client/commands/xml/fan_speed.py (2 hunks)
- tests/commands/xml/test_fan_speed.py (2 hunks)
Additional context used
Ruff
tests/commands/xml/test_fan_speed.py
15-15:
..json.assert_set_commandimported but unusedRemove unused import:
..json.assert_set_command(F401)
deebot_client/commands/xml/common.py
38-38: Use
Falseinstead ofFalse and ...Replace with
False(SIM223)
Additional comments not posted (13)
deebot_client/commands/xml/fan_speed.py (5)
Line range hint
16-46: LGTM!The code changes are approved.
52-52: LGTM!The code changes are approved.
53-53: LGTM!The code changes are approved.
54-54: LGTM!The code changes are approved.
56-59: Improve readability and maintainability.The constructor logic can be simplified for better readability and maintainability.
Apply this diff to simplify the constructor logic:
def __init__(self, speed: FanSpeedLevel | str) -> None: - if isinstance(speed, int): - speed = "strong" if speed in [1, 2] else "normal" + if isinstance(speed, int) and speed in [1, 2]: + speed = "strong" + elif isinstance(speed, int): + speed = "normal" super().__init__({"speed": speed})tests/commands/xml/test_fan_speed.py (4)
Line range hint
18-29: LGTM!The code changes are approved.
Tools
Ruff
15-15:
..json.assert_set_commandimported but unusedRemove unused import:
..json.assert_set_command(F401)
Line range hint
32-45: LGTM!The code changes are approved.
48-51: LGTM!The code changes are approved.
53-56: LGTM!The code changes are approved.
deebot_client/commands/xml/common.py (4)
Line range hint
30-38: Address the FIXME comment.The FIXME comment indicates a potential issue with the
has_sub_elementmethod. The method is defined as a class method but is being called as an instance method.Apply this diff to fix the issue:
- # FIXME TypeError: 'classmethod' object is not callable - if False and self.has_sub_element: + if False and cls.has_sub_element():Tools
Ruff
38-38: Use
Falseinstead ofFalse and ...Replace with
False(SIM223)
Line range hint
49-68: LGTM!The code changes are approved.
71-85: LGTM!The code changes are approved.
88-92: LGTM!The code changes are approved.
flubshi
force-pushed
the
xml_set_fanspeed
branch
from
87b1512 to
e117ac2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- deebot_client/commands/xml/init.py (2 hunks)
- deebot_client/commands/xml/common.py (3 hunks)
- deebot_client/commands/xml/fan_speed.py (2 hunks)
- tests/commands/xml/test_fan_speed.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- deebot_client/commands/xml/fan_speed.py
Additional context used
Ruff
tests/commands/xml/test_fan_speed.py
15-15:
..json.assert_set_commandimported but unusedRemove unused import:
..json.assert_set_command(F401)
deebot_client/commands/xml/common.py
38-38: Use
Falseinstead ofFalse and ...Replace with
False(SIM223)
Additional comments not posted (8)
deebot_client/commands/xml/__init__.py (2)
11-11: LGTM!The import statement for
SetFanSpeedis correctly added.
21-21: LGTM!The addition of
SetFanSpeedto the__all__list is correctly done.tests/commands/xml/test_fan_speed.py (2)
9-9: LGTM!The import statement for
SetFanSpeedis correctly added.
48-51: LGTM!The new test function
test_set_fan_speedis correctly implemented.deebot_client/commands/xml/common.py (4)
11-14: LGTM!The import statements for
SetCommandandHandlingStateare correctly added.
71-73: LGTM!The new class
ExecuteCommandis correctly implemented.
74-85: LGTM!The new method
_handle_xmlis correctly implemented.
88-92: LGTM!The new class
XmlSetCommandis correctly implemented.
flubshi
force-pushed
the
xml_set_fanspeed
branch
from
e117ac2 to
136fb4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (2)
deebot_client/commands/xml/fan_speed.py (1)
Line range hint
23-45: Add a default case to thematchstatement.The
matchstatement does not handle unexpected values gracefully. Consider adding a default case to handle unexpected values.match speed.lower(): case "standard": event = FanSpeedEvent(FanSpeedLevel.NORMAL) case "strong": event = FanSpeedEvent(FanSpeedLevel.MAX) + case _: + _LOGGER.warning('Unexpected fan speed value: %s', speed)tests/commands/xml/test_fan_speed.py (1)
Line range hint
27-30: Remove extraneousfprefix.The f-string does not contain any placeholders.
json = get_request_xml(f"<ctl ret='ok' speed='{speed}'/>") json = get_request_xml("<ctl ret='ok' speed='{speed}'/>")Tools
Ruff
15-15:
..json.assert_set_commandimported but unusedRemove unused import:
..json.assert_set_command(F401)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- deebot_client/commands/xml/init.py (2 hunks)
- deebot_client/commands/xml/common.py (3 hunks)
- deebot_client/commands/xml/fan_speed.py (2 hunks)
- tests/commands/xml/test_fan_speed.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- deebot_client/commands/xml/init.py
Additional context used
Ruff
tests/commands/xml/test_fan_speed.py
15-15:
..json.assert_set_commandimported but unusedRemove unused import:
..json.assert_set_command(F401)
Additional comments not posted (5)
deebot_client/commands/xml/fan_speed.py (1)
56-59: Simplify the constructor logic.The constructor logic can be simplified for better readability and maintainability.
def __init__(self, speed: FanSpeedLevel | str) -> None: if isinstance(speed, int) and speed in [1, 2]: speed = "strong" elif isinstance(speed, int): speed = "normal" super().__init__({"speed": speed})Likely invalid or redundant comment.
tests/commands/xml/test_fan_speed.py (2)
48-51: LGTM!The code changes are approved.
53-56: LGTM!The code changes are approved.
deebot_client/commands/xml/common.py (2)
Line range hint
36-66: LGTM!The code changes are approved.
86-90: LGTM!The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deebot_client/commands/xml/init.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- deebot_client/commands/xml/init.py
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- deebot_client/commands/xml/init.py (1 hunks)
- deebot_client/commands/xml/fan_speed.py (2 hunks)
- deebot_client/events/fan_speed.py (1 hunks)
- tests/commands/xml/test_fan_speed.py (3 hunks)
Additional comments not posted (10)
deebot_client/commands/xml/__init__.py (4)
11-11: LGTM!The import statement is updated to reflect the new commands for cleaning speed control, which is consistent with the list of alterations.
19-20: LGTM!The
__all__list is updated to export the new commands for cleaning speed control, which is consistent with the list of alterations.
30-31: LGTM!The
_COMMANDSlist is updated to include the new commands for cleaning speed control, which is consistent with the list of alterations.
Line range hint
1-45: Overall assessment: The changes look good!The changes in this file are focused on replacing the fan speed control commands with cleaning speed control commands. The import statements,
__all__list, and_COMMANDSlist are updated consistently to reflect these changes.The AI-generated summary accurately captures the essence of the changes, which enhance the module's capabilities related to cleaning operations.
No further changes are required in this file.
tests/commands/xml/test_fan_speed.py (4)
22-29: LGTM!The changes to the test function and the parameterized test cases align with the shift from
GetFanSpeedtoGetCleanSpeed. The expected events have been updated correctly.
40-44: LGTM!The changes to the test function align with the shift from
GetFanSpeedtoGetCleanSpeed. The test cases for error scenarios remain valid.
47-50: LGTM!The new test function
test_set_fan_speedcorrectly validates the successful execution of theSetCleanSpeedcommand when provided with a valid speed level.
53-56: LGTM!The new test function
test_set_fan_speed_errorcorrectly validates the error scenario for theSetCleanSpeedcommand when provided with an invalid speed level.deebot_client/commands/xml/fan_speed.py (2)
Line range hint
20-46: LGTM!The changes to the
GetCleanSpeedclass are approved.
49-60: LGTM!The
SetCleanSpeedclass is correctly implemented and follows the naming convention.
flubshi
changed the title
flubshi
force-pushed
the
xml_set_fanspeed
branch
from
3ba6936 to
e4dc31d
Compare
CodSpeed Performance ReportMerging #560 will not alter performanceComparing Summary
|
| """Set clean speed command.""" | ||
|
|
||
| NAME = "SetCleanSpeed" | ||
| get_command = GetCleanSpeed |
There was a problem hiding this comment.
@edenhaus could you please help here:
I runtime tested get/set methods for my robot and the setting is changed as expected. From runtime testing this feature works as expected. However, there seems to be some typing issue. I think we should fix the issue for this PR before I continue implementing other Set-Commands.
mypy issue:
Incompatible types in assignment (expression has type "type[GetCleanSpeed]", base class "SetCommand" defined the type as "type[GetCommand]") [assignment]
Could you please help me fixing this issue or better fix this issue directly, such that I have a blueprint for the further PRs? Thanks a lot!
There was a problem hiding this comment.
The error is correct as GetCleanSpeed is missing the base class GetCommand.
In json I have a class where the required function is implemented.
client.py/deebot_client/commands/json/common.py
Lines 87 to 98 in dc55832
The best is probably to create also one for XML and then change the base class of GetCleanSpeed from XmlCommandWithMessageHandling to the new class.
Feel free to ask if you need further assistance :)
There was a problem hiding this comment.
@edenhaus I implemented it in ca7ed53 analogous to the JSON function. Tests are working and runtime testing is also fine.
However, especially the implementation in GetCleanSpeed->_handle_body_data_dict feels wrong.
There was a problem hiding this comment.
_handle_body_data_dict is used in json commands as there the data is a dictionary.
handle_set_args should call the correct function on the GetCommand with the args of the SetCommand to simulate the response of the GetCommand (so we can update HA).
In the case of FanSpeed, you need somehow to create <ctl ret='ok' speed='{speed}'/>. Not sure if you have a generic solution to create it. If that is to complicated you could also create a new abstract function that needs to be implemented in every getCommand. This function is than called with the args of the set command and will notify the event bus
There was a problem hiding this comment.
Thanks @edenhaus for your reply! If I understand your message correctly, linking of setCommand and getCommand is only used to re-use the "parse method" of the getCommand to "fake" a response that confirms the set. Wouldn't it be easier to simply send a bus notification event_bus.notify(FanSpeedEvent(FanSpeedLevel(int(data["speed"])))) in SetFanSpeed?
For the current implementation:
I think, I would like to implement the non-generic solution and implement it in every getCommand. At least for now. When implemented a few more getCommands, maybe I can improve the implementation in future.
When implementing it in every getCommand for now, would it be fine to make handle_set_args in XmlGetCommand abstract and implement it in every getCommand for now? Like in dd7a3dd ?
Are there tests to check if this set-/get-mechanism is working as expected? Using the example from readme and calling await bot.execute_command(SetCleanSpeed(FanSpeedLevel.STANDARD)) seems not to call handle_set_args.
There was a problem hiding this comment.
When implementing it in every getCommand for now, would it be fine to make handle_set_args in XmlGetCommand abstract and implement it in every getCommand for now? Like in dd7a3dd ?
That is perfectly fine :)
Wouldn't it be easier to simply send a bus notification event_bus.notify(FanSpeedEvent(FanSpeedLevel(int(data["speed"])))) in SetFanSpeed?
No, as this would work only for commands sent by HA. For example, the mqtt client subscribes to any command sent to the bot and if he detects a successful command sent to the bot, it will call handle_set_args with the arguments of the command.
client.py/deebot_client/mqtt_client.py
Line 334 in c6c4bdf
(Just realized that we currently subscribe only for json commands, but that we should fix in a different PR) Will be fixed with #779, but I your feedback for it
Are there tests to check if this set-/get-mechanism is working as expected? Using the example from readme and calling await bot.execute_command(SetCleanSpeed(FanSpeedLevel.STANDARD)) seems not to call handle_set_args.
I'm simulating it in the test by manually calling the mqtt function
client.py/tests/commands/json/__init__.py
Lines 73 to 77 in c6c4bdf
There was a problem hiding this comment.
Now, since you @edenhaus merged #779 and I merged dev into this Branch, I get
DEBUG:deebot_client.mqtt_client.client:Received PUBLISH (d0, q0, r0, m0), 'iot/p2p/SetCleanSpeed/HelperMQClientId-awseu-sts-ngiot-mqsjmq-59/ecosys/1234/1234a07a-40ec-4b82-abcd-123456789012/ls1ok3/Y1OW/q/pUgV/x', ... (24 bytes)
DEBUG:deebot_client.mqtt_client:Got message: topic=iot/p2p/SetCleanSpeed/HelperMQClientId-awseu-sts-ngiot-mqsjmq-59/ecosys/1234/1234a07a-40ec-4b82-abcd-123456789012/ls1ok3/Y1OW/q/pUgV/x, payload=b'<ctl speed="standard" />'
DEBUG:deebot_client.mqtt_client:Command SetCleanSpeed does not support p2p handling (yet)
DEBUG:deebot_client.mqtt_client.client:Received PUBLISH (d0, q0, r0, m0), 'iot/p2p/SetCleanSpeed/1234a07a-40ec-4b82-abcd-123456789012/ls1ok3/Y1OW/HelperMQClientId-awseu-sts-ngiot-mqsjmq-59/ecosys/1234/p/pUgV/x', ... (15 bytes)
DEBUG:deebot_client.mqtt_client:Got message: topic=iot/p2p/SetCleanSpeed/1234a07a-40ec-4b82-abcd-123456789012/ls1ok3/Y1OW/HelperMQClientId-awseu-sts-ngiot-mqsjmq-59/ecosys/1234/p/pUgV/x, payload=b"<ctl ret='ok'/>"
DEBUG:deebot_client.mqtt_client:Command SetCleanSpeed does not support p2p handling (yet)
There seems something missing in this PR, since I would expected that the implementation handles p2p. But log contains Command SetCleanSpeed does not support p2p handling (yet).
There was a problem hiding this comment.
p2p was not enabled for xml. I did it for you with the commit 4c6e862 in this PR
flubshi
changed the title
| @@ -14,8 +14,10 @@ class FanSpeedLevel(IntEnum): | |||
|
|
|||
| # Values should be sort from low to high on their meanings | |||
| QUIET = 1000 | |||
| STANDARD = -1 | |||
There was a problem hiding this comment.
Can we do the same as what we did with the Lifespan enum
Adds support for deebot 900 XML command "SetFanSpeed".
@edenhaus Could you please review?
Runtime tested:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests