core icon indicating copy to clipboard operation
core copied to clipboard

Add Sungrow solar inverter integration

Open DasBasti opened this issue 2 years ago • 4 comments

Proposed change

Add modbus read out of Sungrow solar inverter. Tested with SG5.0RT but should work with: SG30KTL-M, SG30KTL-M-V31, SG33KTL-M, SG36KTL-M, SG33K3J, SG49K5J, SG34KJ, LP_P34KSG, SG50KTL-M-20, SG60KTL, SG80KTL, SG80KTL-20, SG60KU-M, SG5KTL-MT, SG6KTL-MT, SG8KTL-M, SG10KTL-M, SG10KTL-MT, SG12KTL-M, SG15KTL-M, SG17KTL-M, SG20KTL-M, SG80KTL-M, SG111HV, SG125HV, SG125HV-20, SG33CX, SG40CX, SG50CX, SG110CX, SG30KTL, SG10KTL, SG12KTL, SG15KTL, SG20KTL, SG30KU, SG36KTL, SG36KU, SG40KTL, SG40KTL-M, SG50KTL-M, SG60KTL-M, SG60KU

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [x] New integration (thank you!)
  • [ ] New feature (which adds functionality to an existing integration)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [x] The code has been formatted using Black (black --fast homeassistant tests)
  • [x] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

  • [x] Documentation added/updated for www.home-assistant.io https://github.com/home-assistant/home-assistant.io/pull/21472

If the code communicates with devices, web services, or third-party tools:

  • [x] The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • [x] New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • [ ] Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • [ ] No score or internal
  • [x] 🥈 Silver
  • [ ] 🥇 Gold
  • [ ] 🏆 Platinum

To help with the load of incoming pull requests:

DasBasti avatar May 08 '22 13:05 DasBasti

Hi @DasBasti,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

homeassistant avatar May 08 '22 13:05 homeassistant

I think I managed to fulfill all the requirements in the tests. Could you please trigger a run?

DasBasti avatar May 16 '22 16:05 DasBasti

I added a link to your documentation PR in the description.

davet2001 avatar Nov 01 '22 20:11 davet2001

The code change merged by github somehow mangled the import section of the test. The last commit fixes that.

DasBasti avatar Nov 06 '22 09:11 DasBasti

Is there still something missing for the merge?

DasBasti avatar Nov 16 '22 17:11 DasBasti

The pytests are currently failing. Please could you check why?

davet2001 avatar Nov 16 '22 18:11 davet2001

The tests failed in an unrelated component (history_stats/test_sensor.py::test_measure_sliding_window) and might have been realted to #81175. I rebased the PR to update to the current code.

DasBasti avatar Nov 17 '22 12:11 DasBasti

This time the jobs fail in cache cleanup and recorder component tests. What is the correct way to deal with such issues?

DasBasti avatar Nov 17 '22 14:11 DasBasti

Sorry that there are problems causing difficulty here.

If there are flaky/failing CI tests on the dev branch then probably you are not the only one to notice.

Rebase and retest in 24h often works in that case.

But I had a check on discord and couldn't see it mentioned.

davet2001 avatar Nov 17 '22 20:11 davet2001

Hi @02JanDal

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

home-assistant[bot] avatar Feb 06 '23 20:02 home-assistant[bot]

@davet2001 I've been in contact with @DasBasti as we've both been working on an integration for Sungrow inverters in parallel. As he was further ahead with the integration itself and the process of getting it reviewed and merged while my backing library was more advanced we decided to work together already as part of this PR, since it was not yet merged. As such I have modified Bastians code to use my library.

A review of these changes would be greatly appreciated!

02JanDal avatar Feb 06 '23 20:02 02JanDal

It looks like you have rebased and force pushed.

I can't easily see what has changed since I last reviewed and I would prefer not to re-read all >1000 lines.

Please could you describe what you changed?

davet2001 avatar Feb 09 '23 23:02 davet2001

@davet2001 Thanks for taking the time!

Here is a diff of just the changes to this integration I've made: https://gist.github.com/02JanDal/7c0d50e191e6bc74422a6c81ac6e1282 Note that it excludes changes in generated files like requirements and CODEOWNERS, as those also include changes from other commits so are harder for me to isolate.

The file with the most significant changes is __init__.py, as my library is async (although not 100% until HA switches to pymodbus 3 but that has to wait for upstream and the modbus integration) I had to move some stuff out of the SungrowData constructor into async_setup_entry. It also contains some aspects in SungrowCoordinatorEntity previously found in sensor.py, which is mostly just a preparation for more entity types (which I've got ready locally but will be part of a future PR).

config_flow.py should be functionally equivalent.

const.py lost the sensor definitions, I moved them into sensor.py, again as it makes more sense to me once more entity types are there.

sensor.py gained the sensor definitions, quite a few more as my library supports more, while also losing some details in them (like device class and unit) that I instead compute inside of SungrowCoordinatorEntity based on the metadata in pysungrow.

In the tests the changes are almost exclusively about changing what is mocked, I did however introduce a fixture that takes care of the patching that is done for most tests, as well as creating and registering the config entry.

Other than that it's of course quite a few import changes, as well as some adaptions to changes in the HA API, like using SensorDeviceClass instead of strings and going from async_setup_platforms to async_forward_entry_setups.

02JanDal avatar Feb 10 '23 07:02 02JanDal

Ok, this is quite a big job now.

It's close to a rewrite and using quite a new python library (so maybe that needs reviewing as well).

There is a big benefit to keeping a PR small. Faster to review, faster to re-review etc. Will re-review as soon as possible but it won't be immediately.

davet2001 avatar Feb 15 '23 18:02 davet2001

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

home-assistant[bot] avatar Aug 08 '23 15:08 home-assistant[bot]

@02JanDal can you please have a look into the changes I made, since I currently have no access to a sungrow inverter anymore

DasBasti avatar Aug 12 '23 15:08 DasBasti

Yes, I need to make another push on this, need to finish the update to pymodbus 3 (which the modbus integration was updated to) and fix a few instabilities regarding the various ways to connect (WiFi, LAN, LAN via WiNet stick) and bugs found by a few testers in a Swedish group where I posted this. Hope to get to this early next week.

02JanDal avatar Aug 12 '23 15:08 02JanDal

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions.

github-actions[bot] avatar Nov 24 '23 17:11 github-actions[bot]

Because there hasn't been any activity on this PR for quite some time now, I've decided to close it for being stale.

Feel free to re-open this PR when you are ready to pick up work on it again 👍

../Frenck

frenck avatar Nov 29 '23 21:11 frenck