From b99d18241ace3574f4a3af9e0ad10bb16eac77ac Mon Sep 17 00:00:00 2001 From: Michael Adams Date: Wed, 12 Aug 2026 14:20:43 +1200 Subject: [PATCH 1/3] Fix init packets clobbering output buffer --- README.md | 20 ++++++------ src/CMRI.cpp | 51 +++++++++++++++++++++-------- src/CMRI.h | 9 +++++- test/test_cmri/test_main.cpp | 62 ++++++++++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index d694b2b..f4330e8 100644 --- a/README.md +++ b/README.md @@ -63,38 +63,38 @@ If you wanted to extend this demo to transmit data back to the PC, all you need Documentation ------------- -**CMRI(unsigned int address = 0, unsigned int input\_bits = 24, unsigned int output\_bits = 48)** +`CMRI(unsigned int address = 0, unsigned int input_bits = 24, unsigned int output_bits = 48)` Creates a new CMRI object. The default values will create a device that matches the capabilities of an SMINI node. If you want to bind to a different node address, or address more or less inputs, you can alter it here. The maximum combined number of addressable inputs and outputs is 2048 (C/MRI limitation). The library will work fine with any number of inputs and outputs, it will simply ignore out-of-range data. -**void set\_address(unsigned int address)** +`void set_address(unsigned int address)` Sets the address of the C/MRI node. -**char process()** +`char process()` Reads in available data from the serial port and acts accordingly: * For POLL requests, it replies with the current state of the input data. * For INIT requests, it does nothing. * For SET/TRANSMIT (T) requests, it updates the output data. -Return value is NULL for no valid packet received, or one of CMRI::INIT, CMRI::SET, CMRI::POLL depending on the packet type received. +Return value is `true` for `POLL` and `SET` messages, otherwise `false`. -**bool process\_char(char c)** +`bool process_char(char c)` Similar to the CMRI::process method, but lets you manage the serial data yourself. Use this if you are processing more than 1 CMRI node in a system. Return value is true if a valid packet has been received and processing of it has finished. Otherwise it returns false. -**void transmit()** +`void transmit()` Transmits the current state of the input data back to the PC. Creates a CMRI::GET packet. -**bool get\_bit(int n)** +`bool get_bit(int n)` Reads a bit from of the last valid input data received. Use this to update your signals, points, etc. -**char get\_byte(int n)** +`char get_byte(int n)` Reads an entire byte from the input buffer. Use this with shiftOut and some shift registers to vastly expand your I/O capabilities. -**bool set\_bit(int n, bool b)** +`bool set_bit(int n, bool b)` Updates the output buffer to the specified value. Data will be transmitted to the PC either when transmit() is called, or when the next POLL packet is received. -**bool set\_byte(int n, char b)** +`bool set_byte(int n, char b)` Updates an entire byte of the output buffer. Use this with shiftIn and some shift registers to add many extra digital inputs to your system. diff --git a/src/CMRI.cpp b/src/CMRI.cpp index e359b1e..b348ca3 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), _init_handler(nullptr), _init_buffer(nullptr), _init_length(0) { // clear to zero @@ -57,6 +57,19 @@ void CMRI::set_address(unsigned int address) void CMRI::set_init_handler(void (*handler)(const uint8_t *, int)) { _init_handler = handler; + + // Lazily allocate a dedicated buffer for INIT payloads the first time a + // handler is registered, so INIT bodies are decoded away from the output + // image in _rx_buffer. Sized to the output length (with a small floor to + // still capture the NDP + delay header on input-only nodes), which covers + // standard C/MRI INIT bodies; anything longer is safely truncated. + if (handler && _init_buffer == nullptr) + { + _init_length = _rx_length > 4 ? _rx_length : 4; + _init_buffer = (char *)malloc(_init_length); + if (_init_buffer == nullptr) + _init_length = 0; // allocation failed: discard INIT bodies rather than risk a null write + } } // reads in serial data, decodes packets @@ -88,11 +101,16 @@ bool CMRI::process_char(char c) return true; case SET: - case INIT: - if (ret == INIT && _init_handler) - _init_handler((const uint8_t *)_rx_buffer, _rx_data_len); + // a SET updated the output image; tell the caller to refresh outputs return true; + case INIT: + // an INIT never touches the output image, so it must not signal + // "outputs updated"; deliver the payload via the handler instead + if (_init_handler) + _init_handler((const uint8_t *)_init_buffer, _rx_data_len); + return false; + default: return false; } @@ -162,6 +180,20 @@ void CMRI::transmit() } // Private methods + +// Append one decoded body byte to the buffer belonging to the packet currently +// being parsed: SET data lands in the output image (_rx_buffer), while INIT data +// is kept separate in _init_buffer so it can never corrupt the layout outputs. +// Bytes past the target buffer's length (or with no buffer allocated) are dropped. +void CMRI::_store_data_byte(uint8_t c) +{ + char *buffer = (_rx_packet_type == INIT) ? _init_buffer : _rx_buffer; + int length = (_rx_packet_type == INIT) ? _init_length : _rx_length; + + if (buffer != nullptr && _rx_index < length) + buffer[_rx_index++] = c; +} + uint8_t CMRI::_decode(uint8_t c) { switch (_mode) @@ -216,19 +248,12 @@ uint8_t CMRI::_decode(uint8_t c) _mode = DECODE_ESC_DATA; else if (c == ETX) goto POSTAMBLE_SET; - else if (_rx_index >= _rx_length) - { - } else - _rx_buffer[_rx_index++] = c; + _store_data_byte(c); break; case DECODE_ESC_DATA: - if (_rx_index >= _rx_length) - { - } - else - _rx_buffer[_rx_index++] = c; + _store_data_byte(c); _mode = DECODE_DATA; break; diff --git a/src/CMRI.h b/src/CMRI.h index 52b453a..67865a3 100644 --- a/src/CMRI.h +++ b/src/CMRI.h @@ -86,13 +86,20 @@ class CMRI int _rx_data_len; void (*_init_handler)(const uint8_t *, int); + // INIT ('I') payloads are decoded into their own buffer so they never + // overwrite the SET ('T') output image held in _rx_buffer. Only allocated + // when an init handler is registered; otherwise INIT bodies are discarded. + char *_init_buffer; + int _init_length; + Stream &_serial; // parsing state variables int _mode; int _rx_index; - uint8_t _decode(uint8_t c); // process one character received from serial port + uint8_t _decode(uint8_t c); // process one character received from serial port + void _store_data_byte(uint8_t c); // append a body byte to the buffer for the current packet type }; #endif diff --git a/test/test_cmri/test_main.cpp b/test/test_cmri/test_main.cpp index e4b60aa..4e3a9a8 100644 --- a/test/test_cmri/test_main.cpp +++ b/test/test_cmri/test_main.cpp @@ -141,6 +141,66 @@ void test_set_packet_updates_outputs(void) TEST_ASSERT_EQUAL_UINT8(0x80, cmri.get_byte(2)); } +// Regression for the v1.7.0 INIT bug: an INIT ('I') frame must not disturb the +// output image set by a previous SET. Before the fix, INIT bodies were decoded +// into _rx_buffer, transiently driving the INIT header bytes onto the outputs. +static uint8_t g_init_len; +static uint8_t g_init_data[8]; +static void capture_init(const uint8_t *data, int len) +{ + g_init_len = (uint8_t)len; + for (int i = 0; i < len && i < (int)sizeof(g_init_data); i++) + g_init_data[i] = data[i]; +} + +void test_init_does_not_corrupt_outputs(void) +{ + Stream s; + CMRI cmri(0, 24, 48, s); + cmri.set_init_handler(capture_init); + + // Establish a known output image. + uint8_t set_data[6] = {0xAA, 0x55, 0x0F, 0xF0, 0x12, 0x34}; + feed_packet(s, 0, CMRI::SET, set_data, 6); + TEST_ASSERT_TRUE(cmri.process()); + TEST_ASSERT_EQUAL_UINT8(0xAA, cmri.get_byte(0)); + + // A typical JMRI INIT: NDP='M', DLH, DLL, NS. bit0 of 'M' (0x4D) is 1 — the + // value that used to leak onto output pin 0. + g_init_len = 0xEE; + uint8_t init_data[4] = {'M', 0x00, 0x0A, 0x00}; + feed_packet(s, 0, CMRI::INIT, init_data, 4); + + // INIT must not report "outputs updated"... + TEST_ASSERT_FALSE(cmri.process()); + // ...the handler must have fired with the raw payload... + TEST_ASSERT_EQUAL_UINT8(4, g_init_len); + TEST_ASSERT_EQUAL_UINT8('M', g_init_data[0]); + TEST_ASSERT_EQUAL_UINT8(0x0A, g_init_data[2]); + // ...and the output image must be exactly what the SET left behind. + TEST_ASSERT_EQUAL_UINT8(0xAA, cmri.get_byte(0)); + TEST_ASSERT_EQUAL_UINT8(0x55, cmri.get_byte(1)); + TEST_ASSERT_EQUAL_UINT8(0x34, cmri.get_byte(5)); + TEST_ASSERT_FALSE(cmri.get_bit(0)); // bit0 of byte0 (0xAA) — not the leaked 'M' bit +} + +// Without a registered handler, INIT is ignored entirely (pre-v1.7.0 behaviour): +// no buffer is allocated, outputs are untouched, and process() reports false. +void test_init_ignored_without_handler(void) +{ + Stream s; + CMRI cmri(0, 24, 48, s); + + uint8_t set_data[6] = {0xAA, 0x55, 0x0F, 0xF0, 0x12, 0x34}; + feed_packet(s, 0, CMRI::SET, set_data, 6); + TEST_ASSERT_TRUE(cmri.process()); + + uint8_t init_data[4] = {'M', 0x00, 0x0A, 0x00}; + feed_packet(s, 0, CMRI::INIT, init_data, 4); + TEST_ASSERT_FALSE(cmri.process()); + TEST_ASSERT_EQUAL_UINT8(0xAA, cmri.get_byte(0)); +} + // A packet addressed to another node is ignored: process() is false and no // outputs change. void test_address_filtering(void) @@ -203,6 +263,8 @@ int main(int, char **) RUN_TEST(test_byte_bounds); RUN_TEST(test_poll_produces_get_frame); RUN_TEST(test_set_packet_updates_outputs); + RUN_TEST(test_init_does_not_corrupt_outputs); + RUN_TEST(test_init_ignored_without_handler); RUN_TEST(test_address_filtering); RUN_TEST(test_transmit_escapes_control_bytes); RUN_TEST(test_preamble_resync_after_garbage); From 2577d4f33357bea86494c4bb6bb86f60972788f4 Mon Sep 17 00:00:00 2001 From: Michael Adams Date: Wed, 12 Aug 2026 14:25:23 +1200 Subject: [PATCH 2/3] Linter fix --- src/CMRI.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/CMRI.h b/src/CMRI.h index 67865a3..0a87c3d 100644 --- a/src/CMRI.h +++ b/src/CMRI.h @@ -98,8 +98,8 @@ class CMRI int _mode; int _rx_index; - uint8_t _decode(uint8_t c); // process one character received from serial port - void _store_data_byte(uint8_t c); // append a body byte to the buffer for the current packet type + uint8_t _decode(uint8_t c); // process one character received from serial port + void _store_data_byte(uint8_t c); // append a body byte to the buffer for the current packet type }; #endif From 39ca51da71cd1e1b66c1deb4fcdf9cb094c513d5 Mon Sep 17 00:00:00 2001 From: Michael Adams Date: Wed, 12 Aug 2026 14:30:57 +1200 Subject: [PATCH 3/3] Guard and version bump --- library.json | 2 +- library.properties | 2 +- src/CMRI.cpp | 2 +- src/CMRI.h | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/library.json b/library.json index 2639aa5..d0133fa 100644 --- a/library.json +++ b/library.json @@ -1,6 +1,6 @@ { "name": "CMRI", - "version": "1.7.0", + "version": "1.7.1", "description": "A library for interfacing Arduino with the C/MRI computer control system for model railroads. This library allows you to easily interface your Arduino with JMRI (Java Model Railroad Interface) by emulating Bruce Chubb's Computer/Model Railroad Interface (C/MRI) System. It provides a simple API to handle GET, SET, and POLL requests from JMRI automatically, with support for up to 2048 digital lines.", "keywords": "CMRI, JMRI, model-railroad, arduino, communication", "repository": { diff --git a/library.properties b/library.properties index 23c51b4..d557644 100644 --- a/library.properties +++ b/library.properties @@ -1,5 +1,5 @@ name=CMRI -version=1.7.0 +version=1.7.1 author=Michael Adams maintainer=Michael Adams sentence=A library for interfacing Arduino with the C/MRI computer control system for model railroads. diff --git a/src/CMRI.cpp b/src/CMRI.cpp index b348ca3..6ca705f 100644 --- a/src/CMRI.cpp +++ b/src/CMRI.cpp @@ -107,7 +107,7 @@ bool CMRI::process_char(char c) case INIT: // an INIT never touches the output image, so it must not signal // "outputs updated"; deliver the payload via the handler instead - if (_init_handler) + if (_init_handler && _init_buffer) _init_handler((const uint8_t *)_init_buffer, _rx_data_len); return false; diff --git a/src/CMRI.h b/src/CMRI.h index 0a87c3d..0d55e00 100644 --- a/src/CMRI.h +++ b/src/CMRI.h @@ -26,7 +26,7 @@ #ifndef CMRI_h #define CMRI_h -#define _CMRI_VERSION 1.7.0 // version of this library +#define _CMRI_VERSION 1.7.1 // version of this library #include class CMRI