3343 isy binding #5203
3343 isy binding #5203
Conversation
|
A couple of quick-hit notes while I dive into the rest of the code. Copyright dates are showing "2016". These should be updated to "2017" throughout. |
|
I'll fix those as soon as I get home... |
|
ok a little sed scripting and the copyrights are updated and the since tags are fixed -- I've thing I have the updates properly added to the pull request, but perhaps not -- @9037568 chris can you take a look and see if you see the additional commits beyond the original 3 on my pull request? |
| Bundle-Name: openHAB isy Binding | ||
| Bundle-SymbolicName: org.openhab.binding.isy | ||
| Bundle-Vendor: openHAB.org | ||
| Bundle-Version: 1.9.0.qualifier |
| private InsteonClient insteonClient; | ||
|
|
||
| /** | ||
| * Constructor, nothing to see here. |
There was a problem hiding this comment.
Remove ", nothing to see here"
|
|
||
| runtimeConfiguration = new ISYActiveBindingConfig(config); | ||
|
|
||
| setProperlyConfigured(true); |
There was a problem hiding this comment.
This should go at the end of the method.
There was a problem hiding this comment.
moved as requested.
|
|
||
| runtimeConfiguration = new ISYActiveBindingConfig(config); | ||
|
|
||
| setProperlyConfigured(true); |
There was a problem hiding this comment.
This should go at the end of the method.
There was a problem hiding this comment.
moved as requested.
| } | ||
|
|
||
| /** | ||
| * @{inheritDoc |
There was a problem hiding this comment.
{@inheritDoc}
Some of these were fixed with your two new commits, but others appear to still be in need of help. Please search and replace throughout.
There was a problem hiding this comment.
fixed here, will search and fix elsewhere.
There was a problem hiding this comment.
fixed in all places, fgrep -R confirmed...
| case 903/* System Not Initialized; Restart! */: | ||
| case 905/* Subscription Failed; The device might need reboot! */: | ||
| case 1020/* Couldn't create the event handler socket */: | ||
| fatalError(status, "Exit the applciation; might have to reboot ISY"); |
There was a problem hiding this comment.
"application"
Also, most of these conditions sound like references to the ISY device itself. Does the binding code not include any retry logic? My quick search for such didn't find any, but I may not have used the right terms.
There was a problem hiding this comment.
spelling fixed -- I don't know about the error handling, perhaps @sytone should comment, I am not that familiar with the algorithms used here.
| package org.openhab.binding.isy.internal; | ||
|
|
||
| /** | ||
| * This is the node types in ISY that are supported / translated. |
There was a problem hiding this comment.
"These are the node types ..."
| return; | ||
| } | ||
|
|
||
| // |
There was a problem hiding this comment.
Remove this empty comment line.
| this.logger.info("Authenticated"); | ||
| } | ||
| } catch (NoDeviceException ne) { | ||
| this.logger.error("No device", ne); |
There was a problem hiding this comment.
This should be a warn, not an error.
There was a problem hiding this comment.
changed to warning
| # If true, use UPNP to communicate with the ISY 994i. | ||
| # isy:upnp=false | ||
| # | ||
| # UUID of the ISY router, git it fron the admin panel. |
|
Thanks for the review, I will get to these tomorrow afternoon or this weekend. |
|
ok I made the remaining changes -- I built the jar file so things compile but my openhab is now on the 2.1 nightly build so I will need someone to test it on an openhab1 system. |
|
ok I pushed the last of the changes from your review, please let me know if I need to do anything else! |
|
Thanks, @mitchmitchell ! Much appreciated. |
|
Thank you!! |
updated with formatting and other details to comply with review comments for merging isy binding into openhab1