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

Implement lock-status #506

Merged
merged 14 commits into from
Feb 2, 2022
Merged

Implement lock-status #506

merged 14 commits into from
Feb 2, 2022

Conversation

FRSTR
Copy link
Contributor

@FRSTR FRSTR commented Feb 2, 2022

Added Fixtures for lock-status command, added params to KamereonVehicleLockStatusData, added test and schema for lock-status

FRSTR added 4 commits February 2, 2022 21:08
Added Fixtures for lock-status command, added params to KamereonVehicleLockStatusData
Removed fixture for v2 of lock-status
Added test and schema for lock-status
fixed formating issues
@epenet
Copy link
Collaborator

epenet commented Feb 2, 2022

Could you check the CI results and adjust accordingly?

FRSTR added 2 commits February 2, 2022 22:44
Fix formating and test name
EOL fix
@epenet
Copy link
Collaborator

epenet commented Feb 2, 2022

It looks like the old lock file was referenced elsewhere.
You'll need to adjust the path there also, or revert the name change.

FRSTR and others added 3 commits February 2, 2022 23:07
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
@FRSTR FRSTR marked this pull request as ready for review February 2, 2022 19:34
@epenet
Copy link
Collaborator

epenet commented Feb 2, 2022

CI looks good - but I've had a think about it and I wonder if you could post two fixtures instead of one.
The current fixture is locked with all doors closed. It would be good to have a second fixture with a door open (and therefore I assumed with vehicle unlocked) so that we have both states as samples.

@epenet epenet added the fixtures Provide fixtures label Feb 2, 2022
@epenet epenet changed the title Fixtures and params in KamereonVehicleLockStatusData for lock-status Implement lock-status Feb 2, 2022
@FRSTR
Copy link
Contributor Author

FRSTR commented Feb 2, 2022

I have another fixture with the car unlocked but, with the doors closed. I guess I can add it too, and some time later see how open doors look like too. What should be the name of a second fixture in that case? some prefix/suffix?

@epenet
Copy link
Collaborator

epenet commented Feb 2, 2022

I think vehicle_data/lock-status.1.json and vehicle_data/lock-status.2.json

On this line you update it to lock-status1.json:

"vehicle_data/lock-status.json",

And also you could update the documentation with the samples:
https://github.com/hacf-fr/renault-api/blob/main/docs/endpoints/vehicle_data.lock-status.rst
similar to what is already available in https://github.com/hacf-fr/renault-api/blob/main/docs/endpoints/vehicle_data.location.rst

This is what the documentation looks like: https://renault-api.readthedocs.io/en/latest/endpoints.html#location

FRSTR added 2 commits February 3, 2022 00:05
Added fixture and test for lock-status with unlocked state
@FRSTR
Copy link
Contributor Author

FRSTR commented Feb 2, 2022

Added 2nd fixture and samples to the docs.

@epenet
Copy link
Collaborator

epenet commented Feb 2, 2022

Looks great! Sadly my Zoe40 doesn't support the lock status so I cannot test it.
I think we do need an open door to confirm exactly what is displayed - especially for Home Assistant.

When would you be able to update the 2nd fixture?

@FRSTR
Copy link
Contributor Author

FRSTR commented Feb 2, 2022

Oh my.. Ok, I'm going out to this dark night right now to see the status with an open door. But promise me, we complete this PR today :)

@FRSTR
Copy link
Contributor Author

FRSTR commented Feb 2, 2022

Well, it works weird: when I unlock the car, I get a new status with the new timestamp within a minute or so.
But after that I tried to open and hold open different doors, also tried to lock the car with open doors - but no updates to the status and timestamp, it still shows unlocked but with all doors "closed". My car's dashboard showed particular open doors, but not API.

Once I locked the car with all doors physically closed, I got the locked status in the API but again with all doors closed (which was true at that moment). Maybe they just did not implement it in the API or in my car in particular.

@epenet
Copy link
Collaborator

epenet commented Feb 2, 2022

Ah the joys of the renault API!!!
I'll merge this as it is and hopefully we can get an updated fixture in the future.

@epenet epenet added the documentation Improvements or additions to documentation label Feb 2, 2022
@epenet epenet merged commit edc34b9 into hacf-fr:main Feb 2, 2022
@epenet
Copy link
Collaborator

epenet commented Feb 2, 2022

Thanks @FRSTR !

@FRSTR FRSTR deleted the lock-status_support branch February 2, 2022 21:12
@FRSTR
Copy link
Contributor Author

FRSTR commented Feb 2, 2022

👍

@epenet
Copy link
Collaborator

epenet commented Feb 2, 2022

I guess next step is to add the lock-status to the CLI now that it's available on the base class...
😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request fixtures Provide fixtures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants