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 } /**