SAIC-iSmart-API / saic-python-mqtt-gateway

A service that queries the data from an MG iSMART account and publishes the data over MQTT and to other sources
MIT License
71 stars 23 forks source link

Discussion: should HA show "Unavailable" for entities whenever we fail to poll the SAIC API? #186

Closed nanomad closed 7 months ago

nanomad commented 8 months ago

As title says, I'd like to gather some feedback about the current behaviour and a possibile change:

But sometimes we fail to talk with the SAIC API (either for a specific car or completely), giving HA false information about the data (i.e. if you use an utility meter integration to compute the kWh charged)

The proposal (testable with this docker image: ghcr.io/nanomad/saic-python-mqtt-gateway/saic-mqtt-gateway:0.6.0-rc8) is to make:

What do you think?

nanomad commented 8 months ago

Tagging @tisoft @magior @Tonno87 @tosate for feedback :)

cybersmart-eu commented 8 months ago

I like this idea. In my case with two cars I quite often make the experience that one car provides data and at the same time the other car shows an error and no data can be requested / fetched anymore (sometimes for days). So while MQTT GW and also SAIC API login are successfull, data from one car is not available/reliable anymore.

However as I am not using HA the same information should be provided via MQTT in a dedicated topic for other SmartHome systems to fetch the status per car.

nanomad commented 8 months ago

However as I am not using HA the sams information should be provided via MQTT in a dedicated topic for other SmartHome systems to fetch the status per car.

It will be :) I've added a new topic under the car VIN with the usual 'online' and 'offline' payloads

Tonno87 commented 8 months ago

As a technician i would say: Better no values than wrong values! From this point of view it's a good idea. Additionally in the case of problems we can better specify if it's a gateway or api problem.

On the other hand this behaviour will provoke more issues and discussions.

nanomad commented 8 months ago

On the other hand this behaviour will provoke more issues and discussions.

To be fair, I've already had my fair share of discussion about "commands not working"

Keep in mind the proposal only applies to "polling" errors so essentially mostly during driving and as a response to a successful command execution.

The question now is...how can we make users aware of errors during commands execution? I bet that is a more common scenario, and I don't think putting the whole car into an "unavailable" state is the best choice in terms of user experience

Tonno87 commented 8 months ago

To understand the changes in RC8 without testing your image yet:

If the gateway polls any values without success all car entity's became unavailable like this:

Screenshot_20240312-011645.png

What do you think about sending a notification to home assistant if a command doesn't work?

Screenshot_20240312-014542.png

nanomad commented 8 months ago

That would be nice, I don't see a way of sending those via MQTT though...Should we think about a direct API integration?

Tonno87 commented 8 months ago

Is it possible to create an automation with autodiscovery? If so we can trigger the notification service by sending an mqtt message to home assistant.

A native Integration is a cool idea, but if we change something we need to wait for home assistant updates... Or we need to release it at the Hacs store.

nanomad commented 8 months ago

Is it possible to create an automation with autodiscovery? If so we can trigger the notification service by sending an mqtt message to home assistant.

Unfortunately not :( We can use HA API keys and API call to http://<HA_URI>:8123/api/services/notify though

tosate commented 8 months ago

I also like the idea of setting entities to unavailable if we did not manage to get proper values. This should improve the statistics for certain sensors. I would use notifications in very specific scenarios only. From my point of view it makes sense when a user invokes a command so that he receives a feedback in case of error and does not need to look into the log. For polling errors I would avoid the notifications because they can quickly pile up and annoy the user.

Tonno87 commented 8 months ago

Installed the new pre-release at 19:27:49, everything seems to work. but at 20:57:41 the car became unavailable. Seems that the gateway failed with "code 4: the remote control instruction failed, please try again later. " image Now i need to refresh the gateway to show any data at home assistant...

image

Tonno87 commented 8 months ago

Seems that the car won't go to a sleep state:

image

nanomad commented 8 months ago

Seems that the car won't go to a sleep state:

Interesting, I don't think I changed the refresh logic there. Let me check.

@Tonno87 do you mind uploading logs here or sending those to me via telegram (@nanomad)?

nanomad commented 8 months ago

Ah you're right... I think it's just an edge case that has always been there but we failed to notice in the past.... It may happen that we fetch the car state but not the charge state. If so, the gateway gets stuck in a loop never marking the refresh as successful

Tonno87 commented 8 months ago

Seems that the charging state doesn't work at the moment, but the car state does:

Screenshot_20240313-220449 Screenshot_20240313-220539

Logfiles are not so easy in home assistant... no idea where to find them outside the addon console

nanomad commented 8 months ago

Seems that the charging state doesn't work at the moment, but the car state does:

Logfiles are not so easy in home assistant... no idea where to find them outside the addon console

No worries, I'll have an hotfix shortly. For now it will mark the refresh as successfull, I'll work on having different availabilities depending on the provenance of the data

nanomad commented 8 months ago

@Tonno87 please try RC10. If it works I kindly ask you to push it as an add-on update

Tonno87 commented 8 months ago

image

Seems to work. RC10 is already pushed to the pre addon.