dewet22 / givenergy-modbus

A python library to access GivEnergy inverters via Modbus TCP on a local network, with no dependency on the GivEnergy Cloud.
Other
21 stars 14 forks source link

Some hybrid inverters start with SD not SA #6

Closed zaheerm closed 2 years ago

dewet22 commented 2 years ago

Sorry, been traveling so only getting around to this now! Good to know about this - out of curiosity, what is the text description of your model in the GE dashboard? Mine's Giv-HY5.0

codecov[bot] commented 2 years ago

Codecov Report

Merging #6 (0c92b35) into main (db0cf5c) will increase coverage by 0.05%. The diff coverage is 100.00%.

:exclamation: Current head 0c92b35 differs from pull request most recent head 29ed4a2. Consider uploading reports for the commit 29ed4a2 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   90.05%   90.10%   +0.05%     
==========================================
  Files          17       17              
  Lines        1659     1668       +9     
  Branches      153      155       +2     
==========================================
+ Hits         1494     1503       +9     
  Misses        142      142              
  Partials       23       23              
Impacted Files Coverage Δ
givenergy_modbus/model/inverter.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 48b8d8b...29ed4a2. Read the comment docs.

dewet22 commented 2 years ago

You should also do a tox run over the proposed changes (or install and run pre-commit) - the lint tests failed with:

  givenergy_modbus/model/inverter.py:14:1: D101 Missing docstring in public class
  givenergy_modbus/model/inverter.py:26:1: D102 Missing docstring in public method
  tests/model/test_inverter.py:348:1: D103 Missing docstring in public function
zaheerm commented 2 years ago

what is the text description of your model in the GE dashboard?

It's Giv-HY5.0 for me.

dewet22 commented 2 years ago

Interesting, so they seem to be labeled identically. I took your contribution and reworked it a bit last night – hope you don't mind or feel I appropriated it without giving credit (definitely one for the CHANGELOG) but the JSON serialisation is better now (model name instead of serial number prefix) and I fixed the lint warnings at the same time.

zaheerm commented 2 years ago

Thanks no issues, I'm on holiday now so didn't get to the suggested linting. I read your changes and they make sense. In fact I started my changes, changing to auto() instead of keeping the strs. I just didn't know what implications it may have had elsewhere in any dependent code.

dewet22 commented 2 years ago

It's Giv-HY5.0 for me.

Interesting – talking to the folks at GivEnergy they claim the SA model is the 3.6kW Hybrid inverter. Do you know which model you actually have?

zaheerm commented 2 years ago

I have a model with a serial number starting SD and the inverter is definitely a 5 kW inverter.

On Sun, 20 Feb 2022, 17:34 Dewet Diener, @.***> wrote:

It's Giv-HY5.0 for me.

Interesting – talking to the folks at GivEnergy they claim the SA model is the 3.6kW Hybrid inverter. Do you know which model you actually have?

— Reply to this email directly, view it on GitHub https://github.com/dewet22/givenergy-modbus/pull/6#issuecomment-1046263300, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQ2T6RPORFBLGHVKBZBM3U4ECXPANCNFSM5NVTYSKQ . You are receiving this because you authored the thread.Message ID: @.***>

dewet22 commented 2 years ago

Ah, I got it the wrong way around - mine starts with SA but is also definitely a 5.0kW Hybrid. Odd, I'll try and understand it a bit better. Out of interest, approximately how old is yours?

zaheerm commented 2 years ago

Mine was installed a few weeks ago.

On Sun, 20 Feb 2022, 17:59 Dewet Diener, @.***> wrote:

Ah, I got it the wrong way around - mine starts with SA but is also definitely a 5.0kW Hybrid. Odd, I'll try and understand it a bit better. Out of interest, approximately how old is yours?

— Reply to this email directly, view it on GitHub https://github.com/dewet22/givenergy-modbus/pull/6#issuecomment-1046267990, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAQ2T35SMJ7HMVJ3ALYCZTU4EFXHANCNFSM5NVTYSKQ . You are receiving this because you authored the thread.Message ID: @.***>