Robonect Binding #2249
Robonect Binding #2249
Conversation
kaikreuzer
added
the
new binding
|
Great that you took the challenge to develop an openHAB2 Binding! Thanks! As soon as authentication is working I will start testing and give you feedback. |
|
@itheiss Thank you! Great! I committed the authentication bit. So when adding the thing you can optionally pass username and password as well as the poll interval. On my mower it looked so far ok, although I haven't done extensive tests yet... |
|
Hi i've just installed your Robonect Binding via copying in the addons folder. |
|
Hi idznak, Did you install OH 2.0 release or 2.1-Snapshot? Also do you have any error logs? |
|
Strange... now it's there. Seams like it just take some time to show up. OpenHAB-2.1.0.004-SNAPSHOT-DSM6-syno-noarch-0.001.spk |
There was a problem hiding this comment.
Pretty cool that you started this binding, can't wait for my module to arrive and trying it out!
I left a few comments inline already, although I noticed this is WIP and you are probably still refining some parts. Pretty decent state of the binding though already!
One general aspect I noticed is that all your files are lacking license headers and author comments (see Coding Guidelines - Code Style). While they feel like bloat, they are unfortunately required legally in order to stay clean wrt licensing.
|
|
||
| <name>Robonect Binding</name> | ||
| <description>This is the binding for Robonect.</description> | ||
| <author>marco.meyer</author> |
There was a problem hiding this comment.
Unless you really prefer to have it as an "alias", just write your name here without the dot and with capitals.
| @@ -0,0 +1,13 @@ | |||
| # FIXME: please substitute the xx_XX with a proper locale, ie. de_DE | |||
There was a problem hiding this comment.
Just delete this file if you don't need it
| </channel-groups> | ||
| <config-description> | ||
| <parameter name="host" type="text" required="true"> | ||
| <label>Host</label> |
There was a problem hiding this comment.
Please give UIs a hint on how they should render this field by adding a <context>network-address</context> here (see also Supported Contexts)
There was a problem hiding this comment.
good point. did not know about the context :-)
| <description>The user id if authentication has been enabled on the mower</description> | ||
| </parameter> | ||
| <parameter name="password" type="text" required="false"> | ||
| <label>Password</label> |
| <description>The password if authentication has been enabled on the mower</description> | ||
| </parameter> | ||
| <parameter name="pollInterval" type="integer" required="false" unit="s"> | ||
| <label>Polling interval.</label> |
| updateStatus(ThingStatus.ONLINE); | ||
| } catch (RobonectCommunicationException rce) { | ||
| logger.info("Failed to communicate with the mower. Taking it offline.", rce); | ||
| updateState(CHANNEL_STATUS, new DecimalType(MowerStatus.OFFLINE.getStatusCode())); |
| public class ModelParser { | ||
|
|
||
|
|
||
| private Gson gson; |
|
|
||
| private final Logger logger = LoggerFactory.getLogger(RobonectClient.class); | ||
|
|
||
| private HttpClient httpClient; |
|
|
||
| private HttpClient httpClient; | ||
|
|
||
| private ModelParser parser; |
| @@ -0,0 +1,302 @@ | |||
| package org.openhab.binding.robonect.handler; | |||
There was a problem hiding this comment.
kudos for writing tests! 👍
please move these into a separate test fragment though
There was a problem hiding this comment.
Usually makes my life easer and coding faster... ;-)
Will move it of course.
|
@SJKA Thank you a lot for the valuable early review. I had quite some learnings and hope this will finally bring me closer to an accepted PR. Let me know once your module is installed and you tried it out, if you lack any features. |
|
Argh, your the merge of the latest master into your branch was not good. Now your PR became unreadable and it looks like it grew to 27k lines of code. Can your try rebasing your branch instead (i.e. |
|
ups, @SJKA thank you for the tips. OK, I think I managed to do the proper rebasing.... First I had to revert by using |
reyem
force-pushed
the
2240-robonect-binding
branch
5 times, most recently
from
8f10e4b
to
1e79b7c
Compare
reyem
force-pushed
the
2240-robonect-binding
branch
from
c6a474e
to
18c23c3
Compare
|
@reyem Please note that, because it's marked [WIP], I do and did not review this PR completely yet. If it's finished and ready to review please update the title and mention me so I can remove the "Work in progress" label. |
reyem
force-pushed
the
2240-robonect-binding
branch
from
2034d5a
to
203acb1
Compare
|
@martinvw, thank you for your notice and time. I did my final changes and the PR is ready for review. I think @SJKA brought me already on the right path with an early review (thank you!), and some users are already using the binding in production like @DanielMalmgren and of course myself. So from the runtime it looks stable. |
reyem
force-pushed
the
2240-robonect-binding
branch
from
254e4a5
to
7c7909d
Compare
reyem
force-pushed
the
2240-robonect-binding
branch
from
7c7909d
to
29af20c
Compare
|
Hi, This Binding is great. I installed it yesterday morning and it worked perfectly. Today, my mower flaps between offline (just in openhab2) and online. But i can reach the Webinterface perfectly. |
|
@BOFH90 Do you get any more relevant info in the log if you set the binding to bebug logging? Run the following in Karaf console: |
|
@DanielMalmgren |
|
Too bad this binding didn't get it into 2.2 :-( Is it ok to give @kaikreuzer and @martinvw a little nudge? |
Although the binding polls the API, the beep is caused by the robonect module itself. I have a 105 myself running firmware 0.9e and do not have that issue. It just beeps if I change the mode from the binding. Maybe ask in the robonect forum(http://robonect.de) for advice on how to get rid of the beep... |
Yep, this seems to be a side effect of upgrading to the latest betas. Also noticed it yesterday (I mentioned it in the Robonect thread of the OH forum). I also threw the question out in the Robonect forum (http://robonect.de/viewtopic.php?f=9&t=365&p=12265#p12265) but haven't got any response yet. I've had to remove the AM from OH for now because of this, so I hope Fabian solves it... |
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
…tem files have to be updated! Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
|
@martinvw @kaikreuzer @SJKA @all-snapshot-users: Please check the Readme.md for the new channel names. Unfortunately your item files will not work anymore with the newest SNAPSHOT. Sorry :-/ |
|
I just updated to the latest snapshot and it's indeed a great improvement 👍 |
…e version info properties if thing was offline. Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
|
Any chance we get this merged before openHAB 2.3 code freeze? |
|
from my side all requested changes are done... So @martinvw @kaikreuzer would be great if you find the time for the final review and approve. :-) |
There was a problem hiding this comment.
Thanks @reyem, although I have a Gardena Sileno mower, I took the time to review this binding ;-)
I'd mainly wish to do some further optimisation on the channels - please find my comments below.
|
|
||
| <channel-type id="serialType"> | ||
| <item-type>String</item-type> | ||
| <label>Robonect serial number</label> |
There was a problem hiding this comment.
I assume the serial number will never dynamically change - this should thus not be modelled as a channel, but rather added as a property.
There was a problem hiding this comment.
actually I already implemented it as property, but forgot the definition in the xml :-) txh
|
|
||
| <channel-type id="versionType"> | ||
| <item-type>String</item-type> | ||
| <label>Robonect version</label> |
|
|
||
| <channel-type id="compiledType"> | ||
| <item-type>String</item-type> | ||
| <label>Robonect version</label> |
|
|
||
| <channel-type id="commentType"> | ||
| <item-type>String</item-type> | ||
| <label>Robonect firmware comment</label> |
| <channel-type id="nameType"> | ||
| <item-type>String</item-type> | ||
| <label>Mower name</label> | ||
| <description>The name of the mower</description> |
There was a problem hiding this comment.
hmmm, here I'm unsure. Although the use case to change the name via OH is rather hypothetic. But if I want to get this via in the properties section I have to do an extra call. On the other hand handling it with the other channels via polling does not cost anything. The name is gettable and settable, thus, why not leave it as channel?
There was a problem hiding this comment.
Ok, right, as it is settable, it indeed makes sense as a channel!
| <channel id="timer-status" typeId="timerStatusType"/> | ||
| <channel id="timer-next" typeId="nextTimerType"/> | ||
|
|
||
| <channel id="job-remote-start" typeId="remoteStartType"/> |
There was a problem hiding this comment.
I see that @SJKA already requested to remove these, an idea that you are not very fond of.
But this is really ugly (imho nastier than json-string-channels-on-lametric) as you are using multiple channels for parametrizing an RPC call - the channels by themselves have no value/functionality., thus they should not be channels.
May I suggest some compromise that a) makes it more user friendly for dummy users and b) keeps the full functionality available for the power user? We could:
- Add a switch channel type like "mow", which could have parameters "period" and "afterMode". Such a channel would make it terrifically easy to trigger the mowing manually, e.g. via a voice assistant like Alexa. With the right configuration, this might just be fine for 95% of the cases.
- Add an advanced "scheduleJob" string channel to which you can send the parameters of your RPC call (something like "14:30,15:30,EOD"). Once we have rule actions in place, this channel could then be easily replaced by a proper action.
Wdyt?
There was a problem hiding this comment.
I like the switch proposal! Just have to find out on how to parameterize channels though. But I guess I will find it in the docu or other bindings...
There was a problem hiding this comment.
Yeah, the docu is still a bit weak on that, but check the Astro binding as an example with this config for a channel.
There was a problem hiding this comment.
So, done as well... Nice would be if one could configure multiple channels of theses, but I think this is now a good intermediate way until the action concept is ready.
| # Robonect Binding | ||
|
|
||
| Robonect is a piece of hardware which has to be put into your Husqvarna, Gardena and other branded automower and makes | ||
| it accessible in your internal network. More details about the Robonect module can be found at [robonect.de](http://www.robonect.de) |
There was a problem hiding this comment.
Please add a line break after every sentence.
| RobonectConfig robonectConfig = getConfigAs(RobonectConfig.class); | ||
| RobonectEndpoint endpoint = new RobonectEndpoint(robonectConfig.getHost(), robonectConfig.getUser(), | ||
| robonectConfig.getPassword()); | ||
| httpClient = new HttpClient(); |
There was a problem hiding this comment.
The Jetty client uses a lot of resources, for that reason, we have a shared instance available in ESH that should be used.
Your code should be easy to adapt: Simply depend on the HttpClientFactory in your handler factory and pass the http client instance to the handler in the constructor - see this as an example.
There was a problem hiding this comment.
This is something @SJKA already mentions. But after mentioning that I had to customize the authentication store of the HttpClient instance, he was also skeptical if this is gonna be a good idea.
Concrete I do the following in RobonectClient to get Preemptive Authentication working
AuthenticationStore auth = httpClient.getAuthenticationStore();
URI uri = URI.create(baseUrl);
auth.addAuthenticationResult(new BasicResult(HttpHeader.AUTHORIZATION, uri, "Basic " + B64Code
.encode(endpoint.getUser() + ":" + endpoint.getPassword(), StandardCharsets.ISO_8859_1)));Do you think this will not have a side effect on others using this client? theoretically the authorizationResult is stored together with the URL, thus it could work..
During 2.2 development this was requrired, as the version of jetty client was quite old. Did jetty client version change since 2.2.0?
There was a problem hiding this comment.
No, Jetty is still 9.3.15(+) - so yes, I agree that it is probably safer to keep a separate client instance.
Nonetheless, you might want to retrieve it from HttpClientFactory.createHttpClient() as this will use a smaller thread pool (that can be configured for the system) and named threads, so that it is easier to find them in debugging sessions.
| httpClient.start(); | ||
| robonectClient = new RobonectClient(httpClient, endpoint); | ||
| } catch (Exception e) { | ||
| logger.error("Exception when trying to initialize", e); |
There was a problem hiding this comment.
Is this here an irrecoverable situation with a failing handle instantiation? In that case, don't set the thing status, but throw an exeption from this method.
If it is merely a "COMMUNICATION_ERROR" and your polling runnable will retry, reduce the logging to debug.
And if it might be the one or the other, better catch more specialized exceptions as catching Exception is anyhow not adviced.
There was a problem hiding this comment.
it's the httpClient.start() which may throw an exception here... I guess with the shared httpClient instance this would disappear as well, right. Just still the question if PreEmptive Authentication counts as "specific configuration requirements" and therefore I cannot use it. In this case I understand I would throw the exception. Can I wrap it into a RuntimeException?
There was a problem hiding this comment.
Yeah, wrapping it in a RuntimeException should be fine in this case (as the Exception from Jetty is anyhow very unspecific).
There was a problem hiding this comment.
I don't know if it's related here, but every time the binding tries to poll my mower and doesn't succeed I get an error in the log, "Fatal transport error: java.util.concurrent.TimeoutException". Not being able to reach it should be considered quite normal, classing it as an error doesn't seem right.
There was a problem hiding this comment.
Sounds like a good example why more finegrained exceptions should be caught and handled differently - a TimeoutException should indeed set the Thing to OFFLINE-COMMUNICATION_ERROR and do a periodic retry.
There was a problem hiding this comment.
Not too sure about that @kaikreuzer . There is a setting for offline timeout at Thing level, described by @reyem as "The maximum time the mower may be offline before the offline trigger is triggered." The mower being temporarily outside of range is normal and it's not an error unless it's not still unreachable after this timeout.
There was a problem hiding this comment.
actually this will not happen anymore. This was just possible before, as I was calling refreshInfo already the first time here in a previous version in the binding. But the refactoring removed this. So that should be ok.. now
| } | ||
| try { | ||
| if (httpClient != null) { | ||
| httpClient.stop(); |
There was a problem hiding this comment.
This will obviously have to be removed when using the shared instance.
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
* use HttpClientFactory to create HttpClient * extracted trigger channel from channels table Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
* documented job Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
|
@kaikreuzer ok, I think now the binding is in a quite good shape. Thank you very much for the extremely valuable feedback. That made the binding a bit slimmer and better 👍 |
There was a problem hiding this comment.
Thanks again @reyem for your quick turn around and for the fruitful discussions!
After refreshing my browser 100 times, Github eventually let me view and merge the PR again - so let's go ahead, before the unicorns take over this PR! 🤣
|
🎉 no 🦄 took over! 😆 Many thanks to my first time user @DanielMalmgren for the reliable reports and eager testing. |
|
Thanks for good work @reyem ! I'm noticing that the review only took a little more than a year 🤣 Anyway, what happens now? Does this mean the binding will appear as downloadable in paperui before 2.3 release? |
|
Does this mean the binding will appear as downloadable in paperui before 2.3 release?
It is already there!
|
Yes, it is available in the 2.3.0-SNAPSHOT like all other bindings and will most likely make it in the final 2.3.0 Release.
Being part of the official bistro does not mean the effort stops. Next is extending the binding with features of the new and existing APIs of Robonect. On the TODO List is
The good thing now is, Anybody might create a PR and the PRs are likely much smaller with higher likelihood to get in in shorter time. But please from now on let's discuss enhancements, future improvement whishes in the community form. The currently open thread for this is: https://community.openhab.org/t/robonect-binding-automower-wlan-module/27609/51 Thx for all your help |
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
Signed-off-by: Marco Meyer <marco.g.meyer@gmail.com> (github: reyem)
Implemented features:
tested against robonect firmware 0.9c
Open/Limitations: