[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.

[lsiepel]

Gentle ping @tscholand code freeze for oh5 is sunday, so if you want to have this in oh5, please look at the remaining comments.

[Copilot]

Pull Request Overview

This PR adds numerous new registers to the Sungrow modbus binding, expanding the range of data available from Sungrow inverters. The implementation follows the Communication Protocol of Residential Hybrid Inverter V1.1.5 documentation and includes power flow information, additional MPPT channels, backup power data, and various settings.

Key changes include:

• Added power flow status registers with bit-level state extraction
• Introduced new mapper classes for string state conversion (device type, running state, output type, DRM state)
• Extended channel support for MPPT3/4, backup power information, and high-precision measurements
• Updated test coverage for new functionality

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
SungrowInverterRegistersTest.java	Added comprehensive tests for new device type mapping, power flow states, and percent state handling
thing-types.xml	Extensive XML configuration additions for new channel groups and channel types
sungrow.properties	Added internationalization labels for all new channels and channel groups
RunningStateMapper.java	New mapper for converting hex codes to human-readable running states
OutputTypeMapper.java	New mapper for output type conversion (SINGLE, 3P4L, 3P3L)
DrmStateMapper.java	New mapper for DRM state values with uncertainty noted in comments
DeviceTypeMapper.java	New mapper for device type hex codes to model names
ToStringMapper.java	New interface for BigDecimal to String mapping functionality
SungrowInverterRegisters.java	Major expansion with 40+ new registers, new state factories, and improved scaling constants
ConversionConstants.java	Added new scaling constants and improved precision in temperature conversion
README.md	Updated documentation with new specification version and simplified inverter list

Comments suppressed due to low confidence (1)

bundles/org.openhab.binding.modbus.sungrow/src/test/java/org/openhab/binding/modbus/sungrow/internal/SungrowInverterRegistersTest.java:112

• [nitpick] The variable name POWERFLOW_STATE should follow camelCase convention and be named powerFlowState instead of using SCREAMING_SNAKE_CASE which is typically reserved for constants.

        final String POWERFLOW_STATE = "10010011";

[Copilot]

Using exceptions for control flow is generally discouraged. Consider checking the string length before accessing the character at bitIndex to avoid the exception.

Suggested change

	try {
	String binaryString = Integer.toBinaryString(value.intValue());
	StringBuilder sb = new StringBuilder(binaryString);
	sb.reverse(); // binary notation is backwards (10100: the last 0 is bit0). Note that there are no
	// leading zeros!
	char bitChar = sb.charAt(bitIndex);
	if (Character.getNumericValue(bitChar) == 1) {
	return OnOffType.ON;
	}
	} catch (IndexOutOfBoundsException e) {
	// There was no bit at the bitIndex. No leading zeros in bit representation. Return OFF.
	}
	String binaryString = Integer.toBinaryString(value.intValue());
	StringBuilder sb = new StringBuilder(binaryString);
	sb.reverse(); // binary notation is backwards (10100: the last 0 is bit0). Note that there are no
	// leading zeros!
	if (bitIndex < sb.length()) {
	char bitChar = sb.charAt(bitIndex);
	if (Character.getNumericValue(bitChar) == 1) {
	return OnOffType.ON;
	}
	}
	// If bitIndex is out of bounds or the bit is 0, return OFF.

[Copilot]

Creating a StringBuilder and reversing it for each bit check is inefficient. Consider using bitwise operations like ((value.intValue() >> bitIndex) & 1) == 1 for better performance.

Suggested change

	String binaryString = Integer.toBinaryString(value.intValue());
	StringBuilder sb = new StringBuilder(binaryString);
	sb.reverse(); // binary notation is backwards (10100: the last 0 is bit0). Note that there are no
	// leading zeros!
	char bitChar = sb.charAt(bitIndex);
	if (Character.getNumericValue(bitChar) == 1) {
	return OnOffType.ON;
	}
	} catch (IndexOutOfBoundsException e) {
	// There was no bit at the bitIndex. No leading zeros in bit representation. Return OFF.
	int intValue = value.intValue();
	if (((intValue >> bitIndex) & 1) == 1) { // Extract the bit at the specified index
	return OnOffType.ON;
	}
	} catch (ArithmeticException e) {
	// Handle cases where bitIndex is invalid (e.g., negative or too large). Return OFF.

[lsiepel]

@tscholand Let me know when we can proceed and fix the last few comments, i think we are close to succesfully merging this PR.

[tscholand]

Hi @lsiepel ,
I don't think we can merge this PR. As soenkekueper mentioned above, merging this would break functionality for other devices, so we do have to implement something to still support those. I will not have the time to do this anytime soon.
I'm sorry. Maybe someone else can drive this further?! Maybe we should close this PR or set it to draft?

[lsiepel]

Maybe it is enough to create a configuration option extendedReggistersEnabled or something like that? and disable it by default. It won;t break any setup, fairly easy to implement and when documented a user can enable this manually. wdyt?

If needed i can help to add this as it woudl be a straight forward 15 minuten task and prevnts the work done on this PR to be lost.

[soenkekueper]

I'll give it a try to check, which registers are causing this problems and give a status update, once i've new information on this.

[soenkekueper]

So i've tested and checked today.
Result is very mixed up.

I've checked some of the registers from the error messages with QModbusMaster. Result:
Sometimes it works, sometimes there is an error.

I've checked out the PR and rebased onto the current main and tested some things:

• There are differences when using the internal lan port of inverter and the lan port of the winet-s-dongle.
• These differences are also seen when using QModbusMaster
• Finally Winet-S stopped working totally :/

I now sometimes get a SocketTimeoutException when reading data, but i've recently fixed #19186 and added retries when reading data.
All values seems now to be quite correct. I'm not able to reproduce the error again.

I've only one explanation for this: Recently my inverter got a firmware update and afterwards some modbus registers stopped working (battary level was 100% all the time, btw. iSolarCloud App und Website behaved the same, as they relay on same data). Someday (without any actions, while waiting for technican, the cloud showed the correct data, but not the modbus export. I've then switched from winet-s dongle to internal lan port, (which did not work at all when implementing this binding).

I've also written a tool based on the openhab modbus code, that queries every single sungrow register and checks if this throws an error. But nothing happend, evereything went okay :/

So i've tried to get some version information from the inverter and added thing properties for device type, software version, protocol version etc. This may help to figure out differences, when getting user feedback.
I'll now test how this addon behaves in my installation and give feedback again.

One point i've remarked:
the data type for channel BATTERY_POWER (13022) was changed from uint16 to int16. Since unit16 is what is specified within the spec and worked for me a long time, i'm not happy to simply change this. Maybe this is a difference between internal and winet-s port, so i'll check this, once i've resetted my winet-s dongle.
Maybe we need to duplicate the channel and provide one with signed and one unsigned.

@tscholand I would suggest to omit the channels that provide "static" data as Device model, output and nominal power (and have already added them as properties. Did you have anything in mind to provide them as channel instead of properties?
Otherwise, i would drop the newly added channels.

sungrowRegisters.xlsx

[tscholand]

@soenkekueper I did not have anything special in mind, when adding the static values as channels. I was just trying to get all the values from the registers into openhab. But I guess it does not really make sense to provide static values as channels.

[soenkekueper]

So i've just

• removed the device-information channels
• added a battery-power-signed channel, to keep backward compatiblity and stick to spec while providing both oppertunities
• installed this in my production environment and will check for errors.
• rebased the branch on the current main

see https://github.com/soenkekueper/openhab-addons/tree/sungrow_additions

@tscholand Maybe you want to have a look at my changes and afterwards i would suggest to provide this to the community to do some wider tests (as only us two). If there are no problems occuring - i would be happy to see this merged.

Sorry for my first reactions - but i've made bad experiences during implementation and wanted to keep this binding as stable as possible.

[tscholand]

@soenkekueper I installed the addon with your latest changes and it seems to work for me. The only things, that are still off:

• DRM state: It seems in fact to be 0 = DRM0, 1= DRM1 and so on (which makes sense, but is documented and mapped otherwise). Somehow, I did not notice this before.
• The Power Factor is 100.0 for me, when I guess it should be 1.0. But I don't really know, what that is, so I will not use the value anyway.

But even without fixing that, I would like to see this merged. :-)

[lsiepel]

Please also fix the conflict.

[soenkekueper]

@soenkekueper I installed the addon with your latest changes and it seems to work for me. The only things, that are still off:

• DRM state: It seems in fact to be 0 = DRM0, 1= DRM1 and so on (which makes sense, but is documented and mapped otherwise). Somehow, I did not notice this before.
• The Power Factor is 100.0 for me, when I guess it should be 1.0. But I don't really know, what that is, so I will not use the value anyway.

But even without fixing that, I would like to see this merged. :-)

Hey,

the power factor is explained here:
https://ratedpower.com/glossary/power-factor/
TL;DR: An indication how efficient the system works.

The register mapping was correct, but the channel type was declared as Number:Dimensionless, so the value will be displayed as percentage. Just change the item to be number, and the value will be displayed correct. I've pushed a commit that changes the default value within thing-types.xml for this.

DRM value seems to be valid, as i've no device attached the value is unkown too.

[soenkekueper]

Please also fix the conflict.

I've just rebased my branch upon the latest upstream/main branch without conflicts or problems.

@tscholand May you rebase your branch (and with that this PR) upon my branch so this PR is up-to-date?

[tscholand]

So, I rebased the PR and it now contains @soenkekueper's commits. However, it says, there is still one requested change. I'm not sure, what that change is...

[lsiepel]

There are two burried comments left from me:
https://github.com/openhab/openhab-addons/pull/18364/files#r1986256981
#18364 (comment)

There are also some copilot review comments.

Please comment on each one so we can finish up this PR.

[tscholand]

I commented on the requested changes and resolved them, but it is still showing 1 requested change...

[lsiepel]

Besides the copilot comments, i found some other places to look at. Otherwise LGTM

It shows that there are requested changes, because a maintainer (or copilot) left a review and needs to mark it as approved.

[lsiepel]

Sorry, i asked this before and it somehow got lost in the conversation, but as the existing thing types are changed, upgrade instructions are expected.
For reference: https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types

You can also check a binding like plugwiseha to see this in action.