Added support for Argo WREM-3 A/C remote protocol [part1] #1920

mbronk · 2022-11-19T16:59:28Z

Adds support for ARGO WREM-3 remote

For: #1912

This PR is a standalone part1 of changes detached from: #1913

crankyoldgit

Again, thanks for the massive effort. I understand what it's like.

Thanks for splitting it up too.

Doxyfile

src/IRutils.cpp

src/IRutils.h

crankyoldgit · 2022-11-22T00:44:56Z

src/ir_Argo.cpp

+  switch (messageType) {
+    case argoIrMessageType_t::AC_CONTROL :
+      if (nbits != kArgo3AcControlStateLength * 8) { return false; }
+      break;
+  case argoIrMessageType_t::CONFIG_PARAM_SET:
+      if (nbits != kArgo3ConfigStateLength * 8) { return false; }
+      break;
+  case argoIrMessageType_t::TIMER_COMMAND:
+      if (nbits != kArgo3TimerStateLength * 8) { return false; }
+      break;
+    case argoIrMessageType_t::IFEEL_TEMP_REPORT:
+      if (nbits != kArgo3iFeelReportStateLength * 8) { return false; }
+      break;
+    default:
+      return false;
+  }


Divide by 8 much earlier to get the bytes. And do the comparisons via that. That will reduce the resultant code size, and improve readablity. e.g. statelength.

Also, this really is a "strict" condition IMHO.

That will reduce the resultant code size, and improve readablity. e.g. statelength.

Good point - will simplify (agree with the readability part of the comment).
Must add though, that I'd be very surprised it does anything positive to the resulting code size.
Compiler should be perfectly capable of optimizing it on its own.

Also, this really is a "strict" condition IMHO.

Well, yes and no.
That is, it is about the only thing separating WREM2 from WREM3 (and WREM3 detection must run before WREM2 non-strict, as it is a swallow-all).
While there's indeed a getMessageType() doing the detect, it only takes 2 bits of input, so a chance of a false-positive is way high. Aka. I consider "checksum" strict check, but anything else... I deliberately wanted to fall through to WREM2 and not get handled here.

That said, I only use the detect in strict mode anyway, so the point I'm making above is a bit moot and I could equally well remove 'strict' param entirely...
Leaving as-is for now, let me know where you'd like to take it.

My unwritten policy for strict is, if it is in some way protocol-format related and can cause it to abort decoding, encapsulate it in strict. The purpose is to allow more loose matching with strict set to false, and/or code re-use where the protocol timings etc are compatible, but the length or structure of the data differs in a different protocol.
i.e. Maximum potential for code-reuse to reduce library binary size etc.

Please note though that WREM2 and WREM3 are variants (models) of the same ARGO IR protocol.
The difference is in message decoding and parsing, so in the case you describe:

... but the length or structure of the data differs in a different protocol....

...there's already a decodeArgo(..., strict=false) which would accomplish that result just fine.
The WREM3 flavor was meant signify this is semantically a different sub-proto.

If I make the length-check non-strict, then, arguably, decodeArgoWREM3(..., strict=false) would be almost no different from decodeArgo(..., strict=false)... which makes it a bit redundant IMHO (but arguably consistent)

My preferences (from most-preferred):

Leave as is (possibly less semantically consistent, but the most versatile impl.)

Remove strict param from decodeArgoWREM3 altogether

Make the length non-strict (it has a potential to clash with wrem2, but since I'm not calling it this way anyway... I guess the user would know what they're after).
I.e. when you call it with strict=false a true response no longer means there's any confidence in it really being a WREM3.

Your pick? :)

Users typically can't call the decodeProtocol() routines directly. So, it's just us "internal" users.
Practically every decodeProtocol() has a strict, so I'd like to keep that calling convention.
Of the options, 3. sounds best to me. It is almost always a given if a decodeProtocol() is called with strict=false that it's not a confident match. It assumes the caller knows what they are doing.

src/ir_Argo.h

src/IRtext.h

src/locale/defaults.h

mbronk · 2022-11-22T22:51:21Z

@crankyoldgit Thanks so much for reviewing! I know it wasn't a pleasant read, given the volume of changes in Argo class.

I think I addressed most of your comments in the new update. On a few comments that I left behind, I added replies or questions to you. Please take a look.
I'd also appreciate if you mark the conversations as resolved where you believe the changes address your comments, so that I know if there's anything withstanding. Where I did not comment, I merely incorporated the proposal as-is (hopefully didn't miss any).
For now I'm leaving it as-is and waiting for your inputs as to any follow-ups needed.
Thanks!

crankyoldgit

Looking a lot better. Just a couple of minor things left.

Signed-off-by: Mateusz Bronk <[email protected]>

Incl. a macro 'gadget' to allow capturing raw command sent by IRac. Signed-off-by: Mateusz Bronk <[email protected]>

mbronk · 2022-11-23T21:22:02Z

Reverted the doc/doxy changes. Unfortunately, I wanted to do it (too) cleanly and rebased the whole thing, not to show the add + revert... which seems to have thrown github's incremental diff off :( Sorry for that!

Bottom line, as far as I can tell the only remaining comment from the original round would be the one about "strict" mode. Please take a look

crankyoldgit · 2022-11-24T06:39:21Z

Let me know when the strict changes are in as discussed, and I'm fine to approve & merge.

Signed-off-by: Mateusz Bronk <[email protected]>

mbronk · 2022-11-24T09:08:46Z

Let me know when the strict changes are in as discussed

Just pushed. Please take a look.
Note I have pushed the lax logic into decodeArgoWREM3() (if not strict, bypasses call to isValidWrem3Message entirely).
Reason being: if I were to not make the length check in isValidWrem3Message, it becomes a return true.

crankyoldgit · 2022-11-24T09:23:09Z

Let me know when the strict changes are in as discussed

Just pushed. Please take a look. Note I have pushed the lax logic into decodeArgoWREM3() (if not strict, bypasses call to isValidWrem3Message entirely). Reason being: if I were to not make the length check in isValidWrem3Message, it becomes a return true.

Good. 'Cause that what I'd expect.

crankyoldgit

As discussed, LGTM

_v2.8.5 (20230305)_ **[Bug Fixes]** - Missing argument in use of midea function (#1959 #1958) - IRMQTTServer: Improve HA MQTT climate handling. (#1911) - SEND_SANYO_AC88: Fix poor cut-n-paste error (#1905 #1897) **[Features]** - IRMQTTServer: SHT-3x Temperature Sensor Support (#1951) - IRMQTTServer: HA multi output discovery (#1947) - IRMQTTServer: extended with new A/C common fields (#1940) - IRMQTTServer: Sync the on state to power from mode for HA (#1946) - Experimental basic support for Carrier 84-bit protocol. (#1945 #1943) - Add support the WowWee 11-Bit RoboRaptor-X protocol. (#1939 #1938) - Added 'sensorTemperature' and 'iFeel' to IRac (common) (#1928) - Added extra 'mid' option for Fan & SwingV to IRac (#1929) - Added "commandType" to IRAc (#1921) - Added support for Argo WREM-3 A/C remote protocol [part1] (#1920) - Added Dutch (nl-NL) translation (#1907) - ARGO: Improve code & add support for decoding 32bit sensor msgs. (#1906 #1859) - Added support for Gorenje cooker hood IR protocol (#1888 #1887) **[Misc]** - Update `XMP` status to Stable (#1944) - upgrade to a later version of `googletest` (#1936) - MITSUBISHI128: Added model to supported protocol (#1924) - Added Dutch (nl-NL) README (#1908) - Added GMock to UT Makefile (#1902) - Update HA example config for HA 2022.6+ (#1901 #1900)

@1976

_v2.8.5 (20230508)_ **[Bug Fixes]** - Fix a bug where we never detached the timer interrupt on ESP32s. (#1984 #1983) - Missing argument in use of midea function (#1959 #1958) - IRMQTTServer: Improve HA MQTT climate handling. (#1911) - SEND_SANYO_AC88: Fix poor cut-n-paste error (#1905 #1897) **[Features]** - Add support for a 40bit variant of the standard Panasonic protocol (#1977 @1976) - Initial support for York AC protocol (#1889) - IRMQTTServer: SHT-3x Temperature Sensor Support (#1951) - IRMQTTServer: HA multi output discovery (#1947) - IRMQTTServer: extended with new A/C common fields (#1940) - IRMQTTServer: Sync the on state to power from mode for HA (#1946) - Experimental basic support for Carrier 84-bit protocol. (#1945 #1943) - Add support the WowWee 11-Bit RoboRaptor-X protocol. (#1939 #1938) - Added 'sensorTemperature' and 'iFeel' to IRac (common) (#1928) - Added extra 'mid' option for Fan & SwingV to IRac (#1929) - Added "commandType" to IRAc (#1921) - Added support for Argo WREM-3 A/C remote protocol [part1] (#1920) - Added Dutch (nl-NL) translation (#1907) - ARGO: Improve code & add support for decoding 32bit sensor msgs. (#1906 #1859) - Added support for Gorenje cooker hood IR protocol (#1888 #1887) **[Misc]** - Add Electrolux YKR-H/531E as a supported device (#1981 #1980) - Update `XMP` status to Stable (#1944) - upgrade to a later version of `googletest` (#1936) - MITSUBISHI128: Added model to supported protocol (#1924) - Added Dutch (nl-NL) README (#1908) - Added GMock to UT Makefile (#1902) - Update HA example config for HA 2022.6+ (#1901 #1900) - Add a `d1_mini_noMDNS` build option to `IRMQTTServer`. (#1985)

@1976

## _v2.8.5 (20230508)_ **[Bug Fixes]** - Fix a bug where we never detached the timer interrupt on ESP32s. (#1984 #1983) - Missing argument in use of midea function (#1959 #1958) - IRMQTTServer: Improve HA MQTT climate handling. (#1911) - SEND_SANYO_AC88: Fix poor cut-n-paste error (#1905 #1897) **[Features]** - Add support for a 40bit variant of the standard Panasonic protocol (#1977 @1976) - Initial support for York AC protocol (#1889) - IRMQTTServer: SHT-3x Temperature Sensor Support (#1951) - IRMQTTServer: HA multi output discovery (#1947) - IRMQTTServer: extended with new A/C common fields (#1940) - IRMQTTServer: Sync the on state to power from mode for HA (#1946) - Experimental basic support for Carrier 84-bit protocol. (#1945 #1943) - Add support the WowWee 11-Bit RoboRaptor-X protocol. (#1939 #1938) - Added 'sensorTemperature' and 'iFeel' to IRac (common) (#1928) - Added extra 'mid' option for Fan & SwingV to IRac (#1929) - Added "commandType" to IRAc (#1921) - Added support for Argo WREM-3 A/C remote protocol [part1] (#1920) - Added Dutch (nl-NL) translation (#1907) - ARGO: Improve code & add support for decoding 32bit sensor msgs. (#1906 #1859) - Added support for Gorenje cooker hood IR protocol (#1888 #1887) **[Misc]** - Add Electrolux YKR-H/531E as a supported device (#1981 #1980) - Update `XMP` status to Stable (#1944) - upgrade to a later version of `googletest` (#1936) - MITSUBISHI128: Added model to supported protocol (#1924) - Added Dutch (nl-NL) README (#1908) - Added GMock to UT Makefile (#1902) - Update HA example config for HA 2022.6+ (#1901 #1900) - Add a `d1_mini_noMDNS` build option to `IRMQTTServer`. (#1985)

crankyoldgit self-requested a review November 21, 2022 23:50

crankyoldgit self-assigned this Nov 21, 2022

crankyoldgit requested changes Nov 22, 2022

View reviewed changes

crankyoldgit reviewed Nov 23, 2022

View reviewed changes

mbronk added 2 commits November 23, 2022 21:56

Added support for Argo WREM-3 A/C remote protocol

440df2b

Signed-off-by: Mateusz Bronk <[email protected]>

Added additional UT for Argo AC via IRac/common

d64b6fd

Incl. a macro 'gadget' to allow capturing raw command sent by IRac. Signed-off-by: Mateusz Bronk <[email protected]>

mbronk force-pushed the private/mbronk/argo_WREM3_part1 branch from b26e701 to 914c0ed Compare November 23, 2022 21:00

mbronk force-pushed the private/mbronk/argo_WREM3_part1 branch from 914c0ed to 9d2b66c Compare November 23, 2022 21:29

Review changes

c951ada

Signed-off-by: Mateusz Bronk <[email protected]>

mbronk force-pushed the private/mbronk/argo_WREM3_part1 branch from 9d2b66c to c951ada Compare November 24, 2022 09:04

crankyoldgit self-requested a review November 24, 2022 09:26

crankyoldgit approved these changes Nov 24, 2022

View reviewed changes

crankyoldgit merged commit a5ddcdd into crankyoldgit:master Nov 24, 2022

crankyoldgit mentioned this pull request May 8, 2023

v2.8.5 release #1988

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for Argo WREM-3 A/C remote protocol [part1] #1920

Added support for Argo WREM-3 A/C remote protocol [part1] #1920

mbronk commented Nov 19, 2022

crankyoldgit left a comment

crankyoldgit Nov 22, 2022

crankyoldgit Nov 22, 2022

mbronk Nov 22, 2022

crankyoldgit Nov 23, 2022

mbronk Nov 23, 2022

crankyoldgit Nov 24, 2022

mbronk commented Nov 22, 2022

crankyoldgit left a comment

mbronk commented Nov 23, 2022

crankyoldgit commented Nov 24, 2022

mbronk commented Nov 24, 2022

crankyoldgit commented Nov 24, 2022

crankyoldgit left a comment

Added support for Argo WREM-3 A/C remote protocol [part1] #1920

Added support for Argo WREM-3 A/C remote protocol [part1] #1920

Conversation

mbronk commented Nov 19, 2022

crankyoldgit left a comment

Choose a reason for hiding this comment

crankyoldgit Nov 22, 2022

Choose a reason for hiding this comment

crankyoldgit Nov 22, 2022

Choose a reason for hiding this comment

mbronk Nov 22, 2022

Choose a reason for hiding this comment

crankyoldgit Nov 23, 2022

Choose a reason for hiding this comment

mbronk Nov 23, 2022

Choose a reason for hiding this comment

crankyoldgit Nov 24, 2022

Choose a reason for hiding this comment

mbronk commented Nov 22, 2022

crankyoldgit left a comment

Choose a reason for hiding this comment

mbronk commented Nov 23, 2022

crankyoldgit commented Nov 24, 2022

mbronk commented Nov 24, 2022

crankyoldgit commented Nov 24, 2022

crankyoldgit left a comment

Choose a reason for hiding this comment