[tplinksmarthome] Added configuring devices by deviceId #4182
Conversation
This makes it resilient to ip address changes. It's not using the mac address for this because the mac address can also be changed (by the user). Signed-off-by: Hilbrand Bouwkamp <[email protected]>
| The thing can be configured or by `ipAddress` or by `deviceId`. | ||
| If the one of them is used the other is automatically set by the binding. | ||
| When manually configured it's preferred to set the `deviceId` because if the ip address of the device would change this will be automatically updated. | ||
| When setting the `deviceId` it can take up to 1 minute before the ip address is set. By manually triggering a discovery process it will update it faster. |
There was a problem hiding this comment.
@kaikreuzer @SJKA, is this okay? I remembered this comment: https://github.com/openhab/openhab2-addons/issues/2916#issue-279370881
There was a problem hiding this comment.
If this merely means that a manual discovery creates a DiscoveryResult quicker and that the framework then syncs the discovered ip address property from the DiscoveryResult to the Thing, this sounds alright to me as it does not mean that the discovery code is itself directly linked to the ThingHandlers by passing data to it.
There was a problem hiding this comment.
So @kaikreuzer if you can only use the :deviceId-only-configuration while Discovery is enabled that is not a problem? Great!
There was a problem hiding this comment.
Well, that's the to-be-accepted drawback of that kind of configuration then.
There was a problem hiding this comment.
I've updated the documentation to better state it's dependency on the discovery service. It works @kaikreuzer mentions. it's only used to set the ip address in case the user doesn't set the ip address or in case there the ip address of the device changed. It doesn't depend on discovery for status or controlling the device.
I thought about some more fancy code by triggering the discovery when no ip address is know. This would make it independent on (background) discovery, but would require the binding handling timeout, correctly stopping and some more checks to make it clean and efficient. But that seem to be to much for the gains it would get and make the code much more complex (and probably spending a lot of time to get it correct)
| @@ -105,15 +120,64 @@ Connection createConnection(TPLinkSmartHomeConfiguration config) { | |||
| return new Connection(config.ipAddress); | |||
| } | |||
|
|
|||
| private DeviceState refreshCache() { | |||
| private Optional<DeviceState> refreshCache() { | |||
There was a problem hiding this comment.
Does this make much sense now you have to handle and optional, should you just make this method return a Nullable DeviceState?
There was a problem hiding this comment.
Yes it does make sense. It was the only simple way I could think of to get this to work without getting compiler errors. The problem is I added @NonNullByDefault (silly me 😄) And this causes a problem with the ExpiringCache. The problem is the method getValue on that class is Nullable. But the only way to get a null value is if the Supplier would return a null. However because the @NonNullByDefault in the class enforces the non nullable on the Supplier. This would all not be a problem if my implementation wasn't based on a getValue that does return null in case no value could be given. So I have a value that can be null, but the Supplier doesn't accept the value to be null. To fix this the method should not return null anymore. One way is to introduce an empty object that is returned when no value is present, but that is basically the whole reason to have an Optional. Hence the Optional was introduced.
There was a problem hiding this comment.
Wouldn’t it make more sense to mark it Nullable? Because it shall also return null if the key is not available.
Sorry you already suggested that it might make sense 😉
See you this Thursday!
There was a problem hiding this comment.
Yes but I don't know how. It would require to mark the get method on the FunctionalInterface Supplier Nullable in ExpireCache. Marking refreshCache() nullable won't help much.
There was a problem hiding this comment.
I added a PR to your repo and one to ESH:
...ome/src/main/java/org/openhab/binding/tplinksmarthome/internal/handler/SmartHomeHandler.java
Show resolved
Hide resolved
- Better explained dependency on discovery service. - Added logging on RuntimeException as it might contain valuable information about what caused the exception and where it was triggered. Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Signed-off-by: Martin van Wingerden <[email protected]>
Signed-off-by: Martin van Wingerden <[email protected]>
|
Lets wait for a green build |
|
Thanks! |
* Added configuring devices by deviceId This makes it resilient to ip address changes. It's not using the mac address for this because the mac address can also be changed (by the user). - Better explained dependency on discovery service. - Added logging on RuntimeException as it might contain valuable information about what caused the exception and where it was triggered. Also-by: Martin van Wingerden <[email protected]> Signed-off-by: Hilbrand Bouwkamp <[email protected]>
* Added configuring devices by deviceId This makes it resilient to ip address changes. It's not using the mac address for this because the mac address can also be changed (by the user). - Better explained dependency on discovery service. - Added logging on RuntimeException as it might contain valuable information about what caused the exception and where it was triggered. Also-by: Martin van Wingerden <[email protected]> Signed-off-by: Hilbrand Bouwkamp <[email protected]> Signed-off-by: Pascal Larin <[email protected]>
* Added configuring devices by deviceId This makes it resilient to ip address changes. It's not using the mac address for this because the mac address can also be changed (by the user). - Better explained dependency on discovery service. - Added logging on RuntimeException as it might contain valuable information about what caused the exception and where it was triggered. Also-by: Martin van Wingerden <[email protected]> Signed-off-by: Hilbrand Bouwkamp <[email protected]> Signed-off-by: Maximilian Hess <[email protected]>
With the configuration option a device can be configured using the device id instead of ip address. This makes it resilient to ip address updated on the device.