-
-
Notifications
You must be signed in to change notification settings - Fork 139
Add support for X9 Pro Omni (ilt3k8) #1102
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
Conversation
Some other errors
I'm dumping everything here in the hope someone can help :) |
de70de3
to
05c3ab2
Compare
Did some cleaning to remove stuff that generated errors/warnings in logs but I feel like we miss some controls in HA given what is described in the file (see screenshots above). Still one error though:
|
pre-commit.ci autofix |
I'm happy to help but please be patient as I'm not online 24/7 :) About the Turedetect error, please enable debug logs and change the settings in the app and afterwards please analyse the changes in the command. Probably the api changed again |
I apologize if I came across as pushy, that wasn’t the intention at all! Controls
I didn't see the elephant in the room: the vacuum entity was there all along 🤦🏻♂️ True detect
So it looks like there's a second attribute expected: I suggest mapping it to an enum on our side:
And when sending:
This has the disadvantage of resetting the level value when switching to off, which may lead to an inconsistent experience when using both Ecovacs app and HA. On the other hand, it simplifies controls in HA. I tried implementing that but it touches more than the single robot I'm trying to implement and I got lost in the woods. I prefer leaving that to you, if that's OK. In the meantime, I'll remove the setting from this robot. Map
There are still error logs about the map, I don't know what to do about them and the map doesn't load. Do you have ideas? WaterShould I leave water level capabilities in place even if they don't work, pending #1100 ? OverallI disabled map, water, and true detect to get a good first version of this robot. At least it will be detected in Home Assistant and will allow triggering cleaning which is, in my opinion, what 80% of users need. We could defer the water and true detect features to a second iteration. The water level is already being fixed in #1100 and I feel like we have the information we need for True Detect; we "just"™ need to implement it. I feel like the map is a bigger thing though, maybe yet another new undocumented API. I added some TODOs in the comments to reflect these points. Let me know how you want to proceed 🙂 |
61ac2bc
to
ac94a66
Compare
ac94a66
to
a4c0220
Compare
a4c0220
to
cb4b17c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1102 +/- ##
==========================================
+ Coverage 93.72% 93.77% +0.04%
==========================================
Files 131 132 +1
Lines 5020 5058 +38
Branches 327 327
==========================================
+ Hits 4705 4743 +38
Misses 252 252
Partials 63 63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #1102 will not alter performanceComparing Summary
|
@@ -148,6 +148,7 @@ class LifeSpan(StrEnumWithXml): | |||
HAND_FILTER = "handFilter", "HandFilter" | |||
DUST_CASE_HEAP = "dustCaseHeap", "DustCaseHeap" | |||
STATION_FILTER = "spHeap", "SpHeap" | |||
WATER_SINK = "waterSink", "WaterSink" |
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.
After I bumped the library version in HA, we need to support it in HA by adding translations there.
Can you please do it?
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.
Sure, would you be able to point me to the instructions? I’ve never done that yet.
Also I’ll have to find out how to translate that 😛
Pls check this: I managed to add the water level from 1 to 50. |
@Crocmagnon I have an X9 Pro (not OMNI) |
Please don't comment on merged PRs if they were merged weeks ago. @shmuelzon Crocmagnon, just comment above your comment that he doesn't want to be pinged. Please respect his decision. |
Supersedes #997
Relates to #1052
I have no idea how to test this. Could someone point me in the correct direction? I didn't find contributing documentation, though I may have missed something :)
I have a working HomeAssistant dev environment, I don't know how to install my forked library to test the change.