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

raise validation error for all zero response data #148

Conversation

Darsstar
Copy link
Contributor

@Darsstar Darsstar commented Apr 2, 2024

Succeeds #142, thanks @VadimKraus.

@VadimKraus
Copy link
Contributor

how about we just reopen #142 :D

@Darsstar
Copy link
Contributor Author

Darsstar commented Apr 2, 2024

how about we just reopen #142 :D

I can't. Feel free to rebase your branch on master yourself.

@VadimKraus
Copy link
Contributor

Ah sorry, there were conflicts again? nvm then :P

@Darsstar Darsstar force-pushed the raise-error-all-zero-data-credits-to-VadimKraus branch from 708efaf to d0e57d3 Compare May 4, 2024 21:53
@joseal
Copy link

joseal commented May 16, 2024

@Darsstar, could you please confirm if this change was already merged/deployed? sorry, because I'm already a bit confused...

@Darsstar
Copy link
Contributor Author

For all intents and purposes: no, it has not.

@squishykid squishykid merged commit fedda9d into squishykid:master Jun 5, 2024
6 checks passed
@Darsstar Darsstar deleted the raise-error-all-zero-data-credits-to-VadimKraus branch June 5, 2024 11:15
@squishykid
Copy link
Owner

This change is included in the new release: https://pypi.org/manage/project/solax/release/3.1.1/

If this should be upstreamed into home-assistant, do you mind raising a PR there to update the library? Here is an example of a previous library version bump: https://github.com/home-assistant/core/pull/114617/files#diff-35e8156540ea3b627a1b0b99df30998d35e88a2176c315c8f2cdc0cd972ebaf4

@Darsstar
Copy link
Contributor Author

Darsstar commented Jun 5, 2024

This change is included in the new release: https://pypi.org/manage/project/solax/release/3.1.1/

If this should be upstreamed into home-assistant, do you mind raising a PR there to update the library? Here is an example of a previous library version bump: https://github.com/home-assistant/core/pull/114617/files#diff-35e8156540ea3b627a1b0b99df30998d35e88a2176c315c8f2cdc0cd972ebaf4

I went ahead and created home-assistant/core#118888

@joseal
Copy link

joseal commented Nov 26, 2024

Hi @Darsstar and @squishykid,

I'm still getting zero reads, being forced to manually fix statistics almost daily...

What am I missing? do I need to remove and add again the device in integration?

thank you!

@VadimKraus
Copy link
Contributor

You only need to update homeassistant.
Do you have more information? Like logs etc.?

@Darsstar
Copy link
Contributor Author

It is also possible that the inverter isn't fully initialized yet causing us to read almost all zeroes, but with the non zero value on an index the solax library does not support/know how to interpret. (This is assuming your inverter enters idle mode at night. Which is not the case if your system includes a battery.)

I need to find the time to finish home-assistant/core#114743 and then a new one that discards the first read after some number of failed reads.

@joseal
Copy link

joseal commented Dec 2, 2024

It happened again:

image

It's not every day, but 2-3 times a week I've to manually fix the first hour in statistics...

Probably it's @Darsstar refers, but can we put any workaround in place?

@VadimKraus, could please guide me to collect the necessary logs?

@squishykid
Copy link
Owner

Thinking out loud here. @joseal sorry this won’t fix your issue immediately, but I just want to brainstorm how best to fix this.

@Darsstar do you think the zeros validation should be kept as a responsibility in the solax library or should we make it a responsibility of upstream users- in this case home-assistant.

I can see arguments both ways. In particular, home assistant doesn’t want to get bogged down with bugs and implementation details for devices. But at the same time, the solax library doesn’t really have any way to distinguish between a valid “zeroes” response and a false one.

I am partial to taking responsibility for device specific quirks like this in the solax library. Keen to hear thoughts though.

@squishykid
Copy link
Owner

@joseal can you please capture a inverter response which triggers this? We need to have a snapshot of the problem so we can make sure we are actually fixing your problem. Otherwise we might implement something which doesn’t actually fix your problem.

I understand it might be hard to reproduce, but perhaps you can start by trying to trigger this issue? Then once you understand how to trigger it, you can capture a payload.

@Darsstar
Copy link
Contributor Author

Darsstar commented Dec 2, 2024

@joseal can you please capture a inverter response which triggers this? We need to have a snapshot of the problem so we can make sure we are actually fixing your problem. Otherwise we might implement something which doesn’t actually fix your problem.

I have encountered the same issues on my development HA instance. I haven't kept the responses, but it was consistently the only the first response read by HA in the morning. I did solve it in HA then. Well solved except for what feel like true edge cases. The inverter becoming unresponsive at night is not an edge case. The resolver being turned on long enough for HA to have two requests succeed (the first it discards), turned off and turned on again and so it needs to re-initialize is something I qualify as an edge case.

The problem is something that I feel cannot be solved on a single request basis. Currently the library is designed around single requests. (I'm excluding he discover functionality since that is more bonus for when using the library without HA in my eyes. It is not long running. 6 seconds is nothing compared to the at most once a day interval.)
So unless we redesign the library I vote for solving this in HA and improving documentation.

Edit:
image
image
I haven't looked into the lower value at the end of the day that sometimes happens...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants