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

[FreeboxOs] Introducing new version of the Freebox binding. #9180

Closed
wants to merge 21 commits into from

Conversation

clinique
Copy link
Contributor

As this version introduces many many modifications, it is presented as a new binding, targetted to repleace the current Freebox Binding in the future.

Signed-off-by: clinique gael@lhopital.org

@clinique clinique self-assigned this Nov 30, 2020
@clinique clinique added the new binding If someone has started to work on a binding. For a new binding PR. label Nov 30, 2020
@clinique clinique added this to the 3.0 milestone Nov 30, 2020
@clinique clinique requested a review from a team as a code owner November 30, 2020 16:44
@fwolter fwolter added the work in progress A PR that is not yet ready to be merged label Dec 6, 2020
@clinique clinique modified the milestones: 3.0, 3.1 Jan 7, 2021
@wborn wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 22, 2021
@Skinah
Copy link
Contributor

Skinah commented Jun 17, 2021

Is this binding ready for a review? It is marked as a WIP work in progress which is probably stopping people from looking at it. Feel free to remove the tag and edit the title when your wanting someone to check it out in more depth.

@clinique clinique changed the title [WIP] [FreeboxOs] Introducing new version of the Freebox binding. [FreeboxOs] Introducing new version of the Freebox binding. Jun 17, 2021
@clinique
Copy link
Contributor Author

You're right, forgot it. Done.

@Skinah Skinah removed the work in progress A PR that is not yet ready to be merged label Jun 17, 2021
@wborn wborn removed this from the 3.1 milestone Jun 28, 2021
@lolodomo
Copy link
Contributor

lolodomo commented Sep 2, 2021

@clinique : I think you mixed 2 different things: freebox player and air media player.
A freebox player can be a air media player but not all are (mini 4k is driven by Google OS).
An air media player can be an Apple device like a iPhone for example.
So I think you should define, like in the original binding, a thing type for air media players and keep your player thing type for freebox player but without air media feature.
Your thoughts?

@clinique
Copy link
Contributor Author

clinique commented Sep 3, 2021

@lolodomo : yes I think you're right. I've got some underlaying architecture to review regarding this. I'm going to push my updates to your other comments right now, and will take a bit of time to experiment a bit more and come back with a new proposal.

Signed-off-by: clinique <gael@lhopital.org>
@clinique
Copy link
Contributor Author

clinique commented Sep 3, 2021

@lolodomo : yes I think you're right. I've got some underlaying architecture to review regarding this. I'm going to push my updates to your other comments right now, and will take a bit of time to experiment a bit more and come back with a new proposal.

One question I have regarding network devices is if they should toggle ONLINE/OFFLINE at thing level of if I should keep the "Reachable" channel instead. Your thoughts ?

@lolodomo
Copy link
Contributor

lolodomo commented Sep 4, 2021

@clinique : PR submitted to your repo proposing a proper way to initialize things + few other fix/changes.

@lolodomo
Copy link
Contributor

lolodomo commented Sep 4, 2021

One question I have regarding network devices is if they should toggle ONLINE/OFFLINE at thing level of if I should keep the "Reachable" channel instead. Your thoughts ?

The channel must be kept, it allows to show device reachability in UIs.

@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Sep 4, 2021
@lolodomo
Copy link
Contributor

lolodomo commented Sep 4, 2021

I played a little with the player thing. I think you should set the player-status#power-status channel to "OFF" and the sysinfo#uptime channel to UNDEF when the API is failing due to the player being off (not reachable). Maybe you should first use the general player API to check if the player is defined as reachable or not ?

@lolodomo
Copy link
Contributor

lolodomo commented Sep 4, 2021

To summarize, for a player, you have to combine all these inputs:

  • A player can support or not the API
  • Even if it supports the API, the API will fail if the player is powered off
  • Player API will not be available in any case if the permission to use it is not set.

@lolodomo
Copy link
Contributor

No news ?

@clinique
Copy link
Contributor Author

clinique commented Sep 17, 2021

@lolodomo : yes, I'm progressing, you'll have a PR in the coming days. I had to make important changes under the hood because I figured an issue with ApiHandler. The way it was done, would not work if we have two api bridges. I'm also doing the changes regarding initialization of the properties, removed from the discovery process.

ApiHandler can now be used by multiple bridges
Still pending Airmedia proper handling

Signed-off-by: clinique <gael@lhopital.org>
@lolodomo
Copy link
Contributor

lolodomo commented Sep 26, 2021

Your current version does not compile due to 3 SAT errors. In particulmar, a new class is missing its header and its author.

I will restart the review and the testing in the next coming days (I fixed the compilation errors in my branch).

@clinique
Copy link
Contributor Author

@lolodomo : expect a new PR soon (today hopefully) with work done regarding your comments on AirMedia devices. I'll ensure it builds flawlessly. Best regards

Signed-off-by: clinique <gael@lhopital.org>
Signed-off-by: clinique <gael@lhopital.org>
@lolodomo
Copy link
Contributor

What's the need of all these new dependencies like hinernate??

@clinique
Copy link
Contributor Author

What's the need of all these new dependencies like hinernate??

Required for javax.validation.

@lolodomo
Copy link
Contributor

Why is it suddenly used and necessary ? For what purpose exactly ?

@kaikreuzer : are such dependencies recommended for openHAB?

@lolodomo
Copy link
Contributor

Isn't it something from Java EE?

@clinique
Copy link
Contributor Author

clinique commented Sep 27, 2021

Why is it suddenly used and necessary ? For what purpose exactly ?

This is a validation API for Java Bean. I saw it was used in the ongoing PR that adresses openhab (in the core repo) update and thought it would be nice to try. It's quite convenient to set validation constraints on objects after deserialization. Take a look at Challenge.java class for an usage example.
See also here

@kaikreuzer
Copy link
Member

@kaikreuzer : are such dependencies recommended for openHAB?

Not really. To some extend, add-ons are free to use what is required for them, but for general tooling, we try to define a common set (partially in https://www.openhab.org/docs/developer/guidelines.html#default-libraries) to use.

If there is a general wish for a javax.validator implementation to be available, we should imho rather make add-ons only depend on the API, but not on a specific implementation like here from Hibernate. This should rather be provided by openhab-core (and could be replaced for everyone, if that becomes necessary).

So how strong is your love for this, @clinique? If you think it is worthwhile and that everybody should use it, I'm open for discussion.

@clinique
Copy link
Contributor Author

clinique commented Sep 27, 2021

@kaikreuzer : thanks for jumping on the topic. As written above, I got curious of this when I saw it was used by this core PR. In a first time I though it was available through the core but apparently not. javax.validation seems to be available but an implementation is missing. From my searches, hibernate validation implementation seems to be a well established one.

How am I in leave with it ?
Well, I think it's a very good complement to Null Annotation - and especially when you need to validate DTO deserialized by GSON. This avoids a lot of in-code checks and/or exceptions. I'm just beginning using it - so removing will not be a great effort (while it has been quite tedious to find appropriate versions in pom.xml and features.xml) but on the other hand I find it quite powerfull and could be widely used in bindings to validate configurations, incoming data... if available through the core.

@lolodomo
Copy link
Contributor

No major impact on OH weight?

@lolodomo
Copy link
Contributor

@kaikreuzer : does it mean we put this PR in standby until javax.validator is first embedded in the core framework?

Signed-off-by: clinique <gael@lhopital.org>
Signed-off-by: clinique <gael@lhopital.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR. work in progress A PR that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants