From 707084ca56426e4b88a3eafd7ff0e5f5531c13d6 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 31 Aug 2026 18:33:08 -0300 Subject: [PATCH] feat(io): size the process image per board, and refuse locations outside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every arduino-cli target sized its I/O image from one hardcoded set of MAX_* macros in openplc.h, so a P1AM with a 15-slot expansion backplane (up to 240 discrete points) got the same 56 an Arduino Uno gets. The reporter hit the ceiling at %QX7.0 (openplc-editor#296). The ceiling was not the whole problem. Nothing checked it either: - runtime_bind_located_vars() wrote bool_input/bool_output/int_*/​*_memory at whatever byte_index the descriptor carried. Only the DWord cases had a bound. %QX7.0 on a 56-output image indexes bool_output[7][8] -- one past the end -- and corrupted whatever followed it. - The editor happily allocated and compiled such an address. The Python editor refused it at glue-code generation ("wrong location for var __QX7_0"); that check did not survive the move to strucpp, so between then and now the I/O silently did nothing. - init_mbregs() took its sizes as uint8_t, so 256 coils became 0, and readCoils() truncated the coil address to uint8_t, aliasing every coil above 255 onto a low one and answering with the wrong bit. So: make the sizes a per-target capability sourced from the VPP manifest (riding the path isLicensable already uses), emit them into defines.h, and have openplc.h take them through #ifndef guards. A target that declares none emits nothing and keeps compiling on the header's own #ifdef ladder -- byte-for-byte its current defines.h, which the emitter's snapshot tests pin. Then close the gaps: bound every slot write in the glue, widen the Modbus sizes and the coil address to 16 bits, and add a validate step that refuses an out-of-range location before the build starts, naming the board and the limit. Also replace mapEmptyBuffers()'s malloc(1)-per-unbound-point with one static block. At 240+240 points that was ~480 one-byte allocations, each with its own heap header, fragmenting the heap before the first scan. Editor side of openplc-editor#565 rides along, because the two share this code path: a located ARRAY claims one slot per element, so the range check measures its last element, and location validation checks the ELEMENT type against the address class instead of rejecting arrays outright. The empty error message for an unlocatable type is fixed too -- it rendered as a refusal with no reason at all. Co-Authored-By: Claude Opus 5 (1M context) --- resources/sources/Baremetal/Baremetal.ino | 25 +- .../sources/Baremetal/modbus_registers.cpp | 30 +- .../sources/Baremetal/modbus_registers.h | 4 +- resources/sources/Baremetal/modbus_types.h | 21 +- .../sources/arduino/arduino_runtime_glue.cpp | 45 ++- resources/sources/arduino/openplc.h | 67 ++++- .../__tests__/generate-defines.test.ts | 83 ++++++ .../shared/compile/__tests__/pipeline.test.ts | 72 +++++ .../__tests__/validate-process-image.test.ts | 278 ++++++++++++++++++ src/backend/shared/compile/pipeline.ts | 30 ++ .../shared/compile/steps/generate-defines.ts | 80 +++++ .../compile/steps/validate-process-image.ts | 232 +++++++++++++++ .../slices/project/validation/variables.ts | 32 +- .../shared/utils/target-capabilities/index.ts | 2 +- .../shared/utils/target-capabilities/types.ts | 69 +++++ 15 files changed, 1038 insertions(+), 32 deletions(-) create mode 100644 src/backend/shared/compile/__tests__/validate-process-image.test.ts create mode 100644 src/backend/shared/compile/steps/validate-process-image.ts diff --git a/resources/sources/Baremetal/Baremetal.ino b/resources/sources/Baremetal/Baremetal.ino index c94141608..3894c0b37 100644 --- a/resources/sources/Baremetal/Baremetal.ino +++ b/resources/sources/Baremetal/Baremetal.ino @@ -261,14 +261,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++) @@ -282,8 +299,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 be8fb1d5a..636882016 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 2967f9d45..1fa13a28d 100644 --- a/resources/sources/arduino/arduino_runtime_glue.cpp +++ b/resources/sources/arduino/arduino_runtime_glue.cpp @@ -117,6 +117,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() { @@ -129,10 +146,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: @@ -157,10 +178,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: @@ -183,13 +208,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; } diff --git a/resources/sources/arduino/openplc.h b/resources/sources/arduino/openplc.h index ddfc8d7e4..2308d196f 100644 --- a/resources/sources/arduino/openplc.h +++ b/resources/sources/arduino/openplc.h @@ -3,6 +3,26 @@ #include +/* Pulled in HERE, not left to the includer, so the MAX_* overrides below + * are visible in every translation unit that sees this header. + * + * The sketch and every HAL include "openplc.h" BEFORE "defines.h" (see + * Baremetal.ino), so an override arriving through the includer would land + * after the buffer arrays had already been declared at the fallback sizes + * -- and the .ino would size bool_output[] differently from the HAL that + * walks it. Including it from inside the guard removes the ordering + * question entirely. + * + * Safe to include from the extern "C" blocks the HALs wrap this header + * in, and safe to reach many times per build: defines.h is generated for + * every target, holds nothing but object-like #defines, and re-including + * it only ever re-defines each macro to the identical token sequence, + * which C explicitly permits (C11 6.10.3p2). It carries no include guard + * of its own, and deliberately isn't given one here -- that would change + * the generated bytes for every board, and this change is meant to leave + * boards that declare no process image byte-for-byte as they were. */ +#include "defines.h" + /*********************/ /* IEC Types defs */ /*********************/ @@ -27,16 +47,43 @@ typedef uint64_t IEC_LWORD; typedef float IEC_REAL; typedef double IEC_LREAL; -//OpenPLC Buffers Sizes +/* OpenPLC Buffers Sizes + * + * Every MAX_* below is a FALLBACK, guarded with #ifndef: whatever + * defines.h already set wins. defines.h carries these only when the + * target's VPP manifest declared a `processImage`, which is how a board + * with a 15-slot expansion backplane gets an image big enough to address + * it instead of the one-size-fits-all numbers here (openplc-editor#296). + * + * The two branches exist because the small AVRs have 2 KB of SRAM and + * cannot carry the general-purpose image at all -- they get no %M area + * whatsoever. That is exactly why the capability that feeds defines.h is + * optional rather than defaulted: no single preset can answer for both + * sides of this #if, so a board that declares nothing keeps landing on + * the branch that has always been right for it. */ #if defined(__AVR_ATmega328P__) || defined(__AVR_ATmega168__) || defined(__AVR_ATmega32U4__) || defined(__AVR_ATmega16U4__) +#ifndef MAX_DIGITAL_INPUT #define MAX_DIGITAL_INPUT 8 +#endif +#ifndef MAX_DIGITAL_OUTPUT #define MAX_DIGITAL_OUTPUT 32 +#endif +#ifndef MAX_ANALOG_INPUT #define MAX_ANALOG_INPUT 6 +#endif +#ifndef MAX_ANALOG_OUTPUT #define MAX_ANALOG_OUTPUT 32 +#endif +#ifndef MAX_MEMORY_WORD #define MAX_MEMORY_WORD 0 +#endif +#ifndef MAX_MEMORY_DWORD #define MAX_MEMORY_DWORD 0 +#endif +#ifndef MAX_MEMORY_LWORD #define MAX_MEMORY_LWORD 0 +#endif extern IEC_BOOL *bool_input[MAX_DIGITAL_INPUT/8][8]; extern IEC_BOOL *bool_output[MAX_DIGITAL_OUTPUT/8][8]; @@ -45,15 +92,33 @@ extern IEC_UINT *int_output[MAX_ANALOG_OUTPUT]; #else +#ifndef MAX_DIGITAL_INPUT #define MAX_DIGITAL_INPUT 56 +#endif +#ifndef MAX_DIGITAL_OUTPUT #define MAX_DIGITAL_OUTPUT 56 +#endif +#ifndef MAX_ANALOG_INPUT #define MAX_ANALOG_INPUT 32 +#endif +#ifndef MAX_ANALOG_OUTPUT #define MAX_ANALOG_OUTPUT 32 +#endif +#ifndef MAX_REAL_INPUT #define MAX_REAL_INPUT 32 +#endif +#ifndef MAX_REAL_OUTPUT #define MAX_REAL_OUTPUT 32 +#endif +#ifndef MAX_MEMORY_WORD #define MAX_MEMORY_WORD 20 +#endif +#ifndef MAX_MEMORY_DWORD #define MAX_MEMORY_DWORD 20 +#endif +#ifndef MAX_MEMORY_LWORD #define MAX_MEMORY_LWORD 20 +#endif extern IEC_BOOL *bool_input[MAX_DIGITAL_INPUT/8][8]; extern IEC_BOOL *bool_output[MAX_DIGITAL_OUTPUT/8][8]; diff --git a/src/backend/shared/compile/__tests__/generate-defines.test.ts b/src/backend/shared/compile/__tests__/generate-defines.test.ts index 1c86b0c47..600dc122b 100644 --- a/src/backend/shared/compile/__tests__/generate-defines.test.ts +++ b/src/backend/shared/compile/__tests__/generate-defines.test.ts @@ -73,6 +73,89 @@ describe('generateDefinesContent — board defines section', () => { }) }) +describe('generateDefinesContent — Process image section', () => { + // Slot counts big enough to be unmistakable against openplc.h's own + // fallbacks (56/32/20), so a test asserting these values can't pass by + // accidentally reading the header's numbers back. + const P1AM_200_IMAGE = { + digitalInputs: 240, + digitalOutputs: 240, + analogInputs: 64, + analogOutputs: 64, + realInputs: 64, + realOutputs: 64, + memoryWords: 128, + memoryDwords: 32, + memoryLwords: 32, + } + + it('emits nothing when the target declares no process image', () => { + // THE compatibility guarantee of openplc-editor#296: a board that + // says nothing must produce the exact bytes it produced before this + // section existed, so `openplc.h`'s `#ifdef` ladder stays in charge. + const out = generateDefinesContent(EMPTY_INPUTS) + expect(out).not.toContain('// Process image') + expect(out).not.toContain('MAX_DIGITAL_INPUT') + }) + + it('emits one MAX_* define per field, in header order', () => { + const out = generateDefinesContent({ ...EMPTY_INPUTS, processImage: P1AM_200_IMAGE }) + expect(out).toContain( + '// Process image\n' + + '#define MAX_DIGITAL_INPUT 240\n' + + '#define MAX_DIGITAL_OUTPUT 240\n' + + '#define MAX_ANALOG_INPUT 64\n' + + '#define MAX_ANALOG_OUTPUT 64\n' + + '#define MAX_REAL_INPUT 64\n' + + '#define MAX_REAL_OUTPUT 64\n' + + '#define MAX_MEMORY_WORD 128\n' + + '#define MAX_MEMORY_DWORD 32\n' + + '#define MAX_MEMORY_LWORD 32\n', + ) + }) + + it('lands under the board defines, in the same run of #defines', () => { + const out = generateDefinesContent({ + ...EMPTY_INPUTS, + isLicensable: true, + boardEntry: { define: 'BOARD_X' }, + processImage: P1AM_200_IMAGE, + }) + expect(out.indexOf('// Board defines')).toBeLessThan(out.indexOf('// Process image')) + expect(out.indexOf('// Process image')).toBeLessThan(out.indexOf('//Program MD5')) + }) + + it('accepts zero as a slot count — an area the board simply does not have', () => { + const out = generateDefinesContent({ + ...EMPTY_INPUTS, + processImage: { ...P1AM_200_IMAGE, memoryLwords: 0 }, + }) + expect(out).toContain('#define MAX_MEMORY_LWORD 0\n') + }) + + // A process image arrives from a VPP manifest — third-party JSON that + // the editor's types do not police at runtime. A bad value must not + // reach `#define MAX_DIGITAL_INPUT NaN` and blow up inside the C + // compiler with nothing pointing back at the manifest. The whole block + // drops, because the fields size interlocking buffers and a + // half-applied image is worse than falling back to the header's. + it.each([ + ['a negative count', { digitalInputs: -1 }], + ['a fractional count', { analogInputs: 3.5 }], + ['NaN', { memoryWords: Number.NaN }], + ['Infinity', { memoryWords: Number.POSITIVE_INFINITY }], + ['a non-number', { digitalOutputs: '240' as unknown as number }], + ['a missing field', { realOutputs: undefined as unknown as number }], + ])('drops the whole block when a field carries %s', (_label, bad) => { + const out = generateDefinesContent({ + ...EMPTY_INPUTS, + processImage: { ...P1AM_200_IMAGE, ...bad }, + }) + expect(out).not.toContain('// Process image') + expect(out).not.toContain('MAX_ANALOG_INPUT') + }) +}) + describe('generateDefinesContent — OPENPLC_NO_UNIQUE_ID', () => { // The flag keeps `ArduinoUniqueID` out of the build on any board that // cannot be licensed, which is what makes the library's `#error` on an diff --git a/src/backend/shared/compile/__tests__/pipeline.test.ts b/src/backend/shared/compile/__tests__/pipeline.test.ts index e49766067..1bae14218 100644 --- a/src/backend/shared/compile/__tests__/pipeline.test.ts +++ b/src/backend/shared/compile/__tests__/pipeline.test.ts @@ -322,6 +322,78 @@ describe('runCompilePipeline — blank FBD variable guard', () => { }) }) +describe('runCompilePipeline — process-image range guard', () => { + /** A project whose single POU declares one located variable. */ + const projectLocatedAt = (location: string) => + ({ + ...projectDataFixture, + pous: [ + { + type: 'program', + data: { + name: 'main', + language: 'st', + variables: [{ name: 'coil', location }], + documentation: '', + body: { language: 'st', value: '' }, + }, + }, + ], + }) as unknown as PLCProjectData + + it('bails at the validate stage when a location is past the board’s I/O range', async () => { + const port = makePort() + const { events, emit } = captureEvents() + + // %QX7.0 is slot 56 against the firmware's 56-output fallback — the + // exact address from openplc-editor#296. This used to compile clean + // and bind nowhere. + const result = await runCompilePipeline( + makeArgs({ projectData: projectLocatedAt('%QX7.0'), boardRuntime: 'arduino-cli' }), + port, + emit, + ) + + expect(result.success).toBe(false) + expect(port.transpileToSt).not.toHaveBeenCalled() + const validateError = events.find((e) => e.stage === 'validate' && e.level === 'error') + expect(validateError?.message).toContain('"coil"') + expect(validateError?.message).toContain('%QX7.0') + expect(validateError?.message).toContain('%QX (digital outputs)') + }) + + it('lets an in-range location through to transpilation', async () => { + const port = makePort() + const { emit } = captureEvents() + + await runCompilePipeline( + makeArgs({ projectData: projectLocatedAt('%QX6.7'), boardRuntime: 'arduino-cli' }), + port, + emit, + ) + + expect(port.transpileToSt).toHaveBeenCalled() + }) + + it('skips the check on Runtime v4, which compiles against neither openplc.h nor its limits', async () => { + const port = makePort() + const { emit } = captureEvents() + + await runCompilePipeline( + makeArgs({ + projectData: projectLocatedAt('%QX7.0'), + boardRuntime: 'openplc-compiler', + isSimulator: false, + isRuntimeV4: true, + }), + port, + emit, + ) + + expect(port.transpileToSt).toHaveBeenCalled() + }) +}) + describe('runCompilePipeline — arduino direct path', () => { it('uploads to the physical board when isSimulator=false and not compileOnly', async () => { const port = makePort() diff --git a/src/backend/shared/compile/__tests__/validate-process-image.test.ts b/src/backend/shared/compile/__tests__/validate-process-image.test.ts new file mode 100644 index 000000000..b756d7267 --- /dev/null +++ b/src/backend/shared/compile/__tests__/validate-process-image.test.ts @@ -0,0 +1,278 @@ +/** + * Tests for the pre-compile process-image range guard (openplc-editor#296). + * + * The Python editor refused a location past the end of the I/O image with + * `wrong location for var __QX7_0`; the check was lost in the move to + * strucpp, so the editor built programs whose I/O silently did nothing. + * These pin the detector that brings the refusal back, and the message + * that names the board and the limit. + */ + +import type { ProcessImageSizes } from '@root/middleware/shared/utils/target-capabilities' + +import type { PLCProjectData } from '../../types/PLC/open-plc' +import { + describeOutOfRangeLocation, + FIRMWARE_FALLBACK_PROCESS_IMAGE, + findOutOfRangeLocations, +} from '../steps/validate-process-image' + +type TestVariable = { name: string; location: string; type?: unknown } + +/** A located `ARRAY [start..end] OF ` variable. */ +const arrayVar = (name: string, location: string, start: number, end: number, base = 'WORD'): TestVariable => ({ + name, + location, + type: { + definition: 'array', + value: `ARRAY [${start}..${end}] OF ${base}`, + data: { baseType: { definition: 'base-type', value: base }, dimensions: [{ dimension: `${start}..${end}` }] }, + }, +}) + +function makeProject(options: { pous?: Array<{ name: string; variables: TestVariable[] }>; globals?: TestVariable[] }) { + return { + pous: (options.pous ?? []).map((pou) => ({ + type: 'program', + data: { + name: pou.name, + language: 'st', + variables: pou.variables, + documentation: '', + body: { language: 'st', value: '' }, + }, + })), + dataTypes: [], + configuration: { resource: { tasks: [], instances: [], globalVariables: options.globals ?? [] } }, + } as unknown as PLCProjectData +} + +/** A local `VAR … AT` in one POU — the common shape in these tests. */ +const withLocal = (location: string, name = 'v') => + makeProject({ pous: [{ name: 'main', variables: [{ name, location }] }] }) + +/** Roomy enough that nothing here trips a bound by accident. */ +const BIG_IMAGE: ProcessImageSizes = { + digitalInputs: 240, + digitalOutputs: 240, + analogInputs: 64, + analogOutputs: 64, + realInputs: 64, + realOutputs: 64, + memoryWords: 128, + memoryDwords: 32, + memoryLwords: 32, +} + +describe('findOutOfRangeLocations — bounds per area', () => { + it('accepts the last slot and rejects the one past it', () => { + // 56 digital outputs => %QX0.0..%QX6.7 (slots 0..55). %QX7.0 is slot + // 56 — the exact address from the issue report. + expect(findOutOfRangeLocations(withLocal('%QX6.7'), undefined)).toEqual([]) + + const issues = findOutOfRangeLocations(withLocal('%QX7.0'), undefined) + expect(issues).toEqual([ + { + scope: 'main', + variableName: 'v', + location: '%QX7.0', + slot: 56, + capacity: 56, + area: '%QX (digital outputs)', + slotCount: 1, + }, + ]) + }) + + it.each([ + ['%IX7.0', 'digitalInputs', '%IX (digital inputs)'], + ['%QX7.0', 'digitalOutputs', '%QX (digital outputs)'], + ['%IW32', 'analogInputs', '%IW (analog inputs)'], + ['%QW32', 'analogOutputs', '%QW (analog outputs)'], + ['%ID32', 'realInputs', '%ID (analog inputs, REAL)'], + ['%QD32', 'realOutputs', '%QD (analog outputs, REAL)'], + ['%MW20', 'memoryWords', '%MW (memory words)'], + ['%MD20', 'memoryDwords', '%MD (memory double words)'], + ['%ML20', 'memoryLwords', '%ML (memory long words)'], + ])('bounds %s against %s', (location, _field, area) => { + const issues = findOutOfRangeLocations(withLocal(location), undefined) + expect(issues).toHaveLength(1) + expect(issues[0]?.area).toBe(area) + }) + + it('counts a bit address as byte*8 + bit, not as its byte', () => { + // %QX7.0 and %QX7.7 are both past a 56-slot image, but they are + // different slots — a message quoting the byte would say "7" for both. + expect(findOutOfRangeLocations(withLocal('%QX7.7'), undefined)[0]?.slot).toBe(63) + }) +}) + +describe('findOutOfRangeLocations — which image applies', () => { + it('uses the firmware fallback when the target declares no image', () => { + expect(FIRMWARE_FALLBACK_PROCESS_IMAGE.digitalOutputs).toBe(56) + expect(findOutOfRangeLocations(withLocal('%QX7.0'), undefined)).toHaveLength(1) + }) + + it('accepts what a declared, larger image makes room for', () => { + // The whole point of openplc-editor#296: a P1AM-sized image makes the + // address that fails above legal. + expect(findOutOfRangeLocations(withLocal('%QX7.0'), BIG_IMAGE)).toEqual([]) + expect(findOutOfRangeLocations(withLocal('%MW126'), BIG_IMAGE)).toEqual([]) + }) + + it('rejects what a declared, smaller image takes away', () => { + const tiny: ProcessImageSizes = { ...BIG_IMAGE, memoryWords: 0 } + expect(findOutOfRangeLocations(withLocal('%MW0'), tiny)).toHaveLength(1) + }) +}) + +describe('findOutOfRangeLocations — what it scans', () => { + it('checks CONFIGURATION globals as well as POU locals', () => { + const issues = findOutOfRangeLocations(makeProject({ globals: [{ name: 'g', location: '%QX7.0' }] }), undefined) + expect(issues).toEqual([expect.objectContaining({ scope: 'Global Variables', variableName: 'g' })]) + }) + + it('reports every offender, not just the first', () => { + const issues = findOutOfRangeLocations( + makeProject({ + pous: [ + { + name: 'main', + variables: [ + { name: 'a', location: '%QX7.0' }, + { name: 'b', location: '%MW99' }, + ], + }, + ], + globals: [{ name: 'c', location: '%IW40' }], + }), + undefined, + ) + expect(issues.map((i) => i.variableName)).toEqual(['a', 'b', 'c']) + }) + + it('ignores unlocated variables, local and global alike', () => { + expect(findOutOfRangeLocations(withLocal(''), undefined)).toEqual([]) + expect(findOutOfRangeLocations(makeProject({ globals: [{ name: 'g', location: '' }] }), undefined)).toEqual([]) + }) + + it('ignores an unresolved alias — a name, not an address', () => { + // Aliases are resolved to literals by getCompileReadyProjectData() + // before the pipeline runs. One still standing here resolved to + // nothing, which makes the variable unlocated, not out of range. + expect(findOutOfRangeLocations(withLocal('MotorStart'), undefined)).toEqual([]) + }) + + it.each(['%IB0', '%QB99', '%MB99', '%MX99.0'])( + 'ignores %s — the Arduino firmware has no buffer for that area', + (location) => { + expect(findOutOfRangeLocations(withLocal(location), undefined)).toEqual([]) + }, + ) +}) + +describe('findOutOfRangeLocations — located arrays', () => { + // A located array occupies one slot per element (openplc-editor#565), so + // the base address fitting says nothing about whether the array does. + const withArray = (location: string, start: number, end: number, base?: string) => + makeProject({ pous: [{ name: 'main', variables: [arrayVar('buffer', location, start, end, base)] }] }) + + it('measures the last element, not the base address', () => { + // %MW0 is fine; %MW0..%MW66 is not, against a 20-word fallback image. + const issues = findOutOfRangeLocations(withArray('%MW0', 0, 66), undefined) + expect(issues).toHaveLength(1) + expect(issues[0]).toMatchObject({ slot: 66, capacity: 20, slotCount: 67 }) + }) + + it('accepts an array that ends exactly at the last slot', () => { + expect(findOutOfRangeLocations(withArray('%MW0', 0, 19), undefined)).toEqual([]) + expect(findOutOfRangeLocations(withArray('%MW0', 0, 20), undefined)).toHaveLength(1) + }) + + it('accepts the issue’s own declaration once the target has room for it', () => { + // `HR_myData AT %MW60 : ARRAY [0..66] OF WORD` — needs %MW60..%MW126. + expect(findOutOfRangeLocations(withArray('%MW60', 0, 66), undefined)).toHaveLength(1) + expect(findOutOfRangeLocations(withArray('%MW60', 0, 66), BIG_IMAGE)).toEqual([]) + }) + + it('counts a bit array in bits, crossing byte boundaries', () => { + // 240 outputs = slots 0..239. Starting at %QX29.0 (slot 232), 8 bits + // exactly fill it; 9 do not. + expect(findOutOfRangeLocations(withArray('%QX29.0', 0, 7, 'BOOL'), BIG_IMAGE)).toEqual([]) + expect(findOutOfRangeLocations(withArray('%QX29.0', 0, 8, 'BOOL'), BIG_IMAGE)).toHaveLength(1) + }) + + it.each([ + ['a multi-dimensional array', [{ dimension: '0..3' }, { dimension: '0..3' }]], + ['a malformed dimension', [{ dimension: 'N..M' }]], + ['no dimensions at all', []], + ])('falls back to one slot for %s rather than guessing', (_label, dimensions) => { + // Under-counting costs at most a missed diagnostic (the compiler rejects + // these shapes anyway); guessing high would refuse a valid build. + const project = makeProject({ + pous: [ + { + name: 'main', + variables: [ + { + name: 'buffer', + location: '%MW0', + type: { + definition: 'array', + value: 'ARRAY [...] OF WORD', + data: { baseType: { definition: 'base-type', value: 'WORD' }, dimensions }, + }, + }, + ], + }, + ], + }) + expect(findOutOfRangeLocations(project, undefined)).toEqual([]) + }) + + it('treats an array with no data block as a single slot', () => { + const project = makeProject({ + pous: [ + { + name: 'main', + variables: [ + { name: 'buffer', location: '%MW0', type: { definition: 'array', value: 'ARRAY [0..66] OF WORD' } }, + ], + }, + ], + }) + expect(findOutOfRangeLocations(project, undefined)).toEqual([]) + }) +}) + +describe('describeOutOfRangeLocation', () => { + it('names the board, the slot, the area and the last usable slot', () => { + const [issue] = findOutOfRangeLocations(withLocal('%QX7.0', 'coil'), undefined) + expect(issue).toBeDefined() + // Non-null assertion avoided: the expect above already proved it, but + // TS needs the guard to narrow. + if (!issue) throw new Error('expected an issue') + expect(describeOutOfRangeLocation(issue, 'AutomationDirect P1AM-100')).toBe( + 'main: variable "coil" is located at %QX7.0, which is slot 56 of %QX (digital outputs) — ' + + '"AutomationDirect P1AM-100" supports 56 (last usable: slot 55).', + ) + }) + + it('says the area is absent rather than quoting "last usable: slot -1"', () => { + const [issue] = findOutOfRangeLocations(withLocal('%MW0'), { ...BIG_IMAGE, memoryWords: 0 }) + if (!issue) throw new Error('expected an issue') + expect(describeOutOfRangeLocation(issue, 'Arduino Uno')).toContain('"Arduino Uno" has no %MW (memory words) area') + }) + + it('blames the array’s length, not its (legal) base address', () => { + const [issue] = findOutOfRangeLocations( + makeProject({ pous: [{ name: 'main', variables: [arrayVar('buffer', '%MW0', 0, 66)] }] }), + undefined, + ) + if (!issue) throw new Error('expected an issue') + expect(describeOutOfRangeLocation(issue, 'Arduino Mega')).toBe( + 'main: variable "buffer" is located at %MW0, whose 67 elements reach slot 66 of %MW (memory words) — ' + + '"Arduino Mega" supports 20 (last usable: slot 19).', + ) + }) +}) diff --git a/src/backend/shared/compile/pipeline.ts b/src/backend/shared/compile/pipeline.ts index 01ac1a53f..b27d6f111 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -51,6 +51,7 @@ import { generateRuntimeConfs } from './steps/generate-confs' import { generateDefinesContent } from './steps/generate-defines' import { generateVppConfigContent } from './steps/generate-vpp-config' import { findEmptyFbdVariables } from './steps/validate-empty-variables' +import { describeOutOfRangeLocation, findOutOfRangeLocations } from './steps/validate-process-image' // --------------------------------------------------------------------------- // Public contract @@ -428,6 +429,31 @@ async function runCompilePipelineInner( return bailError(emit, 'validate', 'Compilation aborted: name all variable blocks and try again.') } + // --------------------------------------------------------------------- + // Step 0c: Reject `AT %…` locations with no slot on this target. + // + // Baremetal targets only — `openplc.h`'s `MAX_*` macros are what bound + // the image, and Runtime v3 / v4 compile against neither that header + // nor those limits. + // + // The Python editor refused these at glue-code generation ("wrong + // location for var __QX7_0"); the check was lost in the move to + // strucpp, and the address silently bound nowhere (openplc-editor#296). + // --------------------------------------------------------------------- + if (boardRuntime === 'arduino-cli' || boardRuntime === 'simulator') { + const outOfRange = findOutOfRangeLocations(processedData, targetCapabilities.processImage) + if (outOfRange.length > 0) { + for (const issue of outOfRange) { + emit({ stage: 'validate', message: describeOutOfRangeLocation(issue, boardTarget), level: 'error' }) + } + return bailError( + emit, + 'validate', + 'Compilation aborted: some variables are located outside this board’s I/O range.', + ) + } + } + // --------------------------------------------------------------------- // Step 1: Transpile the project IR straight to Structured Text via // the platform port. Both adapters (editor + web) route through @@ -800,6 +826,10 @@ async function runCompilePipelineInner( // `ArduinoUniqueID` only on a board whose package declares // `isLicensable`, and emit `OPENPLC_NO_UNIQUE_ID` for everyone else. isLicensable: targetCapabilities.isLicensable, + // Absent for every target that doesn't declare one, which leaves + // `openplc.h`'s own `#ifdef` ladder in charge and keeps that + // board's `defines.h` byte-identical (openplc-editor#296). + ...(targetCapabilities.processImage !== undefined ? { processImage: targetCapabilities.processImage } : {}), ...(vppModbusState !== undefined ? { vppModbusState } : {}), }) diff --git a/src/backend/shared/compile/steps/generate-defines.ts b/src/backend/shared/compile/steps/generate-defines.ts index 9c434ead5..548f4e3e7 100644 --- a/src/backend/shared/compile/steps/generate-defines.ts +++ b/src/backend/shared/compile/steps/generate-defines.ts @@ -17,9 +17,67 @@ * into the in-memory file map sent to `/compile-arduino`). */ +import type { ProcessImageSizes } from '@root/middleware/shared/utils/target-capabilities' + import type { DevicePin } from '../../types/PLC/devices' import { generateModbusDefines, resolveDebugBaud, resolveDebugSlave, type VppModbusScreenState } from './modbus-defines' +/** + * `ProcessImageSizes` field → the `MAX_*` macro `openplc.h` declares its + * buffers from. Order is the emission order, chosen to read like the + * header does: inputs before outputs, I/O before memory. + * + * This table is the whole contract between the capability and the + * firmware. A field added to `ProcessImageSizes` without a line here + * is silently not emitted, so the mapping is exhaustive by + * construction: `Record` makes the + * compiler reject a new field until it gets a macro. + */ +const PROCESS_IMAGE_MACROS: Record = { + digitalInputs: 'MAX_DIGITAL_INPUT', + digitalOutputs: 'MAX_DIGITAL_OUTPUT', + analogInputs: 'MAX_ANALOG_INPUT', + analogOutputs: 'MAX_ANALOG_OUTPUT', + realInputs: 'MAX_REAL_INPUT', + realOutputs: 'MAX_REAL_OUTPUT', + memoryWords: 'MAX_MEMORY_WORD', + memoryDwords: 'MAX_MEMORY_DWORD', + memoryLwords: 'MAX_MEMORY_LWORD', +} + +/** + * Emit the `// Process image` block, or `''` when the target didn't + * declare one. + * + * Absent is the normal case and means "let `openplc.h`'s own `#ifdef` + * ladder decide" — that header picks 8 DI / 6 AI / no `%M` area on the + * small AVRs and 56 / 32 / 20 elsewhere, and no capability preset can + * answer for both halves. Emitting nothing keeps those boards on the + * exact numbers they have always built with. + * + * A declared image is validated here rather than trusted: it arrives + * from a VPP manifest, i.e. third-party JSON. A malformed value would + * otherwise reach `#define MAX_DIGITAL_INPUT undefined` and fail deep + * in the C compiler with nothing pointing back at the manifest, so a + * field that isn't a non-negative safe integer drops the WHOLE block + * (all-or-nothing: the fields size interlocking buffers, and a + * half-applied image is worse than none). + */ +function generateProcessImageDefines(processImage: ProcessImageSizes | undefined): string { + if (!processImage) return '' + + const entries = Object.entries(PROCESS_IMAGE_MACROS) as Array<[keyof ProcessImageSizes, string]> + + const isSlotCount = (value: number): boolean => Number.isSafeInteger(value) && value >= 0 + if (!entries.every(([field]) => isSlotCount(processImage[field]))) return '' + + let block = '// Process image\n' + for (const [field, macro] of entries) { + block += `#define ${macro} ${processImage[field]}\n` + } + return block +} + export type { VppModbusScreenState } from './modbus-defines' /** @@ -99,6 +157,15 @@ export interface GenerateDefinesInput { * `id_len = 0`, which `device-probe` and the licence flow already treat * as "this board has no unique id". */ isLicensable?: boolean + /** Process-image slot counts for this target + * (`TargetCapabilities.processImage`), sourced from the VPP manifest. + * + * Absent — the normal case, and every non-VPP board — emits no + * `MAX_*` at all, leaving `openplc.h`'s built-in `#ifdef` ladder to + * pick the sizes exactly as it always has. Present overrides that + * ladder wholesale via the `#ifndef` guards in the header + * (openplc-editor#296). */ + processImage?: ProcessImageSizes } /** @@ -108,6 +175,10 @@ export interface GenerateDefinesInput { * 1. `// Board defines` — `boardEntry.define` plus * `OPENPLC_NO_UNIQUE_ID` on every non-licensable target; omitted * entirely only when both sources are empty. + * 1b. `// Process image` — `MAX_*` slot counts, ONLY when the target + * declared a `processImage`. Absent on every board that doesn't, + * which keeps their output byte-identical to before this section + * existed. * 2. `#define PROGRAM_MD5 ""` — always. * 3. `// Comms Configuration` (simulator-only) — fixed Modbus RTU * over emulated USART0 so avr8js's serial bridge can drive @@ -131,6 +202,7 @@ export function generateDefinesContent(input: GenerateDefinesInput): string { vppModbusState, defaultSerial, isLicensable, + processImage, } = input let DEFINES_CONTENT = '' @@ -163,6 +235,14 @@ export function generateDefinesContent(input: GenerateDefinesInput): string { }) } + // 1b. Process image. Board-level like the block above, so it lands + // in the same run of `#define`s, but sourced from the target's + // capability rather than from `hals.json`. Empty for every + // target that declares no `processImage` — which is every board + // today except the VPP packages that opt in — so this section + // cannot perturb an existing board's `defines.h`. + DEFINES_CONTENT += generateProcessImageDefines(processImage) + // 2. Trailing blank-line pair after the board-defines section // (or at the top of the file when board defines were absent — // intentional so the PROGRAM_MD5 block always lands two blank diff --git a/src/backend/shared/compile/steps/validate-process-image.ts b/src/backend/shared/compile/steps/validate-process-image.ts new file mode 100644 index 000000000..bd9ab1af6 --- /dev/null +++ b/src/backend/shared/compile/steps/validate-process-image.ts @@ -0,0 +1,232 @@ +/** + * Reject `AT %…` locations that fall outside the target's process image. + * + * The firmware declares its I/O buffers from the `MAX_*` macros in + * `resources/sources/arduino/openplc.h`, so a location past the end names + * a slot that does not exist. Nothing downstream can rescue it: the glue + * now skips the binding (it used to write past the array), the HAL never + * reads the slot, and the variable is simply inert on the board. + * + * The Python editor caught this at glue-code generation and refused the + * build with `wrong location for var __QX7_0`. That check did not survive + * the move to strucpp, so between then and now the editor happily built a + * program whose I/O silently did nothing — the failure mode reported in + * openplc-editor#296, where a P1AM backplane wide enough to need more than + * 56 outputs lost every point past the 56th without a word. + * + * This step restores the refusal, and says which board and which limit. + * + * Pure function: no fs I/O, no DOM, no global state. + */ + +import type { ProcessImageSizes } from '@root/middleware/shared/utils/target-capabilities' + +import { type AddressClass, parseAddress } from '../../../../middleware/shared/utils/iec-address/registry' +import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' + +/** + * The sizes `openplc.h` falls back to when the target declares no + * `processImage` — its `#else` branch, i.e. every board that is not one of + * the four small AVRs. + * + * Deliberately NOT the small-AVR branch (8 DI / 6 AI / no `%M` area at + * all). Picking the larger of the two makes this check permissive rather + * than strict on an Uno or a Leonardo, which is the safe direction: the + * cost of being permissive is a variable that stays inert exactly as it + * does today, while the cost of being strict would be refusing to build a + * project that has been building for years. Distinguishing the two + * branches here would mean mapping an arduino-cli FQBN back to its MCU + * define, a mapping the editor does not otherwise keep and that would + * silently rot as cores are added. + */ +export const FIRMWARE_FALLBACK_PROCESS_IMAGE: ProcessImageSizes = { + digitalInputs: 56, + digitalOutputs: 56, + analogInputs: 32, + analogOutputs: 32, + realInputs: 32, + realOutputs: 32, + memoryWords: 20, + memoryDwords: 20, + memoryLwords: 20, +} + +/** One located variable whose address has no slot on the target. */ +export type OutOfRangeLocation = { + /** POU that declares it, or `'Global Variables'` for a config global. */ + scope: string + variableName: string + /** The offending literal address, e.g. `'%QX7.0'`. */ + location: string + /** Highest slot the declaration needs (`byte*8 + bit` for bits). For an + * array this is its LAST element, which is the one that overflows. */ + slot: number + /** How many slots the target actually has in that area. */ + capacity: number + /** Human name of the area, for the message: `'%QX (digital outputs)'`. */ + area: string + /** Elements the declaration occupies; 1 for a scalar. Present so the + * message can explain that the address itself is fine and the array's + * length is what runs off the end. */ + slotCount: number +} + +/** + * `%` → the process-image field that bounds it, plus the + * label used in the error message. + * + * `%IB`/`%QB`/`%MB`/`%MX` are absent on purpose: the Arduino firmware + * declares no byte-addressed buffers and no bit-addressed memory area, so + * there is no capacity to compare against. Leaving them unmapped skips + * them rather than measuring them against a number that means something + * else. + */ +const AREA_BOUNDS: Record = { + IX: { field: 'digitalInputs', label: '%IX (digital inputs)' }, + QX: { field: 'digitalOutputs', label: '%QX (digital outputs)' }, + IW: { field: 'analogInputs', label: '%IW (analog inputs)' }, + QW: { field: 'analogOutputs', label: '%QW (analog outputs)' }, + ID: { field: 'realInputs', label: '%ID (analog inputs, REAL)' }, + QD: { field: 'realOutputs', label: '%QD (analog outputs, REAL)' }, + MW: { field: 'memoryWords', label: '%MW (memory words)' }, + MD: { field: 'memoryDwords', label: '%MD (memory double words)' }, + ML: { field: 'memoryLwords', label: '%ML (memory long words)' }, +} + +const areaKey = (cls: AddressClass): string => `${cls.direction}${cls.size}` + +/** + * Every located variable in the project, paired with the scope that + * declares it. Covers POU-local `VAR … AT` and CONFIGURATION + * VAR_GLOBAL — the only two places IEC allows a location, and the two the + * editor's own validation permits. + */ +function* locatedVariables( + projectData: PLCProjectData, +): Generator<{ scope: string; name: string; location: string; slotCount: number }> { + for (const pou of projectData.pous) { + for (const variable of pou.data.variables) { + if (variable.location) + yield { + scope: pou.data.name, + name: variable.name, + location: variable.location, + slotCount: declaredSlotCount(variable.type), + } + } + } + for (const variable of projectData.configuration.resource.globalVariables) { + if (variable.location) + yield { + scope: 'Global Variables', + name: variable.name, + location: variable.location, + slotCount: declaredSlotCount(variable.type), + } + } +} + +/** + * How many consecutive slots a declaration claims from its address. + * + * A scalar claims one. A located ARRAY claims one per element, laid out from + * the declared address — `AT %MW60 : ARRAY [0..66] OF WORD` needs %MW60 + * through %MW126 (openplc-editor#565), so checking only the base address + * would pass a declaration whose tail runs off the end of the image. + * + * Falls back to 1 for anything whose extent can't be read: a malformed + * dimension, a multi-dimensional array (which the compiler rejects for a + * located variable anyway), or a missing `data` block. Under-counting only + * costs a missed diagnostic, whereas guessing high would refuse builds that + * are fine. + */ +function declaredSlotCount(variableType: PLCVariable['type'] | undefined): number { + // `type` is schema-required, but project.json is a file on disk that the + // user (or an older editor) can have written; a missing type must not + // crash the build with a TypeError instead of a diagnostic. + if (variableType?.definition !== 'array') return 1 + + const dimensions = variableType.data?.dimensions + if (!dimensions || dimensions.length !== 1) return 1 + + const bounds = /^\s*(\d+)\s*\.\.\s*(\d+)\s*$/.exec(dimensions[0]?.dimension ?? '') + if (!bounds) return 1 + + const start = Number(bounds[1]) + const end = Number(bounds[2]) + return end >= start ? end - start + 1 : 1 +} + +/** + * Find every located variable that addresses a slot the target does not + * have. + * + * `processImage` is the target's declared image, or `undefined` for a + * board that declares none — in which case the firmware's own fallback + * sizes apply, since that is what `openplc.h` will compile with. + * + * Only literal `%…` locations are checked. A location that is an alias + * name has already been resolved to a literal by + * `getCompileReadyProjectData()` before the pipeline runs; anything still + * unresolved at this point resolved to nothing, which makes the variable + * unlocated rather than out of range. Addresses in an area the firmware + * has no buffer for (`%IB`, `%MX`) are skipped for the same reason: + * there is no capacity to measure them against. + */ +export function findOutOfRangeLocations( + projectData: PLCProjectData, + processImage: ProcessImageSizes | undefined, +): OutOfRangeLocation[] { + const image = processImage ?? FIRMWARE_FALLBACK_PROCESS_IMAGE + const issues: OutOfRangeLocation[] = [] + + for (const { scope, name, location, slotCount } of locatedVariables(projectData)) { + const parsed = parseAddress(location) + if (parsed === null) continue + + const bound = AREA_BOUNDS[areaKey(parsed.cls)] + if (bound === undefined) continue + + // The LAST slot is what has to fit: an array starting inside the image + // can still run off the end of it. + const lastSlot = parsed.linear + slotCount - 1 + const capacity = image[bound.field] + if (lastSlot < capacity) continue + + issues.push({ + scope, + variableName: name, + location, + slot: lastSlot, + capacity, + area: bound.label, + slotCount, + }) + } + + return issues +} + +/** + * One-line, actionable rendering of an out-of-range location. + * + * Names the board because the same project can be valid on one target and + * not on another — switching targets is exactly how a user lands here — + * and quoting the limit turns "it does not work" into a number the user + * can design against. + */ +export function describeOutOfRangeLocation(issue: OutOfRangeLocation, boardTarget: string): string { + const last = + issue.capacity === 0 + ? `"${boardTarget}" has no ${issue.area} area at all` + : `"${boardTarget}" supports ${issue.capacity} (last usable: slot ${issue.capacity - 1})` + // For an array the base address is usually fine and the LENGTH is what + // overflows, so say which slot the last element lands on rather than + // pointing at an address that looks perfectly legal. + const reach = + issue.slotCount > 1 ? `whose ${issue.slotCount} elements reach slot ${issue.slot}` : `which is slot ${issue.slot}` + return ( + `${issue.scope}: variable "${issue.variableName}" is located at ${issue.location}, ` + + `${reach} of ${issue.area} — ${last}.` + ) +} diff --git a/src/frontend/store/slices/project/validation/variables.ts b/src/frontend/store/slices/project/validation/variables.ts index 63186db6f..4351810bc 100644 --- a/src/frontend/store/slices/project/validation/variables.ts +++ b/src/frontend/store/slices/project/validation/variables.ts @@ -117,6 +117,26 @@ const arrayValidation = ({ value }: { value: string }) => { return { ok: true } } +/** + * The type that has to match the address class, given a variable's declared + * type. + * + * For an array this is the ELEMENT type: `ARRAY [0..66] OF WORD AT %MW60` + * occupies 67 consecutive WORD slots, so what has to fit `%MW` is WORD, not + * the array (openplc-editor#565). Locating an array used to be rejected + * outright — a limitation of the MatIEC-era toolchain that left with MatIEC. + * + * Returns the type's own name for every other definition, which lands + * `user-data-type` and `derived` on the `default` branch below, where they + * belong: a STRUCT has no single address class. + */ +const addressClassTypeOf = (variableType: PLCVariable['type']): string => + // `data` is optional on the port-side type; an array without it is a + // half-built row from the array modal, and falling back to `value` (the + // "ARRAY [...] OF T" text) lands it on the `default` branch — rejected with + // a message, which is the right answer for a type that isn't finished. + variableType.definition === 'array' && variableType.data ? variableType.data.baseType.value : variableType.value + /** * Validate a variable's `location`. Single-field model: `location` is either * an alias name, a literal IEC address, or empty. @@ -178,7 +198,11 @@ const variableLocationValidationErrorMessage = (variableType: string) => { case 'LWORD': return 'Valid locations: %QL0, %IL0, %ML0 (change the number to the desired location)' default: - return '' + // Reached by a structure or an enum — types with no single address + // class. This used to return an empty string, so the dialog showed + // "Please make sure that the location is valid." and nothing else: a + // refusal with no reason and nothing to act on. + return `A variable of type "${variableType}" cannot have a physical location: only the elementary types (BOOL, BYTE, INT, WORD, DINT, REAL, ...) and arrays of them map onto an IEC address.` } } @@ -436,18 +460,18 @@ const updateVariableValidation = ( return response } - if (!variableLocationValidation(location, variableToUpdate.type.value)) { + if (!variableLocationValidation(location, addressClassTypeOf(variableToUpdate.type))) { response = { ok: false, title: 'Location is invalid.', - message: `Please make sure that the location is valid.\n${variableLocationValidationErrorMessage(variableToUpdate.type.value)}`, + message: `Please make sure that the location is valid.\n${variableLocationValidationErrorMessage(addressClassTypeOf(variableToUpdate.type))}`, } return response } } if (dataToBeUpdated.type) { - if (!variableLocationValidation(variableToUpdate.location, dataToBeUpdated.type.value)) { + if (!variableLocationValidation(variableToUpdate.location, addressClassTypeOf(dataToBeUpdated.type))) { response.data = { ...(response.data ? response.data : {}), location: '' } } if (dataToBeUpdated.type.definition === 'derived') { diff --git a/src/middleware/shared/utils/target-capabilities/index.ts b/src/middleware/shared/utils/target-capabilities/index.ts index 66907056d..7237eb097 100644 --- a/src/middleware/shared/utils/target-capabilities/index.ts +++ b/src/middleware/shared/utils/target-capabilities/index.ts @@ -6,4 +6,4 @@ export { SIMULATOR_CAPABILITIES, } from './presets' export { type BoardInfoLike, resolveTargetCapabilities } from './resolve' -export type { AddressProducerCapabilities, DebuggerTransport, TargetCapabilities } from './types' +export type { AddressProducerCapabilities, DebuggerTransport, ProcessImageSizes, TargetCapabilities } from './types' diff --git a/src/middleware/shared/utils/target-capabilities/types.ts b/src/middleware/shared/utils/target-capabilities/types.ts index 3b252fa94..164397545 100644 --- a/src/middleware/shared/utils/target-capabilities/types.ts +++ b/src/middleware/shared/utils/target-capabilities/types.ts @@ -137,6 +137,75 @@ export interface TargetCapabilities { * therefore a FIRMWARE fault (built without the backend), never a * hardware limitation — and the flow says exactly that. */ isLicensable: boolean + + /** How many slots of each IEC area this target's firmware can bind. + * + * Sizes the process image: every `AT %…` location must fall inside + * it, and the firmware's buffer arrays / Modbus banks are declared + * from the same numbers (`defines.h` → `openplc.h`). + * + * Per-target because the cost is RAM and the range of targets is + * wide: raising a limit grows the pointer arrays, each VPP HAL's + * binding tables, and the Modbus banks in lockstep, so a board with + * a 15-slot backplane and 256 KB of RAM and a board with 32 KB + * cannot share one number (openplc-editor#296). + * + * **Optional, and absent on every preset — deliberately.** The + * firmware's own `openplc.h` already picks between two hardcoded + * sets with an `#ifdef` on the MCU: 8 DI / 6 AI / no `%M` area on + * the small AVRs (Uno, Leonardo, Micro — 2 KB of SRAM), 56/32/20 on + * everything else. A preset cannot answer for both halves of that + * ladder, and declaring the 56-series as "the default" would hand a + * Uno seven times the buffers it has RAM for. So absent means "say + * nothing in `defines.h` and let the firmware's `#ifdef` decide" — + * which is byte-for-byte today's behaviour for every board that + * ships without a VPP manifest. + * + * A VPP manifest declares it for hardware whose real capacity it + * knows, and that declaration overrides the firmware default. */ + processImage?: ProcessImageSizes +} + +/** + * Slot counts of the firmware process image, one per IEC area/width. + * + * Names mirror the `MAX_*` macros the Arduino firmware declares its + * buffers from (`resources/sources/arduino/openplc.h`) — the emitter in + * `generate-defines.ts` maps these fields onto those macros one-to-one, + * so a field added here needs a macro there and vice versa. + * + * Units are SLOTS, not bytes: `digitalInputs: 56` means `%IX0.0` + * through `%IX6.7` are bindable, `memoryWords: 20` means `%MW0` + * through `%MW19`. + * + * Every field is required. A partial process image is not a meaningful + * thing to declare — the fields are not independent (the firmware sizes + * one Modbus holding bank from `analogOutputs + memoryWords`), and an + * omitted field silently reading as 0 would disable an entire area. + * `resolveTargetCapabilities` merges this object wholesale for the same + * reason: a manifest declares all of it or none of it. The manifest + * schema enforces the same rule (`required` on all nine fields), so the + * two ends cannot drift into a half-declared image. + */ +export interface ProcessImageSizes { + /** `%IX` bit slots — `MAX_DIGITAL_INPUT`. Rounded up to a byte by the firmware. */ + digitalInputs: number + /** `%QX` bit slots — `MAX_DIGITAL_OUTPUT`. Rounded up to a byte by the firmware. */ + digitalOutputs: number + /** `%IW` word slots — `MAX_ANALOG_INPUT`. */ + analogInputs: number + /** `%QW` word slots — `MAX_ANALOG_OUTPUT`. */ + analogOutputs: number + /** `%ID` REAL slots — `MAX_REAL_INPUT`. */ + realInputs: number + /** `%QD` REAL slots — `MAX_REAL_OUTPUT`. */ + realOutputs: number + /** `%MW` word slots — `MAX_MEMORY_WORD`. */ + memoryWords: number + /** `%MD` dword slots — `MAX_MEMORY_DWORD`. */ + memoryDwords: number + /** `%ML` lword slots — `MAX_MEMORY_LWORD`. */ + memoryLwords: number } /**