Skip to content
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

[LIFX] Add optional host configuration parameter, support new products and improvements #4231

Merged

Conversation

Projects
None yet
4 participants
@wborn
Copy link
Contributor

commented Sep 9, 2017

  • Add host configuration parameter for lights that do not respond to UDP broadcasts (fixes #3913)
  • Add support for new products (most notably the new Downlights)
  • Add deviceId configuration parameter pattern to prevent mistakes/exceptions
  • Fix static code analysis findings
  • Makes the LIFX Binding periodically update network interface information. This for instance resolves the issue that network interfaces are not (or still) being used for broadcasts after they go up (or down).
  • The code for this information was duplicated across the LifxLightDiscovery and LifxLightCommunicationHandler classes and has now been extracted into LifxNetworkUtil.
  • All utility classes have now been grouped in the .util package.
  • Update Thing status to configuration error when Device ID and Host are null
  • Introduce logId property so logging identifies light depending on MAC/IP configuration
  • Add logId where missing to simplify debugging
  • Use lambdas for listener registration and iteratating over listeners
  • Resolve @nonnull warnings
  • Fix properties not always being updated
  • Use a LifxLightContext for sharing common variables between handler helper objects
  • Improve exception handling by catching specific exceptions and using more specific error messages
[LIFX] Add optional host configuration parameter, support new products
* Add host configuration parameter for lights that do not respond to UDP broadcasts (fixes #3913)
* Add support for new products (most notably the new Downlights)
* Add deviceId configuration parameter pattern to prevent mistakes/exceptions
* Fix static code analysis findings

Signed-off-by: Wouter Born <eclipse@maindrain.net>
Make broadcastEnabled final
Signed-off-by: Wouter Born <eclipse@maindrain.net>
<label>LIFX device ID</label>
<description>Identifies the light e.g. "D073D5A1A1A1"</description>
</parameter>
<parameter name="host" type="text" required="false">

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 11, 2017

Member

I am not clear on how the deviceId and the host parameter relate to each other. Which one is required for the handler in order to identify and communicate with a certain bulb? One of both? If so, what happens if both values are set (as in your example in the README), but do not correspond to the same light?

This comment has been minimized.

Copy link
@wborn

wborn Sep 11, 2017

Author Contributor

The binding always uses the deviceId to identify the light. The host parameter is only required for communications when the binding can not automatically discover the light using UDP broadcasts.

If so, what happens if both values are set (as in your example in the README), but do not correspond to the same light?

The binding ignores incoming packets that do not match the light MACAddress (deviceId). So in that scenario the light will remain offline. The binding will still send packets resulting from commands to the light.

Maybe we should add a helpful status description for this scenario? It should be possible to detect this.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 14, 2017

Member

The binding ignores incoming packets that do not match the light MACAddress (deviceId).

Do we know from which IP the packets originate from? If so, are there any real cases where the packet is NOT from the bulb at that IP and thus must be ignored? I know that in early times, LIFX bulbs were meant to act as a router to 6LoWPAN networks, but afaik this isn't true anymore and thus every bulb should have a unique IP - so if this is the case, the IP would indeed be the better configuration parameter (while the mac could be fully omitted).

This comment has been minimized.

Copy link
@wborn

wborn Sep 14, 2017

Author Contributor

Do we know from which IP the packets originate from? If so, are there any real cases where the packet is NOT from the bulb at that IP and thus must be ignored?

The IP of the light from which the packet originates is always known. Using just the IP as a configuration parameter only makes sense when people have a static IP configured for their lights. Otherwise the broadcast is needed to find the light and match it with the MAC.

LIFX bulbs were meant to act as a router to 6LoWPAN networks, but afaik this isn't true anymore and thus every bulb should have a unique IP

Lights no longer act as routers for other lights. It was not stable and thus removed. At the same time the V2 LAN protocol was introduced.

while the mac could be fully omitted

In theory the MAC could be removed as a configuration parameter for those people who run their lights with static IPs. The only issue with that is that most of the current code assumes the MAC is readily available and uses it for instance with logging and locking.

Another issue could be that the user should then configure either the MAC or the IP. Is it possible to define such a constraint within the <config-description>?

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 15, 2017

Member

Another issue could be that the user should then configure either the MAC or the IP. Is it possible to define such a constraint within the ?

I think giving the choice (and not always requiring the MAC) would be the best option. Unfortunately, there isn't such a constraint for config descriptions. Probably the easiest way would be to declare both as optional and set the Thing status to CONFIGURATION_ERROR, if none of the two parameters is provided. Wdyt?

This comment has been minimized.

Copy link
@wborn

wborn Sep 15, 2017

Author Contributor

set the Thing status to CONFIGURATION_ERROR

Yes that would solve this issue.

After some more thought I think there will be other issues when there will be 2 properties (MAC or IP) that identify a Thing.

E.g. the <representation-property> can no longer be set to the MAC. The DiscoveryService will also not be able to tell if a Thing has a static or dynamic IP and thus which property should identify the Thing.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 18, 2017

Member

E.g. the can no longer be set to the MAC.

Why not? The MAC is the invariable identifier, so even if you find the bulb at 5 IP addresses, you can notice that it is the very same. So the MAC should stay as the representation-property.

DiscoveryService will also not be able to tell if a Thing has a static or dynamic IP and thus which property should identify the Thing.

The discovery can decide, which config parameter to use - and if it easily gets hold of the MAC, it should use it. Where is the problem?

This comment has been minimized.

Copy link
@wborn

wborn Sep 18, 2017

Author Contributor

Where is the problem?

Wouldn't discovery then create new discovery results (with MAC configuration parameters) when you have added all your lights using only IP configuration parameters?

A workaround could be to automatically update an empty MAC configuration parameter when a packet is received from the light.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Sep 22, 2017

Member

Hm, good point. I was actually thinking that the MAC would be added as a property and thus the "representation-property" info would do the trick, but you are right that this isn't possible as the MAC is already a config param. So yes, setting this param by the handler might solve the issue.

only reviewed one of two commits

wborn added some commits Sep 22, 2017

Periodically update available network interface information
* Makes the LIFX Binding periodically update network interface information. This for instance resolves the issue that network interfaces are not (or still) being used for broadcasts after they go up (or down).
* The code for this information was duplicated across the LifxLightDiscovery and LifxLightCommunicationHandler classes and has now been extracted into LifxNetworkUtil.
* All utility classes have now been grouped in the .util package.

Signed-off-by: Wouter Born <eclipse@maindrain.net>
Make Device ID an optional configuration paramater, use lambdas and f…
…ix updating properties

* Update Thing status to configuration error when Device ID and Host are null
* Introduce logId property so logging identifies light depending on MAC/IP configuration
* Add logId where missing to simplify debugging
* Use lambdas for listener registration and iteratating over listeners
* Resolve @nonnull warnings
* Fix properties not always being updated
* Use a LifxLightContext for sharing common variables between handler helper objects
* Improve exception handling by catching specific exceptions and using more specific error messages

Signed-off-by: Wouter Born <eclipse@maindrain.net>
@wborn

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2017

With the latest commit the deviceId (MAC) is now an optional property for lights @kaikreuzer. Since a lot of code (logging, throttling) was depending on the MAC, there are quite some code changes. I also made some additional improvements while I was at it.

Don't set host property on discovered lights
Signed-off-by: Wouter Born <eclipse@maindrain.net>

@wborn wborn changed the title [LIFX] Add optional host configuration parameter, support new products [LIFX] Add optional host configuration parameter, support new products and improvements Sep 23, 2017

@wborn wborn closed this Sep 23, 2017

@wborn wborn reopened this Sep 23, 2017

@wborn wborn closed this Sep 24, 2017

@wborn wborn reopened this Sep 24, 2017

@kaikreuzer
Copy link
Member

left a comment

Thanks, indeed huge code changes again (do you reimplement this binding every month? ;-)).
Hope you have thoroughly tested it, code looks good.

@kaikreuzer kaikreuzer merged commit 52d22b3 into eclipse:master Sep 25, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

@wborn wborn deleted the wborn:lifx-add-host-param-support-new-products branch Dec 2, 2017

@openhab-bot

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/lifx-binding-with-multiple-networks-scanning-wrong-subnet/66665/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.