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.

Sign up for GitHub

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

Jump to bottom

[renault] Add channel for pausing/resuming charging #14527

Merged
merged 7 commits into from
Mar 10, 2023
Merged

[renault] Add channel for pausing/resuming charging #14527

merged 7 commits into from
Mar 10, 2023

Conversation

fifipil909
Copy link
Contributor

Adding Pause / Resume API endpoint.

On some Vehicule type, changing the Charge mode to pause / Resume the charge is not working.
Another endpoint is required.

https://renault-api.readthedocs.io/en/latest/endpoints/vehicle_kcm_actions.charge-pause-resume.html

Tested on 3.4 and 4.0.0

Vincent.

@fifipil909 fifipil909 marked this pull request as ready for review March 3, 2023 15:06
@fifipil909 fifipil909 requested a review from dougculnane as a code owner March 3, 2023 15:06
@jlaur jlaur changed the title Adding Pause-Resume Endpoint [renault] Adding Pause-Resume Endpoint Mar 3, 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/betatest-renault-ze-services-binding/72226/238

jlaur
jlaur requested changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur 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 the PR. Please see my proposal about modeling this pause/resume mode into a channel.

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/i18n/renault_fr.properties Outdated Show resolved Hide resolved
@dougculnane dougculnane added the enhancement An enhancement or new feature for an existing add-on label Mar 5, 2023
@dougculnane
Copy link
Contributor

@fifipil909 you have to sign off your commits.

dougculnane
dougculnane requested changes Mar 5, 2023
View reviewed changes
Copy link
Contributor

@dougculnane dougculnane left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks Vincent this looks like it fits with the existing code well keeping the code consistent. So it will get approval from me but you have to sign off your commits and the feedback from @jlaur is very good and fits with feedback I got about the PRs I have submitted.

One thing I would like to see is your valuable comment about the chargingmode etc.. added to the README https://github.com/openhab/openhab-addons/tree/main/bundles/org.openhab.binding.renault#limitations My hope is that making it clear that not all things work on all cars will reduce the user frustration. I use the chargingmode for the example battery limit rule, so maybe a comment here is approximate too and will help some users.

@fifipil909
Copy link
Contributor Author

@fifipil909 you have to sign off your commits.

Done.
But I don't know why Something from Homekit was include in my commit.
I never touch this file !

@dougculnane
Copy link
Contributor

dougculnane commented Mar 5, 2023 •
edited
Loading

Hi Vincent, This can get very confusing but adding the git --signoff to your commits and rebasing can touch other commits. It looks like the homebase changes from somewhere got added in here. What we need is your --signoff commits in this branch with the changes suggested. If you need help with this I can try to help next week.

@fifipil909 fifipil909 force-pushed the Renault-Pause branch 2 times, most recently from ecf7d29 to 96f3e7b Compare March 5, 2023 14:59
@fifipil909
Copy link
Contributor Author

Sorry for the confusion. I'm not using rebase a lot.

I rebase correctly and remove the other commits.
No the PR do have only 1 commit and it's signed.

Hope is fine.

@dougculnane
Copy link
Contributor

@fifipil909 Looks good and the checks have passed. Can you go the changes requested and i think we are there. I will try to find time to test this to see if the pause charge feature works on the Zoe.

jlaur
jlaur requested changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

A few new comments after latest changes. Please have a look at the DCO also, it seems the new commits were not properly signed.

bundles/org.openhab.binding.renault/README.md 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
...nding.renault/src/main/java/org/openhab/binding/renault/internal/handler/RenaultHandler.java Outdated Show resolved Hide resolved
...nding.renault/src/main/java/org/openhab/binding/renault/internal/handler/RenaultHandler.java Show resolved Hide resolved
bundles/org.openhab.binding.renault/src/main/resources/OH-INF/thing/thing-types.xml 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
fifipil909 and others added 4 commits March 8, 2023 18:03
@fifipil909
Adding Pause-Resume Endpoint
aef9d6b 
Signed-off-by: fifipil909 <[email protected]>
Signed-off-by: fifipil909 <[email protected]>
@fifipil909
Adding Pause-Resume Endpoint
1843755 
Signed-off-by: fifipil909 <[email protected]>
@fifipil909
Update documentation
82be235 
Signed-off-by: fifipil909 <[email protected]>
@fifipil909
Pausemode to Pause, Pause as switch
bf125e2 
Signed-off-by: fifipil909 <[email protected]>
jlaur
jlaur requested changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Next round. I provided a few very specific suggestions to avoid misunderstandings. These are related to the comment about not using openHAB types in the Car API classes.

bundles/org.openhab.binding.renault/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.renault/README.md 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
.../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
...ing.renault/src/main/java/org/openhab/binding/renault/internal/api/MyRenaultHttpSession.java Outdated Show resolved Hide resolved
...ing.renault/src/main/java/org/openhab/binding/renault/internal/api/MyRenaultHttpSession.java Outdated Show resolved Hide resolved
...nding.renault/src/main/java/org/openhab/binding/renault/internal/handler/RenaultHandler.java Outdated Show resolved Hide resolved
...nding.renault/src/main/java/org/openhab/binding/renault/internal/handler/RenaultHandler.java Show resolved Hide resolved
...ding.renault/src/main/java/org/openhab/binding/renault/internal/RenaultBindingConstants.java Outdated Show resolved Hide resolved
dougculnane
dougculnane approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@dougculnane dougculnane left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Vincent.

jlaur
jlaur approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur changed the title [renault] Adding Pause-Resume Endpoint [renault] Add channel for pausing/resuming charging Mar 10, 2023
@jlaur jlaur merged commit 8e9bc31 into openhab:main Mar 10, 2023
@jlaur jlaur added this to the 4.0 milestone Mar 10, 2023
@lolodomo
Copy link
Contributor

Was the new thing upgrade mechanism implemented?

@jlaur jlaur mentioned this pull request Mar 10, 2023
Merged
@jlaur
Copy link
Contributor

jlaur commented Mar 10, 2023 •
edited
Loading

Was the new thing upgrade mechanism implemented?

No, I need to get used to this (and we need some documentation). Thanks for the reminder.

Created #14572.

@dougculnane dougculnane mentioned this pull request Mar 12, 2023
Closed
renescherer pushed a commit to renescherer/openhab-addons that referenced this pull request Mar 23, 2023
@fifipil909 @renescherer
[renault] Add channel for pausing/resuming charging (openhab#14527)
74df30e 
* Adding Pause-Resume Endpoint

Signed-off-by: fifipil909 <[email protected]>
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
@fifipil909 @FordPrfkt
[renault] Add channel for pausing/resuming charging (openhab#14527)
7ccc41a 
* Adding Pause-Resume Endpoint

Signed-off-by: fifipil909 <[email protected]>
@lolodomo lolodomo mentioned this pull request May 5, 2023
Closed
29 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur approved these changes

@dougculnane dougculnane dougculnane approved these changes

@andylintner andylintner Awaiting requested review from andylintner

@ccutrer ccutrer Awaiting requested review from ccutrer

@yfre yfre Awaiting requested review from yfre

Assignees
No one assigned
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@fifipil909 @openhab-bot @dougculnane @lolodomo @jlaur