mitch-dc / volkswagen_we_connect_id

Apache License 2.0
202 stars 53 forks source link

v0.1.0.1 wrongly reporting some binary_sensors as active #96

Closed pplucky closed 1 year ago

pplucky commented 2 years ago

Version of the custom_component

v0.1.0.1

Installation method (hacs / manual)

hacs

Installation method of hass (venv, docker, hassio,...)

docker

Configuration

NA

Describe the bug

Reported before in #82.

Some binary_sensors which are not reported by the VW API are shown in HA as active.

image

Looking at the binary_sensors added by the integration and the related extracted information with weconnect-cli:

[climatisation] Elements: 3 items
        [climatisationStatus] (last captured 2022-10-26T13:46:55+00:00)
                State: off
                Remaining Climatization Time: 0 min
        [climatisationSettings] (last captured 2022-10-26T13:46:55+00:00)
                Target Temperature in °C: 22.0 °C
                Target Temperature in °F: 72.0 °F
                Temperature unit in car: celsius
        [windowHeatingStatus] (last captured 2022-10-26T13:46:55+00:00)
                Windows: 2 items
                        front: off
                        rear: off
[charging] Elements: 5 items
        [batteryStatus] (last captured 2022-10-26T13:46:57+00:00)
                Current SoC: 65%
                Range: 267km
        [chargingStatus] (last captured 2022-10-26T13:46:57+00:00)
                State: notReadyForCharging
                Mode: manual
                Charge Power: 0.0 kW
                Charge Rate: 0.0 km/h
                Charge Type: invalid
                Charging Settings: default
        [chargingSettings] (last captured 2022-10-26T13:46:56+00:00)
                Maximum Charge Current AC: maximum
                Auto Unlock When Charged: off
                Auto Unlock When Charged AC: off
                Target SoC: 90 %
        [chargeMode]
                Preferred charge mode: manual
                Available charge modes: [manual]
        [plugStatus] (last captured 2022-10-26T14:05:23+00:00)
                Plug: disconnected, unlocked, External Power: unavailable, Led color: none
[readiness] Elements: 1 items
        [readinessStatus]
                Connection State:
                        Is online: True
                        Battery power level: comfort
                        Daily Power Budget Available: True
                Connection Warning:
                        Insufficient Battery Level Warning: False
                        Daily Power Budget Warning: False

it seems clear that not all the information reported by HA via these binary sensors is correct according to what is reported by the VW api.

Here are also some app screenshots showing that some of these binary sensors should show as inactive: msg-576532132-43988

msg-576532132-43989

Can this please be fixed?

Debug log

ColinRobbins commented 1 year ago

I believe this issue is cause by the following by https://github.com/mitch-dc/volkswagen_we_connect_id/blob/6e3e004720bc28c459cad0731bd0f9845ea46fc7/custom_components/volkswagen_we_connect_id/binary_sensor.py#L168 If you change this to

      return None

the issue goes away,and the erroneus sensors show as unknown.

My reading of the code is, it tries to get the sensor value from the API. If this is not available, it uses a previous (or default) value stored in home assistant.

pplucky commented 1 year ago

My reading of the code is, it tries to get the sensor value from the API. If this is not available, it uses a previous (or default) value stored in home assistant.

The thing is that some of these sensors seem to only be reported by API when 'on', like the 'Car is Active', otherwise it should be 'off' (even if the API does not report it).

ColinRobbins commented 1 year ago

Ah, OK. Then I suggest return False rather than None. Works on my test system. Will migrate to live...

ColinRobbins commented 1 year ago

Sadly, this still does not work long term. It works on a reboot of HA, but once the sensor is turned to On, nothing is ever called to turn it off. I think the issue is, as the sensor is missing for the API data, the call to update the HA value (my ‘false’ fix) does not get called (I’ve closed the PR as a consequence). Somehow we need to trigger an update of all sensors, so they get reset if missing.

pplucky commented 1 year ago

Yes, sadly I had also noticed that today.

Not sure if this logic should be implemented here or in the upstream library.

ColinRobbins commented 1 year ago

Having looked into this more, I think upstream maybe the place to fix it. If we start HA when the EV is inactive, the WeConnect API does not have an data for is, the the WeConnect library does not return data. The code here creates a default sensor value (on or off, depending if you use my 'fix' above or the original code) The EV now becomes active. The upstream library adds isAcitve to its Dict storage entity, and sets isActive to True. The code here sets the sensor to on. The EV now becomes inactive. The WeConnect API does not return any data (indicating inactive). The WeConnect Python library no not uddate the Dict entity - so the dict has an incorrect value. This is reported to HA, so the sensor is now incorrect.

As you say, not sure how we can fix here, so will report upstream, and see where we get to.

ColinRobbins commented 1 year ago

Upstream report: https://github.com/tillsteinbach/WeConnect-python/issues/85

tillsteinbach commented 1 year ago

I would recommend not to rely any logic on the isActive flag. It is not reliably coming from the server.

pplucky commented 1 year ago

I would recommend not to rely any logic on the isActive flag. It is not reliably coming from the server.

@tillsteinbach That’s a bit odd, because running v0.0.13 of this custom integration which uses v0.45.1 of your upstream library seems quite accurate (not sure about reliability).

Change to v0.48.1 (v0.0.14 of this custom component) seems to have brought this wrong behavior to above mentioned binary sensors.

tillsteinbach commented 1 year ago

That is a good hint. Can we find out since when it became unreliable?

pplucky commented 1 year ago

That is a good hint. Can we find out since when it became unreliable?

Somewhere between v0.45.1 and v0.48.1 of your library, I have no smaller interval

ColinRobbins commented 1 year ago

@pplucky having checked upstream (https://github.com/tillsteinbach/WeConnect-python/issues/85#issuecomment-1328238676) it appears a check needs to be made in the code here. I've attempted a fix in PR https://github.com/mitch-dc/volkswagen_we_connect_id/pull/107. Works in my test system. Migrating to live...

ColinRobbins commented 1 year ago

Note, while the PR “improves” the situation, I believe there is a secondary logic error in the upstream library, which means the sensor will only come on once, before remaining permanently off until a HA restart. I’ve re-opened the upstream issue.

pplucky commented 1 year ago

Note, while the PR “improves” the situation, I believe there is a secondary logic error in the upstream library, which means the sensor will only come on once, before remaining permanently off until a HA restart. I’ve re-opened the upstream issue.

I second this behaviour, which I also observed in my test system, where your latest PRs were installed.

ColinRobbins commented 1 year ago

I'm testing 0.50.1 which should fix it.

mitch-dc commented 1 year ago

Please try the new beta, i've bumped weconnect and moved some binary sensors to normal sensors https://github.com/mitch-dc/volkswagen_we_connect_id/releases/tag/v0.1.0.3

ColinRobbins commented 1 year ago

Running 0.1.0.3 now. Looking good so far. Need two car journeys to be sure the binary sensors are now OK…

pplucky commented 1 year ago

Running 0.1.0.3 now. Looking good so far. Need two car journeys to be sure the binary sensors are now OK…

Same here, seems globally consistent, although I haven't driven the car too much lately.