-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[electroluxair] Initial contribution #11116
Conversation
@fwolter I've created a new PR and I would like to have it reviewed :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add your binding to the pom.xml in the root directory and to bom\openhab-addons\pom.xml
. Then, you can check the static code analysis output after running mvn clean install
.
Please add yourself to the CODEOWNERS file.
<p align="center"> | ||
<img src="doc/electrolux_pure_a9.webp" alt="Electrolux Pure A9" width="200px"/> | ||
</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to stick to markdown, even if the image is not centered.
<p align="center"> | |
<img src="doc/electrolux_pure_a9.webp" alt="Electrolux Pure A9" width="200px"/> | |
</p> | |
 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
## Supported Things | ||
|
||
This binding supports the following thing types: | ||
|
||
- Bridge | ||
- Electrolux Pure A9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add the Thing Type UIDs needed for textual configuration.
## Binding Configuration | ||
|
||
You will have to configure the bridge with username and password, these must be the same credentials as used when logging into your Electrolux Wellbeing app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This chapter is inteded for the binding global configuration. As your binding doesn't have this, this chapter could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chapter removed!
#### Bridge | ||
|
||
* `username` - The username used to connect to the Electrolux Wellbeing app | ||
|
||
* `password` - The password used to connect to the Electrolux Wellbeing app | ||
|
||
* `refresh` - Specifies the refresh interval in second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the other bindings, you could add a table with the type (Number, Text), the default value if any and if the parameter is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
If you define the bridge in a things-file the bridge type id is defined as `bridge`, e.g.: | ||
|
||
`Bridge electroluxair:bridge:myElectroluxAirBridge` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be removed if you mention the Thing Type UID above and by the example section below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
} | ||
|
||
private void update(@Nullable ElectroluxPureA9DTO dto) { | ||
logger.debug("update: {}", dto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above. Remove. Please check all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
properties.put(PROPERTY_BRAND, dto.getApplicancesInfo().brand); | ||
properties.put(PROPERTY_COLOUR, dto.getApplicancesInfo().colour); | ||
properties.put(PROPERTY_DEVICE, dto.getApplicancesInfo().device); | ||
properties.put(PROPERTY_MODEL, dto.getApplicancesInfo().model); | ||
properties.put(PROPERTY_SERIAL_NUMBER, dto.getApplicancesInfo().serialNumber); | ||
properties.put(PROPERTY_FW_VERSION, dto.getTwin().getProperties().getReported().frmVerNIU); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are predefined properties you could use.
properties.put(PROPERTY_BRAND, dto.getApplicancesInfo().brand); | |
properties.put(PROPERTY_COLOUR, dto.getApplicancesInfo().colour); | |
properties.put(PROPERTY_DEVICE, dto.getApplicancesInfo().device); | |
properties.put(PROPERTY_MODEL, dto.getApplicancesInfo().model); | |
properties.put(PROPERTY_SERIAL_NUMBER, dto.getApplicancesInfo().serialNumber); | |
properties.put(PROPERTY_FW_VERSION, dto.getTwin().getProperties().getReported().frmVerNIU); | |
properties.put(Thing.PROPERTY_VENDOR, dto.getApplicancesInfo().brand); | |
properties.put(PROPERTY_COLOUR, dto.getApplicancesInfo().colour); | |
properties.put(PROPERTY_DEVICE, dto.getApplicancesInfo().device); | |
properties.put(Thing.PROPERTY_MODEL_ID, dto.getApplicancesInfo().model); | |
properties.put(Thing.PROPERTY_SERIAL_NUMBER, dto.getApplicancesInfo().serialNumber); | |
properties.put(Thing.PROPERTY_FIRMWARE_VERSION, | |
dto.getTwin().getProperties().getReported().frmVerNIU); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
} else if (THING_TYPE_BRIDGE.equals(thingTypeUID)) { | ||
return new ElectroluxAirBridgeHandler((Bridge) thing, httpClient, gson); | ||
} | ||
logger.warn("ThingHandler not found for {}", thing.getThingTypeUID()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already logged by the framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
@@ -0,0 +1,17 @@ | |||
# FIXME: please substitute the xx_XX with a proper locale, ie. de_DE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this file be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I removed it.
</bridge-type> | ||
|
||
<thing-type id="electroluxpurea9"> | ||
<label>ElectroluxAir Pure A9 Thing</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Labels are expected to be as short as possible. Guideline is 2-3 words with max 20-25 chars. See https://www.openhab.org/docs/developer/bindings/thing-xml.html#formatting-labels-and-descriptions
<label>ElectroluxAir Pure A9 Thing</label> | |
<label>ElectroluxAir Pure A9</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fwolter What is the best way to fix the conflicts? Rebase from main and then force push? Or should I do a fetch/merge and then normal push?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do a rebase against main/upstream, resolve the conflicts locally and than do a force-push.
208bb55
to
9e3caa9
Compare
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:
The remotes (
Then, you can rebase your PR's branch onto
Finally force-push the rebased branch to this PR's branch:
DON'T use merge! This can clutter the Git history and make the PR unusable. |
9e3caa9
to
9eff8e6
Compare
## Supported Things | ||
|
||
This binding supports the following types: | ||
|
||
- Bridge | ||
- Electrolux Pure A9 | ||
|
||
If you define the bridge in a things-file the bridge type id is defined as `bridge`, e.g.: | ||
|
||
`Bridge electroluxair:api:myAPI "Electrolux Delta API" [username="user@password.com", password="12345", refresh="300"` | ||
|
||
If you define the thing in a things-file the thing type id is defined as: | ||
|
||
`Thing electroluxpurea9 MyElectroluxPureA9 "Electrolux Pure A9" [ deviceId="123456789" ]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this section you should only mention the ThingTypeUIDs and describe them shortly. The config examples could be moved to the "Full Examples" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
| password | The password used to connect to the Electrolux Wellbeing app | String | NA | yes | | ||
| refresh | Specifies the refresh interval in second | Number | 600 | yes | | ||
|
||
## Thing Configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to make sense in the "Configuration Options" chapter.
## Thing Configuration | |
#### Thing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it!
|
||
Only the bridge require manual configuration. The thing can be added by hand, or you can let the discovery mechanism automatically find your Electrolux Pure A9 thing. | ||
|
||
### Configuration Options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This heading is already present a few lines above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
|
||
| Channel Type ID | Item Type | Description | | ||
|-----------------|-----------|-------------------------------------------------------------------------------------------------| | ||
| status | String | This channel can be used to trigger an instant refresh by sending a RefreshType.REFRESH command.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you push your changes?
| filterLife | Number:Dimensionless | This channel reports the remaining filter life in %. | | ||
| ionizer | Switch | This channel sets and reports the status of the ionizer function (On/Off). | | ||
| doorOpen | Contact | This channel reports the status of door (Opened/Closed). | | ||
| workMode | String | This channel sets and reports the current work mode (Auto, Manual, PowerOff.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| workMode | String | This channel sets and reports the current work mode (Auto, Manual, PowerOff.| | |
| workMode | String | This channel sets and reports the current work mode (Auto, Manual, PowerOff.) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
private final HttpClient httpClient; | ||
private @Nullable ElectroluxDeltaAPI api; | ||
|
||
private static int REFRESH_SEC = 300; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, this shouldn't be static. Otherwise this field would be global and the last loaded ThingHandler's config would overwrite all other ThingHandler's refresh fields.
There are some checkstyle warnings left. You could take a look at There are some compiler warnings about null access left. You could take a look at |
I only see warnings on patterns for 3 variables. |
9eff8e6
to
a2782d5
Compare
Did you finish making all changes? |
Yes sir! :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see warnings on patterns for 3 variables.
Yeah, I see them, too.
This binding supports the following types: | ||
|
||
- Bridge | ||
- Electrolux Pure A9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you name the ThingTypeUIDs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this is sufficient:
api
: Bridge - Implements ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
|
||
| Channel Type ID | Item Type | Description | | ||
|-----------------|-----------|-------------------------------------------------------------------------------------------------| | ||
| status | String | This channel can be used to trigger an instant refresh by sending a RefreshType.REFRESH command.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see the status channel in the readme. Did you push your changes?
public ElectroluxDeltaAPI(ElectroluxAirBridgeConfiguration configuration, Gson gson, HttpClient httpClient) | ||
throws IOException { | ||
this.gson = gson; | ||
this.configuration = configuration; | ||
this.httpClient = httpClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What can throw an IOException here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed!
a2782d5
to
1278fd0
Compare
Please use a regular commit instead of a force-push after implementing review feedback. Then, the reviewer can take a look at your changes instead of reviewing the entire code again. |
I get this error when doing a git push:
I did a git pull and resolved the merge conflict. |
Your branch is broken, now. Here is a howto fix it https://community.openhab.org/t/rebase-your-code-or-how-to-fix-your-git-history-before-requesting-a-pull/129358 |
…ding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
…ding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
6fd60ab
to
866a70c
Compare
Did a new rebase and a |
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Cool! :-) Thank's for your review, I learned a lot! :-) |
Please create a subsequent PR to add the default translation properties file. |
* [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review and also copyright to 2022. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> Signed-off-by: Michael Schmidt <mi.schmidt.83@gmail.com>
- put each sentence in a seperate line - add note that only PNG images are supported. See openhab/openhab-addons#11116 (comment) - add examples to "Supported Things" - add RW column to example Channel table - add advanced and default config value - add note that textual config examples are mandatory - make .sitemap example optional (see small internal discussion) - put emphasis on initialize() shall return quickly - add note about logging to INFO - fix compiler warning Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
* [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review and also copyright to 2022. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
* [skeleton] Add Thing config table to readme; minor improvements - put each sentence in a seperate line - add note that only PNG images are supported. See openhab/openhab-addons#11116 (comment) - add examples to "Supported Things" - add RW column to example Channel table - add advanced and default config value - add note that textual config examples are mandatory - make .sitemap example optional (see small internal discussion) - put emphasis on initialize() shall return quickly - add note about logging to INFO - fix compiler warning - Add default config value for initialization Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
* [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review and also copyright to 2022. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
* [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review and also copyright to 2022. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> Signed-off-by: Nick Waterton <n.waterton@outlook.com>
* [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review and also copyright to 2022. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
* [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review and also copyright to 2022. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * [electroluxair] Initial contribution of the electroluxair openHAB binding Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review. Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> * Updated after code review Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com> Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
…hab#2692) * [skeleton] Add Thing config table to readme; minor improvements - put each sentence in a seperate line - add note that only PNG images are supported. See openhab/openhab-addons#11116 (comment) - add examples to "Supported Things" - add RW column to example Channel table - add advanced and default config value - add note that textual config examples are mandatory - make .sitemap example optional (see small internal discussion) - put emphasis on initialize() shall return quickly - add note about logging to INFO - fix compiler warning - Add default config value for initialization Signed-off-by: Fabian Wolter <github@fabian-wolter.de> GitOrigin-RevId: f79d17f
Signed-off-by: Jan Gustafsson jannegpriv@gmail.com
Initial contribution of the electroluxair openHAB binding.
The binding uses the Electrolux Delta REST API and the binding implements support for the Electrolux Pure A9 Air Purifier and solves #10246.
In the community thread you can find a link to a built binding jar-file.