Fix init packets clobbering output buffer

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.
 
 

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": {

library.properties

@@ -1,5 +1,5 @@
 name=CMRI
-version=1.7.0
+version=1.7.1
 author=Michael Adams <github@michaeladams.org>
 maintainer=Michael Adams <github@michaeladams.org>
 sentence=A library for interfacing Arduino with the C/MRI computer control system for model railroads.

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_buffer)
+			_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;
 

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 <Arduino.h>
 
 class CMRI
@@ -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

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);