-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
base: dev
Are you sure you want to change the base?
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@NEVdataDyne Please don't test library pull requests with Home Assistant. You only needed to run the test py executing |
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? |
Co-authored-by: Robert Resch <[email protected]>
Co-authored-by: Robert Resch <[email protected]>
Co-authored-by: Robert Resch <[email protected]>
Co-authored-by: Robert Resch <[email protected]>
Co-authored-by: Robert Resch <[email protected]>
Tested successfully in HA with Deebot T20 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Co-authored-by: Robert Resch <[email protected]>
Co-authored-by: Robert Resch <[email protected]>
Co-authored-by: Robert Resch <[email protected]>
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 |
@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. |
@lukakama @edenhaus |
Can you please share a screenshot of the app when you set it to 60 minutes? |
@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. |
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. |
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. |
One option could be to handle it using 2 distinct entities:
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). |
@NEVdataDyne Can you please fix the merge conflicts and make sure the CI is passing. After that we can merge this feature |
@edenhaus : I think I solved the conflict. I added you as a collaborator on my repository so I suppose now you can change anything |
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:
I don't need access. Please fix the CI and we can merge this one |
@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 |
Adds the option to set the interval between two washing of the clean pads.
Replaces #339