-
-
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
[electroluxappliance] Initial contribution #17663
Conversation
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
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.
Thank you for contributing and following up on the previous binding. Fully covered the binding. It is already in a good shape, just many small comments. The most important ones are the ones around the channel and type id's i guess.
Would be nice if you comment or thumgbs up the comments and let me mark them as resolved, this way i can keep track.
</channel-type> | ||
|
||
<channel-type id="doorLock"> | ||
<item-type>Switch</item-type> |
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.
As this is a read only status for a lock, did you consider to use the contact type?
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.
Correct, I've changed.
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.
Just a clarification, should the Switch type only be used if there is a possibility to send a command to change the state? Otherwise we should use Contact?
I was thinking of connection-state? It is read-only but currently modelled as a Switch.
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.
Switch can also be marked as read-only. A contact (open/closed) is usually for doors, windows, locks, cicruits etc. I don't think this is used for connection states. I would also ask myself why do you need a connection state channel ?
The Thing state should already reflect the connection state, or am i missing something 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.
The Thing state should already reflect the connection state, or am i missing something here?
Not with current implementation. So then I guess I should use connection-state as a base for setting thing state to Online/Offline, that is not done today.
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.
No rush, don't forget to flush your commits as currently all is pending.
...les/org.openhab.binding.electroluxappliances/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
<item-type>Number:Time</item-type> | ||
<label>Time To End</label> | ||
<description>The time to end of a program.</description> | ||
<state readOnly="true"/> |
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.
Please provide a state pattern and if not the default unit a unithint (seconds is default)
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.
It is seconds.
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.
Both time-to-start and time-to-end are missing unitHint
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.
Should I then add unitHint="s"?
<item-type>Number:Time</item-type> | ||
<label>Appliance Total Working Time</label> | ||
<description>The appliance total working time.</description> | ||
<state readOnly="true"/> |
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.
Please provide a state pattern and i guess a unitHint="h"
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.
Still don't see the unitHint, what did you change?
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 reported in seconds:
Channel: electroluxappliance:washing-machine:6996e7d524:33600001:appliance-total-working-time, State: 1490400
Should I then add unitHint="s"
?
...les/org.openhab.binding.electroluxappliances/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
I think binding name should be in singular form (electroluxappliance), but let me check documentation and other binding names, and let me get back. |
I have not found a single binding using plural, but a few similar named bindings denoting the type of device they are for, and being in singular: |
OK, will then rename binding to electroluxappliance. |
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
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.
Think this is the last review round.
bundles/org.openhab.binding.electroluxappliance/src/main/feature/feature.xml
Outdated
Show resolved
Hide resolved
...rg/openhab/binding/electroluxappliance/internal/handler/ElectroluxWashingMachineHandler.java
Outdated
Show resolved
Hide resolved
Just to be sure they are not missed, some (buried) comments are left open. |
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.
Just a comment regarding OAuth2.
|--------------|-------------------------------------------------------|--------|----------|----------| | ||
| apiKey | Your created API key on developer.electrolux.one | String | NA | yes | | ||
| accessToken | Your created access token on developer.electrolux.one | String | NA | yes | | ||
| refreshToken | Your created access token on developer.electrolux.one | String | NA | yes | |
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 don't know if you discussed this already, but is there any reason not to use to OAuth2 implementation provided by core? See for example the Netatmo, Miele Cloud and Bosch Indego bindings.
I have gone through all and added some questions. |
No, that has not been brought up before. |
Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
Since Electrolux Group API requires an API key and accessToken , that you generate yourself on the developer portal, this API is not following the standard OAuth2 flow. Since the authorization and token handling then is so simple, I do not see the benefits of using the Oauth2 implementation in OH core. |
Previously i briefly looked at the flow from electrolux and it seemed like a custom flow that is not supported by the oauth2 implementaiton in oh core. I must admit that i dont't know all details about the oath2 implementation. I'll try to read it and might come back to this. For now let's continue this PR and i hope to come back to this before we can merge this PR. |
Signed-off-by: Leo Siepel <leosiepel@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.
Re-generated the i18n file as that was the only comment i had. Had a quick read about the oauth2 again and came to the same conclusion that this binding specific flow does not fit. Now we can merge it and get this into m3.
Next step would be to add the binding logo to the openHAB website. See https://www.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website
* First version Signed-off-by: Jan Gustafsson <jannegpriv@gmail.com>
Signed-off-by: Jan Gustafsson jannegpriv@gmail.com
Initial contribution of the Electroluxappliances openHAB binding.
The binding uses the Electrolux Group API and the binding implements support for Electrolux Air Purifiers and Washing Machines and solves #17325.
In the community thread you can find a link to a built binding jar-file.