[solax] Initial contribution

[theater]

This is a OpenHAB binding for a Solax Wi-fi modules that have the option to be connected directly via HTTP (firmware version 3.x+).

Currently the Solax cloud services provide an update on every 5 minutes (sometimes even more) and they also happen to be down sometimes, which makes it hard to automate the decision making by OpenHAB if we have more complex rules.
For example: I want to charge my car if the battery of my inverter is 90%+ and my overall consumption is only from the solar power.
The binding retrieves structured data from the wifi module, parses it and pushes it in to a inverter thing where each channel represents a specific information (inverter output power, voltage, PV1 power, etc)

More about the technical review of the PR:

The HTTP connectivity part is in the connectivity package.
There we have a "connector" which uses the common jetty client from the openhab distribution, connects to the wi-fi module and retrieves the raw data from the module. It is used directly from the bridge handler to connect and retrieve the necessary data.
Then we create the raw data bean which implements the InverterData interface and the methods of the interface directly retrieve data from the raw data bean and provide it as an easy to use Java code. (this is arguable from architecture perspective if it's better bean to have only the raw data and to use a separate parser that returns a DTO but from simplicity point of view it looks better to me this way currently)

The methods of the InverterData interface are used as getters to make the data transformation and set the channel values of the inverter thing in openHAB thing handler by using the InverterDataUpdateListener interface which is called from the bridge.
The InverterData interface is needed because at later point in time I will implement a second bridge which can retrieve a different raw data from the cloud service which could be useful for people who do not own/did not upgraded to firmware version 3.x of the Solax wi-fi module and only support cloud connection. The idea is not to care what bridge do we have but to have a common way to retrieve practically the same data which is structured in a separate way and set the Inverter thing channels.

A marketplace topic: https://community.openhab.org/t/solax-binding/146324

Cheers,
Konstantin

[openhab-bot]

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

https://community.openhab.org/t/solax-binding/146324/1

[openhab-bot]

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

https://community.openhab.org/t/warning-during-build-and-error-in-the-pr-build/147406/1

[wborn]

Thanks for the PR!

You can probably fix the builds by adding a dependency for your add-on to this POM file:

bom/openhab-addons/pom.xml

[theater]

Thanks for the PR!

You can probably fix the builds by adding a dependency for your add-on to this POM file:

bom/openhab-addons/pom.xml

Thanks for the suggestion. That indeed fixed the build issue. I wonder if you could also do a review of the binding. At the current stage I think we can merge it as initial version of the binding with local connection only.
Next improvement could be to create additional bridge for the cloud integration option using the same model...

[jlaur]

Thank you for the contribution! It looks good - I have added some feedback from my first review round. 🙂

[theater]

Hi,
The warning is handled externally as we never pass null to the parser, therefore null cannot be returned.
Fixed the prio3 warnings where possible. In the raw data bean the response from the API is having a capital letters for some fields and that's the reason for them not to be based on the naming convention.
Probably I can create a custom mapper for this, but I don't think investment worths it...
Please let me know of your thoughts.

Cheers,
K.

[theater]

In your case I would probably keep the local API inverter thing, and then dynamically remove the channels that shouldn't exist for a particular inverter type/model.

How can I create / remove channels dynamically? I think that could be very well the solution (instead of creating separate handlers)...
P.S. Maybe I should move this conversation to the communities if you participate there as well...

[jlaur]

How can I create / remove channels dynamically?

You would need to do either one or the other. For this scenario it seems simplest to remove a few statically defined channels:

List<Channel> channelsToRemove = new ArrayList<>();
updateThing(editThing().withoutChannels(channelsToRemove).build());

See for example:

openhab-addons/bundles/org.openhab.binding.hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/handler/HDPowerViewShadeHandler.java

Lines 604 to 650 in 5c6fabb

	/**
	* If the given channel exists in the thing, but is NOT required in the thing, then add it to a list of channels to
	* be removed. Or if the channel does NOT exist in the thing, but is required in the thing, then log a warning.
	*
	* @param removeList the list of channels to be removed from the thing.
	* @param channelId the id of the channel to be (eventually) removed.
	* @param channelRequired true if the thing requires this channel.
	*/
	private void removeListProcessChannel(List<Channel> removeList, String channelId, boolean channelRequired) {
	Channel channel = thing.getChannel(channelId);
	if (!channelRequired && channel != null) {
	removeList.add(channel);
	} else if (channelRequired && channel == null) {
	logger.warn("Shade {} does not have a '{}' channel => please reinitialize the thing", shadeId, channelId);
	}
	}
	/**
	* Remove previously statically created channels if the shade does not support them or they are not relevant.
	*
	* @param capabilities the capabilities of the shade.
	* @param shade the shade data.
	*/
	private void updateDynamicChannels(Capabilities capabilities, ShadeData shade) {
	List<Channel> removeList = new ArrayList<>();
	removeListProcessChannel(removeList, CHANNEL_SHADE_POSITION, capabilities.supportsPrimary());
	removeListProcessChannel(removeList, CHANNEL_SHADE_SECONDARY_POSITION,
	capabilities.supportsSecondary() || capabilities.supportsSecondaryOverlapped());
	removeListProcessChannel(removeList, CHANNEL_SHADE_VANE,
	capabilities.supportsTiltAnywhere() || capabilities.supportsTiltOnClosed());
	boolean batteryChannelsRequired = shade.getBatteryKind() != BatteryKind.HARDWIRED_POWER_SUPPLY;
	removeListProcessChannel(removeList, CHANNEL_SHADE_BATTERY_LEVEL, batteryChannelsRequired);
	removeListProcessChannel(removeList, CHANNEL_SHADE_LOW_BATTERY, batteryChannelsRequired);
	if (!removeList.isEmpty()) {
	if (logger.isDebugEnabled()) {
	StringJoiner joiner = new StringJoiner(", ");
	removeList.forEach(c -> joiner.add(c.getUID().getId()));
	logger.debug("Removing unsupported channels for {}: {}", shadeId, joiner.toString());
	}
	updateThing(editThing().withoutChannels(removeList).build());
	}
	}

In other more dynamic scenarios the best (or only) option is to create all relevant channels dynamically.

[jlaur]

See latest posted comments.

[theater]

Hi,
I'm not sure what has changed recently... checked the commits and couldn't find any meaningful reason for it but the binding did not activate on the Karaf for the following error:
`2023-08-21 11:54:48.355 [WARN ] [org.apache.felix.fileinstall ] - Error while starting bundle: file:/openhab/addons/org.openhab.binding.solax-4.1.0-SNAPSHOT.jar
org.osgi.framework.BundleException: Could not resolve module: org.openhab.binding.solax [243]
Unresolved requirement: Import-Package: javax.measure; version="[2.2.0,3.0.0)"

    at org.eclipse.osgi.container.Module.start(Module.java:463) ~[org.eclipse.osgi-3.18.0.jar:?]
    at org.eclipse.osgi.internal.framework.EquinoxBundle.start(EquinoxBundle.java:445) ~[org.eclipse.osgi-3.18.0.jar:?]

`

I had to tackle it by adding the following dependency to the pom.xml:
<dependencies> <dependency> <groupId>javax.measure</groupId> <artifactId>unit-api</artifactId> <version>1.0</version> </dependency> </dependencies>

Do you know what could be the reason for this? I couldn't find any breaking changes in the recent commits... also tried to update to 4.0.2 my docker infrastructure but it's the same behaviour...

[theater]

Could it be related to this issue?
[WARNING] Some problems were encountered while building the effective model for org.openhab.addons.bundles:org.openhab.binding.solax:jar:4.1.0-SNAPSHOT
[WARNING] 'parent.relativePath' of POM org.openhab.addons:org.openhab.addons.reactor:4.1.0-SNAPSHOT (E:\git\openhab-addons\pom.xml) points at org.openhab.addons.bundles:org.openhab.addons.reactor.bundles instead of org.openhab:openhab-super-pom, please verify your project structure @ org.openhab.addons:org.openhab.addons.reactor:4.1.0-SNAPSHOT, E:\git\openhab-addons\pom.xml, line 6, column 11
[WARNING]
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING]
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.
[WARNING]

The other warnings during build that I have are these:
[WARNING] The POM for org.eclipse.orbit.bundles:net.i2p.crypto.eddsa:jar:0.3.0.v20220506-1020 is missing, no dependency information available
[WARNING] The POM for org.jupnp:org.jupnp:jar:2.7.1.OH1 is missing, no dependency information available
[WARNING] The POM for org.openhab:base-fixes:jar:1.0.0 is missing, no dependency information available

[theater]

OK. Seems that the units of measurement library has changed in 4.1 (or at least it's very similar to what I see written here: #10926)
Currently works with the added dependency, so if I provide the jar to users with 4.0 probably I have to keep it inside but it shouldn't be required for the 4.1 milestone builds.

[theater]

Hi,
the only leftover warning is the one for the NPE which I don't know how to resolve at the moment... Could you please check my comments in the thread about it?
Cheers,
K.

[jlaur]

the only leftover warning is the one for the NPE which I don't know how to resolve at the moment... Could you please check my comments in the thread about it?

Yes, I wanted to clone your branch and try to propose a fix yesterday, but unfortunately ended up not finding the time. I'll try to prioritize it in the evening today.

[theater]

Oh. OK. No worries. Take your time. We all have our other tasks...

…

On Wed, Aug 23, 2023 at 2:59 PM Jacob Laursen ***@***.***> wrote: the only leftover warning is the one for the NPE which I don't know how to resolve at the moment... Could you please check my comments in the thread about it? Yes, I wanted to clone your branch and try to propose a fix yesterday, but unfortunately ended up not finding the time. I'll try to prioritize it in the evening today. — Reply to this email directly, view it on GitHub <#14880 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB7Q3LUJMOKIUYZINS7T6JDXWXWDJANCNFSM6AAAAAAXLFSSFM> . You are receiving this because you were assigned.Message ID: ***@***.***>

[jlaur]

With these suggestions it compiles for me without any warnings or checkstyle issues.

[theater]

OK I confirmed your changes and tested them locally. All looks good except my Eclipse warning but that's fine...

[theater]

How can I create / remove channels dynamically?

You would need to do either one or the other. For this scenario it seems simplest to remove a few statically defined channels:

Before we complete the PR.... just to be sure that I understand the approach - your proposal is to add all the possible channels in one thingtype (inverter) and based on the inverter type I just dynamically remove the unnecessary channels in the handler, is my understanding right?

[jlaur]

Before we complete the PR.... just to be sure that I understand the approach - your proposal is to add all the possible channels in one thingtype (inverter) and based on the inverter type I just dynamically remove the unnecessary channels in the handler, is my understanding right?

Exactly. It might be a good idea to create an issue for the next development iteration. This can also be used for discussing such things. Just give me a go-ahead for merging.

[theater]

Yep. Please merge. I'll create a new PR once I'm ready with the option to support multiple inverters..
Thank you for all the support!

[jlaur]

Thank you for all the support!

You're welcome, and thanks again for your contribution. To not forget, please see https://www.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website