CharlesGillanders / homeassistant-alphaESS

Monitor your energy generation, storage, and usage data using the official API from Alpha ESS.
MIT License
101 stars 22 forks source link

Update default value to None and improve multi-inverter experience #130

Closed Poshy163 closed 2 months ago

Poshy163 commented 2 months ago

This will be shown in the logbook by the EMS staus going from Normal to unavailable image

Closes https://github.com/CharlesGillanders/homeassistant-alphaESS/issues/131 Closes https://github.com/CharlesGillanders/homeassistant-alphaESS/issues/132 Closes https://github.com/CharlesGillanders/homeassistant-alphaESS/issues/127 Closes https://github.com/CharlesGillanders/homeassistant-alphaESS/issues/129

CharlesGillanders commented 2 months ago

How can we confirm that returning none will allow Home Assistant to use the previous value? If so that looks like an excellent solution to this problem of unreliable data returned from Alpha.

Poshy163 commented 2 months ago

Hmm, just tested this, appears it will set the sensor to be unavailable, rather then the old value, ill introduce a check within the native_value to see if the state is None, if not, return the value. this then will just not update the state of the sensor

CharlesGillanders commented 2 months ago

Hmm, just tested this, appears it will set the sensor to be unavailable, rather then the old value, ill introduce a check within the native_value to see if the state is None, if not, return the value. this then will just not update the state of the sensor

Yeh I’m wondering if state unavailable is arguably the right behaviour - from the documentation it looks like that’s the “correct” state to report when a sensor cannot get correct information from a polled integration. I’m happy with either approach but I can certainly see the value in seeing in the UI when a sensor is not able to get reliable data.

DavidqStokes commented 2 months ago

Hmm, just tested this, appears it will set the sensor to be unavailable, rather then the old value, ill introduce a check within the native_value to see if the state is None, if not, return the value. this then will just not update the state of the sensor

Yeh I’m wondering if state unavailable is arguably the right behaviour - from the documentation it looks like that’s the “correct” state to report when a sensor cannot get correct information from a polled integration. I’m happy with either approach but I can certainly see the value in seeing in the UI when a sensor is not able to get reliable data.

I would much rather know that data is unavailable when it is not available, rather than be 'fooled' by old data. If others would rather revert to old (last good) data I think that should be done outside the integration.

Poshy163 commented 2 months ago

Yeh I’m wondering if state unavailable is arguably the right behavior - from the documentation it looks like that’s the “correct” state to report when a sensor cannot get correct information from a polled integration.

Yeah, im inclined to agree with that, if this were to happen with any of the calls, all the associated values and calculations would be protected and handled correctly from safe_calculate, safe_get and process_value and displayed to the user. we wouldn't get any of these crazy energy spikes (going from 0 to the original value)

Poshy163 commented 2 months ago

Alright, seems to be working fine, no more random spikes, rather obvious disconnects, and it doesn't break the energy dashboard at all

image

image

Poshy163 commented 2 months ago

Coolio, @CharlesGillanders this pull should be ready to go