[govee] New Govee LAN-API Binding

[stefan-hoehn]

With great pleasure 😉 I can finally announce that we have a new

LAN API Binding that allows to use Govee lights directly within the house without using the Cloud.
In comparison to the already existing Govee Binding which uses Bluetooth this binding uses the LAN-API that allows to talk to the devices directly via UDP.

I have seen several requests in the forum where people were asking for a binding that supports the Govee lights, so I finally did it. It supports already 50+ lights of their product range.

Thanks in advance for taking the time on reviewing the new addon.

[stefan-hoehn]

@jlaur
Jacob, do you know why I get this error?

Error: ] Could not find the selected project in the reactor: :org.openhab.binding.goveelan @ Error: Could not find the selected project in the reactor: :org.openhab.binding.goveelan -> [Help 1] Error: Error: To see the full stack trace of the errors, re-run Maven with the -e switch. Error: Re-run Maven using the -X switch to enable full debug logging. Error: Error: For more information about the errors and possible solutions, please read the following articles: Error: [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MavenExecutionException Error: Process completed with exit code 1.

And why is this failing: "PR-openHAB-Addons — Build #17310 ended"
I don't see any additional error there.

[stefan-hoehn]

@jlaur And a question: During implementation I had different behaviour in my binding regarding accessing the binding properties. There are two ways accessing the properties:

thing.getConfiguration().getProperties()
thing.getProperties()

In the beginning I had to access them via the second and now "all of a sudden" I can only find them in the first (which is why I had to change this this morning).

Can you explain when to use which?

[openhab-bot]

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

https://community.openhab.org/t/govee-dreamcoulour-led-strips/84435/20

[jlaur]

Jacob, do you know why I get this error?

It might help to add your binding to this POM as well:

openhab-addons/bundles/pom.xml

Lines 155 to 156 in 01dc99c

	<module>org.openhab.binding.goecharger</module>
	<module>org.openhab.binding.gpio</module>

[jlaur]

Thanks for your contribution! I have added some initial comments to get started - no extensive review for now. 🙂

[jlaur]

@jlaur And a question: During implementation I had different behaviour in my binding regarding accessing the binding properties. There are two ways accessing the properties:

thing.getConfiguration().getProperties()
thing.getProperties()

In the beginning I had to access them via the second and now "all of a sudden" I can only find them in the first (which is why I had to change this this morning).

Can you explain when to use which?

Both should be valid. I would usually prefer the shortest form.

[jlaur]

@stefan-hoehn - what are your thoughts about having "LAN" in the binding name?

I understand that there are some unrelated Bluetooth products (thermometers, hygrometers) from the same manifacturer, already supported by the Bluetooth extension bluetooth.govee.

However, there would be no conflict if your binding would simply be govee. In this case the binding could evolve to also support cloud, Zigbee or whatever the future will bring without creating completely separate bindings.

WDYT?

[jlaur]

@stefan-hoehn - there are some compiler warnings and checkstyle warnings left. You could take a look at target/code-analysis/report.html.

[stefan-hoehn]

@jlaur And a question: During implementation I had different behaviour in my binding regarding accessing the binding properties. There are two ways accessing the properties:
thing.getConfiguration().getProperties()
thing.getProperties()
In the beginning I had to access them via the second and now "all of a sudden" I can only find them in the first (which is why I had to change this this morning).
Can you explain when to use which?

Both should be valid. I would usually prefer the shortest form.

As I mentioned, they do not seem to behave the same. I have to change it to the current form because the other one was empty which worries me a bit.

[stefan-hoehn]

@stefan-hoehn - what are your thoughts about having "LAN" in the binding name?

I understand that there are some unrelated Bluetooth products (thermometers, hygrometers) from the same manifacturer, already supported by the Bluetooth extension bluetooth.govee.

However, there would be no conflict if your binding would simply be govee. In this case the binding could evolve to also support cloud, Zigbee or whatever the future will bring without creating completely separate bindings.

WDYT?

IMHO, the problem is that there IS already a govee binding that supports only bluetooth which is a totally different approach. And honestly, currently I have absolutely chance of merging the binding with the author of that bluetooth binding timewise. Maybe sometime in the future we could start that endeavour but currently I am not able to do so.
As mentioned I MAY add the cloud API eventually (or someone who is willing to help) but then we could still perceive WLAN as LAN as well ;-)

[stefan-hoehn]

Thanks for your contribution! I have added some initial comments to get started - no extensive review for now. 🙂

Honestly, that was already a great number of quick feedbacks you provided, Jacob. Really appreciated! 🥇

[jlaur]

IMHO, the problem is that there IS already a govee binding that supports only bluetooth which is a totally different approach. And honestly, currently I have absolutely chance of merging the binding with the author of that bluetooth binding timewise.

I'm not suggesting to merge with the Bluetooth extension binding. I believe it can co-exist as bluetooth.govee while your binding could be just govee.

As mentioned I MAY add the cloud API eventually (or someone who is willing to help) but then we could still perceive WLAN as LAN as well ;-)

My renaming suggestion is to prepare for that scenario so this binding is not too limited in terms of naming. LAN would be confusing for a binding supporting cloud also.

[stefan-hoehn]

IMHO, the problem is that there IS already a govee binding that supports only bluetooth which is a totally different approach. And honestly, currently I have absolutely chance of merging the binding with the author of that bluetooth binding timewise.

I'm not suggesting to merge with the Bluetooth extension binding. I believe it can co-exist as bluetooth.govee while your binding could be just govee.

As mentioned I MAY add the cloud API eventually (or someone who is willing to help) but then we could still perceive WLAN as LAN as well ;-)

My renaming suggestion is to prepare for that scenario so this binding is not too limited in terms of naming. LAN would be confusing for a binding supporting cloud also.

I am cool with that.

Maybe we can add Connor @cpmeister to change that in the README: https://www.openhab.org/addons/bindings/bluetooth.govee/

(honestly I just took it for granted when seeing the readme that the binding was called govee by Connor which of course should I have jumped into my eyes that this is NOT the case because it would have had to appear directly next to mine 🤦‍♂️)

OK, if I did that now, unfortunately that will change EVERY file due to the package rename and it will make hard for you to see my changes on the reviews you gave to me. Should I still proceed with that?

Btw, how do you like me to handle your review comments? Just a thumbs up and you resolve them after re-reviewing or do you like me to resolve them after I did them?

[stefan-hoehn]

@stefan-hoehn - there are some compiler warnings and checkstyle warnings left. You could take a look at target/code-analysis/report.html.

@stefan-hoehn - there are some compiler warnings and checkstyle warnings left. You could take a look at target/code-analysis/report.html.

Believe me, there were many more(!) before I did the initial push! 😄

I may need a few hints how to improve them by you. I'll come back to you with the details asap. (again, you support is very much appreciated!).

[jlaur]

I am cool with that.

👍

OK, if I did that now, unfortunately that will change EVERY file due to the package rename and it will make hard for you to see my changes on the reviews you gave to me. Should I still proceed with that?

IMHO yes, it won't be any easier than now, especially not after the PR is merged. 😉 As for the review, perhaps you could do the renaming in a separate commit, that would make it easier for me to track changes/progress.

Btw, how do you like me to handle your review comments? Just a thumbs up and you resolve them after re-reviewing or do you like me to resolve them after I did them?

I usually go through all comments and check their resolutions, so I would be fine with you leaving them open, and I will then mark them as resolved as I go through them. If you prefer marking as resolved yourself, that's also fine - just please don't do this until pushing the changes. 🙂

[jlaur]

Nice to see that you got the color channel to work! 👍 I have now resumed the review and added a few more comments.

[stefan-hoehn]

Review comments applied and pushed

[stefan-hoehn]

@jlaur last review comments adapted.

[jlaur]

A few minor comments. Most importantly, I still see some issues with the multicast approach.

[jlaur]

These should also be removed as they seem to be properties?

[stefan-hoehn]

I don't understand what you mean with properties. If you create a thing, these should be definitely provided even though the device could work without it but still I feel they should be editable in case the user wants to overwrite them.

[jlaur]

Why would the user need to edit the product name? The binding doesn"t use it for anything, I believe?

[stefan-hoehn]

The binding does not need it but the to me it is important it is shown.
If the product name is discovered incorrectly the user can overwrite it. I wouldn't say that the binding can be 100% correct in detecting it. It only discovers the SKU but the product name is best guessing.

[maniac103]

I can understand @jlaur though. If the binding doesn't actually use the product name, and it neither can be meaningfully discovered, what's the point in showing it in the first place? If it can be discovered properly, it should be a property; if the binding needs it and it can't be discovered properly, it should be a parameter, but if neither applies, I think it shouldn't exist at all.
Don't forget you'll make users think about what to achieve when editing this value. They'll be confused when they hear the answer is 'nothing' ;-)

[stefan-hoehn]

I removed the two hw/sw versions, I made the macAddress a representation property. I made the device type and hostname readonly which looks nice and good after discovery:

But here is the catch: if someone wants to create device manually, then it does't work

• the macAdress isn't shown at all, so it cannot be entered which is a blocker
• hostname and sku are readonly even here, which is a blocker (at least for the hostname)
• "of course" the properties sw/hw version cannot also not be supplied manually.

[maniac103]

MAC address needs to be a parameter, same as hostname. Sorry for being ambiguous above, I thought the relation 'is mandatory' -> 'must be parameter' was clear. For discovery results, those parameters are initialized from the values in the respective withProperty calls in the DiscoveryResult. IOW, the configuration map, which contains the parameter values, is initialized from the property map.

SKU and the versions can probably stay as properties and be updated on thing initialization - similarly to e.g. here

[stefan-hoehn]

Well, no, I though about that a while: if I can and do discover it (due to a scan being started) then I get all information but when someone adds the thing manually I won't trigger a discovery for that, so in that case the sku and product would stay empty.

It kinda hurts my heart making all of these properties...

And this then is the manual creation:

Is it actually possible in the file definition to provide thing properties?

Maybe this discussion is kind of philosophical in terms of manual creation. The likelihood that someone would actually create it manually is very small. I would even think that manual creating in a thing file is actually pretty hard because you would need to find out the hostname and mac address of that device which is very hard to find out without network knowledge.
To compare that with hues: Do people ever add hue lights manually and if, how would they know all the parameters about that?

Which then would lead to the question, how do people deal with that who don't like the WebUI and want to create their things in files?

[maniac103]

Is it actually possible in the file definition to provide thing properties?

I don't think so, but I also don't think there's an actual loss here: properties help with identifying / distinguishing discovery results, but when manually creating things, the user already knows what device he's adding. So while not having the properties in the manual case may not be ideal/perfect, I don't think it's an actual issue.

[jlaur]

The likelihood that someone would actually create it manually is very small. I would even think that manual creating in a thing file is actually pretty hard because you would need to find out the hostname and mac address of that device which is very hard to find out without network knowledge. To compare that with hues: Do people ever add hue lights manually and if, how would they know all the parameters about that?

Which then would lead to the question, how do people deal with that who don't like the WebUI and want to create their things in files?

I create all my Things manually. Sometimes I need to use discovery first and create a managed Thing. I can then copy the configuration parameters into my manually created Thing in a .things file and delete the managed Thing again.

[jlaur]

You don't need to declare these:

Suggested change

	<properties>
	<property name="wifiHardwareVersion"/>
	<property name="wifiSoftwareVersion"/>
	</properties>

However, you should probably in this place declare:

<representation-property>macAddress</representation-property>

[stefan-hoehn]

As you see in the above discussion the macAdress needs to stay a parameter as it would otherwise not be possible to provide on manual creation.

[jlaur]

This is still pending.

[jlaur]

I'm sorry, but this is still not safe, since write operations are not synchronized by the same lock. I have now had a closer look trying to follow the flow of events.

Please correct me if I'm reading this flow wrongly.

• Handler A initializes.
• Handler A adds itself to the static THING_HANDLERS.
• Handler A schedules a future which will create a multicast socket and receive one packet from it.
• This future sets the public static refreshJobRunning in GoveeHandler to true.
• It will then get a Map with all thing handlers from static method in GoveeHandler.
• If the received multicast packet matches the IP address of any handler in the received Map, it will then call methods directly on that thing handler instance to update state and status.

If this is correct, I think there are some design flaws to address.

If handler A and handler B initializes at the same time:

• Both can put into the same HashMap at the same time. This will not work.
• triggerStatusJob can be checked at the same time, in which case two futures will be scheduled, and you'll only keep a reference to the last one of them. Therefore there'll be a wild-running future after that.
• When the multicast future gets the thing handlers, this might happen while handler C is adding itself to that Map. This can cause an exception to be thrown.

In addition to this:

• There are issues when discovery is running at the same time, which you have tried to work around. It looks like this could have some concurrency issues also.
• The multicast socket is recreated every second and receives only one packet. This seems very unusual. I think one would normally keep a loop running to receive all packets as soon as they come in, and not miss any. See for example the implementation in the miele binding, or perhaps just start out with grep -R "new MulticastSocket".

The only solution I can think of to address all these issues at the same time would be to add a bridge handler and let this handler be responsible for having the multicast listener running and notify discovery service as well as all handlers. This is done for example by the miele binding.

I realize there is no physical or logical bridge really, so it would be kind of a "pseudo" bridge.

@openhab/add-ons-maintainers (or @maniac103 🙂) - perhaps someone else has better ideas, for example how to fix this design without introducing a bridge?

[kaikreuzer]

As my comment to @jlaur's request for ideas isn't shown here directly, I want to bring it to the general attention this way. 😄

[stefan-hoehn]

Pushed some changes regarding the slipped comments, the props / params handling.

Also I reduced the amount of updates. Due to the fact that the binding polls pretty often to stay in sync with the devices, it also updated the value on the Channel every time it received an update no matter whether the value was still the same. Now it only sends an update to the channel if the value has changed which reduces the noise a lot (not only) in the logs but also relieves openHAB channel updates.

[stefan-hoehn]

@jlaur Thanks to @maniac103 , to deserves a lot of kudos <3, we now how a much cleaner and thread-safe implementation of the discovery process. Thanks also to him talking a 1-on-1 session within which we together made it run.

This now should hopefully allow this binding to become a final version for release.

Sidenote: there are much more devices available from Govee via LAN and I am waiting for them to be documented as the official document is not up-to-date which was confirmed by their support.

cc: @kaikreuzer

[jlaur]

FYI @openhab/add-ons-maintainers, I'm out of time and away from keyboard until the milestone tomorrow. There remains one commit to review with concurrency redesign.

Thanks for all your efforts, @stefan-hoehn and @maniac103.

[kaikreuzer]

I am not at home tonight, but will have a look at the last commit tomorrow morning, so that we can get it into the release.
@jlaur Do I get you right that you are fine with the rest of the code of this PR?

[jlaur]

@jlaur Do I get you right that you are fine with the rest of the code of this PR?

Yes, exactly. 🙂

[kaikreuzer]

This is all still full of static use, which imho isn't much different to having the static map in the handlers before.
My suggestion in https://github.com/openhab/openhab-addons/pull/15696/files/dcc8e884219a5828b8db7971a36b2c9877701b5e#r1421568312 was to have a dedicated instance/service that does the handling. Is there any reason why this cannot be done?

[maniac103]

This essentially is a singleton doing the communication. If you prefer it can be converted to an actual singleton instance, but I don't see much difference either way, because the singleton instance holding the data will also be static.
Is this what you have in mind or what kind of service are you thinking of exactly?

[kaikreuzer]

Singletons are an anti-pattern in the OSGi world. Instead, it should be a service that is instantiated once and used where needed. This prevents lifecycle issues, e.g. when a bundle is stopped, updated and started - singletons are not properly discarded in such cases, while services are.

[maniac103]

Stopping bundles should dispose the handlers though, thus making them unsubscribe from and eventually stop the thread?
Got any reference example for such a shared service?

[kaikreuzer]

I'm just preparing a (untested) PR that shows what I mean.

[kaikreuzer]

Yes, it helped very much indeed!

[kaikreuzer]

Were you able to test if the binding still works this way?

[stefan-hoehn]

@kaikreuzer Actually it is crashing during startup.

``
2023-12-17 13:03:38.739 [ERROR] [.govee.internal.CommunicationManager] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.CommunicationManager(339)] : Constructor with 0 arguments not found. Component will fail.

...

2023-12-17 13:03:38.743 [ERROR] [.govee.internal.CommunicationManager] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.CommunicationManager(339)] : Error during instantiation of the implementation object: Constructor not found.

2023-12-17 13:03:38.748 [WARN ] [govee.internal.GoveeDiscoveryService] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.GoveeDiscoveryService(340)] : Could not get service from ref {org.openhab.binding.govee.internal.CommunicationManager}={service.id=527, service.bundleid=278, service.scope=bundle, osgi.ds.satisfying.condition.target=(osgi.condition.id=true), component.name=org.openhab.binding.govee.internal.CommunicationManager, component.id=339}
2023-12-17 13:03:38.749 [ERROR] [govee.internal.GoveeDiscoveryService] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.GoveeDiscoveryService(340)] : Error during instantiation of the implementation object: Unable to get service for reference $002
2023-12-17 13:03:38.750 [DEBUG] [govee.internal.GoveeDiscoveryService] - bundle org.openhab.binding.govee:4.1.0.202312171201 (278)[org.openhab.binding.govee.internal.GoveeDiscoveryService(340)] : Failed creating the component instance; see log for reason
2023-12-17 13:03:38.751 [WARN ] [nternal.DiscoveryServiceRegistryImpl] - bundle org.openhab.core.config.discovery:4.1.0.202311121345 (167)[org.openhab.core.config.discovery.internal.DiscoveryServiceRegistryImpl(116)] : Could not get service from ref {org.openhab.core.config.discovery.DiscoveryService}={service.id=528, service.bundleid=278, service.scope=bundle, osgi.ds.satisfying.condition.target=(osgi.condition.id=true), component.name=org.openhab.binding.govee.internal.GoveeDiscoveryService, component.id=340}
2023-12-17 13:03:38.752 [WARN ] [nternal.DiscoveryServiceRegistryImpl] - bundle org.openhab.core.config.discovery:4.1.0.202311121345 (167)[org.openhab.core.config.discovery.internal.DiscoveryServiceRegistryImpl(116)] : DependencyManager : invokeBindMethod : Service not available from service registry for ServiceReference {org.openhab.core.config.discovery.DiscoveryService}={service.id=528, service.bundleid=278, service.scope=bundle, osgi.ds.satisfying.condition.target=(osgi.condition.id=true), component.name=org.openhab.binding.govee.internal.GoveeDiscoveryService, component.id=340} for reference DiscoveryService
``

[kaikreuzer]

Maybe stefan-hoehn#2 helps?

[stefan-hoehn]

Yes, it does. I proudly ;-) discovered the issue myself at the same time.
Testing looks awesome. All devices are discovered and I am able to control them as well.

[stefan-hoehn]

Taking into account that the PR wasn't tested, this was a kind of an impressive move! <3

If you are good, I am good as well now!

[kaikreuzer]

Wonderful - glad we made it in the last minute!
Thanks @stefan-hoehn for all your efforts and to @jlaur for the review and @maniac103 for helping out with the threading!

[jlaur]

Thanks a lot for the help, @kaikreuzer and @maniac103 - and @stefan-hoehn for the contribution.