search

tillsteinbach / WeConnect-python

Python API for the Volkswagen WeConnect Services
MIT License
109 stars 29 forks source link

"isActive" returing an incorrect value. #85

Closed ColinRobbins closed 1 year ago

ColinRobbins commented 1 year ago

Describe the bug The value of the Readiness parameter isActive is sometimes incorrect. The WeConnect API returns the isActive paramter in its data set when the vehicle is active, but does not return anything when the vehicle becomes inactive. Thus, the update function, which updates the local data dictionary does not update the isActive parameter, as the parameter is absent. Thus the data dictionary incorrectly reports the vehicle as active, when it is not.

The issue probably affects outher parameters as well - isActive is my test case!

To Reproduce Run the API, with the EV off. Run the update funtion Turn EV on. Run the update function Turn EV off Run the update function - the API will show the EV incorrectly as being active.

Expected behavior When the EV is switched off, the update function should recognise the absence of the parameter indicate the EV is off, and update the data dictionary accordingly

Screenshots N/A

Logs N/A

Additional context Issue reported in downstream Home Asssiant integration here: https://github.com/mitch-dc/volkswagen_we_connect_id/issues/96

tillsteinbach commented 1 year ago

Hmm, when the parameter is absent its enabled attribute should be false. Is that not the case for you?

ColinRobbins commented 1 year ago

No, that does not appear to be the case.

pplucky commented 1 year ago

Maybe this info can help

ColinRobbins commented 1 year ago

I started to try and understand the code here. It seems about the time @pplucky is reporting the error being introduced the .enabled parameter on an attribute was modified.

I can see that when the vehicle is updated, enabled should be set to false if the attribute is not present in the data returned from the EV.

I don’t fully understand the code yet, but I cannot see where the enabled parameter is checked when accessing the dict value. So I think the old value from the dict is being accessed.

Should library users be checking the enabled status, before accessing the dict value?

tillsteinbach commented 1 year ago

Yes, it is necessary to check the enabled value. As we are speaking I think I might change in the library that an exception is thrown when accessing a disabled value.

ColinRobbins commented 1 year ago

Thanks. Fixed downstream

ColinRobbins commented 1 year ago

I have reopened this, as I think there is a logic error. The behaviour I am seeing is

Not sure of the best way to fix it. Should the value be set to ‘off’ when disabled? Should the valueChanged test be removed?

tillsteinbach commented 1 year ago

Oh wow, I think you are right. The check is wrong. I will doublecheck if that breaks something and otherwise push a fix tomorrow! Great catch!

tillsteinbach commented 1 year ago

@ColinRobbins can you check 0.50.1?

ColinRobbins commented 1 year ago

Thanks - trying it now. Need two car journeys to see if it has worked!

On Wed, Nov 30, 2022 at 6:52 AM Till Steinbach @.***> wrote:

@ColinRobbins https://github.com/ColinRobbins can you check 0.50.1?

— Reply to this email directly, view it on GitHub https://github.com/tillsteinbach/WeConnect-python/issues/85#issuecomment-1331714477, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADGXZ6LESOF6JETMKOCKLB3WK32RHANCNFSM6AAAAAASMCIYGU . You are receiving this because you were mentioned.Message ID: @.***>

pplucky commented 1 year ago

@ColinRobbins can you check 0.50.1?

Looking good, for the last couple of weeks.

ColinRobbins commented 1 year ago

This seems fixed to me, closing