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

Add growatt_rs232 integration for Growatt inverter with RS232 modbus #37846

Closed

Conversation

ArdescoConsulting
Copy link

Proposed change

This integration adds communication with the Growatt solar inverter via RS232 Modbus (for people without the network connection).
If you wish so, avoiding the Growatt cloud monitoring keeps your setup more private and safe while having access to most functionality.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • 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

this integration only supports configuration via the UI
uses pypi growattRS232==1.0.0

Checklist

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

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

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] Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

Although this is my first integration, I have done my best to comply with the Platinum standard but I leave it up to the reviewers to determine the quality.
I would welcome any feedback on how to further improve it. Given my inexperience, please provide feedback with some concrete pointers so I know where to look.

I read them all and hopefully implemented them correctly.

@homeassistant
Copy link
Contributor

Hi @ArdescoConsulting,

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!

@mrand
Copy link

mrand commented Jul 14, 2020

I notice references in the text that will be displayed by the integration, as well as in the main documentation, to a "USB port". I'd like to suggest the this integration follow the precedent of other serial integrations and simply refer to the RS232 port, omitting any mention of USB, since there are plenty of machines that don't require USB converters. At most, a single reference to the possibility of a USB<>RS232 converter would suffice on the documentation page.

@ArdescoConsulting
Copy link
Author

@mrand Thanks for the valuable feedback

Your question touches 2 interesting points:
1.
The integration indeed uses serial communication and aligning vocabulary with other serial integrations is a clear benefit for the user.
There are many versions of serial communication however: tty, RS232, RS485, RS422,…
What all serial communications have is common is that they need to know what to communicate with. In Windows terminology a com port and in *nix a tty device.
There is no consistency in the serial integrations in the use of port and device (e.g. integrations Serial, Serial Particulate Matter, Modbus).
The const.py file also has CONF_DEVICE and CONF_PORT. To have a generic term for both *nix and windows, maybe the term “serial interface” could be used.
Maybe a more generic CONF_SERIAL_INTERFACE in const.py could help also standardize the config flows regardless of the serial protocol used?
This is also more in line what you see in supervisor / hardware

In my example below, I have both an RS232 and RS485 usb adapter attached to my rpi. Both showing as a serial.

Hardware
serial:
/dev/ttyUSB0
/dev/ttyAMA0
/dev/serial/by-id/usb-1a86_USB2.0-Serial-if00-port0
/dev/serial/by-id/usb-Prolific_Technology_Inc._USB-Serial_Controller_D-if00-port0
/dev/ttyUSB1
input:
disk:
/dev/mmcblk0p8

I wonder though how many people have native serial interfaces.

I guess most installations would be rpi based.

To use the built-in serial, you need at least a converter chip (such as the max232) attached to the uart for RS232 because of the voltage difference.
Depending on the version of the rpi you’ll also need to use raspi-config to enable uarts / deactivate the linux console on the primary uart.
https://www.raspberrypi.org/documentation/configuration/uart.md
with the move the home assistant os, raspi-config is not available any more.
The issue is the same as for enabling ic2.
https://www.home-assistant.io/hassio/enable_i2c/
The usb method did not work on my rpi4 and I had to revert to mounting the partitions and changing the files like that. Not really beginner stuff.

It is far easier to just use an usb rs232 adapter.

A top scorer will also be old computers. I checked my own quite extended junkyard and none has a serial connection any more. So most likely also a lot of usb dongles there too.

Industrial sbc’s will most likely have serial interfaces but will be more for the enthusiasts who really know what they are doing.

From this my assumption would be that usb serial adaptors are the most used and will only be increasing in the future.

Home assistant started in the enthusiast space but is making big efforts to make it easy to use for people without a lot of background. Plug and play. You get a rpi with a case, connect a usb dongle so you don’t have to mess with a hat that does not fit in your shiny new case and no need to worry about raspi-config setting. Move away from yaml and to config flow.

From that perspective, starting with the simple usb dongle case but also providing the advanced information would make more sense but I agree it can be prased better so I’ll look into it.

Maybe the easiest would be to make a page like the i2c https://www.home-assistant.io/hassio/enable_i2c/ that all serial integrations could refer to for consistency and deduplication.

Another thing that comes to mind
In my example below
/dev/ttyUSB0
/dev/serial/by-id/usb-Prolific_Technology_Inc._USB-Serial_Controller_D-if00-port0
and
/dev/ttyUSB1
/dev/serial/by-id/usb-1a86_USB2.0-Serial-if00-port0
Refer to the same devices.

As I have also seen in some discussions, /dev/ttyUSB0 is not always guaranteed fixed to a specific device which could lead to hard to find issues were serial devices to swap /dev/ttyxxxx links.

Let’s gather some feedback from the community on this. BTW same for spi.

By researching serial I stumbled upon a very nice config flow that lists the available ports for the user.

Core/homeassistant/components/zha/config_flow.py

Certainly something to look at for a future version when I have some more time available.

To pre-empt another possible remark.
The integration is called growatt_rs232 while in fact it is using Modbus communication.

Initial growatt inverters only had a serial RS232 db9 connector using a proprietary communication protocol (also used for firmware updates and such)
spc: Growatt Inverter Communication Command
http://www.snafu.priv.at/mystuff/growatt.html

In later firmware versions they started using the rs485 modus protocol on their RS232 db9 connector specified in Growatt PV Inverter Modbus RS485 RTU Protocol V3.14

On later hardware versions a dedicated RS485 connector has been added in addition to the RS232 serial db9 connector. From what I can see in the documentation both provide the same functionality but they use RS485 on their integration boxes so I can imagine the RS232 db9 connection disappearing in the future.

For maximum compatibility (and because I have an old inverter without RS485 port) I’ve called it RS232. This is also what is marked on the box and in the documentation.

To use the RS485, you need a RS485 usb adapter which are less common than RS232 adapters. I guess everything else work exactly the same but I can’t test it. To be added if someone can confirm.

Maybe it is better to call the integration growatt_serial then. There is also a growatt_server that uses the Growatt cloud service.

@MartinHjelmare MartinHjelmare changed the title Add growatt_rs232 integration for Growatt inverter with RS232 modbus. Add growatt_rs232 integration for Growatt inverter with RS232 modbus Jul 16, 2020
@mrand
Copy link

mrand commented Jul 16, 2020

Wow - wasn't expecting such a detailed reply! I'm going to pick out a few key points:

Maybe it is better to call the integration growatt_serial then. There is also a growatt_server that uses the Growatt cloud service.

That makes sense to me, especially if it supports models which support RS485.

From this my assumption would be that usb serial adaptors are the most used and will only be increasing in the future.

I mostly agree, but it still seems wrong to me to have multiple mentions for something that isn't even required, never mind the inconsistency with other serial integrations.

As I have also seen in some discussions, /dev/ttyUSB0 is not always guaranteed fixed to a specific device which could lead to hard to find issues were serial devices to swap /dev/ttyxxxx links.

Yep, it's a known challenge with regard to Linux, the discussion of which I think is beyond the scope of HA integration pages maybe except for a very brief suggestion (maybe labelled "advanced topic"?) to set up a persistent name to prevent it from changing (with maybe with an link to more info on the topic).

@frenck frenck added this to Incoming in New Integrations via automation Aug 3, 2020
@stale
Copy link

stale bot commented Aug 16, 2020

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.

@stale stale bot added the stale label Aug 16, 2020
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

We want integrations for the same brand and device to use the same integration domain. So this should be combined with the existing growatt_server integration.

Give the user an option to choose between the network and rs233 connection and then set up the correct logic depending on the user choice. It will require a refactor of the existing growatt_server integration first with adding a config flow and config entries. Probably rename to just growatt.

Dev automation moved this from Needs review to Review in progress Aug 16, 2020
New Integrations automation moved this from Incoming to Awaiting Requested Changes Aug 16, 2020
@stale stale bot removed the stale label Aug 16, 2020
@ArdescoConsulting
Copy link
Author

@MartinHjelmare
This is a free-time project for me. In this limited time, I’m trying to provide as much value to the community as I can.

  1. Aligned with the vision of Home Assistant “your system should run at home, not in the cloud” https://www.home-assistant.io/blog/2016/01/19/perfect-home-automation/ , provide privacy and security by keeping data local (instead of using the growatt cloud service as used by the growatt_server integration)
  2. Quality by having tests, being able to test with actual hardware, and keeping the integration as simple as possible.
  3. Support the integration with the evolving home assistant implementation
    Having returned from vacation, I was going to change the wording from USB to serial based on the feedback as discussed earlier so the pull request was ready. In a second phase and when I have some more time available, I would have liked to add serial port auto-detection.
    Integrating with growatt_server is a lot of additional complexity and effort for which I do not have the time. Maybe @indykoning or somebody from the community is interested in doing this?
    I do not want to use the Growatt cloud service for privacy and security reasons so I’m not going to support something that I can only half test.
    There is an overlap but also a lot of differences in data points between the integrations. This will most likely confuse users resulting in a lot of questions about why someone does not have this specific data point while someone else with the same integration is seeing it.

@MartinHjelmare
Copy link
Member

I don't think it will be confusing. It will be the first question we ask the user, network or rs232. We can also document differences in our docs.

@stale
Copy link

stale bot commented Sep 20, 2020

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.

@stale stale bot added the stale label Sep 20, 2020
@sanderlv
Copy link

Hi, I have a growatt 5000tl xe converter and connector with rs485 output. I am willing to test this "integration" to get logging local. What do I need to do? Where do I need to start?

@stale stale bot removed the stale label Sep 29, 2020
@MartinHjelmare
Copy link
Member

There hasn't been any movement for a month here. I'll close now. We're trying to decrease our open PR buffer. Please open a new PR when ready to finish. Thanks!

Dev automation moved this from Review in progress to Cancelled Sep 30, 2020
New Integrations automation moved this from Awaiting Requested Changes to Cancelled Sep 30, 2020
@sanderlv
Copy link

Hum. Sorry not sure what you are asking here. I need some help... It was very hard to even find this here, so do not know how and where to start from now.

@MartinHjelmare
Copy link
Member

My comment was aimed at the PR author.

@LittlePapaJohn
Copy link

Hi @ArdescoConsulting,
Great work you are doing and I also interested in the RS232 solution for Growatt. However I agree with HA that only one integration should exist. In the past I was confused about similar names for integrations, it creates more confusion than having the choice what to select during the integration in HA. Would be great if you and the Growatt-server programmer could work together on this and bring a complete solution for all. I would have love to take this on, if I just knew how.
Hope you still read these comments, Thanks

@indykoning
Copy link
Contributor

indykoning commented Oct 11, 2020

Once
#41303
is merged, allowing setup via the integrations tab. It should be a lot easier to refactor and add rs232 support.

Sadly i will be unable to help with that part, but of course I'll be happy to test compatability once the integrations have been merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Cancelled
New Integrations
  
Cancelled
Development

Successfully merging this pull request may close these issues.

None yet

7 participants