audiconnect / audi_connect_ha

Adds an audi connect integration to home assistant
MIT License
227 stars 98 forks source link

fix: Oil Level #383

Closed coreywillwhat closed 5 months ago

coreywillwhat commented 5 months ago

This pull request introduces a new binary sensor for the oil_level_binary and updates the logic for the oil level percentage display. The changes ensure that the percentage is only displayed if the retrieved value is a float, and the binary sensor is only displayed if the value is a bool.

The binary sensor addition aims to provide a clearer indication of the oil level status ("OK" or "Problem").

Note: I'm unable to test these changes as my vehicle does not support oil level reporting. The functionality should be validated against a vehicle that provides this data.

Kolbi commented 5 months ago

Seems to be a bug inside the code.

I still have sensor.audi_oil_level with value 1% and binary_sensor with status OK.

2024-04-16 06:25:24.383 DEBUG (MainThread) [custom_components.audiconnect.audi_models] Entering _tryAppendFieldWithTs with textId: OIL_LEVEL_DIPSTICKS_PERCENTAGE, loc: ['oilLevel', 'oilLevelStatus', 'value', 'value'] 2024-04-16 06:25:24.383 DEBUG (MainThread) [custom_components.audiconnect.audi_models] Initial value retrieved for 'OIL_LEVEL_DIPSTICKS_PERCENTAGE': True 2024-04-16 06:25:24.384 DEBUG (MainThread) [custom_components.audiconnect.audi_models] Updated loc for timestamp retrieval: ['oilLevel', 'oilLevelStatus', 'value', 'carCapturedTimestamp'] 2024-04-16 06:25:24.384 DEBUG (MainThread) [custom_components.audiconnect.audi_models] Timestamp retrieved for 'OIL_LEVEL_DIPSTICKS_PERCENTAGE': 2024-04-16 04:11:32+00:00 2024-04-16 06:25:24.384 DEBUG (MainThread) [custom_components.audiconnect.audi_models] Found and appended field with timestamp: textId=OIL_LEVEL_DIPSTICKS_PERCENTAGE, loc=['oilLevel', 'oilLevelStatus', 'value', 'carCapturedTimestamp'], val=True, ts=2024-04-16 04:11:32+00:00 2

coreywillwhat commented 5 months ago

Thanks for testing @Kolbi . I've updated the oil_level_supported logic slightly if you could give it a shot. As with the other, marking this as draft. If the changes work, please feel free to mark ready for review and merge.

neil-bh commented 5 months ago

Thanks for the updates made to cater for the issues experienced with the oil level. I've updated AudiConnect and can see that the sensor.audi_oil_level entity is no longer available. Although, I do not see any new oil sensors. The conversation in this merge mentions a binary sensor that will will report "OK" or "Problem", but I cannot see such a binary sensor in the list of entities available.

Here is my latest debug since I updated:
home-assistant_audiconnect_2024-04-17T10-23-15.140Z.log

Am I misunderstanding something? Or is it simply that reporting on the oil level is no longer feasible no matter if its a integer/percentage or binary?

Kolbi commented 4 months ago

Thanks for the updates made to cater for the issues experienced with the oil level. I've updated AudiConnect and can see that the sensor.audi_oil_level entity is no longer available. Although, I do not see any new oil sensors. The conversation in this merge mentions a binary sensor that will will report "OK" or "Problem", but I cannot see such a binary sensor in the list of entities available.

Here is my latest debug since I updated: home-assistant_audiconnect_2024-04-17T10-23-15.140Z.log

Am I misunderstanding something? Or is it simply that reporting on the oil level is no longer feasible no matter if its a integer/percentage or binary?

From your log:

2024-04-17 11:23:02.621 DEBUG (MainThread) [custom_components.audiconnect.audi_models] TRY APPEND FIELD: Searching for 'OIL_LEVEL_DIPSTICKS_PERCENTAGE' at location=['oilLevel', 'oilLevelStatus', 'value', 'value'] 2024-04-17 11:23:02.621 DEBUG (MainThread) [custom_components.audiconnect.audi_models] TRY APPEND FIELD: Value for 'OIL_LEVEL_DIPSTICKS_PERCENTAGE' is missing; not appending field.

And

"oilLevel":{ "oilLevelStatus":{ "value":{ "carCapturedTimestamp":datetime.datetime(2024, 4, 17, 7, 10, 29, "tzinfo=datetime.timezone.utc)", "value":false

@coreywillwhat shouldn't line: self._vehicle.fields.get("OIL_LEVEL_DIPSTICKS_PERCENTAGE"), bool interprete false as bool?

coreywillwhat commented 4 months ago

technically false is not the same as False. Its interesting your log is reporting True, and not true, if false is the value for another user.

We can account for this by creating a parse_bool() in the .util and calling it to parse the boolean values. I'll put a PR together.

Edit: To clarify this after looking at the logs, the value returned is indeed False.

coreywillwhat commented 4 months ago

looking closer at the logs, i dont think the value is the issue. Seems like the issue might be with the tryAppend

neil-bh commented 4 months ago

technically false is not the same as False. Its interesting your log is reporting True, and not true, if false is the value for another user.

We can account for this by creating a parse_bool() in the .util and calling it to parse the boolean values. I'll put a PR together.

That's interesting. In python booleans must be capitalized, however if the Audi API/JSON is returning a non standard or non structured value, programs that treat bool values in a strict manor will trip up on these gotchas. I guess in this circumstance, the best practice throughout this integration is to use a parse_bool().

neil-bh commented 4 months ago

looking closer at the logs, i dont think the value is the issue. Seems like the issue might be with the tryAppend

I'm on hand if you want to run some tests.

coreywillwhat commented 4 months ago

yeah here is the issue... its in the _tryAppendFieldWithTs function.

        if val and ts:
            self.data_fields.append(
                Field(
                    {
                        "textId": textId,
                        "value": val,
                        "tsCarCaptured": ts,
                    }
                )
            )

The value is a boolean, so this if statement is False and the value is not catching it. We need to change this to if val is not None and ts. I'll submit a PR

wigster commented 4 months ago

I'll just comment that in my model (Q5 hybrid), the sensor got converted to a binary_sensor with the 1.6.1 update (maybe that's ok -- it was always 100% anyway) and then the binary_sensor became unavailable (no longer supplied by the integration) when I upgrade to 1.6.2.

Not sure this is expected?

coreywillwhat commented 4 months ago

@wigster can you supply a log. Rather get this fixed in one go if we can

coreywillwhat commented 4 months ago

I also have a Q5 Hybrid (2021), and these values are unfortunately unavailable for me, so I would not expect either a percentage sensor, or a binary sensor.

coreywillwhat commented 4 months ago

398 PR submitted

wigster commented 4 months ago

@wigster can you supply a log. Rather get this fixed in one go if we can

Try this. Let me know if that's not the right way to capture (eneable debug logging in integrations and then reboot)

https://pastebin.com/gx2RmTtk

coreywillwhat commented 4 months ago

yeah that works. and yeah you're reporting False also, just like @neil-bh . If either of you can test #398 and let us know the result in the comments there, that would be ideal.