-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[hue] Added configuration for port #4728
[hue] Added configuration for port #4728
Conversation
@martinvw Can you give me a hand on the The Philips Hue certificate looks like this - but I do not know if it is sufficient because the idea behind this PR is to allow the Hue binding to connect to similar Hue APIs like deCONZ too.
Current state is that HTTPS is not working because of this exception:
|
@@ -534,6 +535,7 @@ private void updateBridgeThingConfiguration(String userName) { | |||
} | |||
|
|||
private void handleAuthenticationFailure(Exception ex, String userName) { | |||
logger.debug("{}", ex); |
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.
To be removed after we found a solution for the occurring exception.
Before we run into merge conflicts at some point, can we merge this? At least the port fix looks good and https needs further tests, I agree. But better having that in the main tree than rotting, I guess? |
I thought about splitting this up into two separate PRs. One for the port and one for the HTTPS connection. I'll be the nicest way. |
Signed-off-by: Christoph Weitkamp <[email protected]>
Signed-off-by: Christoph Weitkamp <[email protected]>
7597286
to
472dbd5
Compare
@davidgraeff I removed all HTTPS related changes which were visible for the user and obsolete code. |
try { | ||
URI uri = new URI(protocol, null, ip, port, path.isEmpty() ? basePath : basePath + "/" + path, null, null); | ||
return uri.toString(); | ||
} catch (URISyntaxException e) { |
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.
Should we do some logging here? Its unlikely that the url is correct?
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.
Yes, I will. And I will do a second change: I will move the creation of the URI
into the constructor. It is sufficient to resolve it once and not every time we want to connect to the Hue API.
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.
Done. But let me test it for at least one day before we merge.
Signed-off-by: Christoph Weitkamp <[email protected]>
91984bb
to
a2ad04e
Compare
It looks fine. Works smoothly without unexpected errors. I will give it a go. |
I'll merge it in then. The hue binding is really causing havoc on my system because of it constantly forgetting the port at the moment (and colliding with hue emulation therefore). |
* Added configuration for port and protocol Signed-off-by: Christoph Weitkamp <[email protected]>
* Added configuration for port and protocol Signed-off-by: Christoph Weitkamp <[email protected]> Signed-off-by: Pshatsillo <[email protected]>
* Added configuration for port and protocol Signed-off-by: Christoph Weitkamp <[email protected]> Signed-off-by: Maximilian Hess <[email protected]>
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.
N
Continues eclipse-archived/smarthome#6854
Signed-off-by: Christoph Weitkamp [email protected]