diff --git a/src/CMRI.cpp b/src/CMRI.cpp index e359b1e..ede505e 100644 --- a/src/CMRI.cpp +++ b/src/CMRI.cpp @@ -39,7 +39,7 @@ CMRI::CMRI(unsigned int address, unsigned int input_bits, unsigned int output_bi // parsing state , - _mode(PREAMBLE_1), _rx_index(0), _rx_data_len(0), _init_handler(nullptr) + _mode(PREAMBLE_1), _rx_index(0), _rx_data_len(0), _rx_packet_type(NOOP), _init_handler(nullptr) { // clear to zero @@ -202,12 +202,17 @@ uint8_t CMRI::_decode(uint8_t c) else if (c == INIT) _mode = DECODE_DATA; else if (c == POLL) - goto POSTAMBLE_POLL; + // Consume poll body via IGNORE_DATA until ETX (DLE-aware), + // then reply from POSTAMBLE_IGNORE. + _mode = IGNORE_DATA; else _mode = POSTAMBLE_OTHER; break; case IGNORE_CMD: + // A frame addressed to another node must never trigger a reply, even if + // a previous poll left _rx_packet_type == POLL. + _rx_packet_type = NOOP; _mode = IGNORE_DATA; break; @@ -255,11 +260,11 @@ uint8_t CMRI::_decode(uint8_t c) _rx_index = 0; return _rx_packet_type; -POSTAMBLE_POLL: - _mode = PREAMBLE_1; - return POLL; - POSTAMBLE_IGNORE: _mode = PREAMBLE_1; + // POLL frames consume body to ETX above; reply after ETX + // to avoid RS-485 bus contention (host still transmitting). + if (_rx_packet_type == POLL) + return POLL; return NOOP; } diff --git a/src/CMRI.h b/src/CMRI.h index 52b453a..fac017c 100644 --- a/src/CMRI.h +++ b/src/CMRI.h @@ -72,8 +72,6 @@ class CMRI IGNORE_CMD, IGNORE_DATA, IGNORE_ESC_DATA, - POSTAMBLE_SET, - POSTAMBLE_POLL, POSTAMBLE_OTHER }; diff --git a/test/test_cmri/test_main.cpp b/test/test_cmri/test_main.cpp index e4b60aa..5618feb 100644 --- a/test/test_cmri/test_main.cpp +++ b/test/test_cmri/test_main.cpp @@ -176,6 +176,53 @@ void test_transmit_escapes_control_bytes(void) TEST_ASSERT_EQUAL_UINT8(CMRI::ETX, s.tx[10]); } +// A well-formed POLL waits for ETX before replying. +void test_poll_waits_for_etx(void) +{ + Stream s; + CMRI cmri(0, 24, 48, s); + + cmri.set_byte(0, 0x55); + + feed_packet(s, 0, CMRI::POLL, nullptr, 0); + TEST_ASSERT_TRUE(cmri.process()); + TEST_ASSERT_EQUAL_UINT8(CMRI::GET, s.tx[4]); + TEST_ASSERT_EQUAL_UINT8(0x55, s.tx[5]); +} + +// A POLL without ETX produces no reply. +void test_poll_truncated_no_reply(void) +{ + Stream s; + CMRI cmri(0, 24, 48, s); + + cmri.set_byte(0, 0x55); + + s.feed(0xFF); + s.feed(0xFF); + s.feed(CMRI::STX); + s.feed('A' + 0); + s.feed(CMRI::POLL); + + TEST_ASSERT_FALSE(cmri.process()); + TEST_ASSERT_EQUAL_UINT(0u, s.tx.size()); +} + +// A POLL with body bytes waits for ETX before replying. +void test_poll_with_body_waits_for_etx(void) +{ + Stream s; + CMRI cmri(0, 24, 48, s); + + cmri.set_byte(0, 0x77); + + uint8_t body[2] = {0x01, 0x02}; + feed_packet(s, 0, CMRI::POLL, body, 2); + TEST_ASSERT_TRUE(cmri.process()); + TEST_ASSERT_EQUAL_UINT8(CMRI::GET, s.tx[4]); + TEST_ASSERT_EQUAL_UINT8(0x77, s.tx[5]); +} + // Garbage before a valid packet is resynced away by the preamble state machine. void test_preamble_resync_after_garbage(void) { @@ -194,6 +241,81 @@ void test_preamble_resync_after_garbage(void) TEST_ASSERT_EQUAL_UINT8(CMRI::GET, s.tx[4]); } +// Regression: a node that has been polled (its _rx_packet_type is 'P') must NOT +// reply when it hears another node's POLL. Before the fix, finishing an ignored +// frame checked the stale packet type and spuriously transmitted a GET reply, +// which on a multi-node bus collided with the addressed node's response. +void test_no_reply_to_other_nodes_poll(void) +{ + Stream s; + CMRI cmri(0, 24, 48, s); // we are node 0 + + cmri.set_byte(0, 0x55); + + // JMRI polls us first, so _rx_packet_type latches to POLL. + feed_packet(s, 0, CMRI::POLL, nullptr, 0); + TEST_ASSERT_TRUE(cmri.process()); + + // Now JMRI polls node 1; we only hear it. We must stay silent. + s.tx.clear(); + feed_packet(s, 1, CMRI::POLL, nullptr, 0); + TEST_ASSERT_FALSE(cmri.process()); + TEST_ASSERT_EQUAL_UINT(0u, s.tx.size()); +} + +// Regression: same as above, but the ignored frame is another node's GET reply. +// This was the ping-pong: each node's reply triggered the other's stale reply. +void test_no_reply_to_other_nodes_get(void) +{ + Stream s; + CMRI cmri(0, 24, 48, s); // we are node 0 + + cmri.set_byte(0, 0x55); + + // JMRI polls us first, so _rx_packet_type latches to POLL. + feed_packet(s, 0, CMRI::POLL, nullptr, 0); + TEST_ASSERT_TRUE(cmri.process()); + + // Node 1's GET reply frame (as produced by its transmit()): FF FF STX 'B' 'R' data ETX. + s.tx.clear(); + s.feed(0xFF); + s.feed(0xFF); + s.feed(CMRI::STX); + s.feed('A' + 1); + s.feed(CMRI::GET); + s.feed(0xAA); + s.feed(0x00); + s.feed(0x00); + s.feed(CMRI::ETX); + + TEST_ASSERT_FALSE(cmri.process()); + TEST_ASSERT_EQUAL_UINT(0u, s.tx.size()); +} + +// A POLL addressed to us still replies exactly once, even after ignoring other +// nodes' frames in between (stale _rx_packet_type must not suppress it). +void test_poll_still_replies_after_ignoring(void) +{ + Stream s; + CMRI cmri(0, 24, 48, s); // we are node 0 + + cmri.set_byte(0, 0x66); + + // Ignore a SET and a POLL for node 1. + uint8_t set_data[6] = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06}; + feed_packet(s, 1, CMRI::SET, set_data, 6); + feed_packet(s, 1, CMRI::POLL, nullptr, 0); + + // Our own poll must still produce one reply. + feed_packet(s, 0, CMRI::POLL, nullptr, 0); + TEST_ASSERT_TRUE(cmri.process()); + + // Exactly one GET frame, with our staged byte. + TEST_ASSERT_EQUAL_UINT(9u, s.tx.size()); + TEST_ASSERT_EQUAL_UINT8(CMRI::GET, s.tx[4]); + TEST_ASSERT_EQUAL_UINT8(0x66, s.tx[5]); +} + int main(int, char **) { UNITY_BEGIN(); @@ -205,6 +327,12 @@ int main(int, char **) RUN_TEST(test_set_packet_updates_outputs); RUN_TEST(test_address_filtering); RUN_TEST(test_transmit_escapes_control_bytes); + RUN_TEST(test_poll_waits_for_etx); + RUN_TEST(test_poll_truncated_no_reply); + RUN_TEST(test_poll_with_body_waits_for_etx); + RUN_TEST(test_no_reply_to_other_nodes_poll); + RUN_TEST(test_no_reply_to_other_nodes_get); + RUN_TEST(test_poll_still_replies_after_ignoring); RUN_TEST(test_preamble_resync_after_garbage); return UNITY_END(); }