-
-
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
[renault] Initial Contribution #11467
Conversation
… <[email protected]> Signed-off-by: Doug Culnane <[email protected]>
2218aab
to
bc4d516
Compare
… as a result of user testing openhab#11467 Signed-off-by: Doug Culnane <[email protected]>
I am going to review your binding. |
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.
Review part 1 of 2
bundles/org.openhab.binding.renault/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.renault/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.renault/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.renault/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.renault/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Doug Culnane <[email protected]>
Thanks @lolodomo for some great feedback. I think I have got all your points and I will build and test the changes locally this weekend. I hope to push a tested version ASAP. Thanks again. |
I will review the last part (comm part) probably tomorrow. |
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.
Review part 2 of 2
...nhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/renault/api/Car.java
Outdated
Show resolved
Hide resolved
...nhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/renault/api/Car.java
Outdated
Show resolved
Hide resolved
...nhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/renault/api/Car.java
Outdated
Show resolved
Hide resolved
...nhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/renault/api/Car.java
Outdated
Show resolved
Hide resolved
...inding.renault/src/main/java/org/openhab/binding/renault/internal/renault/api/Constants.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.renault/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
Thanks @lolodomo I have a couple of questions in the comments and some work to do. I will do and test these changes and get back with an updated version. Thanks again. |
Signed-off-by: Doug Culnane <[email protected]>
Signed-off-by: Doug Culnane <[email protected]>
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.
New comments after your last set of changes.
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...inding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandlerFactory.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
...nhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/renault/api/Car.java
Outdated
Show resolved
Hide resolved
...nhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/renault/api/Car.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
.../main/java/org/openhab/binding/renault/internal/renault/api/exceptions/RenaultException.java
Outdated
Show resolved
Hide resolved
@dougculnane : thank you for all your changes. I marked as "resolved" most of my old comments. Please take a look to the ones which are not yet marked as resolved and please consider the new ones. |
Signed-off-by: Doug Culnane <[email protected]>
Signed-off-by: Doug Culnane <[email protected]>
@lolodomo Thanks for some good tips and feedback. I have got all currently open issues resolved or attempted to resolve them now. Thanks again. |
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.
Here are my very final comments leading a clean compilation of the binding.
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...nhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/renault/api/Car.java
Outdated
Show resolved
Hide resolved
...enhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultHandler.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
...ult/src/main/java/org/openhab/binding/renault/internal/renault/api/MyRenaultHttpSession.java
Outdated
Show resolved
Hide resolved
…w Forbbidden Error case when car not paired.. Signed-off-by: Culnane Douglas <[email protected]>
Signed-off-by: Culnane Douglas <[email protected]>
@lolodomo I hope this is OK now? Thanks again for your time. |
Honestly, this looks a little weird to me. Displaying a communication error but keeping the thing ONLINE. |
@lolodomo I know it is not great. The problem I am trying to solve is that if the car gets up-paired or some channels have errors there is no way of telling unless you look in the log files. The the binding need to ignore errors and keep polling so it should stay ONLINE (as far as I understand or it will stop polling). Is there a better way of reporting errors or should I create a new channel for a status message or something? Thanks again. |
@lolodomo Is the status message a good idea or is there some other error viability idea? Or are you getting board of this? Thanks again again. |
For requests that are failing systematically (comm error) because not supported by a car, my opinion is that you should just log something and do nothing regarding the thing status. Avoiding next similar requests is of course a good idea. You could have "disableXXX" booleans in your car structure that you will set to true in case a first request is failing. For requests that are failing only in not normal situations but you know that the request is applicable for the car, your current solution could be acceptable. Keep status ONLINE if you are really sure that there is not general comm problem, else set it to OFFLINE. We have to conclude quickly now, even if not perfect, to have your binding part of the final 3.2 distribution. Let me know if you want to add a new final optimization (avoid recalling for ever requests that you know they will fail). |
@lolodomo Thanks for the advice. I will try to push an update this evening. |
Signed-off-by: Culnane Douglas <[email protected]>
@lolodomo I added a not not Implemented error and disableXXX flags for the services. I keep the errors to the log files. I recreate the car Object on init so that you can reset it... Thanks again. |
Signed-off-by: Culnane Douglas <[email protected]>
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.
Very final comment
.../org.openhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/api/Car.java
Outdated
Show resolved
Hide resolved
.../org.openhab.binding.renault/src/main/java/org/openhab/binding/renault/internal/api/Car.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Culnane Douglas <[email protected]>
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
@lolodomo Getters and setter are done. I have tested the binding on my openHAB installation. Thanks again. |
@dougculnane : thank you for your new binding contribution. |
@lolodomo Merci beaucoup for your help and time getting this ready for release. |
* openhab#11465 Initial renault-api binding Signed-off-by: Doug Culnane <[email protected]> Signed-off-by: Nick Waterton <[email protected]>
@dougculnane : if not yet done, 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 |
* openhab#11465 Initial renault-api binding Signed-off-by: Doug Culnane <[email protected]> Signed-off-by: Michael Schmidt <[email protected]>
@lolodomo I have created a PR for the logo here: openhab/openhab-docs#1730 All the best, Doug |
* openhab#11465 Initial renault-api binding Signed-off-by: Doug Culnane <[email protected]>
* openhab#11465 Initial renault-api binding Signed-off-by: Doug Culnane <[email protected]>
* openhab#11465 Initial renault-api binding Signed-off-by: Doug Culnane <[email protected]>
* openhab#11465 Initial renault-api binding Signed-off-by: Doug Culnane <[email protected]> Signed-off-by: Andras Uhrin <[email protected]>
Initial renualt-api binding Issue: #11465
Electric car charging rules can be improved if the battery state of the car is known. There was a Renault ZE service API but this is no longer available and a new renault-api has a complex authentication flow. There is python implemented code that mimics the mobile app and allows a command line interface to pull data from the Renault API. (https://github.com/hacf-fr/renault-api)
It would be nice to have this available as an easy to use binding so that openHAB users can get car battery (and other) status information in openHAB.
This is a long discussion documenting the issue:
https://community.openhab.org/t/betatest-renault-ze-services-binding/72226
I have implemented a basic binding that translates the python code to a Java. It works on at least 3 cars according the the forum. https://community.openhab.org/t/betatest-renault-ze-services-binding/72226/79?u=doug_culnane