Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Sep 6, 2024

Conversation

Poshy163
Copy link
Contributor

@Poshy163 Poshy163 commented Sep 2, 2024

  • Fix for Sunshine at night #127 - By setting the default value to None, and then checking the following calculations to None (if one of the two is None, that means the API has not returned the data, or the datatype is not supported (like in the case of Storion-S5 inverters). then introduce a check within the native_value() to see if the state is None, if not, return the value. this in theory will just not update the state of the sensor for any None values

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

  • Moved increment_inverter_count, add_inverter_to_list to pre-startup checks, this gets the list of inverters before the coordinator is called, meaning the amount of inverters an account has is done before the first .getdata is called (remove the chance of error logging in the first API call)

  • Add system serial number to device page
    image

  • Updated the icons for the self-percentage based sensors and EMS status
    image
    image

Closes #131
Closes #132
Closes #127
Closes #129

@Poshy163 Poshy163 changed the title Update default value to None Update default value to None and improve multi-inverter experience Sep 2, 2024
@CharlesGillanders
Copy link
Owner

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
Copy link
Contributor Author

Poshy163 commented Sep 2, 2024

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
Copy link
Owner

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
Copy link

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
Copy link
Contributor Author

Poshy163 commented Sep 3, 2024

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 Poshy163 mentioned this pull request Sep 3, 2024
@Poshy163
Copy link
Contributor Author

Poshy163 commented Sep 4, 2024

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 Poshy163 marked this pull request as ready for review September 4, 2024 07:12
@Poshy163
Copy link
Contributor Author

Poshy163 commented Sep 5, 2024

Coolio, @CharlesGillanders this pull should be ready to go

@CharlesGillanders CharlesGillanders merged commit 38bcbe0 into CharlesGillanders:main Sep 6, 2024
5 checks passed
CharlesGillanders added a commit that referenced this pull request Sep 6, 2024
Bump version to 0.5.5 after #130
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants