[argoclima] Initial contribution #15481

mbronk · 2023-08-22T23:53:10Z

This PR adds a new binding for ArgoClima HVAC devices.

Please refer to the included README.md for details of this binding's objectives.

Notably, the Connection modes section illustrates various modes this binding operates in.

Exemplary supported device:

Argo Ulisse 13 DCI Eco (WiFi model): User Manual | Installation Manual | WiFi Guide

Highlights (for reviewers)

The most notable items I suggest to focus on during the review are the binding modes which spawn a separate HTTP server thread(s), and a custom HTTP client
- Rationale: see README.md
- Notable classes: RemoteArgoApiServerStub.java | PassthroughHttpClient.java
Since it started as a DYI project (with no initial plans to upstream), I did not start any separate thread in the OpenHab community.
- If this is required for a new binding to get landed and the review process kicked off, I'd appreciate guidance.

Testing / development status

For what it's worth, I've been using this binding myself for a while now, and haven't noticed any anomalies, so as far as I subjectively perceive it, I consider it quite stable and fairly complete. No extensive multi-user large-scale testing has been performed though!
Outside of addressing any feedback stemming from review, I'm not currently planning any major functionality extensions. A.k.a. a 1.0 RC1 :)

Thank you in advance for all your feedback and guidance!

lsiepel

First pass, only looked at the readme and metadata, will try to look at the other files when i have some more time.

Most looks good, mainly some textual comments and some questions.

bundles/org.openhab.binding.argoclima/README.md

bundles/org.openhab.binding.argoclima/src/main/resources/OH-INF/thing/thing-types.xml

bundles/org.openhab.binding.argoclima/.gitignore

lsiepel

Allmost half way, my time is up for now, didn't want to hold it back.

Impressed about the extensive documentation. It is a bit overwhelming, but it helps.

bundles/org.openhab.binding.argoclima/scripts/download_ota_fw_files.py

...clima/src/main/java/org/openhab/binding/argoclima/internal/ArgoClimaTranslationProvider.java

...n/java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationLocal.java

.../java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationRemote.java

...in/java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationBase.java

...ava/org/openhab/binding/argoclima/internal/configuration/IScheduleConfigurationProvider.java

lsiepel

Finished the last part. Did leave some comments, but overall this seems a very solid (but complex) contribution.

Consider my review as an extra set of eyes to speed up the merge proces. I'm no maintainer, so there might be more feedback, or corrections to mine (don;t hope so).

...in/java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationBase.java

...n/java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationLocal.java

...in/java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationBase.java

...main/java/org/openhab/binding/argoclima/internal/device/api/protocol/elements/TimeParam.java

...n/java/org/openhab/binding/argoclima/internal/device/api/protocol/elements/WeekdayParam.java

.../java/org/openhab/binding/argoclima/internal/device/passthrough/RemoteArgoApiServerStub.java

...lima/src/main/java/org/openhab/binding/argoclima/internal/handler/ArgoClimaHandlerLocal.java

lsiepel · 2023-09-04T17:09:51Z

Hey @lsiepel, I know you're likely very busy with other things, but could you do me a favor and indicate (tentatively) when 🕐 would you be able to find a few minutes to take a final sweep through the latest comments and update their status? 🙏

Reason I'm asking is - while I tried to apply all the comments - I'm not 100% sure if I addressed everything, and would like to complete this phase, before I'm out for a short vacation next week (so that I leave it in a mergeable state). Let me know if it is realistic for you to take a look this week. Thank you in advance!

Give me a week max

lsiepel

LGTM Thank you for this binding.

It might take some time, but a maintainer will pick this up and review / merge this eventually.

mbronk · 2023-11-10T08:40:01Z

Hey @jlaur - sorry for bothering you... Since you're had some fly-by interactions with this PR (and are a maintainer), would you happen to know what might be the typical ETA for a new binding review novadays?
I do absolutely realize it's a community effort, and happy to wait as long as required - just making sure this PR didn't fall through the cracks and is on someone's radar still.

[joke] On that occasion, to lighten the mood... any chance to get it merged before christmas 😀 ? [ref: Duke Nukem] :) [/joke]

jlaur · 2023-11-11T10:32:53Z

would you happen to know what might be the typical ETA for a new binding review novadays?

My algorithm for calculating that is broken. 😄 You can have a look at recent merged PR's with label "new binding" to get an impression. Unfortunately our capacity is a bit low, but eventually someone will pick up your PR and start a review. A few things in general that can have an impact on the time for completing a review of a new binding:

Size: I created a PR of similar size on February 9th: [energidataservice] Initial contribution #14376. It was merged on July 3rd, so ~5 months. 10k lines is a rather large PR in comparison with other PR's.
Quality: Will not do any comparisons or references here, but obviously a PR requiring many (perhaps major) comments to be resolved will take longer than a very clean and high quality PR.
Speed of addressing comments: Once a review gets started, the momentum can be kept by addressing comments quickly. It will be easier for a reviewer to check resolutions and provide new feedback when still fresh in mind compared to picking up a PR with months old comments. This is not to be underestimated.
Compliance: If there are many disagreements between the contributor and maintainer, this can prolong the process. What can help here is being reasonable and keeping a good tone, since reviewers are also human. 😉 This makes it easier to reach good compromises. Not everything can be discussed though since there are some guidelines and openHAB architecture/ways of doing things that must be followed. It only happens very rarely that a PR is rejected because of disagreements, usually we reach an agreement and the PR is merged.

Personally I probably won't be able to pick it up for the time being since I'm involved in some other ongoing rather large PR's, and I prefer to not have too many open at the same time because I then risk becoming the bottleneck and momentum is lost.

mbronk · 2023-11-11T15:06:48Z

Thanks for such a thorough reply! Much more info than I had hoped for, appreciate it!
I was actually looking at some typical PR processing times myself, but failed to filter by new binding, so drew wrong conclusions... My bad!

Since 5+ months is not unprecedented, I'll stop getting worried for a while (till err... christmas? 😁 ).

And as for:

10k lines is a rather large PR

Yeah, I know. But also... java 😉

holgerfriedrich

Thanks for the contribution!
This is not a full review, just a few comments.

In addition to my comments, the following things need improvement:

javadoc is currently broken, at least the errors need to be fixed (mvn javadoc:javadoc|grep error:) - mainly & and < to be escaped.....

SAT is fine, warnings are fine, i18n seems ok

....argoclima/src/main/java/org/openhab/binding/argoclima/internal/ArgoClimaConfigProvider.java

...n/java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationLocal.java

...ava/org/openhab/binding/argoclima/internal/configuration/IScheduleConfigurationProvider.java

...main/java/org/openhab/binding/argoclima/internal/device/api/protocol/elements/TimeParam.java

mbronk · 2023-12-22T13:30:48Z

@holgerfriedrich Thanks fot the review. Fixed the javadocs errors (sorry for the miss!). Do you need me to address all javadoc warnings as well?

BTW. I had to extend pom.xml to add @implNote and @apiNote support. Is this typical/expected?
(I was thinking the plugin should inherit configuration from top-level, but it didn't work for me... not sure if it is my setup-specific or if I made an error somewhere...)

holgerfriedrich · 2023-12-22T13:40:35Z

@holgerfriedrich Thanks fot the review. Fixed the javadocs errors (sorry for the miss!). Do you need me to address all javadoc warnings as well?

No, warnings are ok. I have not checked, but most of the warnings should be undocumented elements. This is expected.
I just wanted to make sure that the javadoc build does not fail.

BTW. I had to extend pom.xml to add @implNote and @apiNote support. Is this typical/expected? (I was thinking the plugin should inherit configuration from top-level, but it didn't work for me... not sure if it is my setup-specific or if I made an error somewhere...)

If I got it correctly, you branched quite a wile ago. Those tags are now defined in pom.xml on top level (since ~2 months). Before, we had it on binding level.
Just fetch upstream/main and rebase.

mbronk · 2023-12-22T13:51:07Z

BTW. I had to extend pom.xml to add @implNote and @apiNote support. Is this typical/expected?

If I got it correctly, you branched quite a wile ago. Those tags are now defined in pom.xml on top level (since ~2 months). Before, we had it on binding level. Just fetch upstream/main and rebase.

Yup, that did it. I did a quick check and saw it on GH main, but it didn't occur to me it might not bee there for ages, so didn't check on my local copy... Thx for the pointer!

bundles/org.openhab.binding.argoclima/pom.xml

holgerfriedrich

Remark: I did not carefully review this PR and cannot judge technical details

My comments have been addressed.
Warnings, SAT, javadoc, i18n -> 👍 from my side

lsiepel

I think these are my last comments before a merge. I must say this is quite a complex binding, but it looks very solid.
Personally i prefer not to use Optionals and type inference (var), but there is no rule that forbids them.

bundles/org.openhab.binding.argoclima/README.md

mbronk · 2024-03-02T14:16:53Z

FYI - squashed all the review comments as this PR is around for >6 months already and the growing list of miscellanea (like CP header, snapshot vsn update etc.) started to become difficult for me to manage between branches/rebases etc.
Hope having them in a single commit is not an issue...

@lsiepel - Unless I missed sth, all your feedback above should be adressed already (thanks again for reviewing!), except one comment on the markdown table formatting. Please take a look and let me know. Thanks!

mbronk · 2024-03-24T09:32:48Z

@lsiepel - a friendly reminder that the changes are ready for re-review 😉

mbronk · 2024-04-28T11:42:24Z

Ping (+1 month with no activity)

lsiepel · 2024-04-28T11:47:51Z

Ping (+1 month with no activity)

Yes, have this on my todo list for the upcoming days.

lsiepel

Started another review pass. Struggling with this binding a bit. It is very creative and i love all the tinkering you have done to make it work. It is also complex in many ways. To be honest, i don't expect a large userbase and i'm concerned that this code is hard to maintain.

For instance, the configuration classes not only hold the configuration, but it also has logic, network related calls, dependency on i18n provider etc. We will need some passes to try to get this sorted.

bundles/org.openhab.binding.argoclima/README.md

.../java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationRemote.java

...n/java/org/openhab/binding/argoclima/internal/configuration/ArgoClimaConfigurationLocal.java

mbronk · 2024-05-11T19:25:40Z

Apologies for a slight delay and thank you once more @lsiepel for the time spent reviewing! Appreciate it.

This time around I am not going to wait before applying the feedback till one of the maintainers can weigh in.

@addon-maintainers: If at all possible, could you indicate if you find this merge'able at all (at a concept level), and (if yes) provide a full (even high-level) list of changes required? Otherwise, doing it part-by-part and granting/rescinding approvals is only slow-cooking the frog. I'd much more appreciate a "we won't approve it ever" than death by 1000 cuts and doing montlhly "stylistic" edits. Hope you understand.

@lsiepel , See, I'm a little bit concerned with this statement:

To be honest, i don't expect a large userbase and i'm concerned that this code is hard to maintain.

Esp. since there's probably truth to it. This has been in review for 9 months now, and I did my best applying previous (valid&valuable!) feedback as promptly as I could.
Note though, that this was mostly conformance to style/guidelines of the community and no core binding logic has not changed from it at all.
Don't get me wrong - I understand these are in for a reason, but if I'm doing all this work for it not to get merged at all, it's only creating busy work I'm not benefitting from.

I want to stress, I do very much appreciate your time spent to look through it! Reason I am pausing on the last round of feedback is not due to me not agreeing with it (well... I do consider some of them boiling just to personal style thus controversial, but overall agree w/ their spirit), rather my hopes of applying the updates and this making a difference are currently quite low.

lsiepel · 2024-05-11T22:27:10Z

@lsiepel , See, I'm a little bit concerned with this statement:

To be honest, i don't expect a large userbase and i'm concerned that this code is hard to maintain.

Esp. since there's probably truth to it. This has been in review for 9 months now, and I did my best applying previous (valid&valuable!) feedback as promptly as I could. Note though, that this was mostly conformance to style/guidelines of the community and no core binding logic has not changed from it at all. Don't get me wrong - I understand these are in for a reason, but if I'm doing all this work for it not to get merged at all, it's only creating busy work I'm not benefitting from.

I want to stress, I do very much appreciate your time spent to look through it! Reason I am pausing on the last round of feedback is not due to me not agreeing with it (well... I do consider some of them boiling just to personal style thus controversial, but overall agree w/ their spirit), rather my hopes of applying the updates and this making a difference are currently quite low.

Well i wouldn't do all this work either if i already know it is not going to work out and get merged. So yes, this can get to the point that it get merged, and i think we are not that far away. Since last christmass i became maintainer and can merge PR's, just to assure you that this is no random statement.
There are many reasons these reviews are partly. As this is not a typicall binding it takes some time to know the code, concerns, design etc, and we see that many PR's get abandoned if there is no quick follow up. So i try to iterate on a shorter cycle.

Hopefully you can address the comments, i don't expect high level changes, but i might ask another maintainer to have an extra set of eyes, and maybe i can also learn from that. :-)

mbronk · 2024-05-12T06:51:49Z

@lsiepel Thanks for the feedback and a ray of hope. And congrats on becoming a maintainer. 🎉 Well deserved!

There are many reasons these reviews are partly. (...) So i try to iterate on a shorter cycle.

I have no concerns w/ partials as a concept and do them routinely myself, but if the cycle length is monthly (vs. daily), it makes it very taxing for a side-project (esp. one I do not have a private CI/CD and full-coverage test harness set up for). What I'm saying is a code change cost me more time as a unit than the actual coding - esp. if my dev setup changes between the cycles (and it does 😁 )
Happy to do big/semantic changes separate (also better to review), but all the stylistic refactors... It will help me a lot to bundle in 1 commit.

Hopefully you can address the comments, i don't expect high level changes, but i might ask another maintainer to have an extra set of eyes(...)

Would you agree the remaining open comments are rather small and refactor-ish?
If you you'd want another pair of eyes, would it be possible for ask someone to weigh in before I change this round's things?
Something like a provisional approval "if XYZ were resolved, I'd consider it good to go myself".

I think we're in the place where it can be reviewed as-if the remaining open comments were resolved. And the reason I ask for it is above (doing the changes you request is ~trivial, preparing for it and testing on my local setup after... is orders of magnitude more time-consuming, so I'd like to limit # of cycles).
What do you think? Can it work?

lsiepel · 2024-05-12T08:44:28Z

As i have reviewed the full binding, any comments left open should be resolved. I hope @jlaur or @lolodomo are able to have a look at this binding and comment. If no new issues show up we can get this merged.

Signed-off-by: Mateusz Bronk <bronk.m+gh@gmail.com>

Contents: [argoclima] 1st round of review remarks (editorials) [argoclima] 2nd round of review remarks (config) showCleartextPasswords --> includeDeviceSidePasswordsInProperties (default: NEVER) [argoclima] i18n: Missing localization added Adds i18n for dynamic Channels and Thing statuses (incl. exceptions) [argoclima] 3rd round of review remarks [argoclima] 4th round of review remarks Renamed localDevicePort --> hvacListenPort Used instanceof pattern matching (where applicable) Demoted device communication logs to TRACE (non-review feedback) Added missing indirect command intercept in STUB mode [argoclima] Fixed typos in docs (README, diagrams) [argoclima] Post-review corrections: javadoc, etc. [argoclima] Bump version to 4.2.0-SNAPSHOT [argoclima] README.md minutiae [argoclima] Channels rename to lower-case-hyphen [argoclima] CP header update to 2024 [argoclima] Review remarks (password hashing extracted + minutiae) Signed-off-by: Mateusz Bronk <bronk.m+gh@gmail.com>

Signed-off-by: Mateusz Bronk <bronk.m+gh@gmail.com>

lsiepel

Thanks, LGTM.

I prefer another @openhab/add-ons-maintainers to have a second look. Not necessarily all files, the readme and the ThingHandlers will probably be enough to get a good understanding and provide some feedback.

mbronk requested a review from a team as a code owner August 22, 2023 23:53

mbronk force-pushed the mbronk/argoclima_binding_wip_OH4.1-SNAPSHOT-clean branch from a7a9dc8 to ed0a70f Compare August 22, 2023 23:58

mbronk mentioned this pull request Aug 23, 2023

[argoclima] ArgoClima Binding Logo openhab/openhab-docs#2125

Draft

jlaur added the new binding If someone has started to work on a binding. For a new binding PR. label Aug 23, 2023

mbronk force-pushed the mbronk/argoclima_binding_wip_OH4.1-SNAPSHOT-clean branch 3 times, most recently from b6f8a15 to e22b162 Compare August 24, 2023 18:54

lsiepel reviewed Aug 25, 2023

View reviewed changes

mbronk force-pushed the mbronk/argoclima_binding_wip_OH4.1-SNAPSHOT-clean branch from e22b162 to 7cc19e8 Compare August 25, 2023 20:05

This comment was marked as resolved.

Sign in to view

lsiepel requested changes Aug 27, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

lsiepel requested changes Aug 28, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

mbronk requested a review from lsiepel August 31, 2023 16:13

This comment was marked as outdated.

Sign in to view

lsiepel approved these changes Sep 10, 2023

View reviewed changes

mbronk requested a review from jlaur November 10, 2023 08:28

holgerfriedrich requested changes Dec 21, 2023

View reviewed changes

mbronk force-pushed the mbronk/argoclima_binding_wip_OH4.1-SNAPSHOT-clean branch from 8541027 to d56a032 Compare December 22, 2023 13:48

wborn reviewed Dec 23, 2023

View reviewed changes

bundles/org.openhab.binding.argoclima/pom.xml Outdated Show resolved Hide resolved

mbronk force-pushed the mbronk/argoclima_binding_wip_OH4.1-SNAPSHOT-clean branch from 23700ae to 5f02573 Compare December 23, 2023 10:27

lsiepel requested a review from holgerfriedrich December 29, 2023 15:32

holgerfriedrich approved these changes Dec 31, 2023

View reviewed changes

lsiepel requested changes Feb 20, 2024

View reviewed changes

mbronk force-pushed the mbronk/argoclima_binding_wip_OH4.1-SNAPSHOT-clean branch 2 times, most recently from 47824fb to 04c7f01 Compare March 2, 2024 14:11

mbronk requested a review from lsiepel March 5, 2024 20:48

lsiepel reviewed Apr 29, 2024

View reviewed changes

mbronk force-pushed the mbronk/argoclima_binding_wip_OH4.1-SNAPSHOT-clean branch from 04c7f01 to 58d4fca Compare May 26, 2024 21:30

mbronk requested a review from lsiepel May 26, 2024 21:38

mbronk added 2 commits May 26, 2024 23:44

Initial contribution of the ArgoClima binding

1d6befd

Signed-off-by: Mateusz Bronk <bronk.m+gh@gmail.com>

mbronk force-pushed the mbronk/argoclima_binding_wip_OH4.1-SNAPSHOT-clean branch from 58d4fca to f31fdad Compare May 26, 2024 21:44

lsiepel requested review from a team and removed request for jlaur May 31, 2024 20:23

[argoclima] Review: removing excess comment

f7b70d6

Signed-off-by: Mateusz Bronk <bronk.m+gh@gmail.com>

lsiepel approved these changes Jun 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[argoclima] Initial contribution #15481

[argoclima] Initial contribution #15481

mbronk commented Aug 22, 2023 •

edited

lsiepel left a comment

This comment was marked as resolved.

This comment was marked as resolved.

lsiepel left a comment

This comment was marked as resolved.

lsiepel left a comment

This comment was marked as outdated.

This comment was marked as outdated.

lsiepel commented Sep 4, 2023

lsiepel left a comment

mbronk commented Nov 10, 2023

jlaur commented Nov 11, 2023

mbronk commented Nov 11, 2023

holgerfriedrich left a comment

mbronk commented Dec 22, 2023

holgerfriedrich commented Dec 22, 2023

mbronk commented Dec 22, 2023

holgerfriedrich left a comment

lsiepel left a comment

mbronk commented Mar 2, 2024

mbronk commented Mar 24, 2024

mbronk commented Apr 28, 2024

lsiepel commented Apr 28, 2024

lsiepel left a comment

mbronk commented May 11, 2024

lsiepel commented May 11, 2024

mbronk commented May 12, 2024

lsiepel commented May 12, 2024

lsiepel left a comment •

edited

[argoclima] Initial contribution #15481

Are you sure you want to change the base?

[argoclima] Initial contribution #15481

Conversation

mbronk commented Aug 22, 2023 • edited

This PR adds a new binding for ArgoClima HVAC devices.

Exemplary supported device:

Highlights (for reviewers)

Testing / development status

lsiepel left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

lsiepel left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

lsiepel left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

lsiepel commented Sep 4, 2023

lsiepel left a comment

Choose a reason for hiding this comment

mbronk commented Nov 10, 2023

jlaur commented Nov 11, 2023

mbronk commented Nov 11, 2023

holgerfriedrich left a comment

Choose a reason for hiding this comment

mbronk commented Dec 22, 2023

holgerfriedrich commented Dec 22, 2023

mbronk commented Dec 22, 2023

holgerfriedrich left a comment

Choose a reason for hiding this comment

lsiepel left a comment

Choose a reason for hiding this comment

mbronk commented Mar 2, 2024

mbronk commented Mar 24, 2024

mbronk commented Apr 28, 2024

lsiepel commented Apr 28, 2024

lsiepel left a comment

Choose a reason for hiding this comment

mbronk commented May 11, 2024

lsiepel commented May 11, 2024

mbronk commented May 12, 2024

lsiepel commented May 12, 2024

lsiepel left a comment • edited

Choose a reason for hiding this comment

mbronk commented Aug 22, 2023 •

edited

lsiepel left a comment •

edited