Change from pymodbus.client to pymodbusTCP.client

[nerfherder]

This PR changes the modbus implementation from pymodbus.client to pymodbusTCP.client which introduces auto open and close. This appears to fix the issues around fast data refresh as described in #37.

I have merged the changes from #38 as well.

[WillCodeForCats]

The _stub functions are for testing without inverters connected; they should not be changed to STATE_UNKNOWN as far as I understand their purpose.

[nerfherder]

The changes to state_unknown are from #35

[mpredfearn]

The _stub functions are for testing without inverters connected; they should not be changed to STATE_UNKNOWN as far as I understand their purpose.

@WillCodeForCats is that right? As I'd understood it, the stubs set up default values in self.data which can be returned to home assistant as valid state if the update routine has not yet been called. Every time I restarted home assistant, I would see sensors drop to the default values set here before returning to real value upon the first poll.
This became important with the energy dashboard, which requires that "lifetime increasing" values do just that, and do not return to 0 before the first poll after a restart. Hence my change to make them "unknown" before the first poll.

[mpredfearn]

@binsentsu There are a lot of changes proposed in multiple MRs here. How would you like to proceed (i.e. which order should we try to get things merged?)

[WillCodeForCats]

I deleted the _stub functions completely from my multiple inverters fork (which is just a fork with #12 added) and have not experienced that issue with the energy dashboard and restarting HA. I run a system with two inverters, so I can't use this as-is. See: https://github.com/WillCodeForCats/home-assistant-solaredge-modbus

[WillCodeForCats]

As far as multiple PRs, my vote is not to pull in one massive change like this, but stick with the smaller incremental changes because it makes it easier to check and merge into the multiple inverter fork. I don't really want to maintain a separate fork, but it's necessary for now for those of us who are not just running a single inverter but still want to join the party.

[mpredfearn]

As far as multiple PRs, my vote is not to pull in one massive change like this, but stick with the smaller incremental changes because it makes it easier to check and merge into the multiple inverter fork. I don't really want to maintain a separate fork, but it's necessary for now for those of us who are not just running a single inverter but still want to join the party.

Agreed. It would also be good to get the multiple inverters change merged here, so we have a single repo that is pointed to by HACS.

[mpredfearn]

I deleted the _stub functions completely from my multiple inverters fork (which is just a fork with #12 added) and have not experienced that issue with the energy dashboard and restarting HA. I run a system with two inverters, so I can't use this as-is. See: https://github.com/WillCodeForCats/home-assistant-solaredge-modbus

OK, interesting. Maybe that change of mine can be dropped then.

[WillCodeForCats]

One of the comments in #12 says the _stub functions are just dummy data for testing, so that was my entire basis for dropping them.

[WillCodeForCats]

@mpredfearn I watched the kWh values on mine as it started (installed the latest updates) and saw they came up as "unknown" on boot before the inverters were polled.

[binsentsu]

The _stub functions are for testing without inverters connected; they should not be changed to STATE_UNKNOWN as far as I understand their purpose.

@WillCodeForCats is that right? As I'd understood it, the stubs set up default values in self.data which can be returned to home assistant as valid state if the update routine has not yet been called. Every time I restarted home assistant, I would see sensors drop to the default values set here before returning to real value upon the first poll.
This became important with the energy dashboard, which requires that "lifetime increasing" values do just that, and do not return to 0 before the first poll after a restart. Hence my change to make them "unknown" before the first poll.

Those stubs were indeed just for testing without the hardware. They are never used within a deployment, so can be removed.

[binsentsu]

As far as multiple PRs, my vote is not to pull in one massive change like this, but stick with the smaller incremental changes because it makes it easier to check and merge into the multiple inverter fork. I don't really want to maintain a separate fork, but it's necessary for now for those of us who are not just running a single inverter but still want to join the party.

@WillCodeForCats Proposed way of merging: I will first pull #38 which includes all changes for battery support. Then you can bring your multi-inverter fork in sync and fire a PR for that feature to bring it in upstream. The update of the modbus client library can then be an improvement PR later on.

[WillCodeForCats]

I should be able to handle that. I'm not a Python programmer by trade, only by necessity since that's what HA uses. There was some other stuff I wanted to try to add like a config option for starting unit id number like if someone did 2,3,4 for their inverters instead of 1,2,3. I think that would also address #8

I've looked at Solaredge's docs for battery systems and it appears to also be possible to do multi-inverter with multi-battery, but I wouldn't have any way to test that with real hardware.

[nerfherder]

@binsentsu

The update of the modbus client library can then be an improvement PR later on.

I'm happy with this approach, after you pull #38 I will sync with upstream so that we can treat this as an isolated PR and test it as appropriate.

I'll admit, this is my first Pull Request so I wasn't sure of the best procedure, my main reason for pre-merging with #38 was because I wanted that functionality for my own system.

[mpredfearn]

Hey @nerfherder do you think you could rebase the changes, instead of a merge? We've ended up with 23 commits in here most of which are fixups, but it's impossible to review the changes like this. Could you squash it all down to the one logical change in a single commit?

[nerfherder]

Yeah ok, I'll make a new branch with a single logical commit

[nerfherder]

@mpredfearn rebase done

[nerfherder]

I had previously added a validity check to this PR which checked whether the values coming back made sense, however it has ocured to me that this validity check is probably all that is required to solve #37 so I am going to make a new PR with just this fix in it.

[WillCodeForCats]

Okay, I can't keep up with all these changes. The 1.4.0 release unfortunately pulled in the control stuff that's beyond my Python+HA skill level, so hopefully someone who has better Python skills than I will come along and add multiple inverter support from this point forward.

I will add the battery monitoring sensors to my multiple inverter fork and keep an eye out for fixes that are related, but the control stuff adds too much code for me to keep up.

[mpredfearn]

@WillCodeForCats :-( I'm happy to attempt the work, if you can do the testing on your system? What's the gist of the change, support for multiple unit IDs, right?