[argoclima] Initial contribution

[mbronk]

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)

1. 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
2. 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!

[mbronk]

@lsiepel - thanks for your feedback! I have applied most of your comments. Unless I missed any, this should address all feedback so far, except for:

1. Schedule configurable through Thing Configuration, and not Channels
2. The showCleartextPasswords - param

... waiting on conclusions from the discussion with those.

---

Also, not sure where you stand on the "amend/squash" vs. "multiple commits on PR branch" side, so have for now:

• rebased main onto it
• added review feedback as a separate commit

If you have a preference for the future, let me know and I'll adjust!

[lsiepel]

@lsiepel - thanks for your feedback! I have applied most of your comments. Unless I missed any, this should address all feedback so far, except for:

1. Schedule configurable through Thing Configuration, and not Channels
2. The showCleartextPasswords - param

... waiting on conclusions from the discussion with those.

Also, not sure where you stand on the "amend/squash" vs. "multiple commits on PR branch" side, so have for now:

• rebased main onto it
• added review feedback as a separate commit

If you have a preference for the future, let me know and I'll adjust!

Per review commits are fine. Rebase is only needed if some conflict or similar occurs, otherwise it is not needed. I don't think there is a general policy here, atleast i'm not aware of any.

[mbronk]

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

Thank you! Added next round of changes and replied to the withstanding questions. Should address everything this far - please check.

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

Yeah, I know it's a bit much... TBH I written most of this code ~1.5y ago and had to come back to make it OH4-ready and upstreamable... so the comments are added retroactively (was explaining this code to myself and persisting it as I went :)). In an ideal world, I would have managed to rewrite/refactor and cover it with decent UTs... but can't find this much time outside of my day job, unfortunatelly... so comments it is :)

[mbronk]

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).

@lsiepel Thank you! I've just pushed the last round of changes adressing everything so far (with the only open remaining about channels vs. config for the device-side schedule). Everything else should be convered already - let me know if I missed anything.

And thank you once more. While it took some effort to write, I realize it's also not the simplest/smallest contribution to work with. Thanks for taking the time to share all the valuable feedback. Appreciate it!

---

Btw. do you know if I need to do anything extra for a maintainer to have a look at it (like ping someone), or is it already in the queue and "the process" will pick it up eventually?

[mbronk]

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!

[lsiepel]

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

[mbronk]

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]

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: #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]

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 😉

[mbronk]

@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]

@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]

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!

[mbronk]

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]

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

[mbronk]

Ping (+1 month with no activity)

[lsiepel]

Ping (+1 month with no activity)

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

[mbronk]

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]

@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]

@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]

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.

[lsiepel]

@jlaur if you have some spare time, would be nice if you can look at this pr for a moment

[jlaur]

if you have some spare time, would be nice if you can look at this pr for a moment

I took at very quick look at the README which lead me straight to the thing type definition. I have posted a few comments there, as some of these things will be hard to change later without introducing breaking changes.

However, if the intention with your request is to have review comments added and fixed by tomorrow, I'm afraid this is too late and too out of context. I also doubt @mbronk would have time to even react on the comments I already posted, so please feel free to ignore.

Unit hint for Number:Dimensionless should be added (see #16614), so you could either commit those changes if @mbronk will not be able to respond, or it can be added in a follow-up PR.

[lsiepel]

if you have some spare time, would be nice if you can look at this pr for a moment

I took at very quick look at the README which lead me straight to the thing type definition. I have posted a few comments there, as some of these things will be hard to change later without introducing breaking changes.

However, if the intention with your request is to have review comments added and fixed by tomorrow, I'm afraid this is too late and too out of context. I also doubt @mbronk would have time to even react on the comments I already posted, so please feel free to ignore.

Unit hint for Number:Dimensionless should be added (see #16614), so you could either commit those changes if @mbronk will not be able to respond, or it can be added in a follow-up PR.

Thanks! My request was more about the overall complexity by looking at the readme and the handler. I would have merged it if you didnt have significant feedback at those areas. Your current feedback about channel id's is minor, but altogether enough to postpone a merge. So this will propably not make it into 4.2. Maybe you can revisit the handler on a later time after 4.2 is done.

[jlaur]

Your current feedback about channel id's is minor, but altogether enough to postpone a merge. So this will propably not make it into 4.2. Maybe you can revisit the handler on a later time after 4.2 is done.

The channel type id's can be changed later on using update instructions, but it's really minor anyway, and doesn't violate naming convention. However, the thing type id, although also minor, cannot be changed later.

I would focus first on getting the modelling right (from the start), the implementation can always be refactored if it turns out to be too complex (for example). So I trust your review, and you can still consider merging this for 4.2. I believe strictly it doesn't have to be tomorrow even though we have feature freeze. The feature freeze s is mostly to avoid introducing regressions in existing bindings with last-minute changes. So if you and @mbronk would agree on the thing type id proposal, this is a small change, and there could still be a few days to do that (XML, constants, README).

[mbronk]

The channel type id's can be changed later on using update instructions, but it's really minor anyway, and doesn't violate naming convention. (...) So if you and @mbronk would agree on the thing type id proposal, this is a small change, and there could still be a few days to do that (XML, constants, README).

@jlaur - I do agree with the proposed renames (less boilerplate, less typing). I had them globally-unique 'just in case', to avoid weird x-tenancy issues at development time... but as there are none... yeah, it can/should be shortened.

Given short timelines... behold...

The plan :tm: 😉

You guessed right I won't have time to properly apply (and test) this change in full just today (only have about 1 hour free before I head off for a short weekend getaway). But - it's a straightforward one I can try to apply (=change, make things compile again) very quickly.

This would be a not-yet-tested code, but in a "frozen" state already with all the CI complete and such. When I'm back from a short family trip, I'd be able to test and around Monday, give you a works 👍 / doesn't work 👎 . Worst-case, the last commit can be reverted, or the changeset skips 4.2. Best case... what I'll commit today passes the "quality" checks and would become mergable on Monday. @lsiepel , @jlaur - What do you think? OFC not a good engineering practice, but... sounds like a plan? :grin:

[lsiepel]

The channel type id's can be changed later on using update instructions, but it's really minor anyway, and doesn't violate naming convention. (...) So if you and @mbronk would agree on the thing type id proposal, this is a small change, and there could still be a few days to do that (XML, constants, README).

@jlaur - I do agree with the proposed renames (less boilerplate, less typing). I had them globally-unique 'just in case', to avoid weird x-tenancy issues at development time... but as there are none... yeah, it can/should be shortened.

Given short timelines... behold...

The plan ™️ 😉

You guessed right I won't have time to properly apply (and test) this change in full just today (only have about 1 hour free before I head off for a short weekend getaway). But - it's a straightforward one I can try to apply (=change, make things compile again) very quickly.

This would be a not-yet-tested code, but in a "frozen" state already with all the CI complete and such. When I'm back from a short family trip, I'd be able to test and around Monday, give you a works 👍 / doesn't work 👎 . Worst-case, the last commit can be reverted, or the changeset skips 4.2. Best case... what I'll commit today passes the "quality" checks and would become mergable on Monday. @lsiepel , @jlaur - What do you think? OFC not a good engineering practice, but... sounds like a plan? 😁

Take your time, as it is a new binding I think we can merge it after the milestone. It should just be well tested.

[mbronk]

Take your time, as it is a new binding I think we can merge it after the milestone. It should just be well tested.

Oh, I'm not at all proposing skipping a testing phase. Given the nature of the changes (renames-only) though, early next week is an actual possibility (not cutting corners) - just can't make any guarantees today.

For me it is still far better to try (best-effort) to intercept 4.2 (b/c it's incremental for me) than to do the whole nine yards on 4.3 or later...

[mbronk]

As mentioned - changes suggested by @jlaur are in. I'll separately confirm the testing status early next week.

~~Seems I can't vote (👎 ) for my own changes (or wasn't able to find an option), but please consider it a -1 from me for the interim (this applies to last commit's status only).~~

[mbronk]

As mentioned - changes suggested by @jlaur are in. I'll separately confirm the testing status early next week.

Update

All tests have passed on my end.