[growatt] Enhance support for SPF inverters

[andrewfg]

This PR enhances the support for SPF inverters as follows:

• Adjusts mapping between DTO fields and OH channels to avoid duplicates
• Adds missing channels for SPF inverters
• Adds a channel to show the time offset between the inverter clock and the OH system clock

Signed-off-by: AndrewFG software@whitebear.ch

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/growatt-solar-inverter-binding-4-0-0-4-2-0/150600/27

[andrewfg]

The Jar file for testing is here..

org.openhab.binding.growatt-4.3.0-SNAPSHOT.jar.zip

[jlaur]

Thanks! I have added a few comments after a quick read-through.

[jlaur]

Since new channels were added, update instructions are needed. Otherwise LGTM now.

[andrewfg]

Since new channels were added, update instructions are needed.

Hmm. I am not sure if that is necessary -- for the following reasons -- please advise.

• all new things are created with the full complement of all possible channels; then the unused channels are dynamically removed depending on the Json fields coming from the actual device.
• the previous set of channels was sufficient for all existing devices, so the added channels do not apply to such things
• the added channels only apply to the newly reported SPF device

[jlaur]

• the added channels only apply to the newly reported SPF device

So it didn't make sense at all to have a Thing for an SPF device before this PR, because it was not supported at all? In that case I agree.

But if you could already create the Thing for such device and use it just without the newly added channels, update instructions are needed, because the user will otherwise have to delete and recreate the Thing in order to have those new channels added.

[andrewfg]

@jlaur I did the update instructions. And as they contain labels and descriptions I also ran mvn i18n:generate-default-translations but was surprised that it did not generate any default texts for those labels and descriptions. So I am wondering if a) there is a bug in OH core, or b) OH core gets the texts from the original thing-type xml; in which case I wonder why it is necessary for the update instructions to have labels and descriptions in them. It is not related directly to this PR but WDYT?

[jlaur]

I did the update instructions. And as they contain labels and descriptions I also ran mvn i18n:generate-default-translations but was surprised that it did not generate any default texts for those labels and descriptions.

The update instructions don't support I18N directly, so for overridden labels and descriptions on channel level, there are only two possibilities:

• Include them in update instructions: They will be in English, even if translated.
• Do not include them in update instructions: Channel type texts will be used (rather than overriden channel texts), but they will be translated.

See openhab/openhab-core#3569 for further details.

[andrewfg]

The update instructions don't support I18N

Understood. In this PR I have included the English texts.

(However I am really surprised that nobody is working on a solution for adding i18n to the update instruction semantics in OH core).

[jlaur]

(However I am really surprised that nobody is working on a solution for adding i18n to the update instruction semantics in OH core).

The right solution is for the update to use the already provided channel labels/descriptions, like it does for channel types, but it seems it's hard to do. There is also openhab/openhab-core#3660 related to this topic.

[andrewfg]

The right solution is for the update to use the already provided channel labels/descriptions

Perhaps I am missing something .. but it seems to me that in the doChannel() method (see link below) it should be possible to query the registry and get the channel label and description from the old channel, and (if no 'label' or 'description' is provided in the update instruction) apply those to the new channel. Or ??

https://github.com/openhab/openhab-core/blob/660102e3f93f928473b66f56f2955090dfa3c30e/bundles/org.openhab.core.thing/src/main/java/org/openhab/core/thing/internal/update/UpdateChannelInstructionImpl.java#L106

[jlaur]

but it seems to me that in the doChannel() method (see link below) it should be possible to query the registry and get the channel label and description from the old channel, and (if no 'label' or 'description' is provided in the update instruction) apply those to the new channel. Or ??

Can you put that comment in the linked issue?

[jlaur]

Can you put that comment in the linked issue?

I meant to put that comment into openhab/openhab-core#3569, so that the relevant people will see it.

[jlaur]

LGTM, but commented a few places as you may have misunderstood my last comment? Also looks a bit unusual to document date of changes in the code. But really minor, so just consider, I will merge anyway.

[andrewfg]

Can you put that comment in the linked issue?

Done.