[solax] Add support for Solax X3MIC / G2 inverter and workmode channel #16248

realthk · 2024-01-09T18:01:11Z

Support for my Solax X3MIC / G2 inverter, and include workmode which is useful if the inverter cannot work continuously (for example our area very often has over-voltage problem)

…xisting X1, X3 also Signed-off-by: Henrik Tóth <realthk@gmail.com>

theater · 2024-01-10T08:00:45Z

Hi,
Thanks for the contribution!
I'll have a review when I have some free time as an initial creator of the binding.

Unfortunately additional review maybe needed by some of the openhab-addon maintainers because I don't have a merge permissions.

Cheers,
Konstantin

openhab-bot · 2024-01-10T10:08:35Z

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

https://community.openhab.org/t/solax-local-direct-and-cloud-connect-binding/146326/97

theater

Hi.
Things look pretty much OK to me. I added some comments.
What is missing is an update instructions for the added channels.
See how it's done in the local_connect_inverter_type_update.xml
I don't request changes as I'm not an official reviewer so I added only a comments :)

Cheers,
Konstantin

...ab.binding.solax/src/main/java/org/openhab/binding/solax/internal/SolaxBindingConstants.java

....binding.solax/src/main/java/org/openhab/binding/solax/internal/SolaxLocalAccessHandler.java

...enhab.binding.solax/src/main/java/org/openhab/binding/solax/internal/model/InverterData.java

bundles/org.openhab.binding.solax/src/main/resources/OH-INF/thing/channel_types.xml

bundles/org.openhab.binding.solax/src/main/resources/OH-INF/thing/localConnectInverter.xml

realthk · 2024-01-15T23:05:03Z

Thanks Konstantin!
Sorry for the late reply, I'm quite busy with my daily job now, and have to admit, OpenHab is not my main system, only trying it for curiosity. I have my Solax X3MicG2 inverter integrated into Home Assistant by transforming data fetched by REST with Jinja expressions in simple template sensors.
Also, besides OpenHAB is new for me, I have no experience with Java at all, just duplicated an original class for other inverter, and modified where and how it seemed logical until it was working fine on my system. So this is the explanation why I used Units.ONE, and you're perfectly right, I also felt it fishy, but had no better idea for passing a unit-less state number. Same goes for enum and translation, I agree completely, just did not know how to do better.
As for the two temperature sensors, I could not find any documentation, so yes, that is the reason for just using numbers. From my experience, temp1 seems to be the more useful, covering a greater range, so that is the one I'm using, displaying on my dashboard. But of course, it could be quite different with another Solax model...
Here is a recent day, temp2 did not even start below zero (even though it was cold in the morning, temp1 was about right), and both fall back to 0 at once when the inverter does not work:

theater · 2024-01-16T07:21:57Z

For a non-Java developer you did really well. :) I guess some programming experience you still have :)

theater · 2024-01-16T07:35:56Z

Btw if you don't have the capacity to do the requested changes, maybe you could give me permissions for this branch and I can do them for you. I expect that the reviewer will also request changes but it's up to you...

realthk · 2024-01-16T22:16:44Z

Thanks, I’d appreciate that!
(yes, I mostly work on PHP projects, and as a hobby in Python, since Home Assistant)

Signed-off-by: Konstantin Polihronov <polychronov@gmail.com>

theater · 2024-01-19T16:42:43Z

@realthk I've added two commits with the refactoring I was talking about and also added the update instructions.
Before I start I have also created a branch with the same name, ending with _backup, so you can go anytime to your original changes.
Would appreciate if you could test my changes whenever is possible for you.

realthk · 2024-01-20T08:48:55Z

@theater Just did a rebuild on your changes, and it works fine, thank you very much!

theater · 2024-01-20T12:41:00Z

OK. Great then the PR is ready for a review. Please tag me if someone requests some changes, so I can have a look and implement them...

lsiepel

About the two channels inverter-workmode and inverter-workmode-code what is the benefit of seperating these? They seem to be allways in sync.
I guess it would be fine to drop the non-code variant and supply a option list with value/label pair: https://www.openhab.org/docs/developer/addons/config-xml.html#xml-structure-for-configuration-descriptions

bundles/org.openhab.binding.solax/src/main/resources/OH-INF/thing/localConnectInverter.xml

bundles/org.openhab.binding.solax/src/main/resources/OH-INF/i18n/solax.properties

....binding.solax/src/main/java/org/openhab/binding/solax/internal/SolaxLocalAccessHandler.java

Signed-off-by: Konstantin Polihronov <polychronov@gmail.com>

realthk · 2024-01-20T15:13:51Z

The reason why I thought it might be useful to have a text sensor, if not all Solax inverters use the same workmode codes for the same status, then it is possible to translate these.
But after googling (in this not so well documented subject) it seems the only difference is how many codes exists on a certain model but the most important first few statuses seem to share the same code for all inverter models, so the code might be enough, indeed.

theater · 2024-01-20T15:30:42Z

The reason why I thought it might be useful to have a text sensor, if not all Solax inverters use the same workmode codes for the same status, then it is possible to translate these. But after googling (in this not so well documented subject) it seems the only difference is how many codes exists on a certain model but the most important first few statuses seem to share the same code for all inverter models, so the code might be enough, indeed.

Actually I have added all the statuses that I know about (from the cloud API document). Hopefully they're documented well enough...

Signed-off-by: Konstantin Polihronov <polychronov@gmail.com>

theater · 2024-01-29T16:23:04Z

@jlaur, when you have the capacity and your schedule permits it, could we proceed as I think I have covered most if not all the concerns.
Not trying to push you or anything, just want to make sure that you're not awaiting some changes from my end...

Cheers,
K.

jlaur · 2024-02-01T19:23:52Z

@jlaur, when you have the capacity and your schedule permits it, could we proceed as I think I have covered most if not all the concerns. Not trying to push you or anything, just want to make sure that you're not awaiting some changes from my end...

I assume you mean @lsiepel? 🙂

theater · 2024-02-02T07:21:32Z

@jlaur, when you have the capacity and your schedule permits it, could we proceed as I think I have covered most if not all the concerns. Not trying to push you or anything, just want to make sure that you're not awaiting some changes from my end...

I assume you mean @lsiepel? 🙂

Apologies, mate! Not sure why I messed that up. Excuse me.

theater · 2024-02-08T06:55:49Z

@lsiepel could we continue the review when you have some time?

lsiepel · 2024-02-08T22:07:26Z

@lsiepel could we continue the review when you have some time?

Sure! Hopefully this weekend, might be later. Have some busy days.

theater · 2024-02-09T07:28:41Z

I travel for the weekend so I will be able to react around Sunday. No worries. No rush. Just wanted to check that it's not forgotten :)

…

On Fri, Feb 9, 2024 at 12:07 AM lsiepel ***@***.***> wrote: @lsiepel <https://github.com/lsiepel> could we continue the review when you have some time? Sure! Hopefully this weekend, might be later. Have some busy days. — Reply to this email directly, view it on GitHub <#16248 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB7Q3LTAIABNBZGZZE64WCLYSVECVAVCNFSM6AAAAABBTQKUWSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZVGAYDSMZWHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lsiepel

Thanks, LGTM

openhab#16248) * Support for Solax X3MIC / G2 inverter, and workmode channel for the existing X1, X3 also * Add update instructions and make raise the target version to 1 * Refactor the workmode to be enum instead of string constants Signed-off-by: Henrik Tóth <realthk@gmail.com> Signed-off-by: Konstantin Polihronov <polychronov@gmail.com> Signed-off-by: Leo Siepel <leosiepel@gmail.com> Signed-off-by: Jørgen Austvik <jaustvik@acm.org>

Support for Solax X3MIC / G2 inverter, and workmode channel for the e…

5e707a4

…xisting X1, X3 also Signed-off-by: Henrik Tóth <realthk@gmail.com>

realthk requested a review from theater as a code owner January 9, 2024 18:01

lsiepel changed the title ~~Support for Solax X3MIC / G2 inverter, and workmode channel for the e…~~ [solax] Add support for Solax X3MIC / G2 inverter and workmode channel Jan 9, 2024

lsiepel added the enhancement An enhancement or new feature for an existing add-on label Jan 9, 2024

theater reviewed Jan 10, 2024

View reviewed changes

theater requested a review from jlaur January 15, 2024 16:09

theater added 2 commits January 17, 2024 19:00

Add update instructions and make raise the target version to 1

fbf2a7a

Signed-off-by: Konstantin Polihronov <polychronov@gmail.com>

Refactor the workmode to be enum instead of string constants

b0aa8ae

Signed-off-by: Konstantin Polihronov <polychronov@gmail.com>

lsiepel requested changes Jan 20, 2024

View reviewed changes

Apply I18N plugin

5b504e2

Signed-off-by: Konstantin Polihronov <polychronov@gmail.com>

Remove workmode code channel, add options for workmode string values

43ad418

Signed-off-by: Konstantin Polihronov <polychronov@gmail.com>

theater mentioned this pull request Jan 22, 2024

[solax] Add cloud connection support #16124

Merged

Fix i18n

ccaa779

Signed-off-by: Leo Siepel <leosiepel@gmail.com>

lsiepel approved these changes Feb 10, 2024

View reviewed changes

lsiepel merged commit d1caa31 into openhab:main Feb 10, 2024
3 checks passed

lsiepel added this to the 4.2 milestone Feb 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[solax] Add support for Solax X3MIC / G2 inverter and workmode channel #16248

[solax] Add support for Solax X3MIC / G2 inverter and workmode channel #16248

realthk commented Jan 9, 2024

theater commented Jan 10, 2024

openhab-bot commented Jan 10, 2024

theater left a comment

realthk commented Jan 15, 2024

theater commented Jan 16, 2024

theater commented Jan 16, 2024

realthk commented Jan 16, 2024

theater commented Jan 19, 2024

realthk commented Jan 20, 2024

theater commented Jan 20, 2024

lsiepel left a comment

realthk commented Jan 20, 2024

theater commented Jan 20, 2024

theater commented Jan 29, 2024

jlaur commented Feb 1, 2024

theater commented Feb 2, 2024

theater commented Feb 8, 2024

lsiepel commented Feb 8, 2024

theater commented Feb 9, 2024 via email

lsiepel left a comment

[solax] Add support for Solax X3MIC / G2 inverter and workmode channel #16248

[solax] Add support for Solax X3MIC / G2 inverter and workmode channel #16248

Conversation

realthk commented Jan 9, 2024

theater commented Jan 10, 2024

openhab-bot commented Jan 10, 2024

theater left a comment

Choose a reason for hiding this comment

realthk commented Jan 15, 2024

theater commented Jan 16, 2024

theater commented Jan 16, 2024

realthk commented Jan 16, 2024

theater commented Jan 19, 2024

realthk commented Jan 20, 2024

theater commented Jan 20, 2024

lsiepel left a comment

Choose a reason for hiding this comment

realthk commented Jan 20, 2024

theater commented Jan 20, 2024

theater commented Jan 29, 2024

jlaur commented Feb 1, 2024

theater commented Feb 2, 2024

theater commented Feb 8, 2024

lsiepel commented Feb 8, 2024

theater commented Feb 9, 2024 via email

lsiepel left a comment

Choose a reason for hiding this comment