-
Notifications
You must be signed in to change notification settings - Fork 63
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
raise validation error for all zero response data #148
Conversation
how about we just reopen #142 :D |
I can't. Feel free to rebase your branch on master yourself. |
Ah sorry, there were conflicts again? nvm then :P |
708efaf
to
d0e57d3
Compare
@Darsstar, could you please confirm if this change was already merged/deployed? sorry, because I'm already a bit confused... |
For all intents and purposes: no, it has not. |
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 |
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! |
You only need to update homeassistant. |
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. |
It happened again: 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? |
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. |
@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. |
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.) Edit: |
Succeeds #142, thanks @VadimKraus.