[Freeboxos] New binding potential replacement of Freebox binding

[clinique]

Supersedes this PR

[clinique]

@lolodomo : FYI, I have removed usage of hibernate validator.

[clinique]

@lolodomo : I propose that we come back on this one once the v3.3 is out.

[jlaur]

@clinique - for a PR of this size, perhaps you can add some more context/description to the PR description? Also, without this context, the first question for me would be: Why introduce it as a new binding instead of updating/replacing the existing one - i.e., what are the differences and where do they overlap? Cc @lolodomo.

[clinique]

@clinique - for a PR of this size, perhaps you can add some more context/description to the PR description? Also, without this context, the first question for me would be: Why introduce it as a new binding instead of updating/replacing the existing one - i.e., what are the differences and where do they overlap? Cc @lolodomo.

@jlaur : answer is here

[lolodomo]

We even have a discussion about that and this was my proposal. The new binding has many new features but also has some missing features compared to the current binding and breaking changes if I correctly remember. The idea was to remove the old binding the day the new binding can fully replace the old binding, keeping the two bindings in the distribution during 6 months. @jlaur : note that I already did a partial review. I even tested the binding and push code to @clinique. Several things were not good but @clinique certainly corrected them during the time. This was in my TODO list to review again and test it but you are welcome @jlaur to make it too.

[openhab-bot]

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

https://community.openhab.org/t/freeboxos-binding/137569/1

[clinique]

Binding now published in the add-on market place.

[jlaur]

We even have a discussion about that and this was my proposal.

OK. What about the naming, where does the "OS" come from?

The new binding has many new features but also has some missing features compared to the current binding and breaking changes if I correctly remember.

Would it be possible to implement those missing features, or are they somehow obsoleted? Otherwise they will still disappear when the old binding is finally removed.

The idea was to remove the old binding the day the new binding can fully replace the old binding, keeping the two bindings in the distribution during 6 months.

I get that migration and breaking changes is always an issue with openHAB. Let me just brainstorm a little - sorry if you already went through those discussions previously: We now have Community Marketplace, which is sometimes used for pushing new binding versions before they make it into the openHAB distribution. Perhaps it could be used the other way around: Publishing the old "legacy" Freebox binding to marketplace. Then users needing a longer migration period and immediate compatibility when upgrading openHAB could simply uninstall the binding and install the legacy version from marketplace instead. This could be mentioned in the release notes for breaking changes. Would it work - and WDYT? This way the new version could still replace the old one without any naming issues, i.e. it could still be named "freebox".

The general idea is that the current approach is to deliver a "legacy" version and a new version through the same openHAB distribution channel, where binding names must be unique. But it doesn't have to be that way, at least some other options could be explored.

Secondly: Although I'm not sure if the new Netatmo binding was a success story in terms of migration, is the situation any different from that? I know we were forced to deliver a new version due to API changes, so it would eventually have broken anyway, but still interested in why you are seeking a different approach for Freebox compared to Netatmo.

@kaikreuzer - tagging you in case you would be interested as they are also some general issues about binding migrations, breaking changes and approaches to this, and you might also have some feedback.

@jlaur : note that I already did a partial review. I even tested the binding and push code to @clinique. Several things were not good but @clinique certainly corrected them during the time. This was in my TODO list to review again and test it but you are welcome @jlaur to make it too.

I was really hoping you would resume the review, @lolodomo. :-) I might have a quick look and post some lose comments here and there if you are interested, but currently I will probably not commit fully to reviewing this PR.

[clinique]

According to the documentation :

It should be spelled FreeboxOS

Regarding your comments above :

• the introduction of UoM breaks many channels
• I kind a like the idea of moving legacy binding to the Market Place to ease the transition - but I'll have to rename everything backward once again :-)
• The impact on Freebox should be lower than on Netatmo - being strictly restricted to France where the user base is not the higher audience of openHab (and BTW I think the migration of Netatmo went quite smooth in regard of the number of posts in the community toward the number of users, I did figure it would be very worse)

[lolodomo]

We now have Community Marketplace, which is sometimes used for pushing new binding versions before they make it into the openHAB distribution. Perhaps it could be used the other way around: Publishing the old "legacy" Freebox binding to marketplace. Then users needing a longer migration period and immediate compatibility when upgrading openHAB could simply uninstall the binding and install the legacy version from marketplace instead. This could be mentioned in the release notes for breaking changes. Would it work - and WDYT?

That looks like a good idea. I think the marketplace was not yet existing when we had this discussion with @clinique ;) But in that case, I consider that at least the new binding should not remove any feature from the legacy binding. At least, we should discuss each of them.

The problem with updating the current binding is that there are so many changes that the review is very hard. This is almost as if all the previous code is replaced by new code.

So having a new binding named freeboxos will help everybody, the reviewers and the final users.

[jlaur]

That looks like a good idea. I think the marketplace was not yet existing when we had this discussion with @clinique ;) But in that case, I consider that at least the new binding should not remove any feature from the legacy binding. At least, we should discuss each of them.

That probably applies no matter how the migration is approached, assuming final action still would be to remove the "legacy" binding?

The problem with updating the current binding is that there are so many changes that the review is very hard. This is almost as if all the previous code is replaced by new code.

That's a separate concern. I'm sure we'll manage as reviewers to not look too much at diffs, but concentrate on the new code itself.

So having a new binding named freeboxos will help everybody, the reviewers and the final users.

Assuming the binding rename was out of necessity in order to have both bindings exist simultaneously, this contradicts the idea of providing the legacy binding through Marketplace. So I'd propose to either:

• Provide legacy binding through Marketplace and keep the existing binding name when providing the new version, i.e. have only one binding included in the openHAB distribution.
• Or go ahead with your original plan to provide the new version with a new name and keep the legacy binding, i.e. have two bindings for the same purpose and with overlapping functionality in order to work-around issues with versioning/migration/breaking changes. Marketplace can then be used to distribute new version to early adopters/testers, but is not needed as such for the migration itself.

[kaikreuzer]

@kaikreuzer - tagging you in case you would be interested as they are also some general issues about binding migrations, breaking changes and approaches to this, and you might also have some feedback.

Thanks, @jlaur.

I agree that we should try to only have one version of a binding in the official distro at any time. The marketplace sounds like the best option to make alternative versions available. So once this PR is merged, the "old" binding should be moved to the marketplace. Wrt the name, I personally do not care too much - I think we could leave the name of the new one to be "freeboxos" as it is anyhow not compatible and people will have to recreate their things, if I understand it correctly. It might then even have the benefit that the old binding (then from the marketplace) could be installed by users in parallel (e.g. during migration), since both would have a different namespace and would not interfere with each other.

[clinique]

@lolodomo : when you'll have a minute, I would welcome a review. I would really like have this merged in OH4.0

Major functional additions/evolutions are :

1. usage of the websocket to get notifications from the server instead of polling (for lan devices and vm)
2. switched from classes to records in serialization / deserialization, dramatically simplifying code of the package.
3. Handling of freeplugs
4. Home automation capabilities bootstraped
5. Introduction of DECT handling, separated from Landline
6. Improvement in Call handling, completely changed
7. Usage of UoM everywhere possible and meaningfull

This can not be merged until gson 2.10 is available in the core (end of february I guess). As usually, documentation has to be completely reviewed, I'll do it while we progress in review. @ben12 : for your information

[clinique]

@ben12 : please take a look at this last version. I made huge modifications on the Home Node side - resulting in an important simplification of the configuration of Nodes.

[ben12]

@ben12 : please take a look at this last version. I made huge modifications on the Home Node side - resulting in an important simplification of the configuration of Nodes.

@clinique Code seems OK. (but I have not tested, I tried to run OpenHab 4 into Eclipse IDE without success)

[clinique]

@lolodomo : sorry to ping you here - I would really like to have a chance to merge it before OH4 goes final.

[ben12]

@ben12 : please take a look at this last version. I made huge modifications on the Home Node side - resulting in an important simplification of the configuration of Nodes.

@clinique Finally, my "basic" shutter works with the last version 4.0.0-SNAPSHOT of openhab-core / freeboxos.

But check this: https://github.com/clinique/openhab-addons/commit/9f518ff5eb27d374213a05ad1bad459732fb875e#r111107027

[clinique]

@fwolter : you can proceed with review the PR build failed for a SAT issue. mvn spotless:apply (as well as Eclipse IDE) keeps changing private·static·record·EndpointValue<T>·(T·value)·{ to this

private·static·record·EndpointValue<T>(T·value)·{ mvn install does not complain but the CI/Build fails. I have to think each time to modify this file.

[fwolter]

I'm a bit puzzled, are you saying the CI build succeeds without the space, but the local build fails and with the space the CI build fails, but the local build succeeds?

In any case, building locally succeeds only with the space on my machine. But obviously the CI build succeeded.

Please note that the sign-off message needs to contain your real name. You can keep it for this PR.

The rest LGTM, except the remaining Thread.sleep().

[clinique]

I'm a bit puzzled, are you saying the CI build succeeds without the space, but the local build fails and with the space the CI build fails, but the local build succeeds?

In any case, building locally succeeds only with the space on my machine. But obviously the CI build succeeded.

Please note that the sign-off message needs to contain your real name. You can keep it for this PR.

The rest LGTM, except the remaining Thread.sleep().

I reversed cases. Spotless:apply removes the space and CI breaks

[fwolter]

Seems a bit strange. Before merging this, we should figure out why this is building locally, but the CI build breaks and vice versa.

[clinique]

Seems a bit strange. Before merging this, we should figure out why this is building locally, but the CI build breaks and vice versa.

I think this recent construct (generic record) is not well handled by spotless:apply. It's not a huge issue, noticeable and will have to be adressed at some point, unless I'm the only one facing it.

Before spotless:

    private static record EndpointValue<T>(T value) {
    }

==> this compiles fine locally and in CI

after spotless:

    private static record EndpointValue<T> (T value) {
    }

Note the space inserted after the diamond operator ( and BTW Eclipse IDE does the same when saving file).

==> this compiles fine locally but not in CI

[fwolter]

This doesn't build on my side (current Git HEAD):

    private static record EndpointValue<T>(T value) {
    }

PS C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.freeboxos> mvn clean install
[INFO] Scanning for projects...
[INFO]
[INFO] Using the MultiThreadedBuilder implementation with a thread count of 64
[INFO]
[INFO] ------< org.openhab.addons.bundles:org.openhab.binding.freeboxos >------
[INFO] Building openHAB Add-ons :: Bundles :: FreeboxOS Binding 4.0.0-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[WARNING] The POM for org.eclipse.orbit.bundles:net.i2p.crypto.eddsa:jar:0.3.0.v20220506-1020 is missing, no dependency information available
[INFO]
[INFO] --- maven-clean-plugin:3.0.0:clean (default-clean) @ org.openhab.binding.freeboxos ---
[INFO] Deleting C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons\bundles\org.openhab.binding.freeboxos\target
[INFO]
[INFO] --- maven-enforcer-plugin:3.0.0-M2:enforce (enforce-java) @ org.openhab.binding.freeboxos ---
[INFO]
[INFO] --- directory-maven-plugin:1.0:directory-of (directories) @ org.openhab.binding.freeboxos ---
[INFO] Directory of org.openhab.addons:org.openhab.addons.reactor set to: C:\Users\Fabian Wolter\Documents\GitHub\openhab-addons
[INFO]
[INFO] --- spotless-maven-plugin:2.28.0:check (codestyle_check) @ org.openhab.binding.freeboxos ---
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  5.400 s (Wall Clock)
[INFO] Finished at: 2023-06-25T21:17:02+02:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.28.0:check (codestyle_check) on project org.openhab.binding.freeboxos: The following files had format violations:
[ERROR]     src\main\java\org\openhab\binding\freeboxos\internal\api\rest\HomeManager.java
[ERROR]         @@ -60,7 +60,7 @@
[ERROR]          ········UNKNOWN;
[ERROR]          ····}
[ERROR]
[ERROR]         -····private·static·record·EndpointValue<T>(T·value)·{
[ERROR]         +····private·static·record·EndpointValue<T>·(T·value)·{
[ERROR]          ····}
[ERROR]
[ERROR]          ····private·static·record·EndpointUi(AccessType·access,·DisplayType·display,·String·iconUrl,·@Nullable·String·unit)·{
[ERROR] Run 'mvn spotless:apply' to fix these violations.
[ERROR] -> [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/MojoExecutionException

I'm wondering why the CI build succeeds...

[fwolter]

@jlaur could you have a quick look wether the current Git HEAD or the version with the space builds on your side?

[jlaur]

could you have a quick look wether the current Git HEAD or the version with the space builds on your side?

Current HEAD doesn't build for me:

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.28.0:check (codestyle_check) on project org.openhab.binding.freeboxos: The following files had format violations:
[ERROR]     src\main\java\org\openhab\binding\freeboxos\internal\api\rest\HomeManager.java
[ERROR]         @@ -60,7 +60,7 @@
[ERROR]          ········UNKNOWN;
[ERROR]          ····}
[ERROR] 
[ERROR]         -····private·static·record·EndpointValue<T>(T·value)·{
[ERROR]         +····private·static·record·EndpointValue<T>·(T·value)·{
[ERROR]          ····}
[ERROR] 
[ERROR]          ····private·static·record·EndpointUi(AccessType·access,·DisplayType·display,·String·iconUrl,·@Nullable·String·unit)·{
[ERROR] Run 'mvn spotless:apply' to fix these violations.

Spotless changes to:

    private static record EndpointValue<T> (T value) {
    }

and then it builds.

Unrelated, but now seeing the build - @clinique, there are a few compiler warnings:

[WARNING] openhab-addons\bundles\org.openhab.binding.freeboxos\src\main\java\org\openhab\binding\freeboxos\internal\api\rest\FreeboxOsSession.java:[151,20] Null comparison always yields false: The variable result cannot be null at this location
[WARNING] openhab-addons\bundles\org.openhab.binding.freeboxos\src\main\java\org\openhab\binding\freeboxos\internal\api\ApiHandler.java:[124,44] Potential null pointer access: The variable error may be null at this location
[WARNING] openhab-addons\bundles\org.openhab.binding.freeboxos\src\main\java\org\openhab\binding\freeboxos\internal\api\rest\APManager.java:[124,17] Redundant null check: The variable stations cannot be null at this 
location

[jlaur]

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.28.0:check (codestyle_check) on project org.openhab.binding.freeboxos: The following files had format violations:
[ERROR]     src\main\java\org\openhab\binding\freeboxos\internal\api\rest\HomeManager.java
[ERROR]         @@ -60,7 +60,7 @@
[ERROR]          ········UNKNOWN;
[ERROR]          ····}
[ERROR] 
[ERROR]         -····private·static·record·EndpointValue<T>(T·value)·{
[ERROR]         +····private·static·record·EndpointValue<T>·(T·value)·{
[ERROR]          ····}
[ERROR] 
[ERROR]          ····private·static·record·EndpointUi(AccessType·access,·DisplayType·display,·String·iconUrl,·@Nullable·String·unit)·{
[ERROR] Run 'mvn spotless:apply' to fix these violations.

@clinique - when did you last rebase towards main? The difference in local vs. CI builds could perhaps be explained by upstream changes?

[clinique]

@fwolter : rebased and it builds.

[fwolter]

I can confirm that.

[fwolter]

Just to avoid confusion. There is one open point left.

Under which circumstances is the returned status PENDING?

[clinique]

@fwolter : let me know your thoughts on this alternative implementation proposal.