fix(baremetal): bound every I/O slot write, and stop truncating Modbus sizes

resources/sources/Baremetal/Baremetal.ino

@@ -272,14 +272,31 @@ void setup()
 // MAP EMPTY BUFFERS (for Modbus)
 // =============================================================================
 #ifdef MODBUS_ENABLED
+
+// Backing storage for discrete slots the PLC program did not claim.
+//
+// The analog and memory slots below alias straight into the Modbus banks,
+// which is what makes an unclaimed %QW readable over Modbus. The discrete
+// banks are bit-packed and cannot be aliased that way, so each unclaimed
+// bit needs a byte of its own -- this array is it.
+//
+// One static block rather than a malloc per slot: this used to call
+// malloc(1) once per unbound point, which on a board with a 15-slot
+// expansion backplane is ~480 one-byte allocations, each carrying its own
+// heap header (often 8 bytes, so ~8x the payload) and fragmenting the heap
+// before the program has run a single scan. A flat array costs exactly
+// MAX_DIGITAL_INPUT + MAX_DIGITAL_OUTPUT bytes, needs no allocator, and
+// cannot fail partway through and leave the image half-mapped
+// (openplc-editor#296).
+static IEC_BOOL empty_discrete[MAX_DIGITAL_INPUT + MAX_DIGITAL_OUTPUT] = {};
+
 void mapEmptyBuffers()
 {
     for (int i = 0; i < MAX_DIGITAL_OUTPUT; i++)
     {
         if (bool_output[i/8][i%8] == NULL)
         {
-            bool_output[i/8][i%8] = (IEC_BOOL *)malloc(sizeof(IEC_BOOL));
-            *bool_output[i/8][i%8] = 0;
+            bool_output[i/8][i%8] = &empty_discrete[i];
         }
     }
     for (int i = 0; i < MAX_ANALOG_OUTPUT; i++)
@@ -293,8 +310,8 @@ void mapEmptyBuffers()
     {
         if (bool_input[i/8][i%8] == NULL)
         {
-            bool_input[i/8][i%8] = (IEC_BOOL *)malloc(sizeof(IEC_BOOL));
-            *bool_input[i/8][i%8] = 0;
+            // Offset past the output half -- one array, two disjoint ranges.
+            bool_input[i/8][i%8] = &empty_discrete[MAX_DIGITAL_OUTPUT + i];
         }
     }
     for (int i = 0; i < MAX_ANALOG_INPUT; i++)

resources/sources/Baremetal/modbus_registers.cpp

@@ -9,7 +9,7 @@ Copyright (C) 2022 OpenPLC - Thiago Alves
 // In a debug-only build this whole TU compiles to nothing, saving flash/SRAM.
 #ifdef MODBUS_ENABLED
 
-bool init_mbregs(uint8_t size_holding, uint8_t size_dint_memory, uint8_t size_lint_memory, uint8_t size_coils, uint8_t size_inputregs, uint8_t size_inputstatus)
+bool init_mbregs(uint16_t size_holding, uint16_t size_dint_memory, uint16_t size_lint_memory, uint16_t size_coils, uint16_t size_inputregs, uint16_t size_inputstatus)
 {
     //Save sizes
     modbus.holding_size = size_holding;
@@ -62,9 +62,14 @@ bool init_mbregs(uint8_t size_holding, uint8_t size_dint_memory, uint8_t size_li
     return true;
 }
 
+// byte_addr is uint16_t, not uint8_t: addr is already a 16-bit Modbus
+// address, so addr/8 overflows a uint8_t past 2040 coils. The callers
+// bound `addr` against *_size before getting here, and *_size is now
+// itself 16-bit -- narrowing the index would put the truncation back one
+// step further down.
 bool get_discrete(uint16_t addr, bool regtype)
 {
-    uint8_t byte_addr = addr / 8;
+    uint16_t byte_addr = addr / 8;
     uint8_t bit_addr = addr % 8;
     if (regtype == COILS)
         return bitRead(modbus.coils[byte_addr], bit_addr);
@@ -74,7 +79,7 @@ bool get_discrete(uint16_t addr, bool regtype)
 
 void write_discrete(uint16_t addr, bool regtype, bool value)
 {
-    uint8_t byte_addr = addr / 8;
+    uint16_t byte_addr = addr / 8;
     uint8_t bit_addr = addr % 8;
     if (regtype == COILS)
         bitWrite(modbus.coils[byte_addr], bit_addr, value);
@@ -116,7 +121,9 @@ void readRegisters(uint16_t startreg, uint16_t numregs)
 
     uint16_t val;
     uint16_t i = 0;
-    uint8_t pos = 0;
+    // uint16_t, not uint8_t: pos indexes dint_memory/lint_memory, whose
+    // sizes come from the MAX_MEMORY_* macros and are no longer capped at 255.
+    uint16_t pos = 0;
 	while(numregs--)
     {
         if ((startreg + i) < modbus.holding_size)
@@ -177,7 +184,9 @@ void writeSingleRegister(uint16_t reg, uint16_t value)
         return;
     }
 
-    uint8_t pos = 0;
+    // uint16_t, not uint8_t: pos indexes dint_memory/lint_memory, whose
+    // sizes come from the MAX_MEMORY_* macros and are no longer capped at 255.
+    uint16_t pos = 0;
 
     if (reg < modbus.holding_size)
     {
@@ -254,7 +263,9 @@ void writeMultipleRegisters(uint16_t startreg, uint16_t numoutputs, uint8_t byte
 
     uint16_t value;
     uint16_t i = 0;
-    uint8_t pos = 0;
+    // uint16_t, not uint8_t: pos indexes dint_memory/lint_memory, whose
+    // sizes come from the MAX_MEMORY_* macros and are no longer capped at 255.
+    uint16_t pos = 0;
 	while(numoutputs--)
     {
         value = (uint16_t)mb_frame[7+i*2] << 8 | (uint16_t)mb_frame[8+i*2];
@@ -350,7 +361,12 @@ void readCoils(uint16_t startreg, uint16_t numregs)
 	while (numregs)
     {
         i = (totregs - numregs--) / 8;
-		if (get_discrete((uint8_t)startreg, COILS))
+		// No (uint8_t) cast on startreg: it is a 16-bit coil address, and
+		// truncating it aliased every coil above 255 onto a low one --
+		// FC 0x01 answered with the wrong bit and no error. Harmless while
+		// no board had more than 56 coils; reachable as soon as one does
+		// (openplc-editor#296). readInputStatus below never had the cast.
+		if (get_discrete(startreg, COILS))
 			bitSet(mb_frame[3+i], bitn);
 		else
 			bitClear(mb_frame[3+i], bitn);

resources/sources/Baremetal/modbus_registers.h

@@ -14,7 +14,9 @@ lives in modbus_frame.* because its slave id is shared by every build.
 
 #include "modbus_frame.h"
 
-bool init_mbregs(uint8_t size_holding, uint8_t size_dint_memory, uint8_t size_lint_memory, uint8_t size_coils, uint8_t size_inputregs, uint8_t size_inputstatus);
+// Sizes are uint16_t: they come straight from the MAX_* process-image
+// macros, which a board with an expansion backplane sizes past 255.
+bool init_mbregs(uint16_t size_holding, uint16_t size_dint_memory, uint16_t size_lint_memory, uint16_t size_coils, uint16_t size_inputregs, uint16_t size_inputstatus);
 bool get_discrete(uint16_t addr, bool regtype);
 void write_discrete(uint16_t addr, bool regtype, bool value);
 

resources/sources/Baremetal/modbus_types.h

@@ -56,21 +56,28 @@ protocol, transport, register and debug layers agree on the same contracts.
 // exceptions (0x01-0x04) nor 0x7E/0x81/0x82.
 #define MB_PLC_CTRL_REFUSED_SWITCH       0x86
 
-//Modbus registers struct
+// Modbus registers struct
+//
+// The *_size fields are uint16_t, not uint8_t: they are populated from the
+// MAX_* process-image macros, and a board with an expansion backplane sizes
+// those well past 255 (a 15-slot P1AM reaches 240 discrete points per
+// direction). As uint8_t the assignment in init_mbregs truncated silently --
+// 256 coils became 0 -- and the register map came up wrong with no
+// diagnostic anywhere (openplc-editor#296).
 struct MBinfo {
     uint8_t slaveid;
     uint16_t *holding;
-    uint8_t holding_size;
+    uint16_t holding_size;
     uint32_t *dint_memory;
-    uint8_t dint_memory_size;
+    uint16_t dint_memory_size;
     uint64_t *lint_memory;
-    uint8_t lint_memory_size;
+    uint16_t lint_memory_size;
     uint8_t *coils;
-    uint8_t coils_size;
+    uint16_t coils_size;
     uint16_t *input_regs;
-    uint8_t input_regs_size;
+    uint16_t input_regs_size;
     uint8_t *input_status;
-    uint8_t input_status_size;
+    uint16_t input_status_size;
 };
 
 //Function Codes

resources/sources/arduino/arduino_runtime_glue.cpp

@@ -119,6 +119,23 @@ static uint64_t gcd(uint64_t a, uint64_t b)
 
 // ---------------------------------------------------------------------------
 // I/O binding: walk locatedVars[] and bind to openplc.h buffer pointers
+//
+// Every slot write below is range-checked. locatedVars[] is authored from
+// whatever `AT %...` the user typed, and nothing in the descriptor itself
+// says how big this firmware's process image is -- so an address past the
+// end (`%QX7.0` on a 56-output image: byte_index 7 against bool_output[7][8])
+// used to write straight past the array and corrupt whatever followed it.
+// Only the DWord cases were guarded; the rest are now (openplc-editor#296).
+//
+// The editor rejects an out-of-range location before the build gets here,
+// so reaching a skip is not the expected path -- this is the backstop for a
+// hand-written .st, a project moved to a smaller board, or a stale build.
+// Dropping the binding leaves the slot NULL, which every HAL and the Modbus
+// glue already treat as "not wired" and step over.
+//
+// The bit-addressed buffers are declared [MAX/8][8], so the bound to check
+// is the FIRST dimension: an image whose digital count isn't a multiple of 8
+// rounds down, and the slots in the partial byte are unaddressable.
 // ---------------------------------------------------------------------------
 void runtime_bind_located_vars()
 {
@@ -131,10 +148,14 @@ void runtime_bind_located_vars()
         case LocatedArea::Input:
             switch (lv.size) {
             case LocatedSize::Bit:
-                bool_input[lv.byte_index][lv.bit_index] = (::IEC_BOOL*)lv.pointer;
+                if (lv.byte_index < (MAX_DIGITAL_INPUT / 8) && lv.bit_index < 8) {
+                    bool_input[lv.byte_index][lv.bit_index] = (::IEC_BOOL*)lv.pointer;
+                }
                 break;
             case LocatedSize::Word:
-                int_input[lv.byte_index] = (::IEC_UINT*)lv.pointer;
+                if (lv.byte_index < MAX_ANALOG_INPUT) {
+                    int_input[lv.byte_index] = (::IEC_UINT*)lv.pointer;
+                }
                 break;
 #if !defined(__AVR_ATmega328P__) && !defined(__AVR_ATmega168__) && !defined(__AVR_ATmega32U4__) && !defined(__AVR_ATmega16U4__)
             case LocatedSize::DWord:
@@ -159,10 +180,14 @@ void runtime_bind_located_vars()
         case LocatedArea::Output:
             switch (lv.size) {
             case LocatedSize::Bit:
-                bool_output[lv.byte_index][lv.bit_index] = (::IEC_BOOL*)lv.pointer;
+                if (lv.byte_index < (MAX_DIGITAL_OUTPUT / 8) && lv.bit_index < 8) {
+                    bool_output[lv.byte_index][lv.bit_index] = (::IEC_BOOL*)lv.pointer;
+                }
                 break;
             case LocatedSize::Word:
-                int_output[lv.byte_index] = (::IEC_UINT*)lv.pointer;
+                if (lv.byte_index < MAX_ANALOG_OUTPUT) {
+                    int_output[lv.byte_index] = (::IEC_UINT*)lv.pointer;
+                }
                 break;
 #if !defined(__AVR_ATmega328P__) && !defined(__AVR_ATmega168__) && !defined(__AVR_ATmega32U4__) && !defined(__AVR_ATmega16U4__)
             case LocatedSize::DWord:
@@ -185,13 +210,19 @@ void runtime_bind_located_vars()
 #if !defined(__AVR_ATmega328P__) && !defined(__AVR_ATmega168__) && !defined(__AVR_ATmega32U4__) && !defined(__AVR_ATmega16U4__)
             switch (lv.size) {
             case LocatedSize::Word:
-                int_memory[lv.byte_index] = (::IEC_UINT*)lv.pointer;
+                if (lv.byte_index < MAX_MEMORY_WORD) {
+                    int_memory[lv.byte_index] = (::IEC_UINT*)lv.pointer;
+                }
                 break;
             case LocatedSize::DWord:
-                dint_memory[lv.byte_index] = (::IEC_UDINT*)lv.pointer;
+                if (lv.byte_index < MAX_MEMORY_DWORD) {
+                    dint_memory[lv.byte_index] = (::IEC_UDINT*)lv.pointer;
+                }
                 break;
             case LocatedSize::LWord:
-                lint_memory[lv.byte_index] = (::IEC_ULINT*)lv.pointer;
+                if (lv.byte_index < MAX_MEMORY_LWORD) {
+                    lint_memory[lv.byte_index] = (::IEC_ULINT*)lv.pointer;
+                }
                 break;
             default: break;
             }