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 wash interval commands #376

Open
wants to merge 21 commits into
base: dev
Choose a base branch
from
Open

Conversation

NEVdataDyne
Copy link

@NEVdataDyne NEVdataDyne commented Dec 18, 2023

Adds the option to set the interval between two washing of the clean pads.

Replaces #339

@NEVdataDyne

This comment was marked as off-topic.

@NEVdataDyne

This comment was marked as off-topic.

tests/commands/json/test_wash_interval.py Outdated Show resolved Hide resolved
deebot_client/commands/json/wash_interval.py Show resolved Hide resolved
deebot_client/commands/json/wash_interval.py Outdated Show resolved Hide resolved
deebot_client/commands/json/wash_interval.py Outdated Show resolved Hide resolved
deebot_client/commands/json/wash_interval.py Outdated Show resolved Hide resolved
@edenhaus
Copy link
Contributor

@NEVdataDyne Please don't test library pull requests with Home Assistant. You only needed to run the test py executing pytest in the terminal

@NEVdataDyne
Copy link
Author

@NEVdataDyne Please don't test library pull requests with Home Assistant. You only needed to run the test py executing pytest in the terminal

Thanks for the help, I am not sure if you mean running test.py on the home assistant machine using something like an ssh add on or on GitHub or somewhere else? And how?

@NEVdataDyne
Copy link
Author

Tested successfully in HA with Deebot T20

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ed59c71) 83.11% compared to head (a2fab23) 83.29%.
Report is 1 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #376      +/-   ##
==========================================
+ Coverage   83.11%   83.29%   +0.17%     
==========================================
  Files          70       72       +2     
  Lines        2855     2885      +30     
  Branches      511      514       +3     
==========================================
+ Hits         2373     2403      +30     
  Misses        434      434              
  Partials       48       48              

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

deebot_client/commands/json/wash_interval.py Outdated Show resolved Hide resolved
deebot_client/commands/json/wash_interval.py Outdated Show resolved Hide resolved
deebot_client/events/wash_interval.py Outdated Show resolved Hide resolved
@edenhaus edenhaus changed the title Clean pads interval (wash interval) Add wash interval commands Dec 28, 2023
@NEVdataDyne
Copy link
Author

The idea was for the comments to link with the name of the functionality in the UI but anyway I committed your request for change

@lukakama
Copy link
Contributor

lukakama commented Jan 4, 2024

@edenhaus just sharing that I tested the PR successfully on my T20 for both set and get commands, checking them using also the mobile app.
NB: The mobile app supports only values of 6, 10 and 15 minutes. Using arbitrary values, for example 12, lead to an undefined (unselected) "mop wash interval" inside the mobile app. I don't know how Ecovacs servers would handle these situations... maybe it is safer to use an Enum of supported values for the SetWashInterval command instead of an arbitrary number.

@NEVdataDyne
Copy link
Author

@lukakama @edenhaus
The robot works with any mop wash internal value (tested up to 60 minutes). Setting a bigger value than 15 min allows to wash with mops on a place where there is no docking station without the robot stopping to clean the mops. For that reason, limiting the values to 6, 10 and 15 is not an option.

@edenhaus
Copy link
Contributor

edenhaus commented Jan 4, 2024

The robot works with any mop wash internal value (tested up to 60 minutes)

Can you please share a screenshot of the app when you set it to 60 minutes?
Does the app show any error when it is set to 60 minutes? Can you change it back to the defined values in the app?

@NEVdataDyne
Copy link
Author

@edenhaus it doesn't show any error, no interval time is selected in the app. If one click on one of the pre defined time in the app it works as expected and the value is updated in ha.
Screenshot_20240104_183832_ECOVACS HOME

@lukakama
Copy link
Contributor

lukakama commented Jan 4, 2024

My concerns were more about the possibility to detect these "officially unsupported" configurations server side. I don't know how Ecovacs will handle them if detected (probably the worst that can happen is an account ban). However, if the desire is to keep the value unrestricted, a warning log message could be an acceptable compromise when using an unsupported app value, just to let users aware of it.

@edenhaus
Copy link
Contributor

edenhaus commented Jan 5, 2024

I'm fine with removing the app's limitation for the following command, but I agree with @lukakama that we should inform the user about it. Maybe we should also offer in the frontend only the value from the app, and the user needs to manually insert another value if he really wants it.

Still need to think about how I will present it in the frontend and any ideas are welcome.

@lukakama
Copy link
Contributor

lukakama commented Jan 6, 2024

Still need to think about how I will present it in the frontend and any ideas are welcome.

One option could be to handle it using 2 distinct entities:

  • One named as a standard "Mop/s wash interval" of domain "select" with the ability to set just standard values.
  • One named as "Advanced - Mop/s wash interval" (or "Unsupported- xxxx" to make it clear that its usage is at user own risk) of domain "number", to be explicitly and manually enabled in order to insert an arbitrary value.

Also I would say that when the advanced/unsupported entity is enabled, the standard one should be automatically disabled.

That would then need some sort of support from the client.py library code, in the form of additional Capability and Command classes metadata to report, somehow, the list of "standard" values for each specific command.

Additionally, but its not mandatory, it could be added an "Advanced/Unsupported mode" option to the HASS component configuration, in order to enable the creation of "Advanced/Unsupported xxx" entities. This way users must explicitly and manually opt-in to use such features, and it would be possible to add a short explanation of what it could cause (possible account bans, bot or app issues, as things could change between bot firmware, app, or Ecovacs servers updates).

@edenhaus
Copy link
Contributor

edenhaus commented Feb 1, 2024

@NEVdataDyne Can you please fix the merge conflicts and make sure the CI is passing.
I'm not allowed to push on your branch

After that we can merge this feature

@NEVdataDyne
Copy link
Author

@edenhaus : I think I solved the conflict. I added you as a collaborator on my repository so I suppose now you can change anything

@edenhaus
Copy link
Contributor

The Ci is failing. Please resolve the issue.

I still can't push the required changes to your branch as I get the following error:

remote: Resolving deltas: 100% (93/93), completed with 40 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/dev.
remote: error: Changes must be made through a pull request.
To github.com:NEVdataDyne/client.py
 ! [remote rejected] HEAD -> dev (protected branch hook declined)
error: failed to push some refs to 'github.com:NEVdataDyne/client.py'

I don't need access. Please fix the CI and we can merge this one

@NEVdataDyne
Copy link
Author

NEVdataDyne commented Feb 18, 2024

@edenhaus : I don't know what the CI is, could you please accept the invitation to collaborate on my repo before it expires and fix what has to be fixed?

https://github.com/NEVdataDyne/client.py/invitations

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.

3 participants