[pilight] Pilight Binding initial contribution + add discovery

[niklasdoerfler]

Migrate the already existing pilight binding from openhab1-addons (https://www.openhab.org/addons/bindings/pilight1/) to openHAB 3. Also some discovery logic to auto discover the pilight daemon as a bridge and the configured devices from the pilight daemon is added.

This PR is based on the still unmerged PR #8469, which could be closed afterwards, since it was used as a basis for this PR.

Built .jar of the binding is available here: https://openhab.jfrog.io/artifactory/libs-pullrequest-local/org/openhab/addons/bundles/org.openhab.binding.pilight/3.1.0-SNAPSHOT/org.openhab.binding.pilight-3.1.0-SNAPSHOT.jar

[stefanroellin]

Thanks for taking care of my pull request. Since I have not yet switched to openHAB 3, I didn't continue with it.
However I use the binding in my openHAB 2 installation since about four months without any issues.

[niklasdoerfler]

I don't know why, but for some reason the last line break at the end of file bundles/pom.xml was accidentally removed, so the build failed in the spotless check. I have fixed that now.

@wborn could you please trigger a new build again? Thanks!
This time jenkins detected the changes automatically.

[niklasdoerfler]

Could someone please set the rebuild label again? Thanks!

Jenkins complained about the following in the last build, which has nothing to do with this binding from my point of view...

[ERROR] /home/jenkins/jenkins-agent1/workspace/PR-openHAB-Addons@4/bundles/org.openhab.binding.dsmr/src/test/java/org/openhab/binding/dsmr/internal/device/DSMRSerialAutoDeviceTest.java:[136,23] The constructor org.openhab.core.io.transport.serial.PortInUseException() is undefined

[fwolter]

Thanks for your contribution! I reviewed your code and here is my feedback.

You code doesn't build, because the license header's year parameter isn't updated, yet.

To make the binding compile, you need to rebase your branch.

Here are the commands for rebasing your branch:

If not already done, add the upstream openHAB addon repo as a remote to your local repo and fetch it:

git remote add upstream https://github.com/openhab/openhab-addons.git
git fetch upstream

The remotes (git remote --verbose) should look like this, now:

origin  https://github.com/[your name]/openhab-addons.git (fetch)
origin  https://github.com/[your name]/openhab-addons.git (push)
upstream        https://github.com/openhab/openhab-addons.git (fetch)
upstream        https://github.com/openhab/openhab-addons.git (push)

Then, you can rebase your PR's branch onto main:

git rebase upstream/main

Finally force-push the rebased branch to this PR's branch:

git push origin [your branch name of this PR] --force-with-lease

DON'T use merge! This can clutter the Git history and make the PR unusable.

[fwolter]

Is there a reason why you don't use the default libraries for JSON processing? See https://www.openhab.org/docs/developer/guidelines.html#default-libraries

[niklasdoerfler]

I continue using jackson, because most of the code has been adopted from the Pilight binding for openHAB v1 and some functions are based on Jackson for which there is no direct equivalent in Gson. Maybe we can try to replace Jackson by Gson later.

[niklasdoerfler]

Thank you @fwolter for your detailed comments. I'll take a look at the points and get back here as soon as I'm done with them.
I have already made the rebase and the Jenkins build is now passing successfully (link to the built can be found in the first comment).

[cpmeister]

Can you also try to address the compiler warnings that show up when building your project?

[cpmeister]

You can avoid making loop an AtomicBoolean by using Stream.peek().count() instead of foreach. You can use the returned count value to set your loop value.

Suggested change

	scanner.findAll("Location:([0-9.]+):(.*)").forEach(matchResult -> {
	scanner.findAll("Location:([0-9.]+):(.*)").peek(matchResult -> {

[niklasdoerfler]

Good idea, but is it good practise to use peek for this? According to the documentation, it is mainly intended for debugging.

This method exists mainly to support debugging, where you want to see the elements as they flow past a certain point in a pipeline.

(see https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html#peek-java.util.function.Consumer-)

[niklasdoerfler]

Can you also try to address the compiler warnings that show up when building your project?

@cpmeister: Yes, I already tried to get rid of the compiler warnings, but to be honest I'm not quite sure, why some of the warnings occur and how to fix them, e.g.:

@Nullable Object test;

if (test != null) {
   test.doSomething();
}

This leads to a Potential null pointer access: this expression has a '@Nullable' type warning at the method call "test.doSomething()". Why does the compiler see a null pointer access here and how should I fix this?

[cpmeister]

In order to resolve most of the compiler warnings you need to cache the @Nullable field to a local variable and then perform all of your logic on the local variable instead of on the field directly. The reason that it is issuing this warning is that the system will make the assumption that a nullable field can become null at any time due to multithreading. The warnings can be a little overzealous but they do help cut down almost all of the NPEs that we might otherwise see.

[niklasdoerfler]

I resolved most of the compiler warnings, except for two, that I'm not sure how to solve:

[WARNING] pom.xml [0:0]: Unused Export-Package instructions: [org.openhab.*]
[WARNING] pom.xml [0:0]: Unused Import-Package instructions: [io.swagger.v3.oas.annotations.*, javax.annotation.security.*, org.eclipse.jdt.annotation.*,
org.openhab.core.automation.annotation.*, com.google.common.*]

[cpmeister]

You did a fantastic job on addressing the warnings. The remaining warnings don't need to be addressed.
I think there is only this last change then we are good to go.

[cpmeister]

I think these libraries are already available to you from the core. They are version 2.10.1 though. Would it be alright to use those instead? If it doesn't work to remove these dependencies from the pom file then you can just manually specify them as a provided dependency like so:

Suggested change

	<dependency>
	<groupId>com.fasterxml.jackson.core</groupId>
	<artifactId>jackson-databind</artifactId>
	<version>2.10.4</version>
	</dependency>
	<dependency>
	<groupId>com.fasterxml.jackson.core</groupId>
	<artifactId>jackson-annotations</artifactId>
	<version>2.10.4</version>
	</dependency>
	<dependency>
	<groupId>com.fasterxml.jackson.core</groupId>
	<artifactId>jackson-core</artifactId>
	<version>2.10.4</version>
	</dependency>
	<dependency>
	<groupId>com.fasterxml.jackson.core</groupId>
	<artifactId>jackson-databind</artifactId>
	<version>2.10.1</version>
	<scope>provided</scope>
	</dependency>
	<dependency>
	<groupId>com.fasterxml.jackson.core</groupId>
	<artifactId>jackson-annotations</artifactId>
	<version>2.10.1</version>
	<scope>provided</scope>
	</dependency>
	<dependency>
	<groupId>com.fasterxml.jackson.core</groupId>
	<artifactId>jackson-core</artifactId>
	<version>2.10.1</version>
	<scope>provided</scope>
	</dependency>

If you are forced to add it as a provided dependency then you will also need to add those dependencies to the feature.xml file as well. Not terribly difficult but I would rather have you use an existing dependency than to bring in a new one.

[niklasdoerfler]

In the first step, I removed the dependencies from the .pom and my local build was successful. Let's see what Jenkins says. Looks like Jenkins is happy too.

[cpmeister]

This would cancel the already canceled future.

[niklasdoerfler]

I'm not quite sure, what you mean. This call takes place in the lambda function of the fixed delay scheduler. In each run of it, I check if there are remaining elements in the delayedActionQueue and poll one of them to call doSendAction(). If the queue is empty I need to cancel the scheduler. I think it is a valid case here or am I wrong?
Maybe you can explain in more detail what you are thinking?

[cpmeister]

I'm saying that the old code referenced the delayedActionWorkerFuture field, but now it is referencing the delayedActionWorkerFuture local variable. The local variable has already been determined to be cancelled at this point in the code. It clear that your intent is to cancel the future field rather than the local variable. You need to make a second local variable inside the lambda to grab a fresh reference of the field.

[niklasdoerfler]

Ah sure, now I get your point!

[fwolter]

LGTM