-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[Denon] Send commands over telnet connection + several other improvements #5342
Conversation
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.
Looks good aside from the thread issue.
Please also address the compiler warnings and run the code through the Eclipse formatter.
*/ | ||
private void requestState() { | ||
// Run in new thread to not delay the set up of the binding. Handling responses is done in the listener thread. | ||
new Thread(new Runnable() { |
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.
Dev guidelines say creation of threads must be avoided.
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.
Not sure how to fix this. Could you tell me where those existing schedulers are defined?
I have made a little change, the thread is now scheduled by an single threadExecutorService
(which is defined in the DenonConnector
) which is a bit more neat and guarantees that only 1 instance of the thread is being executed at a time.
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.
This looks good to me. There's not a lot of reference in the docs about how this should be done, but what you have looks like some of the other code I see in the repo.
@@ -22,7 +22,6 @@ Import-Package: javax.xml.bind, | |||
org.apache.commons.io, | |||
org.apache.commons.lang, | |||
org.apache.commons.net, |
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.
Eclipse complains that "No available bundle exports package 'org.apache.commons.net'"
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.
Since the org.apache.commons.net.telnet
import was removed the general .net
import was no longer necessary so I've also removed it.
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/denon-binding-for-new-avr-models/36188/2 |
@9037568 Can you merge this PR? |
Yes, I meant to get to this sooner. |
Thanks! |
This PR has several improvements for the 1.x Denon binding. It fixes issues found in #5175
TelnetClient
since the (not replaceable)TelnetOutputStream
it uses adds a '\n' to every command send to the receiver, which practically 'hangs' the connection after the second command is send.Connecting to null
in the logs)