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

[meteofrance] New binding #15462

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Conversation

clinique
Copy link
Contributor

@clinique clinique commented Aug 19, 2023

Resolves #15461 .

Data feed changed completely, this will have major impact on binding organization.

This binding also integrates another Meteo France service : rain previsions over the coming hour.

Being more generic, it has been renamed and meteoalerte binding should be removed.

@clinique clinique added bug An unexpected problem or unintended behavior of an add-on work in progress A PR that is not yet ready to be merged labels Aug 19, 2023
@clinique clinique self-assigned this Aug 19, 2023
@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/meteo-alerte-binding-do-not-receive-data-since-may-2023/148545/5

@clinique clinique removed the work in progress A PR that is not yet ready to be merged label Apr 2, 2024
@clinique
Copy link
Contributor Author

clinique commented Apr 2, 2024

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clinique I am not sure if I am a good reviewer for this binding.... For now, I just checked a few code style topics, javadoc has only expected warnings, i18n property file is in sync. That is what I can do today.
Find my suggestions below.

@clinique
Copy link
Contributor Author

clinique commented Apr 2, 2024

@lolodomo : if you can start to have a look at this. I think it's pretty ready codewise. Documentation still has to be updated.

@clinique clinique requested review from lolodomo and jlaur April 2, 2024 23:23
@clinique clinique changed the title [meteoalerte] Adapting binding to new data feed [meteoalerte] Resurrect the binding Apr 3, 2024
@clinique
Copy link
Contributor Author

@lolodomo : can I kindly request a review ?

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just 2 minor comments regarding the README...

bundles/org.openhab.binding.meteoalerte/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.meteoalerte/README.md Outdated Show resolved Hide resolved
Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this one seems missing.
Otherwise the parts I have looked at LGTM, leaving the code review to @lolodomo.

| apikey | Data-platform token to access the service. Mandatory. |

To obtain an API key, you must create an account on https://portail-api.meteofrance.fr/web/fr/
Inside the Portail API, create an application on "Bulletin Vigilance" and generate they API Key (set duration to 0).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Inside the Portail API, create an application on "Bulletin Vigilance" and generate they API Key (set duration to 0).
Inside the Portail API, create an application on "Bulletin Vigilance" and generate they API key (set duration to 0).

@clinique
Copy link
Contributor Author

@lolodomo : as you're currently working only for me 😀 , maybe you can have a look at this one also ?

@lolodomo
Copy link
Contributor

@lolodomo : as you're currently working only for me 😀 , maybe you can have a look at this one also ?

I hope you appreciate :)
That is not fully true, I am also working for @mherwege , my top priority regarding reviews ;)

@clinique clinique requested a review from a team as a code owner June 25, 2024 10:12
@clinique clinique changed the title [meteoalerte] Resurrect the binding [meteofrance] New binding Jun 25, 2024
Small and slow progress

Signed-off-by: clinique <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Data is ok.
Still has to take care of auto refresh and we're ready to go.

Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Signed-off-by: clinique <[email protected]>
Switching internal dept Id from string name of the department to department number.

Signed-off-by: [email protected] <[email protected]>
Signed-off-by: clinique <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Renamed from Meteo Alerte to Meteo France

Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
@clinique clinique added the enhancement An enhancement or new feature for an existing add-on label Aug 21, 2024
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviving this binding. As @lolodomo has chosen to work for anotther contributer ;-) i took the change and reviewed this PR.
Covered all files and after this initial round i expect jus a smaller last round before a merge. Ping me when you are ready to proceed.

bom/openhab-addons/pom.xml Show resolved Hide resolved

The Meteo Alerte binding gives alert level regarding major weather related risk factors.
The Meteo France binding gives alert level regarding major weather related risk factors as well as rain intensity forecasts.
This binding provides its own icon set and provides appropriate static and dynamic SVG icons (see items examples below).

Copy link
Contributor

@lsiepel lsiepel Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A section with the supported Things is missing. When added, it really helps to get a quick overisght of the bindings basics.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rearranged the section.

| apikey | Data-platform token to access the service. Mandatory. |

To obtain an API key, you must create an account on https://portail-api.meteofrance.fr/web/fr/
Inside the Portail API, create an application on "Bulletin Vigilance" and generate they API Key (set duration to 0).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Inside the Portail API, create an application on "Bulletin Vigilance" and generate they API Key (set duration to 0).
Inside the API Portal, create an application on "Bulletin Vigilance" and generate they API Key (set duration to 0).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected


## Thing Configuration

The thing has a few configuration parameters:
The `vigilance` thing has these configuration parameters:
Copy link
Contributor

@lsiepel lsiepel Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i could not get a clear meaning of vigilance why did you choose this name as thing id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the name coming from the source dataset - let me see if I can find a better naming.

Comment on lines 85 to 94
| oh:meteofrance:vent | Yes | ![](src/main/resources/icon/vent.svg) |
| oh:meteofrance:pluie-inondation | Yes | ![](src/main/resources/icon/pluie-inondation.svg) |
| oh:meteofrance:orage | Yes | ![](src/main/resources/icon/orage.svg) |
| oh:meteofrance:inondation | Yes | ![](src/main/resources/icon/inondation.svg) |
| oh:meteofrance:neige | Yes | ![](src/main/resources/icon/neige.svg) |
| oh:meteofrance:canicule | Yes | ![](src/main/resources/icon/canicule.svg) |
| oh:meteofrance:grand-froid | Yes | ![](src/main/resources/icon/grand-froid.svg) |
| oh:meteofrance:avalanches | Yes | ![](src/main/resources/icon/avalanches.svg) |
| oh:meteofrance:vague-submersion | Yes | ![](src/main/resources/icon/vague-submersion.svg) |
| oh:meteofrance:meteo_france | No | ![](src/main/resources/icon/meteo_france.svg) |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make the icons viewable in the docs, you have to copy the files to the docs folder and alter these links.
Unoftunately that means these icons are doubled in the repository.

Copy link
Contributor Author

@clinique clinique Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated images in doc/images. Added the intensity icon.

Comment on lines 34 to 35
BridgeHandler handler = bridge.getHandler();
if (handler instanceof MeteoFranceBridgeHandler maHandler) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BridgeHandler handler = bridge.getHandler();
if (handler instanceof MeteoFranceBridgeHandler maHandler) {
if (bridge.getHandler() instanceof MeteoFranceBridgeHandler maHandler) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed


updateStatus(ThingStatus.UNKNOWN);

ForecastConfiguration config = getConfigAs(ForecastConfiguration.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to check the configuration ? For like isBlank() or for having a numeric value? (i assumed the department code are all numeric ?)
and set the thing state to CONFIGURATION_ERROR accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

location should be a valid PointType. Modified accordingly.


private void updateDate(String channelId, @Nullable ZonedDateTime zonedDateTime) {
if (isLinked(channelId)) {
updateState(channelId, zonedDateTime != null ? new DateTimeType(zonedDateTime) : UnDefType.NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the framework does not automatically change the zoneddatetime to the configured timezone of openHAB.
Not sure in what timezone the API responds, but it would be nice to use the openHAB api -> timezoneprovider to update it to the openHAB configured timezone.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're more than right.

}
updateStatus(ThingStatus.ONLINE);
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.NONE, "No data available for the department");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a check, what would be the cause of no data being available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that I has happened during tests or developments if it is there.


private void updateDate(String channelId, ZonedDateTime zonedDateTime) {
if (isLinked(channelId)) {
updateState(channelId, new DateTimeType(zonedDateTime));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See earlier comment about respecting the openHAB configured timezone with the TimeZoneProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

Signed-off-by: Gaël L'hopital <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[meteoalerte] Data frozen since May
5 participants