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

Fix X1 Hybrid Gen4 support with new firmware #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nazar-pc
Copy link

@nazar-pc nazar-pc commented Jun 23, 2024

Fixes #161, new firmware just has a bunch more zeroes on this inverter.
The whole approach with counting those is flawed though in my opinion judging by recent PRs. Maybe minimum is okay, but maximum clearly can change in newer firmwares.

@MatthieuCoder
Copy link

This is appending to me too

@squishykid
Copy link
Owner

Hi, thank you so much for raising a PR. Apologies for the late response.

Can you please add a test case which exercises this new behavior?

This recently merged PR can serve as an example for adding a new test fixture: https://github.com/squishykid/solax/pull/167/files#diff-5dbc88d6e5c3615caf10e32a9d6fc6ff683f5b5814948928cb84c3ab91c038b6

When you create an 'InverterUnderTest' in fixtures.py, just use the existing X1 Hybrid G4 inverter.

Copy link
Owner

@squishykid squishykid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the comment, please add some tests to validate the new schema.

@nazar-pc
Copy link
Author

This is a dead simple change, not sure it is actually required to have a whole test just for this. I'd argue checking the length of data is useless to begin with, if it wasn't present then this PR wouldn't be necessary. They just decided to add more zeroes in newer firmware and I suspect there will be more people coming over time with other inverters showing the same behavior with newer firmware on them.

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.

X1 Hybrid Gen4 doesn't work with new firmware
3 participants