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

[ferroamp] Binding for Ferroamp 20241010 #17536

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

basse04
Copy link
Contributor

@basse04 basse04 commented Oct 10, 2024

This is the binding for local connection to the Ferroamp Energy System

This is the binding for local connection to the Ferroamp Energy System

Signed-off-by: Örjan Backsell <orjan.backsell@gmail.com>
@basse04 basse04 requested a review from a team as a code owner October 10, 2024 10:17
Changes done in FerroampHandlerFactory.java

Signed-off-by: Örjan Backsell <orjan.backsell@gmail.com>
@lsiepel lsiepel added enhancement An enhancement or new feature for an existing add-on new binding If someone has started to work on a binding. For a new binding PR. and removed enhancement An enhancement or new feature for an existing add-on labels Oct 10, 2024
@basse04
Copy link
Contributor Author

basse04 commented Oct 10, 2024

Hi @lsiepel ,
I have had an system-disc crach, mine SSD got full without telling me before it went totally out of order. So I had to install an entire new OS ( Linux Mint 21.3) and everything else I need. So I thought that the best then was to also create a new branch ( ferroamp_v4).
There was initially problems with Maven, I had an old version installed that mvn locally liked but not the checks as above, this is now solved.
And then a question regarded to that, mvn locally complained and said I shold use Maven 3.8.1, I have now installed Maven 3.8.8 and it seems to work, do you think I should use any later version or will this one be fine?
BR Basse

Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

As it had some passaes before i keep this short. Left comments mainly looking at the readme. Some suggestions need to be adpated on multiple places. The binding is improving, so good to see progress.

bundles/org.openhab.binding.ferroamp/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ferroamp/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ferroamp/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ferroamp/README.md Outdated Show resolved Hide resolved
| userName | text | Username to access the device | N/A | yes | no |
| password | text | Password to access the device | N/A | yes | no |
| hasBattery | boolean | Is there a battery connected or not? | N/A | no | yes |
| ssoS0 | boolean | Is there a first SSO unit connected? | N/A | no | no |
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a sso ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sso is the connection part between the Pv-panels and the Energy hub. Solar String Optimizer.

Copy link
Contributor

@lsiepel lsiepel Oct 10, 2024

Choose a reason for hiding this comment

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

In that case change the description, so it is more clear (applies to all four):

Is there a first Solar String Optimizer (SSO) unit connected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, see the new text. This is complicated for me as not English native speaker. I use Google translate sometimes but I can see there shortcomings in the reverse translations.

bundles/org.openhab.binding.ferroamp/README.md Outdated Show resolved Hide resolved
bundles/org.openhab.binding.ferroamp/README.md Outdated Show resolved Hide resolved
## Thing

```java
Thing ferroamp:energyhub:myenergyhub [ hostName="energyhub-ip", userName="myUserName", password="myPassword", hasBattery=false ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you extend the example with other Things

| eso-battery-energy-produced | Number:Energy | R | Eso Battery Energy Produced | Total energy produced by ESO, i.e total energy charged |
| eso-battery-energy-consumed | Number:Energy | R | Eso Battery Energy Consumed | Total energy consumed by ESO, i.e total energy discharged |
| eso-soc | Number:Dimensionless | R | Eso State of Charge | State of Charge for ESO |
| eso-relay-status | String | R | Eso Relay Status | 0 = relay closed, 1 = relay open |
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change this to a Contact itemtype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -484,12 +480,10 @@
<module>org.openhab.voice.watsonstt</module>
<module>org.openhab.voice.whisperstt</module>
</modules>

Copy link
Contributor

Choose a reason for hiding this comment

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

these are unexpected. Only the change on line 145 is expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, don't know, but I can see them also in mine fork and in openhab/openhab-addons

Copy link
Contributor

Choose a reason for hiding this comment

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

I do know and they are unexpected :-)
Please revert all changes and only change the line 145. You can copy paste the contents of:
https://raw.githubusercontent.com/openhab/openhab-addons/refs/heads/main/bundles/pom.xml

Changes done in README.md, 20241010

Örjan Backsell <orjan.backsell@gmail.com>
| ssoS0 | boolean | Is there a first SSO unit connected? | N/A | no | no |
| ssoS1 | boolean | Is there a second SSO unit connected? | N/A | no | no |
| ssoS2 | boolean | Is there a third SSO unit connected? | N/A | no | no |
| ssoS3 | boolean | Is there a fourth SSO unit connected? | N/A | no | no |
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a system limit to 4 SSO's ? Did you consider to model this parameter as a single number: 0-9 representing the amount of SSO's this way it would be more dynamic?


The following configuration parameters are available.

| Name | Type | Description | Default | Required | Advanced |
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the thing-types.xml the eso and esm parameters are missing.

Would be nice if these setting scould be auto detected by the binding instead of manually configuring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in README.
I will think about auto detect.

xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:thing="https://openhab.org/schemas/thing-description/v1.0.0"
xsi:schemaLocation="https://openhab.org/schemas/thing-description/v1.0.0 https://openhab.org/schemas/thing-description-1.0.0.xsd">
<thing-type id="energyhub">
Copy link
Contributor

Choose a reason for hiding this comment

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

Both ferroamp.xml and thing-type.xml have the same energyhub thing id. Please remove one.

Done changes in EhubParameters.java, FerroampBindingConstants.java,
FerroampChannelConfiguration.java, ferroamp.xml, README.md

Signed-off-by: Örjan Backsell <orjan.backsell@gmail.com>
Changes done in:
EsmParameters.java
EsoParameters.java
ferroamp.xml
FerroampBindingConstants.java
FerroampChannelConfiguration.java
README.md
SsoParameters.java

Signed-off-by: Örjan Backsell <orjan.backsell@gmail.com>
Changes done in README.md

Signed-off-by: Örjan Backsell <orjan.backsell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants