Skip to content
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 support for set/getWashInfo commands and onWashInfo message #387

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

lukakama
Copy link
Contributor

@lukakama lukakama commented Jan 5, 2024

On T20 Omni it is possible to control mops washing mode (with cold or hot water) and level (standard or deep washing).

These features are handled by getWashInfo and setWashInfo commands, with a push onWashInfo message which is sent when settings are changed.

The getWashInfo returns 3 different information:

  • mode[int]: this is the wash mode; it corresponds to "hot water cleaning" setting in the app, which can be disabled (value 0) or enabled (value 1)
  • hot_wash_amount[int]: this is the washing level of mops, as cycles of 3 water charges and discharges on the station; the app allows to select only 2 values, one for "standard" washing (value 1 - 3 water charges and discharges) and one for deep washing (value 3 - 9 water charges and discharges).
  • interval[int]: this is the interval between two mop washings while mopping.

The app uses the setWashInfo command to distrincly set mops washing mode and level, passing only mode or hot_wash_amount with the desired value. The interval, instead, is set using a different command setWashInterval (handled in the PR #376).

Performing a setWashInfo command results in a onWashInfo message to be received from the MQTT server (with all the information of the getWashInfo command), regardless if setWashInfo is invoked passing only mode or only hot_wash_amount. The onWashInfo message is sent also when is performd a setWashInterval command.

Copy link

codecov bot commented Jan 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.83%. Comparing base (e766567) to head (52f19e4).
Report is 4 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #387      +/-   ##
==========================================
+ Coverage   83.30%   83.83%   +0.53%     
==========================================
  Files          74       78       +4     
  Lines        2977     3075      +98     
  Branches      531      541      +10     
==========================================
+ Hits         2480     2578      +98     
  Misses        443      443              
  Partials       54       54              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor Author

@lukakama lukakama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I created 2 distinct commands (and thus two capabilities), one to set the only the mode and another to set only the hot_wash_amount, in order to mimic the app which handles them distinctly, but they could be merged into a single command that, however, should then send only one parameter between mode and hot_wash_amount when executed.

Also, due this asymmetry between get command (which returns 3 attributes) and set one (which support only 1, or 2 in case of merging), I had difficulties implementing set command tests. Any help is welcome :) .

@lukakama
Copy link
Contributor Author

lukakama commented Jan 6, 2024

Additionally, I used an Enum for mode, as, IMHO, the name seems much more related to a "washing mode" than to a "hot water washing on/off" option, where currently it is supported only "standard washing" and "hot water washing", and so, for future proof compatibility, an Enum allows to easily handle additional values.
However, for the sake of simplicity, it can be changed to a bool "hot water on/off" value, but then, in case of future additional modes, a library (and library usages) refactor will be needed.

@lukakama
Copy link
Contributor Author

lukakama commented Jan 6, 2024

UPDATE: I did some additional tests and I can tell that, when mode is set to 0, mops are always washed using a single wash cycle (3 charges and discharges of the station), regardless the value of hot_wash_amount, so it resemble even more that mode set to 0 correspond to a "Standard washing" (cold water with 1 cycle), and set to 1 correspond to a "Hot water washing" (Hot water with a configurable number of cycles).

@lukakama
Copy link
Contributor Author

lukakama commented Jan 7, 2024

UPDATE: I just found that the library does not support multiple command classes mapped to the same MQTT command in p2p commands handling:

def _handle_p2p(
self, topic_split: list[str], payload: str | bytes | bytearray
) -> None:
try:
if (data_type := DataType.get(topic_split[11])) is None:
_LOGGER.warning('Unsupported data type: "%s"', topic_split[11])
return
command_name = topic_split[2]
command_type = COMMANDS_WITH_MQTT_P2P_HANDLING.get(data_type, {}).get(
command_name, None
)

As it can be seen, the code look for a command class mapped to the MQTT command topic, but in the PR there are 2 classes, SetWashInfoMode and SetWashInfoHotWashAmount, mapped to the same setWashInfo MQTT command topic, and only one of them is added to the COMMANDS_WITH_MQTT_P2P_HANDLING mapping dictionary (I think that it depends on the order of class loading, where the last command loaded overwrite the previous and become the one used to handle p2p commands for that type). This then lead the code to raise an error when handling p2p setWashInfo commands when parameters does not match ones of the command class mapped into COMMANDS_WITH_MQTT_P2P_HANDLING in this method, as it would have only one of mode and hot_wash_amount parameters:

def _pop_or_raise(name: str, type_: type, data: dict[str, Any]) -> Any:
try:
value = data.pop(name)
except KeyError as err:
msg = f'"{name}" is missing in {data}'
raise DeebotError(msg) from err
try:
return type_(value)
except ValueError as err:
msg = f'Could not convert "{value}" of {name} into {type_}'
raise DeebotError(msg) from err

@edenhaus I don't know how to proceed... It seems that to support this setWashInfo command with different parameters it is required some sort of refactor on other parts of the library which are currently quite complex for me to identify, and that will also depend on how this command handling will be implemented, between using two command classes vs using a single command class that, however, will have two mutually exclusive parameters... Maybe we should talk about it on discord to identify how this can be managed.

Copy link
Contributor

@edenhaus edenhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. It was a long month by porting the custom integration to core

deebot_client/commands/json/wash_info.py Outdated Show resolved Hide resolved
deebot_client/events/wash_info.py Outdated Show resolved Hide resolved
deebot_client/commands/json/wash_info.py Outdated Show resolved Hide resolved
deebot_client/commands/json/wash_info.py Outdated Show resolved Hide resolved
@edenhaus
Copy link
Contributor

edenhaus commented Feb 1, 2024

I think by merging both commands together I solved some issues you had.
When executing the command the caller can decide to pass both or only one argument

@lukakama lukakama force-pushed the wash_info branch 4 times, most recently from 7e59994 to 225ac58 Compare February 18, 2024 19:56
@lukakama
Copy link
Contributor Author

lukakama commented Feb 19, 2024

@edenhaus I refactored the code to work with a single command class, and thus I added a couple of tests for command execution, but it seems that the standard test assert_set_command does not handle well command with possible missing mqtt parameters (it rely on 1:1 relation between comand args and mqtt parameters). I don't know if it is just a test issue, or if it is something that could create issue at runtime. Do you have any hint on it? In case I could just change the test code in order to skip mqtt payload test.

@edenhaus
Copy link
Contributor

I fixed it for you.
First I added a default value to InitParam and afterwards I fixed the new command and the test for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants