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 #68

Merged
merged 1 commit into from
Mar 3, 2024

Conversation

VadimKraus
Copy link
Contributor

This is a possible solution for #67.

After this PR:

  • the response dict will be validate using a schema definition, therefore the access to the dict properties are more safe now
  • the check includes a validation that at least one value in 'Data' is not 0

Consequences for Home assistant:
If an zeros data event occurs, the validation error will cause the sensor update to fail and thereby the inherent home assistant retry will be triggered. No Changes to the solax integration are required.

solax/utils.py Outdated Show resolved Hide resolved
@VadimKraus VadimKraus force-pushed the feature/query_check branch 2 times, most recently from 222d91d to a7fecf0 Compare July 31, 2022 14:45
solax/inverter.py Outdated Show resolved Hide resolved
@Azelphur
Copy link

Azelphur commented Sep 5, 2023

Any way we could get some movement on this? happy to contribute, this has been broken for over a year now and it is bugging me :)

@VadimKraus
Copy link
Contributor Author

Reworked based on current master.

@joseal
Copy link

joseal commented Nov 5, 2023

Hi VK, thanks for the push! Quick question: do you know the version/build when it will hit GA?

@VadimKraus
Copy link
Contributor Author

Hey @joseal, that depends on @squishykid. He might have different plans about his issue :)

@joseal
Copy link

joseal commented Nov 11, 2023

@squishykid , any concern? This issue is disturbing a ton of people for a year... it was great if it could be fixed. Thank you.

@joseal
Copy link

joseal commented Jan 17, 2024

Any news about this issue? I still need to be constantly manual fixing the values...

@joseal
Copy link

joseal commented Mar 3, 2024

Hi @squishykid any news?

@squishykid squishykid merged commit 2c20404 into squishykid:master Mar 3, 2024
3 checks passed
@squishykid
Copy link
Owner

Thanks for the reminder!

I will also cut a new release, so it can be updated in home-assistant

@joseal
Copy link

joseal commented Mar 3, 2024

Brilliant! Thank you very much.

squishykid added a commit that referenced this pull request Mar 4, 2024
squishykid added a commit that referenced this pull request Mar 4, 2024
* update actions to latest major version

Signed-off-by: Robin Wohlers-Reichel <[email protected]>

* Revert "raise validation error for all zero response data (#68)"

This reverts commit 2c20404.

---------

Signed-off-by: Robin Wohlers-Reichel <[email protected]>
@squishykid
Copy link
Owner

@joseal can you please take another look at this? I think CI was broken, but I had to revert this commit because it broke discovery

@joseal
Copy link

joseal commented Mar 9, 2024

@joseal can you please take another look at this? I think CI was broken, but I had to revert this commit because it broke discovery

If I'm not wrong, this change was made by @VadimKraus... VK, could you please check it? Thank you!

@VadimKraus
Copy link
Contributor Author

The discovery error is unrelated to this change.
Try running the test multiple times, you will find them failing eventually.
pytest --count=10 tests/test_discovery.py

@VadimKraus
Copy link
Contributor Author

VadimKraus commented Mar 10, 2024

Did some testing: the discovery has been in this broken state since a7a95f7.

This causes a race condition where 'solax.inverters.x1_boost.X1Boost' and 'solax.inverters.x1_mini_v34.X1MiniV34' are being confused with each other. They have basically an overlapping schema...

@squishykid I will reopen this PR, since the problem is unrelated.

However, the discovery needs to be fixed ...

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