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

Refactor discovery #145

Merged
merged 4 commits into from
Mar 31, 2024
Merged

Conversation

Darsstar
Copy link
Contributor

First commit:

  • test with 3.11 and 3.12
  • require combined coverage from all versions to be 100% instead of individual Python versions, this allows sys.version_info < (3, 10) checks to be used in the code base.

Second commit:

  • The main purpose is to speed up discovery.
  • But is also provides similair functionality to Optional model parameter to bypass discovery #144.
    • The difference is this PR does not use the class name, but creates names entry points instead.
    • This PR allows for a list, instead of a single class/model. The Home Assistant integration could add a config entry to manage which inverter classes/models are enabled. During discovery it would then only pass those enabled inverter classes/models to solax.discover()
      • solax.discover() can also return a list of all inverter models/classes instead of returning the fist that succeeded.
        Which can be used by Home Assistant to have the user resolve such situations.

Third commit:

Fourth commit:

  • Adds inverter SN to the named tuple. Home Assistant could then use that instead of the dongle SN. Which would have the benefit of having only one entity if a broken dongle needs to be replaced.

@squishykid It took me longer to finish than expected. Hope you enjoyed your holiday!
PS. I assume you are the one who started following me on Trakt :p

@brew-your-own
Copy link
Contributor

Nice!
Do you also have patches for HA to apply your approach in the component?

@squishykid
Copy link
Owner

looks good to me! nice work mate.

@@ -15,28 +16,20 @@ class X1HybridGen4(Inverter):
"sn",
): str,
vol.Required("ver"): str,
vol.Required("Data"): vol.Schema(
vol.Required("data"): vol.Schema(
Copy link

@nazar-pc nazar-pc Jun 20, 2024

Choose a reason for hiding this comment

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

It is Data, not data in JSON. Just like Information below. Why did you change it?

UPD: Hm... it does seem to work regardless though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I lowercased all keys in a previous PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accedentilly hit "Comment" in the mobile app...

I meant to write commit not PR, but both are wrong, it was in the same commit.

Anyway, I assume #161 is why you decided to comment.

Choose a reason for hiding this comment

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

Yes, I was going through recent changes to see if something broke, but then remembered about firmware upgrade and found the root cause

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