[modbus.sungrow] Added some more registers

[tscholand]

[modbus.sungrow] Added some more registers

This is my first contribution to openhab and my first work with modbus.

Description

I added some registers, among others (power flow state, mppt3 and mppt4 and some more), using the documentation "Communication Protocol of Residential Hybrid Inverter V1.1.5".

This should fix issue #17486

Testing

We need to get some positive testing feedback from people, who use the current version of the addon, as I am not 100% sure, if querying the added registers might produce errors or even render the addon unusable for them.

Testing can be done with the jar at https://github.com/tscholand/openhab-addons/releases/download/TESTING/org.openhab.binding.modbus.sungrow-5.0.0-SNAPSHOT.jar

[tscholand]

Ok, I just realized, that the checks didn't succeed. I guess, I will have to look into the problems, before this should be reviewed.

[tscholand]

I fixed the errors stated by the build pipeline.

[lsiepel]

Thanks you for your contribution. There are some minor comments to fix. Besides the added registers, there are more changes (like default poll interval) could you list the changes clearly in the start post? It's ok for now, but we usually prefer to have the changes seperated, it makes it easier to backport changes, improve review speed and it improve the release notes / documentation.

As some channels are removed and others are added, this PR also needs upgrade instructions. For reference: https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types You could also inspect a binding (like plugwiseha) to see how it works.

[soenkekueper]

As mentioned in the issue #17486, this breaks compatibility with inverters that don't provide this additional registers and requires a little more refactoring.

[soenkekueper]

I just had one idea how to solve this problem:

We could add a new flag for all registers "optional".

• Some base-registers, like model, sn, version, PV-Production, grid consumption, that are safe known to work for all devies are required
• others like backup-load, MPPT3/4, that (may) produce errors for certain device types / configurations are marked as optional

The thing-initialization could then be done as follows:

1. Read a few basic registers (SN, Model), to check, if connection can be established.
2. If successful check each optional register, if it is accessible. If not put on a transient block list.
3. On update build blocks (as already done) and exclude all blocklisted (optional) registers.

As in 2 only the optional registers are checked, the amount of modbus/network requests is lower (than checking every register).

This has the following advantages:

• Users must not configure device-type
• Users must not configure thing-details (like is backup connected, has device MPPT3,...)
• For new device types we must not perform any code-changes (as we had to do, if there is any kind of Device-Type to valid registers map)
• Reading is more resilient when sungrow registers change.

How do you think about?

[tscholand]

I just had one idea how to solve this problem:

We could add a new flag for all registers "optional".

* Some base-registers, like model, sn, version, PV-Production, grid consumption, that are safe known to work for all devies are required

* others like backup-load, MPPT3/4, that (may) produce errors for certain device types / configurations are marked as optional

The thing-initialization could then be done as follows:

1. Read a few basic registers (SN, Model), to check, if connection can be established.

2. If successful check each optional register, if it is accessible. If not put on a transient block list.

3. On update build blocks (as already done) and exclude all blocklisted (optional) registers.

As in 2 only the optional registers are checked, the amount of modbus/network requests is lower (than checking every register).

This has the following advantages:

* Users must not configure device-type

* Users must not configure thing-details (like is backup connected, has device MPPT3,...)

* For new device types we must not perform any code-changes (as we had to do, if there is any kind of Device-Type to valid registers map)

* Reading is more resilient when sungrow registers change.

How do you think about?

Hi, your idea sounds good. I'll have a look at this later. For now I tried to correct the points from the review above.

[lsiepel]

Hi, your idea sounds good. I'll have a look at this later. For now I tried to correct the points from the review above.

Only 2 comments are left open.