Add support for X3 MIC/PRO G2 #116
Conversation
|
when can we have a release with this PR? |
|
I think it is ready to get merged. Please note that Home Assistant 2023.6 is required (home-assistant/core#92914). @squishykid Are you happy with simply enumerating the sensors instead of using their internal id (fix for #112)? Using their internal id breaks Home Assistant if one inverter property is used twice (e.g. returning inverter state as text and as numerical value). Let me know if you have a better solution. I can implement it. |
solax/__init__.py
Outdated
| @@ -2,7 +2,7 @@ | |||
| import asyncio | |||
| import logging | |||
|
|
|||
| import async_timeout | |||
There was a problem hiding this comment.
I don't think these changes are strictly required for this PR. Are you doing this because there is a new API available?
There was a problem hiding this comment.
I reverted this change. Yes, there is an API change and a deprecation warning:
tests/test_smoke.py: 18 warnings
tests/test_solax.py: 3 warnings
/Users/niclas/PycharmProjects/solax/solax/__init__.py:25: DeprecationWarning: with timeout() is deprecated, use async with timeout() instead with async_timeout.timeout(REQUEST_TIMEOUT)
Shall I raise another PR for this change?
There was a problem hiding this comment.
I raised a PR #123 to address this issue.
solax/inverter.py
Outdated
| @@ -82,19 +82,15 @@ def sensor_map(cls) -> Dict[str, Tuple[int, Measurement]]: | |||
| Warning, HA depends on this | |||
| """ | |||
| sensors: Dict[str, Tuple[int, Measurement]] = {} | |||
| for name, mapping in cls.response_decoder().items(): | |||
| for idx, (name, mapping) in enumerate(cls.response_decoder().items()): | |||
There was a problem hiding this comment.
My understanding is that you're implementing these changes to address #112 . However we need to be careful not to change the names of existing sensors in home-assistant, as this would be a breaking change. Please revert this for now. We need to implement this separately
There was a problem hiding this comment.
I reverted this change. Additionally, I commented out the numerical run mode sensor to not return two sensors with the same id. We should consider to also comment out line 71 of x3_hybrid_g4.py for the same reason:
solax/solax/inverters/x3_hybrid_g4.py
Lines 71 to 72 in 9bc8d3d
Shall I raise another PR for this?
There was a problem hiding this comment.
Yep, feel free to raise a separate PR to address #112. I think the easy one would just be to remove the non-text-mode "run mode" sensor from x3_hybrid_g4. But if you implement something which can keep both and doesn't change the names for existing sensors, that's great!
There was a problem hiding this comment.
I raised a PR #122 to address this issue.
|
Just revert the changes in those two files and this is ready to merge! Thank you for raising the PR! |
This reverts commit 05f219d.
… numerical value)
|
This looks great. Thank you! |
|
v0.3.2 should be available on PyPi shortly, which includes your change To have your new inverter appear in home-assistant, you will need to make another PR in home-assistant/core. There is an example here: home-assistant/core#78219 |
|
Thanks for merging! I have just raised the Home Assistant PR: home-assistant/core#94545 |
I implemented support for the X3-MIC/PRO-G2 using the tables published in issue #101. This pull request should solve issue #84. I tested this implementation with my inverter at home.
@squishykid Thank you for starting this great project!