[do not merge / consolidated PR] Added support for Argo WREM-3 A/C remote protocol

[mbronk]

Adds support for ARGO WREM-3 remote
Closes #1912

[mbronk]

This is a large PR, unfortunately, and I am building this on WSL/Windows, w/o full setup so have not run all the checks (I expect some builds to fail). Will fix as I go and update this PR. Feel free to review, though!

---

Some rationale about a few high-level choices I made:

1. WREM-3 protocol is so significantly different from WREM-2 I went with separate class for it, even though it uses the same(-ish) IR transfer protocol
   • "-ish" as there doesn't seem to be a footer in WREM-2, but that may well be just impl. quirk
   • The WREM-2 vs. WREM-3 choice is modelled as A/C device models, not protocols.
2. I don't own a WREM-2 device, so tried to keep the old impl. intact, even where I personally believe it could be refactored or improved. The only backwards-incompat. change I intended to make is the toString() for WREM2
3. To be able to reuse codebase yet not creating perf. penalty, I went with static polymorphism (in some places it is quite awkward as I had to use full specializations, but it gets the job done w/o a vtable)
4. The more fundamental change is modeling the multiple sub-protocols as commandType (common AC class is extended for that). The ac_command_t::kControlCommand is the default, catch-all
   • Even though the added iFeel and roomTemp common params could be added to other A/C devices, I've decided NOT to make a lib-wide update in order to contain changes (this PR is large enough already). These can be added later as a pure add-in.

[NiKiZe]

Have you considered splitting this into separate PRs?
The common parts are only for things that are common, some of the things you are adding might have been decided to not add (might be wrong here)

Consider to split this first into adding basic support. Later add full AC support, and then discuss the other parts separately?

[mbronk]

Have you considered splitting this into separate PRs?

Oh, I have :) Multiple times over... :)
Not sure which revision this one is, and while it is technically touching struct state_t I would hope it makes logical sense.
The arguments for not splitting were:

• About 90-95% of changes is Argo-specific anyway
• Going through state_t allows me to write quite clean E2E unit tests to have everything working
  (see also my comment at the bottom of this message)

---

The common parts are only for things that are common, some of the things you are adding might have been decided to not add (might be wrong here)

Actually, I claim these things are common. Not in the sense of "every possible A/C unit has them", but iFeel or roomTemp are very much available in most protos.
As for the commandType... this is a design decision indeed, which I went with after multiple other attempts. Unfortunately, I didn't find a way to properly solve this anywhere in the library (in case there were 2 types of commands, the 2nd one was mostly plastered on top of the first and not exposed uniformly).
This approach is ultimately trying to model a real-world concept (there's a single remote sending more than one protocolar commands, each command only has a sub-slice of data).
As far as my solution... it's about all I could come up with without making major changes to the library i-face, and probably can be improved. I'd say though that I don't believe the problem itself is uncommon, so it should be solved some day anyway.

---

Consider to split this first into adding basic support. Later add full AC support, and then discuss the other parts separately?

@NiKiZe If you feel very strongly about separating these, I'd of course do that...
At the same time, the split is not going to take away more than 10% (that's my ballpark, may be wrong), and given these constitute a logical change my kind request to you would be to... well... try to review as-is. :) :)
I know how it sounds (don't fancy reviewing large PRs myself) but getting these into merge'able form was quite a headache already, so I figured I'd swallow my pride and ask anyway.

If, during the course of review some changes become clearly controversial, I'll of course pull them.
For now, I am afraid, my claim is "this is a single logical change" (and I've definitely been considering mutiple PRs and refactored this about 4 times already) :)

[crankyoldgit]

Sorry for the delay, I've been very busy with other things.
Thanks for submitting a huge piece of work like this. It's great!

First, I'm in agreement with @NiKiZe. Please split this up. e.g. Just adding the protocol. Then the other changes e.g. IRMQTTServer. Basically, lets get the protocol in first, then we can play with IRMQTTServer additions/changes.

Reguarding controversial, yep. Plenty there I'm not thrilled with.
e.g. Extending some of the enums to add extra options/modes etc.
Some of those changes may or will have impacts on other protocols behaviour. (Yes, we should have added UNIT_TESTs for that eventuality, but I didn't.)

The "common" AC interface (IRac class) is meant to be a least common denominator between A/C's. i.e. It covers the basics. It is NOT meant to be a "you can control the device (near) perfectly via it". e.g. Things that are pretty much universal between A/Cs and not confusing. Even "filter" & "clean" operations get messy. "sleep" behaviour is ambiguous too but we kind-of manage that.
i.e. Not all vane positions or fan speeds are supported.
If a device (or person) requires full/better support, they are to go via the non-generic specific class for that device/protocol/model etc.
Yes, there probably is a strong case to support sensor temps etc. Let's add that in a separate addition, and look at adding it for all the protocol classes that support it.

[mbronk]

Thanks for the feedback! I very much appreciate taking a look at it as-is, as I hope it paints a picture of the general heading I took and provides additional context on the "why" (and of course there'd have to be adjustments).

Now that I get a sense of where we might need more discussion, I'll do my best to split as @crankyoldgit indicated.
Tactically, I'll probably leave this PR as-is, for contextual info and general discussion on the direction itself, and try to break out individual chunks into separate PRs, then finally close/abandon this one. If you have other preference, let me know.

Can't commit to any particular timeline though. I can spend at most 1-2h/week at it lately, so depending on how it goes... it might be today or weeks for now :) Any help you can provide would be appreciated of course!

[mbronk]

(..) leave this PR for (...) general discussion on the direction itself

In this spirit, let me add a few high-level comments as well (ahead of the discussion on individual changes, when split)

---

Reguarding controversial, yep. Plenty there I'm not thrilled with.
e.g. Extending some of the enums to add extra options/modes etc.

I have no particular love for the "common enum extension" changes, actually :) Added, as I considered them quite harmless and in line with the convention (see comment below). Same with the extension of IRMQTTServer - it's merely a follow on to extending "common" (completely optional)

The "common" AC interface (IRac class) is meant to be a least common denominator between A/C's.

• I definitely lack the historics here and haven't scanned the entire code base either, but... :)
  ... from what I've seen, it has long evolved past being least common denominator and has become more of a "generic superset" in some aspects.
  When I read it the 1st time, I got a sense that IRAc is about commonality of concepts, but not necessarily an LCD (not a must-implement for all A/C protos).

  • For example, out of the enum options mentioned above, AC protocols I've seen typically implement like 50-80% of the values for options like fan or swing.
    Similarly, I have deliberately not added Argo's schedule timers, nor config commands to common, as they're... well... not that common. Sensor temp on the other hand, might be...

  • I'm not advocating one way is better vs. another by the way (perhaps the long-term direction might even be to shrink the "common" back to become a true LCD),
    more like saying... today we're at the crossroads somewhat.

• It'll probably be easier to discuss on a case-by-case basis, so I think this part might be more appropriate to continue once the PR is split though.

If a device (or person) requires full/better support, they are to go via the non-generic specific class for that device/protocol/model etc.

• It fully matches my line of thinking as well.
  At the same time, I hope "common" to be/become powerful enough to express the typical interactions with the device (like "sensor temp" report).

• And here's where there's a quirk. That is, in order to support even today's part of "common" (like sleep timer), there's a need to model the, what I called a commandType.
  I believe this is single biggest semantic change in the whole PR.
  And I am not particularly fond of how I modelled it, actually. However, I'm afraid that, unless IR remotes like Argo are considered an unsupported outlier for IRAc... the multi-command concept used by them probably needs be modelled in some way, anyway.

• My stake in the game is, I'm consuming the library via Tasmota, which only uses the generic/common interface [code reference here], so can't easily switch to "native" impl.

Yes, there probably is a strong case to support sensor temps etc. Let's add that in a separate addition, and look at adding it for all the protocol classes that support it.

Makes total sense.
Direction-wise, is it generally OK to add a new functionality to common, which is initially a no-op for most protos, and they organically adopt it, or would you prefer to get all possible protocols have it in one add?
if the latter, I might need help and teaming up on refactoring the other classes though.
Would you be able to throw in extra changes?

[crankyoldgit]

@mbronk How are we going on this meta-PR? Have we covered it all yet?

[mbronk]

@mbronk How are we going on this meta-PR? Have we covered it all yet?

Almost there :)
Last part remaining is f3beb7e (IRMQTTServer).

It requires some slight adjustments, which I have coded already, but didn't yet have time to flash it to a device and test E2E (holidays etc.). I expect to have it ready in a few days from now (maybe even today, depending how things go).

[crankyoldgit]

@mbronk How are we going on this meta-PR? Have we covered it all yet?

Almost there :) Last part remaining is f3beb7e (IRMQTTServer).

It requires some slight adjustments, which I have coded already, but didn't yet have time to flash it to a device and test E2E (holidays etc.). I expect to have it ready in a few days from now (maybe even today, depending how things go).

Cool. No rush. I just had some spare cycles in the last 48hrs and was just checking what I could.

[mbronk]

Closing this meta-PR, as all of its components have been fully merged.

@crankyoldgit - a bonus Q for you: When are you expecting to release a new version of the lib? :)

[crankyoldgit]

Yeah. I should get on to that.