diff --git a/resources/sources/Baremetal/Baremetal.ino b/resources/sources/Baremetal/Baremetal.ino index 292784180..c1c8ff44e 100644 --- a/resources/sources/Baremetal/Baremetal.ino +++ b/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++) diff --git a/resources/sources/Baremetal/modbus_registers.cpp b/resources/sources/Baremetal/modbus_registers.cpp index a1be65c24..472556cda 100644 --- a/resources/sources/Baremetal/modbus_registers.cpp +++ b/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); diff --git a/resources/sources/Baremetal/modbus_registers.h b/resources/sources/Baremetal/modbus_registers.h index 27258273b..44b19e00a 100644 --- a/resources/sources/Baremetal/modbus_registers.h +++ b/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); diff --git a/resources/sources/Baremetal/modbus_types.h b/resources/sources/Baremetal/modbus_types.h index 85f3dcda4..709ad62bd 100644 --- a/resources/sources/Baremetal/modbus_types.h +++ b/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 diff --git a/resources/sources/arduino/arduino_runtime_glue.cpp b/resources/sources/arduino/arduino_runtime_glue.cpp index 48b77c812..11b20f0b5 100644 --- a/resources/sources/arduino/arduino_runtime_glue.cpp +++ b/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; }