[remote_base] Decode the full MagiQuest frame and check its checksum (#10622)

Co-authored-by: J. Nick Koston <nick@home-assistant.io>
Co-authored-by: J. Nick Koston <nick@koston.org>

esphome/components/remote_base/__init__.py

@@ -953,16 +953,22 @@ MAGIQUEST_SCHEMA = cv.Schema(
     }
 )
 
+# A wand_id of 0 matches any wand. Magnitude is still accepted but not matched on.
+MAGIQUEST_BINARY_SENSOR_SCHEMA = cv.Schema(
+    {
+        cv.Optional(CONF_WAND_ID, default=0): cv.hex_uint32_t,
+        cv.Optional(CONF_MAGNITUDE): cv.hex_uint16_t,
+    }
+)
 
-@register_binary_sensor("magiquest", MagiQuestBinarySensor, MAGIQUEST_SCHEMA)
+
+@register_binary_sensor(
+    "magiquest", MagiQuestBinarySensor, MAGIQUEST_BINARY_SENSOR_SCHEMA
+)
 def magiquest_binary_sensor(var, config):
     cg.add(
         var.set_data(
-            cg.StructInitializer(
-                MagiQuestData,
-                ("magnitude", config[CONF_MAGNITUDE]),
-                ("wand_id", config[CONF_WAND_ID]),
-            )
+            cg.StructInitializer(MagiQuestData, ("wand_id", config[CONF_WAND_ID]))
         )
     )
 

esphome/components/remote_base/magiquest_protocol.cpp

@@ -2,7 +2,7 @@
 #include "esphome/core/log.h"
 
 /* Based on protocol analysis from
- * https://arduino-irremote.github.io/Arduino-IRremote/ir__MagiQuest_8cpp_source.html
+ * https://arduino-irremote.github.io/Arduino-IRremote/ir__MagiQuest_8hpp_source.html
  */
 
 namespace esphome::remote_base {
@@ -10,12 +10,25 @@ namespace esphome::remote_base {
 ESPHOME_LOG_TAG(TAG, "remote.magiquest");
 
 static constexpr uint32_t MAGIQUEST_UNIT = 288;  // us
+// Half a unit is the widest window that still tells a one unit mark or space from a two unit one.
+static constexpr uint32_t MAGIQUEST_TOLERANCE = MAGIQUEST_UNIT / 2;
 static constexpr uint32_t MAGIQUEST_ONE_MARK = 2 * MAGIQUEST_UNIT;
 static constexpr uint32_t MAGIQUEST_ONE_SPACE = 2 * MAGIQUEST_UNIT;
 static constexpr uint32_t MAGIQUEST_ZERO_MARK = MAGIQUEST_UNIT;
 static constexpr uint32_t MAGIQUEST_ZERO_SPACE = 3 * MAGIQUEST_UNIT;
 
+static bool checksum_is_valid(uint32_t wand_id, uint32_t magnitude_and_checksum) {
+  // The frame's six payload bytes, which start at the wand id's top bit, must sum to zero.
+  const uint32_t id_bytes = wand_id << 1;
+  uint8_t sum = 0;
+  for (uint8_t shift = 0; shift < 32; shift += 8) {
+    sum += (id_bytes >> shift) + (magnitude_and_checksum >> shift);
+  }
+  return sum == 0;
+}
+
 void MagiQuestProtocol::encode(RemoteTransmitData *dst, const MagiQuestData &data) {
+  // Still the old frame layout, which the decoder no longer reads; changing it would alter what existing configs send.
   dst->reserve(101);  // 2 start bits, 48 data bits, 1 stop bit
   dst->set_carrier_frequency(38000);
 
@@ -41,41 +54,48 @@ void MagiQuestProtocol::encode(RemoteTransmitData *dst, const MagiQuestData &dat
   dst->mark(MAGIQUEST_UNIT);
 }
 optional<MagiQuestData> MagiQuestProtocol::decode(RemoteReceiveData src) {
-  MagiQuestData data{
-      .magnitude = 0,
-      .wand_id = 0,
-  };
-  // Two start bits
-  if (!src.expect_item(MAGIQUEST_ZERO_MARK, MAGIQUEST_ZERO_SPACE) ||
-      !src.expect_item(MAGIQUEST_ZERO_MARK, MAGIQUEST_ZERO_SPACE)) {
-    return {};
+  src.set_tolerance(MAGIQUEST_TOLERANCE, TOLERANCE_MODE_TIME);
+
+  MagiQuestData data{};
+
+  // 8 bit header, always zero
+  for (uint8_t i = 0; i < 8; i++) {
+    if (!src.expect_item(MAGIQUEST_ZERO_MARK, MAGIQUEST_ZERO_SPACE)) {
+      return {};
+    }
   }
 
-  for (uint32_t mask = 1 << 31; mask; mask >>= 1) {
+  // 31 bit wand id
+  for (uint32_t mask = 1 << 30; mask; mask >>= 1) {
     if (src.expect_item(MAGIQUEST_ONE_MARK, MAGIQUEST_ONE_SPACE)) {
       data.wand_id |= mask;
-    } else if (src.expect_item(MAGIQUEST_ZERO_MARK, MAGIQUEST_ZERO_SPACE)) {
-      data.wand_id &= ~mask;
-    } else {
+    } else if (!src.expect_item(MAGIQUEST_ZERO_MARK, MAGIQUEST_ZERO_SPACE)) {
       return {};
     }
   }
 
-  for (uint16_t mask = 1 << 15; mask; mask >>= 1) {
+  // 9 bit magnitude, then 8 bit checksum
+  uint32_t magnitude_and_checksum = 0;
+  for (uint32_t mask = 1 << 16; mask > 1; mask >>= 1) {
     if (src.expect_item(MAGIQUEST_ONE_MARK, MAGIQUEST_ONE_SPACE)) {
-      data.magnitude |= mask;
-    } else if (src.expect_item(MAGIQUEST_ZERO_MARK, MAGIQUEST_ZERO_SPACE)) {
-      data.magnitude &= ~mask;
-    } else {
+      magnitude_and_checksum |= mask;
+    } else if (!src.expect_item(MAGIQUEST_ZERO_MARK, MAGIQUEST_ZERO_SPACE)) {
       return {};
     }
   }
+  // The last bit has no trailing space. A wrong or missing mark here is caught by the checksum.
+  if (src.expect_mark(MAGIQUEST_ONE_MARK)) {
+    magnitude_and_checksum |= 1;
+  }
 
-  src.expect_mark(MAGIQUEST_UNIT);
+  if (!checksum_is_valid(data.wand_id, magnitude_and_checksum)) {
+    return {};
+  }
+  data.magnitude = magnitude_and_checksum >> 8;
   return data;
 }
 void MagiQuestProtocol::dump(const MagiQuestData &data) {
-  ESP_LOGI(TAG, "Received MagiQuest: wand_id=0x%08" PRIX32 ", magnitude=0x%04X", data.wand_id, data.magnitude);
+  ESP_LOGI(TAG, "Received MagiQuest: wand_id=0x%08" PRIX32 ", magnitude=%u", data.wand_id, data.magnitude);
 }
 
 }  // namespace esphome::remote_base

esphome/components/remote_base/magiquest_protocol.h

@@ -5,23 +5,24 @@
 #include <cinttypes>
 
 /* Based on protocol analysis from
- * https://arduino-irremote.github.io/Arduino-IRremote/ir__MagiQuest_8cpp_source.html
+ * https://arduino-irremote.github.io/Arduino-IRremote/ir__MagiQuest_8hpp_source.html
  */
 
 namespace esphome::remote_base {
 
+// Older decoders read the wand id 5 bits too early, so ids in older configs lack its 5 low bits.
+static constexpr uint8_t MAGIQUEST_LEGACY_WAND_ID_SHIFT = 5;
+
 struct MagiQuestData {
   uint16_t magnitude;
   uint32_t wand_id;
 
+  // Not symmetric: `this` is the decoded frame and `rhs` the configured match. A configured wand_id of 0 matches
+  // any wand. Magnitude is not compared; it appears to encode the cast type rather than a strength. The legacy
+  // match applies to every configured id, so an id below 2^26 also matches the 32 wands whose full id shifts to it.
   bool operator==(const MagiQuestData &rhs) const {
-    // Treat 0xffff as a special, wildcard magnitude
-    // In testing, the wand never produces this value, and this allows us to match
-    // on just the wand_id if wanted.
-    if (rhs.wand_id != this->wand_id) {
-      return false;
-    }
-    return (this->wand_id == 0xffff || rhs.wand_id == 0xffff || this->wand_id == rhs.wand_id);
+    return rhs.wand_id == 0 || rhs.wand_id == this->wand_id ||
+           rhs.wand_id == (this->wand_id >> MAGIQUEST_LEGACY_WAND_ID_SHIFT);
   }
 };
 

tests/components/remote_base/__init__.py

@@ -14,6 +14,7 @@ def override_manifest(manifest: ComponentManifestOverride) -> None:
             "aeha",
             "keeloq",
             "nexus",
+            "magiquest",
             "rc5x",
             "panasonic",
         ):

tests/components/remote_base/magiquest_test.cpp

@@ -0,0 +1,126 @@
+#include <gtest/gtest.h>
+#include "esphome/components/remote_base/magiquest_protocol.h"
+
+namespace esphome::remote_base::testing {
+
+namespace {
+
+constexpr int32_t UNIT = 288;
+constexpr uint32_t WAND_ID = 0x2A5B3C1D;
+constexpr uint16_t MAGNITUDE = 258;
+constexpr MagiQuestData DECODED{.magnitude = MAGNITUDE, .wand_id = WAND_ID};
+
+void add_bit(RawTimings &timings, bool one, bool last = false) {
+  timings.push_back(one ? 2 * UNIT : UNIT);
+  if (!last) {
+    timings.push_back(-(one ? 2 * UNIT : 3 * UNIT));
+  }
+}
+
+// Overwrite bit `bit` of a built frame with a one
+void set_one(RawTimings &timings, size_t bit) {
+  timings[2 * bit] = 2 * UNIT;
+  timings[2 * bit + 1] = -2 * UNIT;
+}
+
+// Build a frame the way a wand sends it: 8 zero header bits, a 31 bit wand id, a 9 bit magnitude and a checksum
+// that makes the six payload bytes sum to zero. The last bit has no trailing space.
+RawTimings build_frame(uint32_t wand_id, uint16_t magnitude, uint8_t checksum_offset = 0) {
+  const uint32_t id_bytes = wand_id << 1;
+  const uint32_t magnitude_bits = uint32_t(magnitude) << 8;
+  uint8_t sum = 0;
+  for (uint8_t shift = 0; shift < 32; shift += 8) {
+    sum += (id_bytes >> shift) + (magnitude_bits >> shift);
+  }
+  const uint32_t magnitude_and_checksum = magnitude_bits | uint8_t(-sum + checksum_offset);
+
+  RawTimings timings;
+  for (int i = 0; i < 8; i++) {
+    add_bit(timings, false);
+  }
+  for (uint32_t mask = 1 << 30; mask; mask >>= 1) {
+    add_bit(timings, wand_id & mask);
+  }
+  for (uint32_t mask = 1 << 16; mask; mask >>= 1) {
+    add_bit(timings, magnitude_and_checksum & mask, mask == 1);
+  }
+  return timings;
+}
+
+// The receiver's tolerance is passed for completeness; decode() replaces it with the protocol's fixed window
+optional<MagiQuestData> decode(const RawTimings &timings) {
+  return MagiQuestProtocol().decode(RemoteReceiveData(timings, 25, TOLERANCE_MODE_PERCENTAGE));
+}
+
+void expect_decodes_to(const RawTimings &timings, uint32_t wand_id, uint16_t magnitude) {
+  auto decoded = decode(timings);
+  ASSERT_TRUE(decoded.has_value());
+  // clang-tidy's unchecked-optional-access models neither gtest's ASSERT_TRUE nor value() as a check
+  if (decoded.has_value()) {
+    EXPECT_EQ(decoded->wand_id, wand_id);
+    EXPECT_EQ(decoded->magnitude, magnitude);
+  }
+}
+
+}  // namespace
+
+TEST(MagiQuestProtocolTest, DecodesAValidFrame) {
+  expect_decodes_to(build_frame(WAND_ID, MAGNITUDE), WAND_ID, MAGNITUDE);
+}
+
+// Magnitude 1 with wand id 0 needs a checksum of 0xFF, so the last bit is a one; magnitude 2 needs 0xFE.
+TEST(MagiQuestProtocolTest, DecodesBothValuesOfTheLastBit) {
+  expect_decodes_to(build_frame(0, 1), 0, 1);
+  expect_decodes_to(build_frame(0, 2), 0, 2);
+}
+
+TEST(MagiQuestProtocolTest, RejectsABadChecksum) {
+  EXPECT_FALSE(decode(build_frame(WAND_ID, MAGNITUDE, 1)).has_value());
+}
+
+TEST(MagiQuestProtocolTest, RejectsAFlippedDataBit) {
+  auto timings = build_frame(WAND_ID, MAGNITUDE);
+  set_one(timings, 8);  // first wand id bit, a zero in WAND_ID
+  EXPECT_FALSE(decode(timings).has_value());
+}
+
+TEST(MagiQuestProtocolTest, RejectsANonZeroHeader) {
+  auto timings = build_frame(WAND_ID, MAGNITUDE);
+  set_one(timings, 0);
+  EXPECT_FALSE(decode(timings).has_value());
+}
+
+TEST(MagiQuestProtocolTest, RejectsATruncatedFrame) {
+  auto timings = build_frame(WAND_ID, MAGNITUDE);
+  timings.resize(timings.size() - 10);
+  EXPECT_FALSE(decode(timings).has_value());
+}
+
+// Receivers stretch marks and shrink spaces; 120 us is more than 25% of a unit but within the fixed window.
+TEST(MagiQuestProtocolTest, DecodesWithReceiverJitterUnderTheDefaultTolerance) {
+  auto timings = build_frame(WAND_ID, MAGNITUDE);
+  for (auto &timing : timings) {
+    timing += 120;
+  }
+  expect_decodes_to(timings, WAND_ID, MAGNITUDE);
+}
+
+TEST(MagiQuestProtocolTest, MatchesAConfiguredWand) {
+  EXPECT_TRUE(DECODED == (MagiQuestData{.magnitude = 0, .wand_id = WAND_ID}));
+  EXPECT_FALSE(DECODED == (MagiQuestData{.magnitude = 0, .wand_id = WAND_ID + 1}));
+}
+
+TEST(MagiQuestProtocolTest, MatchesAnyWandWhenNoIdIsConfigured) {
+  EXPECT_TRUE(DECODED == (MagiQuestData{.magnitude = 0, .wand_id = 0}));
+}
+
+// Configs written for the old decoder hold the id without its 5 low bits.
+TEST(MagiQuestProtocolTest, MatchesAnIdFromTheOldDecoder) {
+  EXPECT_TRUE(DECODED == (MagiQuestData{.magnitude = 0, .wand_id = WAND_ID >> 5}));
+}
+
+TEST(MagiQuestProtocolTest, IgnoresMagnitudeWhenMatching) {
+  EXPECT_TRUE(DECODED == (MagiQuestData{.magnitude = 0xFFFF, .wand_id = WAND_ID}));
+}
+
+}  // namespace esphome::remote_base::testing

tests/components/remote_receiver/esp32-common.yaml

@@ -32,6 +32,13 @@ binary_sensor:
     name: Hob2Hood Light On
     hob2hood:
       command: light_on
+  - platform: remote_receiver
+    name: MagiQuest Wand
+    magiquest:
+      wand_id: 0x0F1E2D3C
+  - platform: remote_receiver
+    name: MagiQuest Any Wand
+    magiquest: {}
   - platform: remote_receiver
     name: Nexus Sensor
     nexus: