openhab-addons
openhab-addons copied to clipboard
openhab
[ecovacs] Initial contribution
Add initial version of a binding for vacuum cleaners made by Ecovacs. It should work for all devices made in the last 3 years, but only a limited subset of those have been tested actively (the README spells out the respective devices).
Release for testing in the marketplace: here
@Flowdalic l would appreciate some help regarding the integration of Smack here. My problem is the following:
- I only need smack-core for this binding, as I don't need any actual chat features, and Ecovacs' XMPP server doesn't support them anyway
- The XMPP client binding, on the other side, does need IM features
- Since smack-core is loaded as OSGi bundle, there's only one instance of both code and static variables for both
- Smack's initializer code will try to load any relevant classes in the classpath, which will automagically enable any feature in the class path for all connections by a callback system in static variables
- This means that (at least) when the XMPP binding is loaded, the IM features (e.g. roster) are active for my connections as well
- Ecovacs' server does not support roster, so each connection attempt yields error messages and stack traces in the log
- Unfortunately, the XMPP connection to the vacuum seems a bit unstable, so I have to reconnect rather often, which leads to a lot of clutter in the log
I now need some suggestions how to proceed from here. Could I somehow disable the extended features just for a single connection (I tried, but didn't find a way)? Can the error message on roster reload failure be toned down in case the server doesn't support it? (I would have submitted a PR for that, but I seemingly can't build Smack on my machine; probably due to wrong Gradle version?) Is there another way to split the use cases of the XMPP and Ecovacs bindings? I'd appreciate any pointers. Thanks!
The WIP tag is only for the XMPP issue described above, everything else in the binding should be ready for review.
- This means that (at least) when the XMPP binding is loaded, the IM features (e.g. roster) are active for my connections as well
Have you tried Roster.setLoadedOnLogin(false)?
Can the error message on roster reload failure be toned down in case the server doesn't support it?
IIRC the XMPP roster is a non discoverable feature. But the IQ request that yield an IQ error response that indicates that the service does not support the roster, which we may could use to change the logging behavior.
I would have submitted a PR for that, but I seemingly can't build Smack on my machine; probably due to wrong Gradle version?
Smack currently needs Gradle 6.8.3 to build.
Is there another way to split the use cases of the XMPP and Ecovacs bindings?
Ideally, and if I understood your issue(s) correctly, we wouldn't need that.
Have you tried Roster.setLoadedOnLogin(false)?
I can not find that method in Smack's sources?
But the IQ request that yield an IQ error response that indicates that the service does not support the roster, which we may could use to change the logging behavior.
It does:
2022-02-03 07:43:50.151 [TRACE] [.internal.api.impl.EcovacsXmppDevice] - XXX: SENT (0): <iq id='GWpLy-10' type='get'><query xmlns='jabber:iq:roster'></query></iq>
2022-02-03 07:43:50.161 [TRACE] [.internal.api.impl.EcovacsXmppDevice] - XXX: RECV (0): <iq type="error" to="[email protected]/e7a4f07a" from="ecouser.net" id="GWpLy-10"><error type="cancel" code="501"><feature-not-implemented xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/></error></iq>
2022-02-03 07:43:50.180 [ERROR] [org.jivesoftware.smack.roster.Roster] - Exception reloading roster
org.jivesoftware.smack.XMPPException$XMPPErrorException: XMPP error reply received from ecouser.net: XMPPError: feature-not-implemented - cancel
at org.jivesoftware.smack.XMPPException$XMPPErrorException.ifHasErrorThenThrow(XMPPException.java:185) ~[?:?]
at org.jivesoftware.smack.XMPPException$XMPPErrorException.ifHasErrorThenThrow(XMPPException.java:179) ~[?:?]
at org.jivesoftware.smack.AbstractXMPPConnection$7.processStanza(AbstractXMPPConnection.java:1542) ~[?:?]
at org.jivesoftware.smack.AbstractXMPPConnection$5.run(AbstractXMPPConnection.java:1223) ~[?:?]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
at java.lang.Thread.run(Thread.java:829) [?:?]
... and that was my idea for the logging change: reduce the logging level if the error indicates the feature is not supported.
Smack currently needs Gradle 6.8.3 to build.
That one works, thank you. I'll submit a PR for the logging change. (Edit: PR done)
And thank you for the prompt and detailed responses :+1:
Have you tried Roster.setLoadedOnLogin(false)?
I can not find that method in Smack's sources?
Sorry it is Roster.setRosterLoadedAtLogin().
Sorry it is Roster.setRosterLoadedAtLogin().
That one works and fixes the log spam, thanks!
@openhab/add-ons-maintainers May I ask for a review and/or help with the OSGi question, please? Thanks.
@openhab/add-ons-maintainers Any chance of getting this reviewed/merged for 3.4? From my side it should be ready, only open question is the XMPP SASL jar seemingly not being auto-installed for all users.
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/ecovacs-vacuum-cleaners-binding/132989/1
Any chance of getting review on this? It's sitting since almost 1 year now. Since the binding has been in Marketplace testing during all that time, I consider it stable/ready for review; I lately just added some more features. Only open question is the XMPP install thing mentioned here
I can confirm that the binding works very well with the DEEBOT OZMO T8 AIVI for quite a number of months. Thanks @maniac103 for the contribution. I hope it will soon make it into 4.0
This pull request has been mentioned on openHAB Community. There might be relevant details there:
https://community.openhab.org/t/openhab-4-0-snapshot-discussion/142322/223
@maniac103 - removing "help wanted" as I assume you got the OSGi question sorted out?
removing "help wanted" as I assume you got the OSGi question sorted out?
@jlaur Unfortunately not, that's why the thread above is still open. Whenever people stumble upon this, I tell them to manually install the mentioned bundle (smack-sasl-javax) or to alternatively install the xmppclient binding. I don't know at all what the issue could be, though, since I do have it listed as dependency. Maybe it could be an issue with the kar file generation, where code not explicitly called isn't bundled into the kar, even if listed as dependency in feature.xml?
I'll get to applying your comments in the next few days.
removing "help wanted" as I assume you got the OSGi question sorted out?
@jlaur Unfortunately not, that's why the thread above is still open. Whenever people stumble upon this, I tell them to manually install the mentioned bundle (
smack-sasl-javax) or to alternatively install the xmppclient binding.
Okay, I have re-added the label. Perhaps the PR should be marked as draft to prevent it from being merged while this issue persists?
Perhaps the PR should be marked as draft to prevent it from being merged while this issue persists?
From my side it is ready though (that is, I do not have any additional improvement ideas at this point). The open thread should block it from being merged ... but I would really appreciate any insights on what I'm doing wrong there.
From my side it is ready though (that is, I do not have any additional improvement ideas at this point). The open thread should block it from being merged ... but I would really appreciate any insights on what I'm doing wrong there.
Unfortunately I'm not able to help with that. Open comments do not prevent a merge and can easily be missed as the comment history grows. The most effective way to block it from being merged is by marking it as draft, but otherwise I would suggest to add the "Work in progress" label.
The most effective way to block it from being merged is by marking it as draft, but otherwise I would suggest to add the "Work in progress" label.
Won't either ultimately drive reviewers away from reviewing this? The author isn't done yet, so it's pointless to review at this point.? The "Help wanted" label indeed seems like the most appropriate to me.
Won't either ultimately drive reviewers away from reviewing this?
The author isn't done yet, so it's pointless to review at this point.?
Potentially yes, but with some sense, since from a priority perspective, time is better spent reviewing PR's that can actually be merged soon. However, in this case I already started a review, and plan to continue although not entirely ready.
The "Help wanted" label indeed seems like the most appropriate to me.
Indeed that label is appropriate.
since from a priority perspective, time is better spent reviewing PR's that can actually be merged soon.
Just a remark regarding that prioritization from the perspective of a first time contributor: that mentioned approach, while I completely understand the rationale, potentially blocks contributions with minor/few, but hard to resolve, issues ad infinitum. As I have no experience at all with OSGi, I have no idea on how to resolve this issue (assuming there actually is an issue in my code), so there's absolutely no chance of ever getting rid of the 'not finished yet' label without external help. It would be great if there would be a way to flag 'I need help from experts/maintainers with a specific question'. (One unfortunately can't mention a team without being member of the OH org - which in this particular case I am only since a few days for being one of the Android app maintainers)
However, in this case I already started a review, and plan to continue although not entirely ready.
Thanks :+1:
since from a priority perspective, time is better spent reviewing PR's that can actually be merged soon.
Just a remark regarding that prioritization from the perspective of a first time contributor: that mentioned approach, while I completely understand the rationale, potentially blocks contributions with minor/few, but hard to resolve, issues ad infinitum. As I have no experience at all with OSGi, I have no idea on how to resolve this issue (assuming there actually is an issue in my code), so there's absolutely no chance of ever getting rid of the 'not finished yet' label without external help. It would be great if there would be a way to flag 'I need help from experts/maintainers with a specific question'. (One unfortunately can't mention a team without being member of the OH org - which in this particular case I am only since a few days for being one of the Android app maintainers)
I fully understand this frustration, but I think we are discussing two different things. You need:
- Help from anyone able to help (e.g. other contributors).
- Review from maintainers.
Perhaps help could be requested from the community forum in the Development category? I see @J-N-K just stepped in with some advise, so suggestion is general.
For the review part, labels and PR status are means for maintainers to ease decision making, e.g. for filtering - and yes, possibly prioritizing ready PR's higher than WIP, because of current capacity issues.
Draft status will effectively prevent the merge button from being accessible, while a lot of text conversations can be harder to interpret and ultimately could lead to a wrong decision being made. It should not happen, but we are only human. I just wanted to add this reasoning for my suggestion, although in this case I think other maintainers will now see these new comments and decide not to merge just yet. 😉
maniac103 force-pushed the ecovacs-rebase branch from 59a58e1 to 30d9487
I unfortunately had to rebase the PR branch to main as otherwise the binding didn't build for me:
[ERROR] Failed to execute goal org.apache.karaf.tooling:karaf-maven-plugin:4.3.7:verify (karaf-feature-verification) on project org.openhab.binding.ecovacs: Feature resolution failed for [openhab-binding-ecovacs/4.0.0.SNAPSHOT]
[ERROR] Message: Unable to resolve root: missing requirement [root] osgi.identity; osgi.identity=openhab-binding-ecovacs; type=karaf.feature; version=4.0.0.SNAPSHOT; filter:="(&(osgi.identity=openhab-binding-ecovacs)(type=karaf.feature)(version>=4.0.0.SNAPSHOT))" [caused by: Unable to resolve openhab-binding-ecovacs/4.0.0.SNAPSHOT: missing requirement [openhab-binding-ecovacs/4.0.0.SNAPSHOT] osgi.identity; osgi.identity=openhab-runtime-base; type=karaf.feature [caused by: Unable to resolve openhab-runtime-base/4.0.0.SNAPSHOT: missing requirement [openhab-runtime-base/4.0.0.SNAPSHOT] osgi.identity; osgi.identity=openhab-core-base; type=karaf.feature [caused by: Unable to resolve openhab-core-base/4.0.0.SNAPSHOT: missing requirement [openhab-core-base/4.0.0.SNAPSHOT] osgi.identity; osgi.identity=org.openhab.core; type=osgi.bundle; version="[4.0.0.202303090301,4.0.0.202303090301]"; resolution:=mandatory [caused by: Unable to resolve org.openhab.core/4.0.0.202303090301: missing requirement [org.openhab.core/4.0.0.202303090301] osgi.wiring.package; filter:="(&(osgi.wiring.package=org.osgi.framework)(version>=1.10.0)(!(version>=2.0.0)))"]]]]
It contains only new changes from the main branch, I did not do any changes to the binding code during the rebase.
Draft status will effectively prevent the merge button from being accessible, while a lot of text conversations can be harder to interpret and ultimately could lead to a wrong decision being made.
I lost track. I still see the "help wanted" label. Besides the comments from my side, is this PR still blocked because of dependency issues?
I still see the "help wanted" label. Besides the comments from my side, is this PR still blocked because of dependency issues?
No, it should be fine with @J-N-K's comment.
@maniac103 - there are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.
there are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.
Fixed, thanks for the heads-up.
Now, you could add your binding's logo to the openHAB website.
Done: https://github.com/openhab/openhab-docs/pull/2035 Thanks for the review and merging!
Happy to see how much you invested into this after the initial poc we built together 😍 Well done! Keep the great work and thank you so much for this contribution!
