[Freeboxos] New binding alternative to 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.

[jlaur]

I guess it should be:

Suggested change

	# Freebox Binding
	# FreeboxOS Binding

? But please see latest PR comments before changing anything.

[jlaur]

FreeboxOs or FreeboxOS?

[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.

[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.

[fwolter]

The current implementation spawns a new thread pool, which should only be done in specific cases like creating a server thread listening on a socket or reading data from a serial interface.

We had a very similar case here #13570 (comment). I'm sure we can work out a viable solution, but I need a bit more context about the API returning PENDING. Is this returned on a regular basis or only in error cases? Can you somehow preemptively prevent the situation of receiving a PENDING?

[fwolter]

Are you sure this is working? You're calling wait() on the Optional. However, wait() shouldn't be used directly, and if so, it must be called in a loop. https://wiki.sei.cmu.edu/confluence/display/java/THI03-J.+Always+invoke+wait%28%29+and+await%28%29+methods+inside+a+loop You could call get() on the ScheduledFuture.

[clinique]

The current implementation spawns a new thread pool, which should only be done in specific cases like creating a server thread listening on a socket or reading data from a serial interface.

We had a very similar case here #13570 (comment). I'm sure we can work out a viable solution, but I need a bit more context about the API returning PENDING. Is this returned on a regular basis or only in error cases? Can you somehow preemptively prevent the situation of receiving a PENDING?

Here's the current process :
1- The binding tries to authenticated with its current token (that may be empty on first run)
2- If no token has been stored previously, the authentication will fail (this is expected) and then the binding will send a pairing request
3- During the pairing request, the user has to move to its Free router to approve the pairing request. During this time lapse, the API will answer PENDING, until either TIMEOUT, or GRANTED if the user pressed "OK" on its device.
4- If GRANTED is received, the token is validated and given back to the handler so it is stored in the handler configuration. On next run of (1) it will success, so we'll never enter again in this loop.
5- The authentication is granted, now the binding and handlers lives their live and does their job - we do not get back in these 2 ->4 steps.

[jlaur]

@clinique - I don't know if relevant for this PR, but @lolodomo pushed some changes in #15121.

[fwolter]

What about moving the pairing code into initialize() of the Thing handler (asynchronously)? The Thing status could be ONLINE with the Thing status detail CONFIGURATION_PENDING as long as the API returns PENDING. The Thing status detail message could point the user to press the pairing button.

Thing commands will fail as long as the Thing status detail is CONFIGURATION_PENDING, but this should be OK.

During CONFIGURATION_PENDING, you could poll the API every 5s or so by scheduling another task via the framework's scheduler and finally set the Thing ONLINE without a Thing status detail or to OFFLINE when the API returned the TIMEOUT state. In the latter case you could set a Thing status detail message pointing the user to re-enable the Thing to start the pairing process again.

Then, you don't need to block Thing commands when the granting process is ongoing.

[clinique]

What about moving the pairing code into initialize() of the Thing handler (asynchronously)? The Thing status could be ONLINE with the Thing status detail CONFIGURATION_PENDING as long as the API returns PENDING. The Thing status detail message could point the user to press the pairing button.

Thing commands will fail as long as the Thing status detail is CONFIGURATION_PENDING, but this should be OK.

During CONFIGURATION_PENDING, you could poll the API every 5s or so by scheduling another task via the framework's scheduler and finally set the Thing ONLINE without a Thing status detail or to OFFLINE when the API returned the TIMEOUT state. In the latter case you could set a Thing status detail message pointing the user to re-enable the Thing to start the pairing process again.

Then, you don't need to block Thing commands when the granting process is ongoing.

Thanks for your ideas. I'll have a look at this no later than tomorrow.

[fwolter]

Did you finish making all changes?

[clinique]

Did you finish making all changes?

Yes. Tested and working.

[fwolter]

LGTM
Now, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website

Please make sure your sign-off contains your realname for future PRs.

[fwolter]

Are you going to remove the legacy freebox binding from this repo and submit it to the marketplace?

[lolodomo]

Are you going to remove the legacy freebox binding from this repo and submit it to the marketplace?

No ;)

I will first intensively test the new binding and check that no feature from the original binding is missing.
You can plan to remove it in a next version.

[jlaur]

Are you going to remove the legacy freebox binding from this repo and submit it to the marketplace?

No ;)

I will first intensively test the new binding and check that no feature from the original binding is missing. You can plan to remove it in a next version.

Please see #12342 (comment). I think the agreement was to upload the legacy binding the Community Marketplace and remove it from the distribution, so that 4.0 will only contain the new freeboxos version. Users not ready to migrate can then uninstall the bundled version and install the community version, and everything will still work.

[lolodomo]

I did not remember about that !
So please don't remove it from the distribution until it is available in the marketplace and I have verified that it is working.

[jlaur]

I did not remember about that !
So please don't remove it from the distribution until it is available in the marketplace and I have verified that it is working.

@lolodomo, @clinique - what is the status here? We are about to release 4.0 with two overlapping bindings, which was not the intention. Cc @kaikreuzer

[clinique]

@jlaur : there's not much I can do here - pending @lolodomo on this.