From 2d4659cd174ee5b555b2ef70e674e54053f282c5 Mon Sep 17 00:00:00 2001 From: David Conran Date: Wed, 14 Jul 2021 23:01:24 +1000 Subject: [PATCH] Refactor `decodeMitsubishiAC()` (#1532) * Use standard code methods to decode it rather than spaghetti. * Remove Unit Test cases that tried to match bad data. We are just going to ignore bad signals from now on. * Ensure repeated messages are identical. * Do the message compliance tests earlier so if it isn't a valid message we can quit earlier. Fixes #1523 --- src/ir_Mitsubishi.cpp | 149 ++++++++++-------------------------- test/ir_Mitsubishi_test.cpp | 128 ------------------------------- 2 files changed, 39 insertions(+), 238 deletions(-) diff --git a/src/ir_Mitsubishi.cpp b/src/ir_Mitsubishi.cpp index bcd109b71..592d93b54 100644 --- a/src/ir_Mitsubishi.cpp +++ b/src/ir_Mitsubishi.cpp @@ -257,120 +257,49 @@ void IRsend::sendMitsubishiAC(const unsigned char data[], const uint16_t nbytes, bool IRrecv::decodeMitsubishiAC(decode_results *results, uint16_t offset, const uint16_t nbits, const bool strict) { - if (results->rawlen <= ((kMitsubishiACBits * 2) + 2) + offset) { - DPRINTLN("Shorter than shortest possibly expected."); - return false; // Shorter than shortest possibly expected. - } - if (strict && nbits != kMitsubishiACBits) { - DPRINTLN("Request is out of spec."); - return false; // Request is out of spec. - } - for (uint8_t i = 0; i < kMitsubishiACStateLength; i++) results->state[i] = 0; - bool failure = false; - uint8_t rep = 0; - do { - failure = false; - // Header: - // Sometime happens that junk signals arrives before the real message - bool headerFound = false; - while (!headerFound && - offset < (results->rawlen - (kMitsubishiACBits * 2 + 2))) { - headerFound = - matchMark(results->rawbuf[offset], kMitsubishiAcHdrMark) && - matchSpace(results->rawbuf[offset + 1], kMitsubishiAcHdrSpace); - offset += 2; - } - if (!headerFound) { - DPRINTLN("Header mark not found."); - return false; - } - DPRINT("Header mark found at #"); - DPRINTLN(offset - 2); - // Decode byte-by-byte: - match_result_t data_result; - for (uint8_t i = 0; i < kMitsubishiACStateLength && !failure; i++) { - results->state[i] = 0; - data_result = - matchData(&(results->rawbuf[offset]), 8, kMitsubishiAcBitMark, - kMitsubishiAcOneSpace, kMitsubishiAcBitMark, - kMitsubishiAcZeroSpace, - _tolerance + kMitsubishiAcExtraTolerance, 0, false); - if (data_result.success == false) { - failure = true; - DPRINT("Byte decode failed at #"); - DPRINTLN((uint16_t)i); - } else { - results->state[i] = data_result.data; - offset += data_result.used; - DPRINT((uint16_t)results->state[i]); - DPRINT(","); - } - DPRINTLN(""); - } - // HEADER validation: - if (failure || results->state[0] != 0x23 || results->state[1] != 0xCB || - results->state[2] != 0x26 || results->state[3] != 0x01 || - results->state[4] != 0x00) { - DPRINTLN("Header mismatch."); - failure = true; - } else { - // DATA part: - - // FOOTER checksum: - if (!IRMitsubishiAC::validChecksum(results->state)) { - DPRINTLN("Checksum error."); - failure = true; - } - } - if (rep != kMitsubishiACMinRepeat && failure) { - bool repeatMarkFound = false; - while (!repeatMarkFound && - offset < (results->rawlen - (kMitsubishiACBits * 2 + 4))) { - repeatMarkFound = - matchMark(results->rawbuf[offset], kMitsubishiAcRptMark) && - matchSpace(results->rawbuf[offset + 1], kMitsubishiAcRptSpace); - offset += 2; - } - if (!repeatMarkFound) { - DPRINTLN("First attempt failure and repeat mark not found."); - return false; + // Compliance + if (strict && nbits != kMitsubishiACBits) return false; // Out of spec. + // Do we need to look for a repeat? + const uint16_t expected_repeats = strict ? kMitsubishiACMinRepeat : kNoRepeat; + // Enough data? + if (results->rawlen <= (nbits * 2 + kHeader + kFooter) * + (expected_repeats + 1) + offset - 1) return false; + uint16_t save[kStateSizeMax]; + // Handle repeats if we need too. + for (uint16_t r = 0; r <= expected_repeats; r++) { + // Header + Data + Footer + uint16_t used = matchGeneric(results->rawbuf + offset, results->state, + results->rawlen - offset, nbits, + kMitsubishiAcHdrMark, kMitsubishiAcHdrSpace, + kMitsubishiAcBitMark, kMitsubishiAcOneSpace, + kMitsubishiAcBitMark, kMitsubishiAcZeroSpace, + kMitsubishiAcRptMark, kMitsubishiAcRptSpace, + r < expected_repeats, // At least? + _tolerance + kMitsubishiAcExtraTolerance, + 0, false); + if (!used) return false; // No match. + offset += used; + if (r) { // Is this a repeat? + // Repeats are expected to be exactly the same. + if (std::memcmp(save, results->state, nbits / 8) != 0) return false; + } else { // It is the first message. + // Compliance + if (strict) { + // Data signature check. + static const uint8_t signature[5] = {0x23, 0xCB, 0x26, 0x01, 0x00}; + if (std::memcmp(results->state, signature, 5) != 0) return false; + // Checksum verification. + if (!IRMitsubishiAC::validChecksum(results->state)) return false; } + // Save a copy of the state to compare with. + std::memcpy(save, results->state, nbits / 8); } - rep++; - // Check if the repeat is correct if we need strict decode: - if (strict && !failure) { - DPRINTLN("Strict repeat check enabled."); - // Repeat mark and space: - if (!matchMark(results->rawbuf[offset++], kMitsubishiAcRptMark) || - !matchSpace(results->rawbuf[offset++], kMitsubishiAcRptSpace)) { - DPRINTLN("Repeat mark error."); - return false; - } - // Header mark and space: - if (!matchMark(results->rawbuf[offset++], kMitsubishiAcHdrMark) || - !matchSpace(results->rawbuf[offset++], kMitsubishiAcHdrSpace)) { - DPRINTLN("Repeat header error."); - return false; - } - // Payload: - for (uint8_t i = 0; i < kMitsubishiACStateLength; i++) { - data_result = - matchData(&(results->rawbuf[offset]), 8, kMitsubishiAcBitMark, - kMitsubishiAcOneSpace, kMitsubishiAcBitMark, - kMitsubishiAcZeroSpace, - _tolerance + kMitsubishiAcExtraTolerance, 0, false); - if (data_result.success == false || - data_result.data != results->state[i]) { - DPRINTLN("Repeat payload error."); - return false; - } - offset += data_result.used; - } - } // strict repeat check - } while (failure && rep <= kMitsubishiACMinRepeat); + } + + // Success. results->decode_type = MITSUBISHI_AC; results->bits = nbits; - return !failure; + return true; } #endif // DECODE_MITSUBISHI_AC diff --git a/test/ir_Mitsubishi_test.cpp b/test/ir_Mitsubishi_test.cpp index 2b28dc588..ce726f43b 100644 --- a/test/ir_Mitsubishi_test.cpp +++ b/test/ir_Mitsubishi_test.cpp @@ -824,134 +824,6 @@ TEST(TestDecodeMitsubishiAC, DecodeRealExample) { EXPECT_STATE_EQ(expected, irsend.capture.state, kMitsubishiACBits); } -// Tests for decodeMitsubishiAC() when the first payload has an error. -TEST(TestDecodeMitsubishiAC, DecodeRealExampleRepeatNeeded) { - IRsendTest irsend(kGpioUnused); - IRrecv irrecv(kGpioUnused); - irsend.begin(); - - irsend.reset(); - // Mitsubishi AC "Power On, 16C, low fan, vane auto move". - uint16_t rawData[583] = { - 3476, 1708, 416, 1264, 420, 1260, 414, 400, 448, 390, 446, 392, 444, 1236, - 440, 400, 446, 392, 446, 1234, 440, 1266, 418, 396, 442, 1264, 420, 394, - 444, 394, 442, 1264, 422, 1260, 414, 398, 440, 1266, 418, 1264, 420, 392, - 446, 392, 444, 1264, 422, 392, 446, 392, 446, 1260, 414, 400, 448, 390, - 446, 392, 444, 394, 442, 396, 442, 398, 440, 424, 412, 400, 448, 390, 446, - 392, 446, 392, 444, 394, 442, 396, 442, 396, 440, 398, 438, 400, 448, 390, - 446, 392, 446, 392, 444, 396, 442, 396, 440, 398, 440, 400, 438, 400, 448, - 390, 446, 392, 444, 1236, 440, 1266, 418, 394, 442, 396, 440, 398, 438, - 402, 446, 1232, 444, 396, 440, 1268, 418, 394, 442, 396, 440, 398, - // space 699 is not recognizable: - 440, 400, 448, 390, 448, 1232, 442, 1266, 420, 394, 444, 1264, 699, 1260, - 416, 396, 440, 398, 450, 1230, 444, 396, 442, 398, 440, 1266, 418, 1264, - 422, 1258, 416, 1266, 418, 394, 442, 396, 440, 398, 440, 398, 438, 400, - 446, 392, 446, 392, 446, 392, 444, 396, 442, 396, 440, 398, 438, 398, 438, - 400, 448, 392, 446, 392, 444, 394, 444, 396, 442, 396, 440, 398, 438, 400, - 448, 390, 448, 392, 444, 394, 444, 394, 442, 396, 442, 396, 440, 398, 438, - 400, 448, 390, 446, 392, 446, 392, 444, 394, 442, 396, 442, 396, 440, 398, - 438, 400, 448, 390, 446, 392, 444, 394, 444, 394, 442, 396, 440, 398, 440, - 398, 438, 400, 448, 390, 446, 392, 444, 394, 444, 394, 442, 396, 440, 398, - 438, 400, 438, 400, 448, 392, 446, 392, 444, 394, 442, 396, 442, 396, 440, - 398, 438, 1240, 444, 1264, 422, 390, 446, 392, 446, 1260, 414, 1268, 418, - 1264, 422, 12984, 3478, 1708, 418, 1264, 422, 1234, 442, 398, 448, 390, - 446, 392, 446, 1234, 440, 400, 448, 390, 446, 1234, 442, 1266, 420, 392, - 444, 1264, 420, 392, 446, 394, 444, 1236, 448, 1260, 416, 398, 440, 1266, - 418, 1262, 422, 390, 446, 392, 444, 1234, 440, 400, 448, 392, 446, 1234, - 440, 398, 450, 390, 446, 392, 444, 394, 444, 394, 442, 396, 442, 398, 440, - 400, 438, 400, 448, 390, 446, 392, 444, 394, 442, 396, 442, 396, 440, 398, - 438, 400, 448, 390, 446, 392, 446, 392, 444, 394, 442, 396, 442, 396, 440, - 398, 438, 400, 448, 416, 420, 392, 444, 1234, 440, 1240, 446, 394, 442, - 396, 440, 398, 438, 400, 448, 1232, 444, 396, 440, 1240, 446, 394, 442, - 396, 440, 398, 440, 400, 448, 390, 446, 1232, 444, 1238, 446, 394, 444, - 1236, 448, 1232, 442, 396, 440, 398, 448, 1232, 444, 396, 440, 398, 438, - 1242, 444, 1238, 448, 1234, 442, 1240, 444, 394, 442, 396, 440, 398, 438, - 400, 448, 390, 446, 394, 444, 420, 416, 394, 444, 396, 440, 398, 440, 398, - 438, 400, 448, 418, 420, 418, 418, 394, 442, 396, 442, 396, 440, 424, 412, - 400, 448, 390, 446, 392, 446, 420, 418, 420, 416, 396, 440, 398, 440, 424, - 412, 426, 420, 418, 420, 392, 444, 394, 444, 422, 416, 422, 414, 398, 440, - 426, 422, 388, 448, 416, 420, 418, 418, 422, 416, 422, 414, 424, 414, 398, - 438, 426, 422, 418, 420, 390, 446, 418, 418, 420, 416, 396, 440, 424, 412, - 426, 412, 400, 446, 418, 420, 420, 418, 420, 416, 422, 414, 422, 414, 424, - 412, 426, 422, 390, 446, 1232, 442, 1240, 446, 394, 444, 394, 442, 1238, - 446, 1234, 440, 1240, 444}; // UNKNOWN F6FDB82B - - irsend.sendRaw(rawData, 583, 33); - irsend.makeDecodeResult(); - - ASSERT_TRUE(irrecv.decode(&irsend.capture)); - EXPECT_EQ(MITSUBISHI_AC, irsend.capture.decode_type); - EXPECT_EQ(kMitsubishiACBits, irsend.capture.bits); - uint8_t expected[kMitsubishiACStateLength] = { - 0x23, 0xCB, 0x26, 0x01, 0x00, 0x00, 0x18, 0x0A, 0x36, - 0x79, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xE6}; - EXPECT_STATE_EQ(expected, irsend.capture.state, kMitsubishiACBits); -} - -// Tests for decodeMitsubishiAC() when the repeat mark is wrong. -TEST(TestDecodeMitsubishiAC, DecodeRealExampleRepeatMarkError) { - IRsendTest irsend(kGpioUnused); - IRrecv irrecv(kGpioUnused); - irsend.begin(); - - irsend.reset(); - // Mitsubishi AC "Power On, 16C, low fan, vane auto move". - uint16_t rawData[583] = { - 3476, 1708, 416, 1264, 420, 1260, 414, 400, 448, 390, 446, 392, 444, 1236, - 440, 400, 446, 392, 446, 1234, 440, 1266, 418, 396, 442, 1264, 420, 394, - 444, 394, 442, 1264, 422, 1260, 414, 398, 440, 1266, 418, 1264, 420, 392, - 446, 392, 444, 1264, 422, 392, 446, 392, 446, 1260, 414, 400, 448, 390, - 446, 392, 444, 394, 442, 396, 442, 398, 440, 424, 412, 400, 448, 390, 446, - 392, 446, 392, 444, 394, 442, 396, 442, 396, 440, 398, 438, 400, 448, 390, - 446, 392, 446, 392, 444, 396, 442, 396, 440, 398, 440, 400, 438, 400, 448, - 390, 446, 392, 444, 1236, 440, 1266, 418, 394, 442, 396, 440, 398, 438, - 402, 446, 1232, 444, 396, 440, 1268, 418, 394, 442, 396, 440, 398, 440, - 400, 448, 390, 448, 1232, 442, 1266, 420, 394, 444, 1264, 422, 1260, 416, - 396, 440, 398, 450, 1230, 444, 396, 442, 398, 440, 1266, 418, 1264, 422, - 1258, 416, 1266, 418, 394, 442, 396, 440, 398, 440, 398, 438, 400, 446, - 392, 446, 392, 446, 392, 444, 396, 442, 396, 440, 398, 438, 398, 438, 400, - 448, 392, 446, 392, 444, 394, 444, 396, 442, 396, 440, 398, 438, 400, 448, - 390, 448, 392, 444, 394, 444, 394, 442, 396, 442, 396, 440, 398, 438, 400, - 448, 390, 446, 392, 446, 392, 444, 394, 442, 396, 442, 396, 440, 398, 438, - 400, 448, 390, 446, 392, 444, 394, 444, 394, 442, 396, 440, 398, 440, 398, - 438, 400, 448, 390, 446, 392, 444, 394, 444, 394, 442, 396, 440, 398, 438, - 400, 438, 400, 448, 392, 446, 392, 444, 394, 442, 396, 442, 396, 440, 398, - 438, 1240, 444, 1264, 422, 390, 446, 392, 446, - // Repeat mark (1111) wrong: - 1260, 414, 1268, 418, 1264, 422, 1111, 347, 1708, 418, 1264, 422, 1234, - 442, 398, 448, 390, 446, 392, 446, 1234, 440, 400, 448, 390, 446, 1234, - 442, 1266, 420, 392, 444, 1264, 420, 392, 446, 394, 444, 1236, 448, 1260, - 416, 398, 440, 1266, 418, 1262, 422, 390, 446, 392, 444, 1234, 440, 400, - 448, 392, 446, 1234, 440, 398, 450, 390, 446, 392, 444, 394, 444, 394, - 442, 396, 442, 398, 440, 400, 438, 400, 448, 390, 446, 392, 444, 394, 442, - 396, 442, 396, 440, 398, 438, 400, 448, 390, 446, 392, 446, 392, 444, 394, - 442, 396, 442, 396, 440, 398, 438, 400, 448, 416, 420, 392, 444, 1234, - 440, 1240, 446, 394, 442, 396, 440, 398, 438, 400, 448, 1232, 444, 396, - 440, 1240, 446, 394, 442, 396, 440, 398, 440, 400, 448, 390, 446, 1232, - 444, 1238, 446, 394, 444, 1236, 448, 1232, 442, 396, 440, 398, 448, 1232, - 444, 396, 440, 398, 438, 1242, 444, 1238, 448, 1234, 442, 1240, 444, 394, - 442, 396, 440, 398, 438, 400, 448, 390, 446, 394, 444, 420, 416, 394, 444, - 396, 440, 398, 440, 398, 438, 400, 448, 418, 420, 418, 418, 394, 442, 396, - 442, 396, 440, 424, 412, 400, 448, 390, 446, 392, 446, 420, 418, 420, 416, - 396, 440, 398, 440, 424, 412, 426, 420, 418, 420, 392, 444, 394, 444, 422, - 416, 422, 414, 398, 440, 426, 422, 388, 448, 416, 420, 418, 418, 422, 416, - 422, 414, 424, 414, 398, 438, 426, 422, 418, 420, 390, 446, 418, 418, 420, - 416, 396, 440, 424, 412, 426, 412, 400, 446, 418, 420, 420, 418, 420, 416, - 422, 414, 422, 414, 424, 412, 426, 422, 390, 446, 1232, 442, 1240, 446, - 394, 444, 394, 442, 1238, 446, 1234, 440, 1240, 444}; // UNKNOWN F6FDB82B - - irsend.sendRaw(rawData, 583, 33); - irsend.makeDecodeResult(); - - ASSERT_TRUE(irrecv.decode(&irsend.capture)); - EXPECT_EQ(MITSUBISHI_AC, irsend.capture.decode_type); - EXPECT_EQ(kMitsubishiACBits, irsend.capture.bits); - uint8_t expected[kMitsubishiACStateLength] = { - 0x23, 0xCB, 0x26, 0x01, 0x00, 0x00, 0x18, 0x0A, 0x36, - 0x79, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xE6}; - EXPECT_STATE_EQ(expected, irsend.capture.state, kMitsubishiACBits); -} - // Tests for decodeMitsubishiAC() when first payload has an error and the // repeat mark is wrong. TEST(TestDecodeMitsubishiAC, DecodeRealExampleRepeatNeededButError) {