[remote_base] Fix NEC command_repeats to send repeat frames (#17663)

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

esphome/components/remote_base/__init__.py

@@ -1076,7 +1076,8 @@ NEC_SCHEMA = cv.Schema(
     {
         cv.Required(CONF_ADDRESS): cv.hex_uint16_t,
         cv.Required(CONF_COMMAND): cv.hex_uint16_t,
-        cv.Optional(CONF_COMMAND_REPEATS, default=1): cv.uint16_t,
+        # Non-RMT transmitters hold an interrupt lock for the whole send, ~108 ms per extra repeat
+        cv.Optional(CONF_COMMAND_REPEATS, default=1): cv.int_range(min=1, max=10),
     }
 )
 

esphome/components/remote_base/nec_protocol.cpp

@@ -1,4 +1,5 @@
 #include "nec_protocol.h"
+#include "esphome/core/helpers.h"
 #include "esphome/core/log.h"
 
 namespace esphome::remote_base {
@@ -10,35 +11,42 @@ static constexpr uint32_t HEADER_LOW_US = 4500;
 static constexpr uint32_t BIT_HIGH_US = 560;
 static constexpr uint32_t BIT_ONE_LOW_US = 1690;
 static constexpr uint32_t BIT_ZERO_LOW_US = 560;
+static constexpr uint32_t REPEAT_HEADER_LOW_US = HEADER_LOW_US / 2;
+// A held button repeats every 108 ms: the first gap depends on the frame's own length, later ones follow a repeat frame
+static constexpr uint32_t REPEAT_PERIOD_US = 108000;
+static constexpr uint32_t REPEAT_FRAME_US = HEADER_HIGH_US + REPEAT_HEADER_LOW_US + BIT_HIGH_US;
+static constexpr uint32_t REPEAT_GAP_US = REPEAT_PERIOD_US - REPEAT_FRAME_US;
+// Transmitters without an RMT peripheral hold an interrupt lock for the whole send, about 108 ms per repeat
+static constexpr uint16_t MAX_COMMAND_REPEATS = 10;
 
 void NECProtocol::encode(RemoteTransmitData *dst, const NECData &data) {
   ESP_LOGD(TAG, "Sending NEC: address=0x%04X, command=0x%04X command_repeats=%d", data.address, data.command,
            data.command_repeats);
 
-  dst->reserve(2 + 32 + 32 * data.command_repeats + 2);
+  // The schema bounds literal values; a lambda can produce anything, so clamp here too
+  const uint16_t repeats = clamp<uint16_t>(data.command_repeats, 1, MAX_COMMAND_REPEATS);
+  // Frame: header pair, 32 bit pairs, stop mark. Each extra repeat: gap space, header pair, mark.
+  dst->reserve(2 + 64 + 1 + (repeats - 1) * 4);
   dst->set_carrier_frequency(38000);
 
   dst->item(HEADER_HIGH_US, HEADER_LOW_US);
+  uint32_t frame_us = HEADER_HIGH_US + HEADER_LOW_US + BIT_HIGH_US;
 
-  for (uint16_t mask = 1; mask; mask <<= 1) {
-    if (data.address & mask) {
-      dst->item(BIT_HIGH_US, BIT_ONE_LOW_US);
-    } else {
-      dst->item(BIT_HIGH_US, BIT_ZERO_LOW_US);
-    }
-  }
-
-  for (uint16_t repeats = 0; repeats < data.command_repeats; repeats++) {
+  for (uint16_t value : {data.address, data.command}) {
     for (uint16_t mask = 1; mask; mask <<= 1) {
-      if (data.command & mask) {
-        dst->item(BIT_HIGH_US, BIT_ONE_LOW_US);
-      } else {
-        dst->item(BIT_HIGH_US, BIT_ZERO_LOW_US);
-      }
+      const uint32_t low_us = (value & mask) ? BIT_ONE_LOW_US : BIT_ZERO_LOW_US;
+      dst->item(BIT_HIGH_US, low_us);
+      frame_us += BIT_HIGH_US + low_us;
     }
   }
 
   dst->mark(BIT_HIGH_US);
+
+  for (uint16_t repeat = 1; repeat < repeats; repeat++) {
+    dst->space(repeat == 1 ? REPEAT_PERIOD_US - frame_us : REPEAT_GAP_US);
+    dst->item(HEADER_HIGH_US, REPEAT_HEADER_LOW_US);
+    dst->mark(BIT_HIGH_US);
+  }
 }
 optional<NECData> NECProtocol::decode(RemoteReceiveData src) {
   NECData data{
@@ -69,26 +77,7 @@ optional<NECData> NECProtocol::decode(RemoteReceiveData src) {
     }
   }
 
-  while (src.peek_item(BIT_HIGH_US, BIT_ONE_LOW_US) || src.peek_item(BIT_HIGH_US, BIT_ZERO_LOW_US)) {
-    uint16_t command = 0;
-    for (uint16_t mask = 1; mask; mask <<= 1) {
-      if (src.expect_item(BIT_HIGH_US, BIT_ONE_LOW_US)) {
-        command |= mask;
-      } else if (src.expect_item(BIT_HIGH_US, BIT_ZERO_LOW_US)) {
-        command &= ~mask;
-      } else {
-        return {};
-      }
-    }
-
-    // Make sure the extra/repeated data matches original command
-    if (command != data.command) {
-      return {};
-    }
-
-    data.command_repeats += 1;
-  }
-
+  // Repeat frames arrive as separate captures (receiver idle is far below the repeat gap) and are not decoded
   src.expect_mark(BIT_HIGH_US);
   return data;
 }

tests/components/remote_base/__init__.py

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

tests/components/remote_base/nec_protocol_test.cpp

@@ -0,0 +1,132 @@
+#include <gtest/gtest.h>
+#include <vector>
+#include "esphome/components/remote_base/nec_protocol.h"
+
+namespace esphome::remote_base::testing {
+
+// NEC timings from the spec (esphome/issues#6603), independent of the encoder constants
+static constexpr int32_t HEADER_MARK = 9000;
+static constexpr int32_t HEADER_SPACE = -4500;
+static constexpr int32_t REPEAT_HEADER_SPACE = -2250;
+static constexpr int32_t BIT_MARK = 560;
+static constexpr int32_t ONE_SPACE = -1690;
+static constexpr int32_t ZERO_SPACE = -560;
+static constexpr int32_t REPEAT_PERIOD = 108000;
+static constexpr int32_t REPEAT_GAP = -(REPEAT_PERIOD - (HEADER_MARK - REPEAT_HEADER_SPACE + BIT_MARK));
+
+// Builds a full NEC frame by hand: header, address and command LSB first, stop mark
+static std::vector<int32_t> build_frame(uint16_t address, uint16_t command) {
+  std::vector<int32_t> raw{HEADER_MARK, HEADER_SPACE};
+  for (uint16_t value : {address, command}) {
+    for (int bit = 0; bit < 16; bit++) {
+      raw.push_back(BIT_MARK);
+      raw.push_back((value >> bit) & 1 ? ONE_SPACE : ZERO_SPACE);
+    }
+  }
+  raw.push_back(BIT_MARK);
+  return raw;
+}
+
+/// The space that puts the first repeat frame 108 ms after the start of this frame
+static int32_t first_repeat_gap(const std::vector<int32_t> &frame) {
+  int32_t duration = 0;
+  for (int32_t item : frame) {
+    duration += item < 0 ? -item : item;
+  }
+  return -(REPEAT_PERIOD - duration);
+}
+
+TEST(NECProtocolTest, EncodeWithoutRepeats) {
+  NECProtocol protocol;
+  RemoteTransmitData dst;
+  protocol.encode(&dst, NECData{.address = 0x7F80, .command = 0xF20D, .command_repeats = 1});
+
+  EXPECT_EQ(dst.get_carrier_frequency(), 38000u);
+  EXPECT_EQ(dst.get_data(), build_frame(0x7F80, 0xF20D));
+}
+
+TEST(NECProtocolTest, EncodeWithRepeats) {
+  NECProtocol protocol;
+  RemoteTransmitData dst;
+  protocol.encode(&dst, NECData{.address = 0x7F80, .command = 0xF20D, .command_repeats = 3});
+
+  std::vector<int32_t> expected = build_frame(0x7F80, 0xF20D);
+  expected.insert(expected.end(), {first_repeat_gap(expected), HEADER_MARK, REPEAT_HEADER_SPACE, BIT_MARK});
+  expected.insert(expected.end(), {REPEAT_GAP, HEADER_MARK, REPEAT_HEADER_SPACE, BIT_MARK});
+  EXPECT_EQ(dst.get_data(), expected);
+}
+
+TEST(NECProtocolTest, FirstRepeatGapFollowsFrameLength) {
+  NECProtocol protocol;
+  RemoteTransmitData all_zeros;
+  protocol.encode(&all_zeros, NECData{.address = 0, .command = 0, .command_repeats = 2});
+  RemoteTransmitData all_ones;
+  protocol.encode(&all_ones, NECData{.address = 0xFFFF, .command = 0xFFFF, .command_repeats = 2});
+
+  const std::vector<int32_t> zeros_frame = build_frame(0, 0);
+  const std::vector<int32_t> ones_frame = build_frame(0xFFFF, 0xFFFF);
+  EXPECT_EQ(all_zeros.get_data()[zeros_frame.size()], first_repeat_gap(zeros_frame));
+  EXPECT_EQ(all_ones.get_data()[ones_frame.size()], first_repeat_gap(ones_frame));
+  // Both frames start their repeat 108 ms after they began
+  EXPECT_EQ(first_repeat_gap(zeros_frame) - first_repeat_gap(ones_frame),
+            static_cast<int32_t>(32 * (ONE_SPACE - ZERO_SPACE)));
+}
+
+TEST(NECProtocolTest, EncodeClampsRepeats) {
+  NECProtocol protocol;
+  RemoteTransmitData none;
+  protocol.encode(&none, NECData{.address = 0x1234, .command = 0x5678, .command_repeats = 0});
+  EXPECT_EQ(none.get_data().size(), 67u);
+
+  RemoteTransmitData many;
+  protocol.encode(&many, NECData{.address = 0x1234, .command = 0x5678, .command_repeats = 500});
+  EXPECT_EQ(many.get_data().size(), 67u + 4u * 9u);
+}
+
+TEST(NECProtocolTest, EncodeReserveMatchesSize) {
+  NECProtocol protocol;
+  RemoteTransmitData dst;
+  protocol.encode(&dst, NECData{.address = 0x1234, .command = 0x5678, .command_repeats = 5});
+
+  EXPECT_EQ(dst.get_data().size(), 67u + 4u * 4u);
+  EXPECT_GE(dst.get_data().capacity(), dst.get_data().size());
+}
+
+TEST(NECProtocolTest, DecodeFullFrame) {
+  NECProtocol protocol;
+  std::vector<int32_t> raw = build_frame(0x1234, 0x5678);
+  RemoteReceiveData rx(raw, 25, TOLERANCE_MODE_PERCENTAGE);
+  auto decoded = protocol.decode(rx);
+
+  ASSERT_TRUE(decoded.has_value());
+  // clang-tidy's unchecked-optional-access does not model gtest's ASSERT_TRUE as a check
+  if (decoded.has_value()) {
+    EXPECT_EQ(decoded->address, 0x1234);
+    EXPECT_EQ(decoded->command, 0x5678);
+    EXPECT_EQ(decoded->command_repeats, 1);
+  }
+}
+
+TEST(NECProtocolTest, DecodeWithoutStopMark) {
+  NECProtocol protocol;
+  std::vector<int32_t> raw = build_frame(0x1234, 0x5678);
+  raw.pop_back();
+  RemoteReceiveData rx(raw, 25, TOLERANCE_MODE_PERCENTAGE);
+  auto decoded = protocol.decode(rx);
+
+  ASSERT_TRUE(decoded.has_value());
+  if (decoded.has_value()) {
+    EXPECT_EQ(decoded->address, 0x1234);
+    EXPECT_EQ(decoded->command, 0x5678);
+  }
+}
+
+TEST(NECProtocolTest, DecodeStandaloneRepeatFrameRejected) {
+  NECProtocol protocol;
+  std::vector<int32_t> raw{HEADER_MARK, REPEAT_HEADER_SPACE, BIT_MARK};
+  RemoteReceiveData rx(raw, 25, TOLERANCE_MODE_PERCENTAGE);
+
+  EXPECT_FALSE(protocol.decode(rx).has_value());
+}
+
+}  // namespace esphome::remote_base::testing