-
-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add XML command "SetFanSpeed" #560
Open
flubshi
wants to merge
15
commits into
DeebotUniverse:dev
Choose a base branch
from
flubshi:xml_set_fanspeed
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
136fb4d
Add XML command "SetFanSpeed"
flubshi 366e96a
Merge branch 'dev' into xml_set_fanspeed
edenhaus 315307a
Add feedback for Get/SetCleanSpeed commands
flubshi e4dc31d
Merge branch 'dev' into xml_set_fanspeed
flubshi 5ae93c3
XML SetCleanSpeed: Adjust NAME attribute
flubshi f80cc31
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 9c69dc7
Improve tests for XML clean_speed commands
flubshi faa24ea
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 967c07a
Merge branch 'dev' into xml_set_fanspeed
flubshi ca7ed53
xml clean_speed: add XmlGetCommand
flubshi 86e44f1
Merge branch 'xml_set_fanspeed' of github.com:flubshi/client.py into …
flubshi dd7a3dd
Implement handle_set_args of XmlGetCommand in every command
flubshi 8d9c8e9
Merge branch 'dev' into xml_set_fanspeed
flubshi 4c6e862
enable p2p for xml commands
edenhaus 528cbf9
Merge branch 'dev' into xml_set_fanspeed
edenhaus File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,10 @@ class FanSpeedLevel(IntEnum): | |
|
||
# Values should be sort from low to high on their meanings | ||
QUIET = 1000 | ||
STANDARD = -1 | ||
flubshi marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do the same as what we did with the Lifespan enum |
||
NORMAL = 0 | ||
MAX = 1 | ||
STRONG = -2 | ||
flubshi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
MAX_PLUS = 2 | ||
|
||
|
||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is correct as
GetCleanSpeed
is missing the base classGetCommand
.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
fromXmlCommandWithMessageHandling
to the new class.Feel free to ask if you need further assistance :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_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 busThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"]))))
inSetFanSpeed
?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
inXmlGetCommand
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 callhandle_set_args
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is perfectly fine :)
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 itI'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, since you @edenhaus merged #779 and I merged dev into this Branch, I get
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2p was not enabled for xml. I did it for you with the commit 4c6e862 in this PR