From 0e4694a6eb76c68e09cbf90affa1e571757790b7 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 09:00:42 -0300 Subject: [PATCH 1/6] feat(compile): one table list, every server counted, and no output driven twice Four things that belong together, because the first is what makes the rest safe to add. ONE CANONICAL TABLE LIST. "Which tables exist, which IEC prefix each stores, in which unit, on which runtime" was written out in five places -- the two area sets, image.conf's keys, the bare-metal macros and the S7comm buffer enum -- and a sixth was about to be added for the S7comm sizer. Every way five copies can disagree is silent: a table in one list and not another is an area sized to zero, a prefix paired wrongly sizes the wrong storage, a unit written differently is a factor of eight nobody sees until an address stops answering. All five now derive from `middleware/shared/utils/io-image/tables.ts`, and adding a table is one line there. No output moves. The list is in image_tables.h declaration order, and filtering it to the entries carrying a macro yields exactly the order defines.h already emitted -- checked by a test, because a gratuitous reorder rebuilds every firmware for nothing. EVERY SERVER COUNTS, NOT JUST MODBUS. serverExposure looked for `protocol === 'modbus-tcp'` and nothing else, so a project with an S7comm server and no Modbus one sized nothing at all from its servers. It now dispatches per protocol. Still the FIRST server of each protocol, deliberately: both config emitters ship one file built from the first one carrying a config, so a second never reaches the device and sizing for it would reserve memory nothing can use. An S7comm data block sizes the table it names, converting `sizeBytes` to that table's elements and adding `startBuffer`. Two conversions meet there and pull opposite ways -- bytes become elements, while a BOOL table's element index becomes bit addresses -- so it is written once and asserted from both directions. AND AN S7COMM BLOCK BACKS WHAT IT COVERS, INPUTS INCLUDED, which is the opposite of the Modbus answer. The difference is a fact about the protocols, not a preference: Modbus discrete inputs and input registers are read-only to the master BY THE PROTOCOL, so exposing an %IX through Modbus cannot put anything into it. S7comm has no such restriction and the plugin implements none -- write_buffer_to_openplc_journal dispatches every buffer type, BUFFER_TYPE_*_INPUT included. An S7 client writes an input table as readily as an output one, so BR14's question answers yes in both directions here. TWO POUS CANNOT DRIVE ONE OUTPUT. checkIfLocationExists reads one variable list, so each POU passes on its own while IEC located addresses are global. The generated code then assigns to the same storage from two places and the last write in the scan wins, decided by POU order. computeIoImage already walks every located variable project-wide and the ST text path cannot bypass it, so the check goes there. Outputs only: two POUs reading one input is ordinary, and sharing a memory address is what memory is for. Checked per slot, so overlapping located arrays are caught at the slot they share rather than only when their base addresses match -- and reported once per pair, because a 4000-element array declared twice is one mistake. The same fact is answered while editing, through the amber glyph the alias scan already uses, so the user hears it before they press build rather than after. A VPP manifest channel's address prefix is checked against the eight the package schema admits. It arrives as an unchecked string through an `addressMapping` typed `unknown`, and a manifest declaring `%MW` would have memory allocated for it as if a module produced it -- the one thing BR14 says nothing can do. The channel is dropped with a warning rather than throwing: one bad channel must not take down the device screen. Verified: 3317 tests across the areas touched, 100% statements/lines/functions on compute-io-image.ts, tsc and validate:arch clean, and compare-surfaces "match": true over 1117 files. Co-Authored-By: Claude Opus 5 --- .../__tests__/compute-io-image.test.ts | 258 +++++++++++++- src/backend/shared/compile/pipeline.ts | 9 +- .../shared/compile/steps/compute-io-image.ts | 323 ++++++++++++------ .../shared/compile/steps/generate-defines.ts | 19 +- .../compile/steps/generate-image-conf.ts | 19 +- src/backend/shared/types/PLC/open-plc.ts | 28 +- .../variables-table/editable-cell.tsx | 27 +- .../hooks/use-duplicate-output-locations.ts | 68 ++++ .../__tests__/resolve-module-channels.test.ts | 44 ++- .../utils/vpp/resolve-module-channels.ts | 48 ++- .../utils/io-image/__tests__/tables.test.ts | 184 ++++++++++ .../shared/utils/io-image/tables.ts | 155 +++++++++ 12 files changed, 1032 insertions(+), 150 deletions(-) create mode 100644 src/frontend/hooks/use-duplicate-output-locations.ts create mode 100644 src/middleware/shared/utils/io-image/__tests__/tables.test.ts create mode 100644 src/middleware/shared/utils/io-image/tables.ts diff --git a/src/backend/shared/compile/__tests__/compute-io-image.test.ts b/src/backend/shared/compile/__tests__/compute-io-image.test.ts index df48a165f..aed05daaa 100644 --- a/src/backend/shared/compile/__tests__/compute-io-image.test.ts +++ b/src/backend/shared/compile/__tests__/compute-io-image.test.ts @@ -14,6 +14,7 @@ import type { AddressProducerCapabilities } from '@root/middleware/shared/utils/ import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' import { computeIoImage, + describeDuplicateOutput, describeUnbackedLocation, describeUnsupportedArea, IMAGE_AREAS_BAREMETAL, @@ -122,7 +123,7 @@ const compute = (projectData: PLCProjectData, extra: Partial { it('sizes nothing for an empty project', () => { // FR21 / BR12: the floor is zero, and zero is expressed by absence. - expect(compute(makeProject({}))).toEqual({ sizes: {}, unbacked: [], unsupported: [] }) + expect(compute(makeProject({}))).toEqual({ sizes: {}, unbacked: [], unsupported: [], duplicateOutputs: [] }) }) it('sizes an area from the pins that claim it', () => { @@ -362,6 +363,259 @@ describe('computeIoImage — server exposure', () => { }) }) +describe('computeIoImage — two declarations on one output', () => { + /** A project whose POUs each declare one located variable. */ + const pousWith = (...decls: Array<[pou: string, name: string, location: string]>) => + makeProject({ + pous: decls.map(([pou, name, location]) => ({ + name: pou, + variables: [variable(name, location)], + })), + }) + + it('refuses the same output slot in two POUs, naming both', () => { + // The gap: checkIfLocationExists reads ONE variable list, so each POU + // passes on its own. IEC located addresses are global. + const image = compute(pousWith(['motor', 'run', '%QX0.0'], ['pump', 'start', '%QX0.0'])) + expect(image.duplicateOutputs).toHaveLength(1) + expect(image.duplicateOutputs[0]).toMatchObject({ + prefix: '%QX', + slot: 0, + first: { scope: 'motor', variableName: 'run' }, + second: { scope: 'pump', variableName: 'start' }, + }) + }) + + it('refuses it within one POU as well', () => { + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('a', '%QW5'), variable('b', '%QW5')] }], + }), + ) + expect(image.duplicateOutputs).toHaveLength(1) + }) + + it('refuses a POU-local colliding with a configuration global', () => { + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('local', '%QW3')] }], + globals: [variable('shared', '%QW3')], + }), + ) + expect(image.duplicateOutputs).toHaveLength(1) + expect(image.duplicateOutputs[0].second.scope).toBe('Global Variables') + }) + + it('allows two POUs to read the same input', () => { + // Ordinary: both read the value the producer put there. + const image = compute(pousWith(['a', 'x', '%IX0.0'], ['b', 'y', '%IX0.0'])) + expect(image.duplicateOutputs).toEqual([]) + }) + + it('allows two POUs to share a memory address', () => { + // Which is what memory is FOR. + const image = compute(pousWith(['a', 'x', '%MW7'], ['b', 'y', '%MW7'])) + expect(image.duplicateOutputs).toEqual([]) + }) + + it('allows distinct outputs', () => { + const image = compute(pousWith(['a', 'x', '%QX0.0'], ['b', 'y', '%QX0.1'])) + expect(image.duplicateOutputs).toEqual([]) + }) + + it('catches overlapping located arrays at the slot they share', () => { + // Their base addresses differ, so a base-address comparison would miss it. + const image = compute( + makeProject({ + pous: [ + { name: 'a', variables: [arrayVar('first', '%QW0', 0, 9)] }, + { name: 'b', variables: [arrayVar('second', '%QW5', 0, 9)] }, + ], + }), + ) + expect(image.duplicateOutputs).toHaveLength(1) + expect(image.duplicateOutputs[0].slot).toBe(5) + }) + + it('reports one entry per pair, not one per overlapping slot', () => { + // A 4000-element array declared twice is one mistake, not 4000 errors. + const image = compute( + makeProject({ + pous: [ + { name: 'a', variables: [arrayVar('first', '%QW0', 0, 3999)] }, + { name: 'b', variables: [arrayVar('second', '%QW0', 0, 3999)] }, + ], + }), + ) + expect(image.duplicateOutputs).toHaveLength(1) + }) + + it('says "both in" when the two are in one scope', () => { + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('a', '%QW5'), variable('b', '%QW5')] }], + }), + ) + expect(describeDuplicateOutput(image.duplicateOutputs[0])).toContain('both in main') + }) + + it('describes the clash so either side can be the one that moves', () => { + const image = compute(pousWith(['motor', 'run', '%QX0.0'], ['pump', 'start', '%QX0.0'])) + const message = describeDuplicateOutput(image.duplicateOutputs[0]) + expect(message).toContain('run') + expect(message).toContain('start') + expect(message).toContain('motor') + expect(message).toContain('pump') + }) +}) + +describe('computeIoImage — S7comm exposure', () => { + const s7Server = (dataBlocks: unknown[], systemAreas?: unknown) => [ + { + name: 's7', + protocol: 's7comm', + s7commSlaveConfig: { server: { enabled: true }, dataBlocks, systemAreas }, + }, + ] + + const block = (type: string, startBuffer: number, sizeBytes: number) => ({ + dbNumber: 1, + description: '', + sizeBytes, + mapping: { type, startBuffer, bitAddressing: false }, + }) + + it('sizes the table a data block names', () => { + // A project with an S7comm server and no Modbus one used to size nothing + // at all from its servers: the walk only ever looked for modbus-tcp. + const image = compute(makeProject({ servers: s7Server([block('int_output', 0, 128)]) })) + // 128 bytes of a word table is 64 words, not 128. + expect(image.sizes).toEqual({ '%QW': 64 }) + }) + + it('converts bytes to elements per width', () => { + const image = compute( + makeProject({ + servers: s7Server([ + block('byte_output', 0, 8), + block('int_memory', 0, 8), + block('dint_memory', 0, 8), + block('lint_memory', 0, 8), + ]), + }), + ) + expect(image.sizes).toEqual({ '%QB': 8, '%MW': 4, '%MD': 2, '%ML': 1 }) + }) + + it('counts a BOOL block in bits, from its element start', () => { + // startBuffer is an element index and bool elements are bytes, so a block + // at element 2 four bytes long reaches bit 47 and needs 48 bits. + const image = compute(makeProject({ servers: s7Server([block('bool_output', 2, 4)]) })) + expect(image.sizes).toEqual({ '%QX': 48 }) + }) + + it('adds the start buffer to the extent', () => { + // The block does not start at zero, and the image is a contiguous buffer. + const image = compute(makeProject({ servers: s7Server([block('int_output', 100, 8)]) })) + expect(image.sizes).toEqual({ '%QW': 104 }) + }) + + it('takes the largest extent when blocks overlap a table', () => { + const image = compute( + makeProject({ servers: s7Server([block('int_output', 0, 8), block('int_output', 50, 8)]) }), + ) + expect(image.sizes).toEqual({ '%QW': 54 }) + }) + + it('BACKS an input block, unlike Modbus', () => { + // The difference is a fact about the protocols. Modbus discrete inputs and + // input registers are read-only to the master BY THE PROTOCOL, so exposing + // one cannot put anything into it. S7comm has no such restriction and the + // plugin implements none: write_buffer_to_openplc_journal dispatches every + // buffer type, _INPUT included. So an S7 client drives these addresses. + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('v', '%IW2')] }], + servers: s7Server([block('int_input', 0, 16)]), + }), + ) + expect(image.unbacked).toEqual([]) + expect(image.sizes['%IW']).toBe(8) + }) + + it('still refuses an input address the block does not reach', () => { + // Backing is per slot, as everywhere else: a block covering eight words + // vouches for eight, not for the ninth. + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('v', '%IW20')] }], + servers: s7Server([block('int_input', 0, 16)]), + }), + ) + expect(image.unbacked).toHaveLength(1) + expect(image.unbacked[0].location).toBe('%IW20') + }) + + it('counts an enabled system area with a mapping', () => { + // PE, PA and MK carry the same mapping shape and reach the same tables. + const image = compute( + makeProject({ + servers: s7Server([], { + paArea: { enabled: true, sizeBytes: 16, mapping: { type: 'int_output', startBuffer: 0, bitAddressing: false } }, + }), + }), + ) + expect(image.sizes).toEqual({ '%QW': 8 }) + }) + + it('sizes nothing for a block too small to hold one element', () => { + // Seven bytes of an lword table is zero addressable lwords. Claiming zero + // would be harmless but claiming ONE would size storage the block does + // not carry. + const image = compute(makeProject({ servers: s7Server([block('lint_memory', 0, 7)]) })) + expect(image.sizes).toEqual({}) + }) + + it('ignores a system area that is switched off', () => { + const image = compute( + makeProject({ + servers: s7Server([], { + paArea: { enabled: false, sizeBytes: 16, mapping: { type: 'int_output', startBuffer: 0, bitAddressing: false } }, + }), + }), + ) + expect(image.sizes).toEqual({}) + }) + + it('ignores an enabled system area with no mapping yet', () => { + const image = compute( + makeProject({ servers: s7Server([], { mkArea: { enabled: true, sizeBytes: 16 } }) }), + ) + expect(image.sizes).toEqual({}) + }) + + it('sizes a Modbus and an S7comm server in the same project', () => { + const servers = [ + { + name: 'mb', + protocol: 'modbus-tcp', + modbusSlaveConfig: { enabled: true, networkInterface: '', port: 502, bufferMapping: { holdingRegisters: { qwCount: 10 } } }, + }, + ...s7Server([block('int_memory', 0, 40)]), + ] + const image = compute(makeProject({ servers })) + expect(image.sizes).toEqual({ '%QW': 10, '%MW': 20 }) + }) + + it('takes the first S7comm server, as the config emitter does', () => { + // generateS7commConfig ships the FIRST s7comm server carrying a config, so + // a second one never reaches the device and must not reserve memory. + const servers = [...s7Server([block('int_output', 0, 8)]), ...s7Server([block('int_output', 0, 800)])] + const image = compute(makeProject({ servers })) + expect(image.sizes).toEqual({ '%QW': 4 }) + }) +}) + describe('computeIoImage — memory is its own producer', () => { it('does not walk a huge array element by element', () => { // The memory path used to mark every declared slot as backed, which was @@ -502,7 +756,7 @@ describe('computeIoImage — BR14, an address with no producer', () => { }, ], }) - expect(compute(project)).toEqual({ sizes: {}, unbacked: [], unsupported: [] }) + expect(compute(project)).toEqual({ sizes: {}, unbacked: [], unsupported: [], duplicateOutputs: [] }) }) }) diff --git a/src/backend/shared/compile/pipeline.ts b/src/backend/shared/compile/pipeline.ts index 12477e4e4..faea35f7e 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -53,6 +53,7 @@ import type { PLCProjectData } from '../types/PLC/open-plc' import { buildCBlocksFromPous, composeFirmwareBundle } from './steps/compose-firmware-bundle' import { computeIoImage, + describeDuplicateOutput, describeUnbackedLocation, describeUnsupportedArea, IMAGE_AREAS_BAREMETAL, @@ -452,7 +453,10 @@ async function runCompilePipelineInner( areas: isRuntimeV4 ? IMAGE_AREAS_RUNTIME_V4 : IMAGE_AREAS_BAREMETAL, }) - if (sizesTheImage && (ioImage.unsupported.length > 0 || ioImage.unbacked.length > 0)) { + if ( + sizesTheImage && + (ioImage.unsupported.length > 0 || ioImage.unbacked.length > 0 || ioImage.duplicateOutputs.length > 0) + ) { // Both lists, not the first non-empty one: a project can carry each kind of // mistake, and reporting one round at a time turns a single fix into // several compile attempts. @@ -462,6 +466,9 @@ async function runCompilePipelineInner( for (const issue of ioImage.unbacked) { emit({ stage: 'validate', message: describeUnbackedLocation(issue), level: 'error' }) } + for (const issue of ioImage.duplicateOutputs) { + emit({ stage: 'validate', message: describeDuplicateOutput(issue), level: 'error' }) + } return bailError(emit, 'validate', 'Compilation aborted: every located variable needs an address that exists.') } diff --git a/src/backend/shared/compile/steps/compute-io-image.ts b/src/backend/shared/compile/steps/compute-io-image.ts index 8de84105a..d9a06c88f 100644 --- a/src/backend/shared/compile/steps/compute-io-image.ts +++ b/src/backend/shared/compile/steps/compute-io-image.ts @@ -58,6 +58,12 @@ import { prefixOf, } from '../../../../middleware/shared/utils/iec-address/registry' import type { AddressProducerCapabilities } from '../../../../middleware/shared/utils/target-capabilities' +import { + extentForDataBlock, + IMAGE_AREAS_BAREMETAL, + IMAGE_AREAS_RUNTIME_V4, + IMAGE_TABLES, +} from '../../../../middleware/shared/utils/io-image/tables' import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' /** @@ -119,67 +125,53 @@ export interface UnsupportedArea { prefix: string } +/** The slice of an S7comm mapping this step reads. Structural rather than + * imported so the sizer stays free of the server schema's exact shape. */ +interface S7CommMappingLike { + type: string + startBuffer: number +} + +/** + * Two declarations driving the same output slot. + * + * IEC located addresses are GLOBAL, but the editor's own duplicate check reads + * one variable list at a time (`validation/variables.ts`), so two POUs can each + * declare `AT %QX0.0` and both pass. Nothing downstream notices: the generated + * code assigns to the same storage from two places and the last write in the + * scan wins, which is a coin toss decided by POU order. + * + * Reported for OUTPUTS only. Two POUs reading one input is ordinary -- they + * read the same value -- and sharing a memory address is what memory is for. + * An output is the one direction where two writers contradict each other. + */ +export interface DuplicateOutput { + location: string + prefix: string + /** The slot both declarations cover, which for arrays need not be either + * declaration's base address. */ + slot: number + /** Both sides, in declaration order, so the message can name them. */ + first: { scope: string; variableName: string } + second: { scope: string; variableName: string } +} + export interface IoImage { sizes: IoImageSizes /** Empty when every input and output declaration is backed. */ unbacked: UnbackedLocation[] /** Empty when every declaration names an area the target actually has. */ unsupported: UnsupportedArea[] + /** Empty when no two declarations drive the same output slot. */ + duplicateOutputs: DuplicateOutput[] } -/** - * The areas the bare-metal firmware declares buffers for. - * - * Read off the `extern` declarations in `resources/sources/arduino/openplc.h`: - * `bool_input`, `bool_output`, `int_input`, `int_output`, `real_input`, - * `real_output`, `int_memory`, `dint_memory`, `lint_memory`. There is no - * byte-addressed buffer of any kind and no bit-addressed MEMORY area, so - * `%IB`, `%QB`, `%MB` and `%MX` name storage that does not exist. - * - * Deliberately the larger of the header's two MCU branches. The small-AVR - * branch (ATmega328P and friends) declares no `real_*` and no memory arrays at - * all, and telling the two apart 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. Permissive is the safe direction: the - * cost is a variable that stays inert exactly as it does today, while being - * strict would refuse to build projects that have been building for years. - */ -export const IMAGE_AREAS_BAREMETAL: ReadonlySet = new Set([ - '%IX', - '%QX', - '%IW', - '%QW', - '%ID', - '%QD', - '%MW', - '%MD', - '%ML', -]) +/* The two area sets are DERIVED, not listed here: which tables exist and + * which runtime declares each one is stated once, in + * `middleware/shared/utils/io-image/tables.ts`, and re-exported so the + * pipeline keeps importing them from the step that uses them. */ +export { IMAGE_AREAS_BAREMETAL, IMAGE_AREAS_RUNTIME_V4 } -/** - * The areas Runtime v4 declares tables for — the fourteen of - * `core/src/plc_app/image_tables.h`, the same fourteen the S7comm buffer - * enumeration in `types/PLC/open-plc.ts` names. - * - * Note the one gap: there is `byte_input` and `byte_output` but no - * `byte_memory`, so `%MB` has no storage on v4 either. - */ -export const IMAGE_AREAS_RUNTIME_V4: ReadonlySet = new Set([ - '%IX', - '%QX', - '%MX', - '%IB', - '%QB', - '%IW', - '%QW', - '%MW', - '%ID', - '%QD', - '%MD', - '%IL', - '%QL', - '%ML', -]) export interface ComputeIoImageInput { /** Compile-ready project data — locations already resolved from aliases to @@ -339,55 +331,137 @@ function producerClaims(input: ComputeIoImageInput, backed: Map>): Record { const sizes: Record = {} - const server = (servers ?? []).find((entry) => entry.protocol === 'modbus-tcp' && entry.modbusSlaveConfig) - const mapping: ModbusBufferMapping | undefined = server?.modbusSlaveConfig?.bufferMapping - - if (mapping) { - // Each IEC segment is laid out from index 0 of its own prefix space; only - // the Modbus offsets are sequential across segments. `address-mapping.ts` - // is the authority on that layout, and it agrees with the runtime plugin. - // EXPOSURE SIZES EVERY SEGMENT, BUT ONLY BACKS THE WRITABLE ONES. - // - // Backing means "something on the other side gives this address meaning", - // which for BR14 is what the area needs to not be inert. Holding registers - // and coils are writable by the master, so a `%QW` or `%QX` the server - // publishes has a counterpart: the master reads what the program wrote, or - // writes it itself. Either way it is drained. - // - // Discrete inputs and input registers are READ-ONLY to the master. Nothing - // writes `%IX` or `%IW` through them — the server only publishes whatever - // is already there. So exposing them cannot make an input backed, and - // treating it as if it did would let `AT %IW7 : INT` pass the gate with no - // pin, no master point and no EtherCAT channel anywhere, which is exactly - // the declaration BR14 exists to catch. - // - // They still SIZE, because the server has to have the storage to read - // from; they just do not vouch for anything living in it. - const sizing: Array<[string, number | undefined]> = [ - ['%QW', mapping.holdingRegisters?.qwCount], - ['%MW', mapping.holdingRegisters?.mwCount], - ['%MD', mapping.holdingRegisters?.mdCount], - ['%ML', mapping.holdingRegisters?.mlCount], - ['%QX', mapping.coils?.qxBits], - ['%MX', mapping.coils?.mxBits], - ['%IX', mapping.discreteInputs?.ixBits], - ['%IW', mapping.inputRegisters?.iwCount], - ] - const WRITABLE_BY_THE_MASTER = new Set(['%QW', '%MW', '%MD', '%ML', '%QX', '%MX']) - - for (const [prefix, count] of sizing) { - // A count of zero is the `%MX` default and a legitimate answer: the - // segment exists and is switched off, so it exposes nothing and sizes - // nothing. - if (count === undefined || count <= 0) continue - claim(sizes, prefix, count) - if (WRITABLE_BY_THE_MASTER.has(prefix)) markBacked(backed, prefix, 0, count) - } - } + + // EVERY PROTOCOL, but still the FIRST server of each one. + // + // The generalisation that was missing is across protocols: a project with an + // S7comm server and no Modbus one used to size nothing at all from its + // servers, because this function only ever looked for `modbus-tcp`. + // + // What is NOT generalised is the count per protocol, and that is deliberate. + // Each emitter ships one file built from the FIRST server of its protocol + // carrying a config -- `generateModbusSlaveConfig` and `generateS7commConfig` + // both `.find(...)`. A second server of the same protocol never reaches the + // device, so sizing for its exposure would reserve memory nothing can use, + // which is BR10 backwards. The rule here mirrors the emitters rather than + // inventing one, and if they ever ship more than one, this follows. + const list = servers ?? [] + const modbus = list.find((server) => server.protocol === 'modbus-tcp' && server.modbusSlaveConfig) + const s7comm = list.find((server) => server.protocol === 's7comm' && server.s7commSlaveConfig) + + if (modbus?.modbusSlaveConfig) modbusExposure(modbus.modbusSlaveConfig.bufferMapping, sizes, backed) + if (s7comm?.s7commSlaveConfig) s7commExposure(s7comm.s7commSlaveConfig, sizes, backed) return sizes } +/** + * What a Modbus server publishes. + * + * EXPOSURE SIZES EVERY SEGMENT, BUT ONLY BACKS THE WRITABLE ONES. + * + * Backing means "something on the other side gives this address meaning", + * which for BR14 is what the area needs to not be inert. Holding registers + * and coils are writable by the master, so a `%QW` or `%QX` the server + * publishes has a counterpart: the master reads what the program wrote, or + * writes it itself. Either way it is drained. + * + * Discrete inputs and input registers are READ-ONLY to the master. Nothing + * writes `%IX` or `%IW` through them -- the server only publishes whatever is + * already there. So exposing them cannot make an input backed, and treating + * it as if it did would let `AT %IW7 : INT` pass the gate with no pin, no + * master point and no EtherCAT channel anywhere, which is exactly the + * declaration BR14 exists to catch. + * + * They still SIZE, because the server has to have the storage to read from; + * they just do not vouch for anything living in it. + */ +function modbusExposure( + mapping: ModbusBufferMapping | undefined, + sizes: Record, + backed: Map>, +): void { + if (!mapping) return + + // Each IEC segment is laid out from index 0 of its own prefix space; only + // the Modbus offsets are sequential across segments. `address-mapping.ts` + // is the authority on that layout, and it agrees with the runtime plugin. + const sizing: Array<[string, number | undefined]> = [ + ['%QW', mapping.holdingRegisters?.qwCount], + ['%MW', mapping.holdingRegisters?.mwCount], + ['%MD', mapping.holdingRegisters?.mdCount], + ['%ML', mapping.holdingRegisters?.mlCount], + ['%QX', mapping.coils?.qxBits], + ['%MX', mapping.coils?.mxBits], + ['%IX', mapping.discreteInputs?.ixBits], + ['%IW', mapping.inputRegisters?.iwCount], + ] + const WRITABLE_BY_THE_MASTER = new Set(['%QW', '%MW', '%MD', '%ML', '%QX', '%MX']) + + for (const [prefix, count] of sizing) { + // A count of zero is the `%MX` default and a legitimate answer: the + // segment exists and is switched off, so it exposes nothing and sizes + // nothing. + if (count === undefined || count <= 0) continue + claim(sizes, prefix, count) + if (WRITABLE_BY_THE_MASTER.has(prefix)) markBacked(backed, prefix, 0, count) + } +} + +/** + * What an S7comm server publishes. + * + * EVERY BLOCK BACKS WHAT IT COVERS, INPUTS INCLUDED -- and that is the + * opposite of the Modbus answer above, for a reason that is a fact about the + * protocols rather than a preference. + * + * Modbus discrete inputs and input registers are read-only to the master BY + * THE PROTOCOL: there is no function code that writes them, so exposing an + * `%IX` through Modbus cannot put anything into it. S7comm has no such + * restriction, and the OpenPLC plugin implements none: its write path + * (`write_buffer_to_openplc_journal`, s7comm_plugin.cpp) dispatches every + * buffer type including `BUFFER_TYPE_BOOL_INPUT`, `BUFFER_TYPE_INT_INPUT`, + * `BUFFER_TYPE_DINT_INPUT` and `BUFFER_TYPE_LINT_INPUT`. An S7 client writes + * an input table as readily as an output one. + * + * So BR14's question -- is there something external giving this address + * meaning -- answers yes in both directions here. A block mapped onto + * `int_input` is a producer for the `%IW`s it covers, and a program reading + * them is reading what the client wrote rather than a constant zero. + * + * Both the data blocks and the three system areas (PE, PA, MK) are read: they + * carry the same mapping shape and reach the same tables, so leaving the + * system areas out would size an area the server is serving. + */ +function s7commExposure( + config: NonNullable, + sizes: Record, + backed: Map>, +): void { + const blocks: Array<{ mapping?: S7CommMappingLike; sizeBytes: number }> = [ + ...(config.dataBlocks ?? []), + ...[config.systemAreas?.peArea, config.systemAreas?.paArea, config.systemAreas?.mkArea] + .filter((area): area is NonNullable => Boolean(area?.enabled)) + .map((area) => ({ mapping: area.mapping, sizeBytes: area.sizeBytes })), + ] + + for (const block of blocks) { + // A system area may be enabled with no mapping yet, which publishes + // nothing and sizes nothing. + if (!block.mapping) continue + const table = IMAGE_TABLES.find((entry) => entry.key === block.mapping?.type) + /* istanbul ignore next -- the schema admits only table names, so a block + naming something else cannot reach here today. Sizing nothing for it is + the safe reading if that ever changes. */ + if (!table) continue + + const extent = extentForDataBlock(table, block.mapping.startBuffer, block.sizeBytes) + if (extent <= 0) continue + claim(sizes, table.prefix, extent) + markBacked(backed, table.prefix, 0, extent) + } +} + /** * Every located variable in the project, paired with the scope declaring it. * @@ -479,6 +553,9 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { const unbacked: UnbackedLocation[] = [] const unsupported: UnsupportedArea[] = [] + const duplicateOutputs: DuplicateOutput[] = [] + /** prefix -> slot -> the first declaration that claimed it. Outputs only. */ + const outputOwners = new Map>() for (const { scope, name, location, slotCount } of locatedVariables(input.projectData)) { const parsed = parseAddress(location) @@ -501,6 +578,35 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { continue } + if (directionOf(prefix) === 'Q') { + // Every slot the declaration covers, so a located array overlapping + // another one is caught at the slot they share rather than only when + // their base addresses match. + let owners = outputOwners.get(prefix) + if (!owners) { + owners = new Map() + outputOwners.set(prefix, owners) + } + for (let slot = parsed.linear; slot < parsed.linear + slotCount; slot++) { + const owner = owners.get(slot) + if (owner) { + duplicateOutputs.push({ + location, + prefix, + slot, + first: owner, + second: { scope, variableName: name }, + }) + // One report per pair of declarations, not one per overlapping slot: + // a 4000-element array declared twice is one mistake. + break + } + } + for (let slot = parsed.linear; slot < parsed.linear + slotCount; slot++) { + if (!owners.has(slot)) owners.set(slot, { scope, variableName: name }) + } + } + if (directionOf(prefix) === 'M') { // Memory is its own producer (BR14), so the declaration SIZES the area // and is never reported unbacked (FR24). Without this a program using @@ -540,7 +646,7 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { }) } - return { sizes, unbacked, unsupported } + return { sizes, unbacked, unsupported, duplicateOutputs } } /** @@ -567,6 +673,27 @@ export function describeUnbackedLocation(issue: UnbackedLocation): string { ) } +/** + * One-line rendering of two declarations driving the same output. + * + * Names BOTH, because either one may be the mistake and the user cannot tell + * which from an address alone -- and because the two are usually in different + * POUs, which is the whole reason the editor's per-list check missed it. + */ +export function describeDuplicateOutput(issue: DuplicateOutput): string { + const where = + issue.first.scope === issue.second.scope + ? `both in ${issue.first.scope}` + : `${issue.first.scope} and ${issue.second.scope}` + return ( + `Two variables drive the same output: "${issue.first.variableName}" and ` + + `"${issue.second.variableName}" (${where}) both cover slot ${issue.slot} of ` + + `${issue.prefix}. IEC located addresses are global, so the last write in the scan ` + + 'would win and which one that is depends on POU order — give one of them another ' + + 'address, or have one read the other rather than both writing.' + ) +} + /** * One-line rendering of a declaration in an area the target does not have. * diff --git a/src/backend/shared/compile/steps/generate-defines.ts b/src/backend/shared/compile/steps/generate-defines.ts index 5cafd811b..f8489f220 100644 --- a/src/backend/shared/compile/steps/generate-defines.ts +++ b/src/backend/shared/compile/steps/generate-defines.ts @@ -18,6 +18,7 @@ */ import type { DevicePin } from '../../types/PLC/devices' +import { IMAGE_TABLES } from '../../../../middleware/shared/utils/io-image/tables' import type { IoImageSizes } from './compute-io-image' import { generateModbusDefines, resolveDebugBaud, resolveDebugSlave, type VppModbusScreenState } from './modbus-defines' @@ -42,17 +43,13 @@ export type { VppModbusScreenState } from './modbus-defines' * writes `bool_input[BUFFER_SIZE][8]` and does not. Each emitter therefore * states its own unit rather than sharing a "converted" number. */ -const PROCESS_IMAGE_MACROS: ReadonlyArray = [ - ['%IX', 'MAX_DIGITAL_INPUT'], - ['%QX', 'MAX_DIGITAL_OUTPUT'], - ['%IW', 'MAX_ANALOG_INPUT'], - ['%QW', 'MAX_ANALOG_OUTPUT'], - ['%ID', 'MAX_REAL_INPUT'], - ['%QD', 'MAX_REAL_OUTPUT'], - ['%MW', 'MAX_MEMORY_WORD'], - ['%MD', 'MAX_MEMORY_DWORD'], - ['%ML', 'MAX_MEMORY_LWORD'], -] +/* The nine macros bare metal declares, and the order it declares them in, + * both derived from the one table list. Filtering that list to the entries + * carrying a macro yields exactly the order this file emitted when the nine + * were written out by hand, so no output moves. */ +const PROCESS_IMAGE_MACROS: ReadonlyArray = IMAGE_TABLES.filter( + (table): table is typeof table & { macro: string } => table.macro !== undefined, +).map((table) => [table.prefix, table.macro] as const) /** * Bit areas reach the firmware as a whole number of bytes (FR06, BR04, CON05). diff --git a/src/backend/shared/compile/steps/generate-image-conf.ts b/src/backend/shared/compile/steps/generate-image-conf.ts index a95a18414..ca485577a 100644 --- a/src/backend/shared/compile/steps/generate-image-conf.ts +++ b/src/backend/shared/compile/steps/generate-image-conf.ts @@ -46,6 +46,7 @@ * string into the upload bundle. */ +import { IMAGE_TABLES } from '../../../../middleware/shared/utils/io-image/tables' import type { IoImageSizes } from './compute-io-image' /** @@ -57,22 +58,6 @@ import type { IoImageSizes } from './compute-io-image' * able to go down both in step. Fixed order is also what makes the output * byte-stable for the same project (FR07). */ -const TABLES: ReadonlyArray<{ key: string; prefix: string; unit: string }> = [ - { key: 'bool_input', prefix: '%IX', unit: 'bits' }, - { key: 'bool_output', prefix: '%QX', unit: 'bits' }, - { key: 'byte_input', prefix: '%IB', unit: 'bytes' }, - { key: 'byte_output', prefix: '%QB', unit: 'bytes' }, - { key: 'int_input', prefix: '%IW', unit: 'words' }, - { key: 'int_output', prefix: '%QW', unit: 'words' }, - { key: 'dint_input', prefix: '%ID', unit: 'dwords' }, - { key: 'dint_output', prefix: '%QD', unit: 'dwords' }, - { key: 'lint_input', prefix: '%IL', unit: 'lwords' }, - { key: 'lint_output', prefix: '%QL', unit: 'lwords' }, - { key: 'int_memory', prefix: '%MW', unit: 'words' }, - { key: 'dint_memory', prefix: '%MD', unit: 'dwords' }, - { key: 'lint_memory', prefix: '%ML', unit: 'lwords' }, - { key: 'bool_memory', prefix: '%MX', unit: 'bits' }, -] /** Bumped whenever a reader would misread an older file. Version 2 is the * first version any device has ever seen: version 1 was written but never @@ -110,7 +95,7 @@ export function generateImageConf(sizes: IoImageSizes): string { `format_version=${FORMAT_VERSION}`, ] - for (const table of TABLES) { + for (const table of IMAGE_TABLES) { lines.push(`${table.key}=${sizes[table.prefix] ?? 0} ${table.unit}`) } diff --git a/src/backend/shared/types/PLC/open-plc.ts b/src/backend/shared/types/PLC/open-plc.ts index 4c3ef7ded..527794097 100644 --- a/src/backend/shared/types/PLC/open-plc.ts +++ b/src/backend/shared/types/PLC/open-plc.ts @@ -1,5 +1,8 @@ import { z } from 'zod' +import type { ImageTableKey } from '../../../../middleware/shared/utils/io-image/tables' +import { IMAGE_TABLES } from '../../../../middleware/shared/utils/io-image/tables' + import { zodFBDFlowSchema, zodLadderFlowSchema } from '../../../../middleware/shared/ports/flow-schemas' // One source of truth for the IEC base-type list: the canonical // schema lives in `middleware/shared/ports/plc-schemas` and is @@ -318,22 +321,15 @@ const ModbusSlaveConfigSchema = z.object({ type ModbusSlaveConfig = z.infer // S7Comm Buffer Type Enumeration -const S7CommBufferTypeSchema = z.enum([ - 'bool_input', - 'bool_output', - 'bool_memory', - 'byte_input', - 'byte_output', - 'int_input', - 'int_output', - 'int_memory', - 'dint_input', - 'dint_output', - 'dint_memory', - 'lint_input', - 'lint_output', - 'lint_memory', -]) +/* The tables an S7comm data block may be mapped onto are the tables the + * runtime HAS, so the list is derived rather than transcribed. A table added + * to the runtime and forgotten here would be a block the user cannot declare; + * one removed and forgotten would be a block that sizes storage that is gone. + * + * `z.enum` needs a non-empty tuple literal, hence the cast on the spread. */ +const S7CommBufferTypeSchema = z.enum( + IMAGE_TABLES.map((table) => table.key) as [ImageTableKey, ...ImageTableKey[]], +) type S7CommBufferType = z.infer // S7Comm Server Settings Schema diff --git a/src/frontend/components/_molecules/variables-table/editable-cell.tsx b/src/frontend/components/_molecules/variables-table/editable-cell.tsx index 100462652..ca4562bdf 100644 --- a/src/frontend/components/_molecules/variables-table/editable-cell.tsx +++ b/src/frontend/components/_molecules/variables-table/editable-cell.tsx @@ -1,5 +1,6 @@ import * as PrimitivePopover from '@radix-ui/react-popover' import { useAliasRegistry } from '@root/frontend/hooks/use-alias-registry' +import { useDuplicateOutputLocations } from '@root/frontend/hooks/use-duplicate-output-locations' import { useProjectAliasBindings } from '@root/frontend/hooks/use-project-alias-bindings' import { useTargetCapabilities } from '@root/frontend/hooks/use-target-capabilities' import { isLiteralLocation } from '@root/middleware/shared/utils/iec-address/registry' @@ -503,7 +504,16 @@ const EditableLocationCell = ({ ? aliasBindings.find((binding) => binding.address === locationValue) : undefined const isManualConflict = locationConflict !== undefined - const hasLocationWarning = isOrphaned || isManualConflict + // Two literals on one OUTPUT address, which the per-list duplicate check + // cannot see: it reads this POU's variables, and the other declaration is + // usually in another POU or the global scope. Same glyph, same reasoning as + // the alias conflict above — the compiler refuses this (DOPE-615, B5), and + // saying so while the user is still typing is cheaper than saying it at + // build time. Inputs and memory are excluded: only two WRITERS contradict. + const duplicateOutputs = useDuplicateOutputLocations() + const isDuplicateOutput = + isLocationCell && (duplicateOutputs.get(locationValue)?.length ?? 0) > 1 + const hasLocationWarning = isOrphaned || isManualConflict || isDuplicateOutput // When the input is blurred, we'll call our table meta's updateData function const onBlur = (value: string) => { @@ -550,11 +560,16 @@ const EditableLocationCell = ({ // the variable is alias-bound, the literal address when manual. The // combobox `value` is the same string, so picking an alias option (whose // value is the alias name) or typing a literal both operate on `location`. + const otherWriters = (duplicateOutputs.get(locationValue) ?? []).filter( + (name) => name !== variable?.name, + ) const warningTooltip = isOrphaned ? `Alias "${cellValue}" is not declared by any active I/O source — this variable is unlocated at compile time.` : locationConflict ? `Address ${cellValue} conflicts with alias "${locationConflict.aliasName}" assigned to "${locationConflict.variableName}". Two variables cannot share a location.` - : undefined + : isDuplicateOutput + ? `Output ${cellValue} is also driven by ${otherWriters.map((name) => `"${name}"`).join(', ')}. IEC located addresses are global, so the last write in the scan would win — the compiler refuses this.` + : undefined // The warning glyph must stay visible whether or not the row is selected. // The selected branch renders an editable combobox; previously the glyph @@ -563,7 +578,13 @@ const EditableLocationCell = ({ const warningGlyph = hasLocationWarning && warningTooltip ? ( ) : null diff --git a/src/frontend/hooks/use-duplicate-output-locations.ts b/src/frontend/hooks/use-duplicate-output-locations.ts new file mode 100644 index 000000000..25c88b249 --- /dev/null +++ b/src/frontend/hooks/use-duplicate-output-locations.ts @@ -0,0 +1,68 @@ +/** + * Literal output addresses declared more than once in the project + * (DOPE-615, B6). + * + * IEC located addresses are GLOBAL. The variables table's own duplicate check + * reads one variable list at a time, so two POUs can each declare `AT %QX0.0` + * and both pass; the compiler refuses it (`computeIoImage`), but only once the + * user has finished and pressed build. + * + * This is the same fact answered while editing. It is the literal-against- + * literal counterpart of the alias scan `useProjectAliasBindings` already + * does, and it is project-wide for the same reason that one is. + * + * OUTPUTS ONLY, matching the compile-time rule: two POUs reading one input is + * ordinary, and sharing a memory address is what memory is for. An output is + * the one direction where two writers contradict each other and the last write + * in the scan wins. + * + * Exact addresses only. A located array overlapping another one is a real + * clash and the compiler reports it, but expanding every declaration into its + * slots on each keystroke would cost far more than the warning is worth here. + */ + +import { useOpenPLCStore } from '@root/frontend/store' +import type { PLCVariable } from '@root/middleware/shared/ports/types' +import { isLiteralLocation } from '@root/middleware/shared/utils/iec-address/registry' + +/** `'%QX0.0'` -> the names of every variable declaring exactly that address. */ +export type DuplicateOutputMap = ReadonlyMap + +interface Cache { + pous: unknown + globals: unknown + map: DuplicateOutputMap +} + +let cache: Cache | null = null + +/** `%Q…` — the only direction where a second declaration is a contradiction. */ +function isOutputLocation(location: string): boolean { + return isLiteralLocation(location) && location.charAt(1) === 'Q' +} + +export function useDuplicateOutputLocations(): DuplicateOutputMap { + const pous = useOpenPLCStore((s) => s.project.data.pous) + const globals = useOpenPLCStore((s) => s.project.data.configurations.resource.globalVariables) + + // Same single-entry cache as the alias scan next door: dozens of cells ask + // for this in one render pass, and Zustand keeps identity stable when + // nothing changed. + if (cache && cache.pous === pous && cache.globals === globals) return cache.map + + const map = new Map() + const collect = (variables: PLCVariable[] | undefined): void => { + for (const variable of variables ?? []) { + const location = variable.location ?? '' + if (!isOutputLocation(location)) continue + const names = map.get(location) + if (names) names.push(variable.name) + else map.set(location, [variable.name]) + } + } + for (const pou of pous) collect(pou.interface?.variables) + collect(globals) + + cache = { pous, globals, map } + return map +} diff --git a/src/frontend/utils/vpp/__tests__/resolve-module-channels.test.ts b/src/frontend/utils/vpp/__tests__/resolve-module-channels.test.ts index f0e821981..2985b4542 100644 --- a/src/frontend/utils/vpp/__tests__/resolve-module-channels.test.ts +++ b/src/frontend/utils/vpp/__tests__/resolve-module-channels.test.ts @@ -1,4 +1,4 @@ -import { resolveModuleChannels, type ResolverModuleDef } from '../resolve-module-channels' +import { isValidManifestPrefix, resolveModuleChannels, type ResolverModuleDef } from '../resolve-module-channels' const rawChannels = [{ name: 'AI1', type: 'analogInput', dataType: 'UINT', addressPrefix: '%IW' }] const euChannels = [{ name: 'AI1', type: 'analogInput', dataType: 'REAL', addressPrefix: '%ID' }] @@ -165,3 +165,45 @@ describe('resolveModuleChannels', () => { expect(resolveModuleChannels(md, { data_format: 'engineering', i1_mode: 'bool' })).toEqual(euChannels) }) }) + +describe('manifest address prefixes (DOPE-615, B7)', () => { + const withChannels = (channels: unknown[]) => + resolveModuleChannels({ addressMapping: { channels } } as never, undefined) + + it('accepts the eight the package schema allows', () => { + for (const prefix of ['%IX', '%QX', '%IW', '%QW', '%ID', '%QD', '%IL', '%QL']) { + expect(isValidManifestPrefix(prefix)).toBe(true) + } + }) + + it('refuses a memory prefix', () => { + // The one that matters. Memory has no external producer, so a channel + // claiming %MW would have memory allocated for a module that is not + // driving it -- the address space sized for a producer that is not there. + expect(isValidManifestPrefix('%MW')).toBe(false) + expect(isValidManifestPrefix('%MX')).toBe(false) + }) + + it('refuses a byte prefix, which the schema does not admit either', () => { + expect(isValidManifestPrefix('%IB')).toBe(false) + expect(isValidManifestPrefix('%QB')).toBe(false) + }) + + it('refuses anything that is not a prefix at all', () => { + expect(isValidManifestPrefix('')).toBe(false) + expect(isValidManifestPrefix('QW')).toBe(false) + expect(isValidManifestPrefix('%ZZ')).toBe(false) + }) + + it('drops the offending channel and keeps the rest', () => { + // One bad channel must not take down the whole device screen. + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}) + const out = withChannels([ + { name: 'good', type: 'analogInput', dataType: 'UINT', addressPrefix: '%IW' }, + { name: 'bad', type: 'analogInput', dataType: 'UINT', addressPrefix: '%MW' }, + ]) + expect(out.map((channel) => channel.name)).toEqual(['good']) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('bad')) + warn.mockRestore() + }) +}) diff --git a/src/frontend/utils/vpp/resolve-module-channels.ts b/src/frontend/utils/vpp/resolve-module-channels.ts index 835400cf1..913c91ff8 100644 --- a/src/frontend/utils/vpp/resolve-module-channels.ts +++ b/src/frontend/utils/vpp/resolve-module-channels.ts @@ -82,6 +82,42 @@ export type ResolverModuleDef = { export type SlotFieldValue = string | number | boolean +/** + * The address prefixes a VPP manifest may declare for a channel. + * + * The package schema (`schema/manifest.schema.json`, `addressPrefix`) admits + * exactly these eight: inputs and outputs, bit through lword. No byte-addressed + * prefix and no memory prefix. + * + * The editor enforces the same set rather than trusting the manifest, and the + * reason is narrow but real: `addressMapping` is typed `unknown` where the + * board entry is read (`backend/editor/hardware/types.ts`), so the string + * reaches `nextFreeAddress` unchecked. A manifest declaring `%MW` -- hand + * edited, from an older packaging tool, or simply wrong -- would then have + * memory allocated for it as if a module produced it, which is the one thing + * BR14 says nothing can do: memory has no external producer, and an address + * space that thinks otherwise is sized for a producer that is not there. + * + * A refused channel is DROPPED rather than throwing: one bad channel in a + * manifest must not take down the whole device screen, and a channel that + * allocates nothing is visibly missing in a way the user can report. + */ +const MANIFEST_ADDRESS_PREFIXES: ReadonlySet = new Set([ + '%IX', + '%QX', + '%IW', + '%QW', + '%ID', + '%QD', + '%IL', + '%QL', +]) + +/** Whether a manifest channel names a prefix the schema allows. */ +export function isValidManifestPrefix(prefix: string): boolean { + return MANIFEST_ADDRESS_PREFIXES.has(prefix) +} + export function resolveModuleChannels( moduleDef: ResolverModuleDef | undefined, slotConfig: Record | undefined, @@ -124,5 +160,15 @@ export function resolveModuleChannels( } } } - return out + return out.filter((channel) => { + if (isValidManifestPrefix(channel.addressPrefix)) return true + // Warned rather than silent: the channel disappears from the screen, and + // without this line there is nothing anywhere saying why. + console.warn( + `VPP manifest: channel "${channel.name}" declares address prefix ` + + `"${channel.addressPrefix}", which is not one the manifest schema allows ` + + `(${[...MANIFEST_ADDRESS_PREFIXES].join(', ')}). The channel is ignored.`, + ) + return false + }) } diff --git a/src/middleware/shared/utils/io-image/__tests__/tables.test.ts b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts new file mode 100644 index 000000000..89ea01be4 --- /dev/null +++ b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts @@ -0,0 +1,184 @@ +/** + * The canonical table list, pinned (DOPE-615, B1). + * + * This list is the single source for five things that used to be written out + * separately: the two area sets, the `image.conf` keys and units, the + * bare-metal macros, and the S7comm buffer enumeration. That makes it worth + * pinning hard — a change here moves all five at once, which is the point, + * and should therefore be deliberate rather than incidental. + * + * The other half of the contract lives in the runtime repository, whose own + * pytest reads the C sources and checks the enum, the key array, the struct + * fields and the units against its Python list. Neither repository can import + * the other, so both hold the same order for the same stated reason: the + * declaration order of `core/src/plc_app/image_tables.h`. + */ + +import { + elementsFor, + extentForDataBlock, + IMAGE_AREAS_BAREMETAL, + IMAGE_AREAS_RUNTIME_V4, + IMAGE_TABLES, + tableForPrefix, +} from '../tables' + +describe('IMAGE_TABLES', () => { + it('lists the fourteen tables in the header declaration order', () => { + // Pinned as a literal rather than derived: the whole value of this list is + // that the order is the header's, and deriving the expectation from the + // list would agree with itself no matter what happened to it. + expect(IMAGE_TABLES.map((table) => table.key)).toEqual([ + 'bool_input', + 'bool_output', + 'byte_input', + 'byte_output', + 'int_input', + 'int_output', + 'dint_input', + 'dint_output', + 'lint_input', + 'lint_output', + 'int_memory', + 'dint_memory', + 'lint_memory', + 'bool_memory', + ]) + }) + + it('has no byte-addressed memory table', () => { + // Not an oversight to be tidied: the runtime declares byte_input and + // byte_output but no byte_memory, so `%MB` has no storage anywhere. + expect(IMAGE_TABLES.map((table) => table.key)).not.toContain('byte_memory') + expect(tableForPrefix('%MB')).toBeUndefined() + }) + + it('gives every table exactly one prefix, and every prefix one table', () => { + const prefixes = IMAGE_TABLES.map((table) => table.prefix) + expect(new Set(prefixes).size).toBe(prefixes.length) + for (const table of IMAGE_TABLES) { + expect(tableForPrefix(table.prefix)).toBe(table) + } + }) + + it('counts only the BOOL tables in bits', () => { + // The three whose storage unit and address unit differ, which is the one + // place a factor of eight can hide. + const inBits = IMAGE_TABLES.filter((table) => table.unit === 'bits').map((t) => t.key) + expect(inBits).toEqual(['bool_input', 'bool_output', 'bool_memory']) + }) + + it('pairs each unit with the width its prefix addresses', () => { + const expected: Record = { + X: 'bits', + B: 'bytes', + W: 'words', + D: 'dwords', + L: 'lwords', + } + for (const table of IMAGE_TABLES) { + expect(table.unit).toBe(expected[table.prefix.charAt(2)]) + } + }) +}) + +describe('the area sets derived from it', () => { + it('gives Runtime v4 every table', () => { + expect(IMAGE_AREAS_RUNTIME_V4.size).toBe(IMAGE_TABLES.length) + }) + + it('gives bare metal only the areas it declares a buffer for', () => { + expect([...IMAGE_AREAS_BAREMETAL].sort()).toEqual( + ['%ID', '%IW', '%IX', '%MD', '%ML', '%MW', '%QD', '%QW', '%QX'].sort(), + ) + }) + + it('withholds from bare metal exactly the areas with no macro', () => { + // %MX is the one that matters: bare metal has no bool_memory, which is why + // a `%MX` declaration there is reported rather than silently dropped. + const withoutMacro = IMAGE_TABLES.filter((t) => !t.macro).map((t) => t.prefix) + expect(withoutMacro).toEqual(['%IB', '%QB', '%IL', '%QL', '%MX']) + for (const prefix of withoutMacro) { + expect(IMAGE_AREAS_BAREMETAL.has(prefix)).toBe(false) + expect(IMAGE_AREAS_RUNTIME_V4.has(prefix)).toBe(true) + } + }) + + it('emits the bare-metal macros in the order defines.h already used', () => { + // Filtering the canonical list must not move any macro, or every firmware + // rebuilds for a reordering that means nothing. + expect(IMAGE_TABLES.filter((t) => t.macro).map((t) => t.macro)).toEqual([ + 'MAX_DIGITAL_INPUT', + 'MAX_DIGITAL_OUTPUT', + 'MAX_ANALOG_INPUT', + 'MAX_ANALOG_OUTPUT', + 'MAX_REAL_INPUT', + 'MAX_REAL_OUTPUT', + 'MAX_MEMORY_WORD', + 'MAX_MEMORY_DWORD', + 'MAX_MEMORY_LWORD', + ]) + }) +}) + +describe('elementsFor', () => { + it('rounds a bit count up to whole bytes', () => { + // Rounding down would make the slots of the partial byte unaddressable. + expect(elementsFor({ unit: 'bits' }, 0)).toBe(0) + expect(elementsFor({ unit: 'bits' }, 1)).toBe(1) + expect(elementsFor({ unit: 'bits' }, 8)).toBe(1) + expect(elementsFor({ unit: 'bits' }, 9)).toBe(2) + }) + + it('leaves every other unit alone', () => { + expect(elementsFor({ unit: 'words' }, 9)).toBe(9) + expect(elementsFor({ unit: 'dwords' }, 9)).toBe(9) + expect(elementsFor({ unit: 'lwords' }, 9)).toBe(9) + expect(elementsFor({ unit: 'bytes' }, 9)).toBe(9) + }) +}) + +describe('extentForDataBlock', () => { + // Asserted from BOTH directions, because the two conversions inside it pull + // opposite ways and a single-direction test passes with either one inverted. + + it('turns wire bytes into elements: 128 bytes of a word table is 64 words', () => { + expect(extentForDataBlock({ unit: 'words' }, 0, 128)).toBe(64) + }) + + it('and the other way: 64 words of a word table is 128 bytes on the wire', () => { + // The inverse, stated as the size a block must declare to reach 64 words. + expect(extentForDataBlock({ unit: 'words' }, 0, 64 * 2)).toBe(64) + expect(extentForDataBlock({ unit: 'words' }, 0, 63 * 2)).toBe(63) + }) + + it('converts each width by its own byte count', () => { + expect(extentForDataBlock({ unit: 'bytes' }, 0, 8)).toBe(8) + expect(extentForDataBlock({ unit: 'words' }, 0, 8)).toBe(4) + expect(extentForDataBlock({ unit: 'dwords' }, 0, 8)).toBe(2) + expect(extentForDataBlock({ unit: 'lwords' }, 0, 8)).toBe(1) + }) + + it('drops a partial element rather than rounding it up', () => { + // Three bytes of a word table is one addressable word. Rounding up would + // size storage for a word the block does not actually carry. + expect(extentForDataBlock({ unit: 'words' }, 0, 3)).toBe(1) + expect(extentForDataBlock({ unit: 'lwords' }, 0, 7)).toBe(0) + }) + + it('reports a BOOL block in bits while taking its start in elements', () => { + // The one table where the two units differ. A block at element 2, four + // bytes long, covers bytes 2..5 — bits 16..47 — so it needs 48 bits. + expect(extentForDataBlock({ unit: 'bits' }, 2, 4)).toBe(48) + expect(extentForDataBlock({ unit: 'bits' }, 0, 1)).toBe(8) + }) + + it('and the other way for BOOL: 48 bits is six bytes from element zero', () => { + expect(extentForDataBlock({ unit: 'bits' }, 0, 6)).toBe(48) + }) + + it('adds the start buffer, because the image is contiguous', () => { + expect(extentForDataBlock({ unit: 'words' }, 100, 8)).toBe(104) + expect(extentForDataBlock({ unit: 'words' }, 0, 0)).toBe(0) + }) +}) diff --git a/src/middleware/shared/utils/io-image/tables.ts b/src/middleware/shared/utils/io-image/tables.ts new file mode 100644 index 000000000..fc9d6790b --- /dev/null +++ b/src/middleware/shared/utils/io-image/tables.ts @@ -0,0 +1,155 @@ +/** + * The I/O image tables, written down once (DOPE-615). + * + * "Which tables exist, which IEC prefix each one stores, in which unit, and on + * which runtime" was stated in five places in this repository, and a sixth was + * about to be added for the S7comm sizer. Five copies of one fact is five + * chances for them to disagree, and every way they can disagree is silent: + * + * - a table in the runtime's list but not the editor's is sized to zero and + * the program loses that area; + * - a prefix paired with the wrong table sizes the wrong storage; + * - a unit written differently on the two sides is a factor of eight nobody + * sees until an address above the first eighth stops answering. + * + * So the five derive from this. Adding a table is one line here. + * + * ORDER IS PART OF THE CONTRACT and is the declaration order of + * `core/src/plc_app/image_tables.h`. `image.conf` is written in this order so + * a reader can go down the file and the header in step, the runtime's own + * contract test checks that order from the C side, and the compile cache + * depends on the bytes being stable. It is not alphabetical and should not be + * made so. + * + * The bare-metal macro order falls out of the same list: filtering to the + * entries that have one yields exactly the order `defines.h` already emitted, + * so there is no second order to maintain. + */ + +/** How a table's addresses are counted. NOT always how its storage is shaped: + * the BOOL tables are `IEC_BOOL *[N][8]`, so their storage is in bytes while + * `%QX` addresses bits. The file carries the ADDRESS's unit and each consumer + * converts if its own storage needs it. */ +export type ImageUnit = 'bits' | 'bytes' | 'words' | 'dwords' | 'lwords' + +export interface ImageTable { + /** The runtime's name for it, which is the `image.conf` key. */ + key: string + /** The IEC prefix whose addresses it stores. */ + prefix: string + unit: ImageUnit + /** The firmware macro, for the areas bare metal has. `undefined` means bare + * metal declares no buffer of that kind at all — a fact about that + * firmware, not an omission to be tidied up. Spelled out on every row + * rather than left off, so the reader sees the answer for each table + * instead of inferring it from a missing key. */ + macro: string | undefined +} + +export const IMAGE_TABLES = [ + { key: 'bool_input', prefix: '%IX', unit: 'bits', macro: 'MAX_DIGITAL_INPUT' }, + { key: 'bool_output', prefix: '%QX', unit: 'bits', macro: 'MAX_DIGITAL_OUTPUT' }, + { key: 'byte_input', prefix: '%IB', unit: 'bytes', macro: undefined }, + { key: 'byte_output', prefix: '%QB', unit: 'bytes', macro: undefined }, + { key: 'int_input', prefix: '%IW', unit: 'words', macro: 'MAX_ANALOG_INPUT' }, + { key: 'int_output', prefix: '%QW', unit: 'words', macro: 'MAX_ANALOG_OUTPUT' }, + { key: 'dint_input', prefix: '%ID', unit: 'dwords', macro: 'MAX_REAL_INPUT' }, + { key: 'dint_output', prefix: '%QD', unit: 'dwords', macro: 'MAX_REAL_OUTPUT' }, + { key: 'lint_input', prefix: '%IL', unit: 'lwords', macro: undefined }, + { key: 'lint_output', prefix: '%QL', unit: 'lwords', macro: undefined }, + { key: 'int_memory', prefix: '%MW', unit: 'words', macro: 'MAX_MEMORY_WORD' }, + { key: 'dint_memory', prefix: '%MD', unit: 'dwords', macro: 'MAX_MEMORY_DWORD' }, + { key: 'lint_memory', prefix: '%ML', unit: 'lwords', macro: 'MAX_MEMORY_LWORD' }, + { key: 'bool_memory', prefix: '%MX', unit: 'bits', macro: undefined }, +] as const satisfies readonly ImageTable[] + +/** The table names, as a literal union — what an `image.conf` key is, and + * what an S7comm data block may be mapped onto. Derived so the union cannot + * drift from the list. */ +export type ImageTableKey = (typeof IMAGE_TABLES)[number]['key'] + +/** + * The areas Runtime v4 declares tables for. + * + * Note the gap this makes visible: `byte_input` and `byte_output` exist but + * there is no `byte_memory`, so `%MB` has no storage on v4 at all. + */ +export const IMAGE_AREAS_RUNTIME_V4: ReadonlySet = new Set( + IMAGE_TABLES.map((table) => table.prefix), +) + +/** + * The areas bare metal declares buffers for — the ones with a macro. + * + * Fewer than v4's: no byte-addressed buffer and no bit-addressed memory area, + * which is why `%MX` on bare metal is reported as unsupported rather than + * silently dropped as it is today (DOPE-605). + */ +export const IMAGE_AREAS_BAREMETAL: ReadonlySet = new Set( + IMAGE_TABLES.filter((table) => table.macro).map((table) => table.prefix), +) + +/** How many elements of `table` a count in the file's unit amounts to. + * The BOOL tables are the only ones whose address unit and storage unit + * differ, and rounding UP is what keeps the slots of a partial byte + * addressable. */ +export function elementsFor(table: Pick, count: number): number { + return table.unit === 'bits' ? Math.ceil(count / 8) : count +} + +/** + * How many BYTES one element of this table occupies. + * + * The BOOL tables are one byte per element because their storage is + * `IEC_BOOL *[N][8]` — eight addressable bits packed into the byte. + */ +const BYTES_PER_ELEMENT: Record = { + bits: 1, + bytes: 1, + words: 2, + dwords: 4, + lwords: 8, +} + +/** How many ADDRESSES one element of this table carries. One, except for the + * BOOL tables, where an element is a byte and the addresses are its bits. */ +const ADDRESSES_PER_ELEMENT: Record = { + bits: 8, + bytes: 1, + words: 1, + dwords: 1, + lwords: 1, +} + +/** + * The extent an S7comm data block requires of the table it is mapped onto, + * in that table's ADDRESS unit — which is what the image is sized in. + * + * Two conversions meet here and they pull in opposite directions, which is + * why it is written down once with a test from both sides (DOPE-615, B3): + * + * - `sizeBytes` is a size on the WIRE and has to become elements. 128 bytes + * of `int_output` is 64 words, not 128. A partial element is dropped: + * three bytes of a word table is one addressable word, not one and a half. + * - `startBuffer` is already an ELEMENT index into that table, and the + * result has to come back out in addresses. For the BOOL tables those + * differ by eight: a block at element 2 of `bool_output`, four bytes long, + * reaches bit 47 and so needs 48 bits. + * + * Getting either backwards sizes an area by a factor of two, four or eight, + * with no diagnostic: the block simply stops answering partway through. + */ +export function extentForDataBlock( + table: Pick, + startBuffer: number, + sizeBytes: number, +): number { + const elements = Math.floor(sizeBytes / BYTES_PER_ELEMENT[table.unit]) + return (startBuffer + elements) * ADDRESSES_PER_ELEMENT[table.unit] +} + +/** Look a table up by the prefix it stores, or `undefined` for a prefix no + * runtime has storage for (`%MB`). */ +export function tableForPrefix(prefix: string): (typeof IMAGE_TABLES)[number] | undefined { + return IMAGE_TABLES.find((table) => table.prefix === prefix) +} From d7298a4808c4fa9fdd979b420afaa6406f0d103d Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 16:59:17 -0300 Subject: [PATCH 2/6] fix(compile): back only what a block covers, and say it in addresses Marcone's review on #1114. One blocker and three changes required. AN S7COMM BLOCK BACKED ADDRESSES IT DOES NOT COVER. `extent` is a high-water mark -- startBuffer plus elements -- which is right for sizing, because the image is contiguous. It is not the covered range, and `markBacked(..., 0, extent)` marked from zero. A block at startBuffer 100 therefore vouched for %IW0 through %IW103 while the plugin only ever writes 100..103, so `AT %IW0 : INT` compiled clean with nothing producing it and read zero forever on the machine. That is the declaration BR14 exists to refuse, and it contradicted the per-slot rule this same file applies in two other places. extentForDataBlock now returns { start, end }: `end` sizes, `start` backs. The Modbus path above may legitimately mark from zero, because its segments always start at IEC index 0 -- S7comm blocks do not, which is what startBuffer is for. THE DUPLICATE-OUTPUT SCAN NO LONGER WALKS EVERY ELEMENT. It kept a slot -> owner map, one Map entry with an object value per declared element, in the Electron main process: `AT %QW0 : ARRAY [0..10000000] OF WORD` inserted ten million of them before the platform compiler got to refuse the size. That is the same blow-up the memory branch thirty lines below carries a comment about having removed. It now keeps one range per declaration and compares with `slotRangesOverlap`, the primitive the registry already owns and had no non-test caller for. THE MESSAGE REPORTS AN ADDRESS. `slot` is linear within the prefix space, so for a bit class it counts bits and two variables at %QX3.2 were reported as "slot 26" -- leaving the user to divide by eight to get back to what they typed, in the commonest duplicate-output case there is. Both locations are now carried on the DTO and the overlap point is rendered through `formatAddress` rather than by reimplementing the bit maths. AND THE EDIT-TIME TOOLTIP STOPS EMPTYING ITSELF. The map held bare names, so a cell excluding itself by name removed the other writer too whenever both were called the same thing -- which is the common case, because the two declarations are usually in different POUs and `run` or `out` is ordinary in each. It rendered as "is also driven by ." with the amber glyph still showing. The map now holds { scope, name }, the cell excludes by both, and the tooltip names the POU, which is what the user needs in order to go and fix it. Tests for all four, since none had any: the block that backs nothing below its start and the control that does, the %QX3.2 message, a ten-million-element array that has to stay under two seconds, and six cases for the hook including two POUs whose variables share a name. 3327 tests, tsc and validate:arch clean, mirror byte-identical. Co-Authored-By: Claude Opus 5 --- .../__tests__/compute-io-image.test.ts | 72 +++++++++++ .../shared/compile/steps/compute-io-image.ts | 118 ++++++++++++------ .../variables-table/editable-cell.tsx | 9 +- .../use-duplicate-output-locations.test.ts | 99 +++++++++++++++ .../hooks/use-duplicate-output-locations.ts | 32 +++-- .../utils/io-image/__tests__/tables.test.ts | 41 +++--- .../shared/utils/io-image/tables.ts | 12 +- 7 files changed, 315 insertions(+), 68 deletions(-) create mode 100644 src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts diff --git a/src/backend/shared/compile/__tests__/compute-io-image.test.ts b/src/backend/shared/compile/__tests__/compute-io-image.test.ts index aed05daaa..328678833 100644 --- a/src/backend/shared/compile/__tests__/compute-io-image.test.ts +++ b/src/backend/shared/compile/__tests__/compute-io-image.test.ts @@ -459,6 +459,49 @@ describe('computeIoImage — two declarations on one output', () => { expect(describeDuplicateOutput(image.duplicateOutputs[0])).toContain('both in main') }) + it('reports the ADDRESS, not the linear slot number', () => { + // `slot` counts BITS for a bit class, so two variables at %QX3.2 are slot + // 26. Printing that leaves the user to divide by eight to get back to what + // they typed, in the commonest duplicate-output case there is. + const image = compute(pousWith(['motor', 'run', '%QX3.2'], ['pump', 'start', '%QX3.2'])) + const message = describeDuplicateOutput(image.duplicateOutputs[0]) + expect(message).toContain('%QX3.2') + expect(message).not.toContain('slot 26') + }) + + it('names the overlap address when two arrays clash at neither base', () => { + const image = compute( + makeProject({ + pous: [ + { name: 'a', variables: [arrayVar('first', '%QW0', 0, 9)] }, + { name: 'b', variables: [arrayVar('second', '%QW5', 0, 9)] }, + ], + }), + ) + const message = describeDuplicateOutput(image.duplicateOutputs[0]) + expect(message).toContain('%QW0') + expect(message).toContain('%QW5') + expect(message).toContain('%QW5') + }) + + it('does not walk a huge located array element by element', () => { + // The memory branch carries a comment about having removed exactly this, + // and the output branch reintroduced it — one Map entry with an OBJECT + // value per declared element, in the Electron main process, before the + // platform compiler ever gets to refuse the size. + const started = Date.now() + const image = compute( + makeProject({ + pous: [ + { name: 'a', variables: [arrayVar('first', '%QW0', 0, 10_000_000)] }, + { name: 'b', variables: [arrayVar('second', '%QW0', 0, 10_000_000)] }, + ], + }), + ) + expect(image.duplicateOutputs).toHaveLength(1) + expect(Date.now() - started).toBeLessThan(2000) + }) + it('describes the clash so either side can be the one that moves', () => { const image = compute(pousWith(['motor', 'run', '%QX0.0'], ['pump', 'start', '%QX0.0'])) const message = describeDuplicateOutput(image.duplicateOutputs[0]) @@ -543,6 +586,35 @@ describe('computeIoImage — S7comm exposure', () => { expect(image.sizes['%IW']).toBe(8) }) + it('does NOT back an address below the block start', () => { + // The case no test covered: `still refuses an input address the block does + // not reach` uses startBuffer 0 and probes ABOVE the extent, and `adds the + // start buffer` asserts only sizes. A block at word 100 produces nothing + // whatsoever at word 0, and backing from zero let `AT %IW0 : INT` compile + // clean and read zero forever on the machine. + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('v', '%IW0')] }], + servers: s7Server([block('int_input', 100, 8)]), + }), + ) + expect(image.unbacked).toHaveLength(1) + expect(image.unbacked[0].location).toBe('%IW0') + // It still SIZES to the high-water mark: the image is contiguous. + expect(image.sizes['%IW']).toBe(104) + }) + + it('backs an address the block does cover', () => { + // The control, so the refusal above is not simply "nothing is ever backed". + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('v', '%IW100')] }], + servers: s7Server([block('int_input', 100, 8)]), + }), + ) + expect(image.unbacked).toEqual([]) + }) + it('still refuses an input address the block does not reach', () => { // Backing is per slot, as everywhere else: a block covering eight words // vouches for eight, not for the ninth. diff --git a/src/backend/shared/compile/steps/compute-io-image.ts b/src/backend/shared/compile/steps/compute-io-image.ts index d9a06c88f..bf1dbd381 100644 --- a/src/backend/shared/compile/steps/compute-io-image.ts +++ b/src/backend/shared/compile/steps/compute-io-image.ts @@ -50,12 +50,15 @@ import type { DevicePin, ModbusBufferMapping, PLCServer } from '../../../../middleware/shared/ports/types' import type { PoolVppIoInput } from '../../../../middleware/shared/utils/iec-address' +import type { AddressClass, ParsedAddress } from '../../../../middleware/shared/utils/iec-address/registry' import { activeKindsFor, allocateAddresses, migrateToRegistry, + formatAddress, parseAddress, prefixOf, + slotRangesOverlap, } from '../../../../middleware/shared/utils/iec-address/registry' import type { AddressProducerCapabilities } from '../../../../middleware/shared/utils/target-capabilities' import { @@ -146,14 +149,18 @@ interface S7CommMappingLike { * An output is the one direction where two writers contradict each other. */ export interface DuplicateOutput { - location: string prefix: string - /** The slot both declarations cover, which for arrays need not be either - * declaration's base address. */ + /** The address class, carried so the message can render `slot` back into an + * address through `formatAddress` rather than reimplementing the bit maths. */ + cls: AddressClass + /** The first slot both declarations cover, which for two arrays is neither + * one's base address. Linear within the prefix space, so for a bit class it + * counts BITS — `%QX3.2` is slot 26. Render it with `formatAddress` rather + * than showing the number, which no user can map back to what they typed. */ slot: number - /** Both sides, in declaration order, so the message can name them. */ - first: { scope: string; variableName: string } - second: { scope: string; variableName: string } + /** Both sides, in declaration order, each with the address as WRITTEN. */ + first: { scope: string; variableName: string; location: string } + second: { scope: string; variableName: string; location: string } } export interface IoImage { @@ -455,10 +462,23 @@ function s7commExposure( the safe reading if that ever changes. */ if (!table) continue - const extent = extentForDataBlock(table, block.mapping.startBuffer, block.sizeBytes) - if (extent <= 0) continue - claim(sizes, table.prefix, extent) - markBacked(backed, table.prefix, 0, extent) + const { start, end } = extentForDataBlock(table, block.mapping.startBuffer, block.sizeBytes) + if (end <= start) continue + + // SIZES to the high-water mark and BACKS only what the block covers, and + // the two are not the same range. A block at startBuffer 100 makes the + // area 104 long because the image is contiguous, but it produces nothing + // whatsoever below 100 -- backing from zero would vouch for a hundred + // addresses the plugin never writes, and `AT %IW0 : INT` would compile + // clean and read zero forever on the machine. That is the declaration BR14 + // exists to refuse, and it is the same per-slot rule `producerClaims` and + // the unbacked loop already apply. + // + // The Modbus path above may legitimately mark from zero: its segments + // always start at IEC index 0. S7comm blocks do not, which is what + // startBuffer is for. + claim(sizes, table.prefix, end) + markBacked(backed, table.prefix, start, end - start) } } @@ -554,8 +574,18 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { const unbacked: UnbackedLocation[] = [] const unsupported: UnsupportedArea[] = [] const duplicateOutputs: DuplicateOutput[] = [] - /** prefix -> slot -> the first declaration that claimed it. Outputs only. */ - const outputOwners = new Map>() + /** + * Outputs already declared, per prefix, as RANGES rather than slots. + * + * One entry per declaration, not per element. The obvious version kept a + * slot -> owner map, which puts one Map entry — with an object value — per + * declared element in the Electron main process: `AT %QW0 : ARRAY + * [0..10000000] OF WORD` inserts ten million of them before the platform + * compiler ever gets to refuse the size. That is the same blow-up the memory + * branch below carries a comment about having removed, and `slotRangesOverlap` + * is the primitive the registry already owns for exactly this. + */ + const declaredOutputs = new Map>() for (const { scope, name, location, slotCount } of locatedVariables(input.projectData)) { const parsed = parseAddress(location) @@ -579,32 +609,27 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { } if (directionOf(prefix) === 'Q') { - // Every slot the declaration covers, so a located array overlapping - // another one is caught at the slot they share rather than only when - // their base addresses match. - let owners = outputOwners.get(prefix) - if (!owners) { - owners = new Map() - outputOwners.set(prefix, owners) - } - for (let slot = parsed.linear; slot < parsed.linear + slotCount; slot++) { - const owner = owners.get(slot) - if (owner) { - duplicateOutputs.push({ - location, - prefix, - slot, - first: owner, - second: { scope, variableName: name }, - }) - // One report per pair of declarations, not one per overlapping slot: - // a 4000-element array declared twice is one mistake. - break - } - } - for (let slot = parsed.linear; slot < parsed.linear + slotCount; slot++) { - if (!owners.has(slot)) owners.set(slot, { scope, variableName: name }) + // Range against range, so a located array overlapping another one is + // caught at the slot they share rather than only when their base + // addresses match — and at a cost proportional to the number of + // DECLARATIONS rather than the number of elements they cover. + const declared = declaredOutputs.get(prefix) ?? [] + const clash = declared.find((other) => slotRangesOverlap(other.at, other.slots, parsed, slotCount)) + if (clash) { + duplicateOutputs.push({ + prefix, + cls: parsed.cls, + // The first slot the two actually share, which for two arrays is + // neither one's base address. + slot: Math.max(clash.at.linear, parsed.linear), + first: { scope: clash.scope, variableName: clash.variableName, location: clash.location }, + second: { scope, variableName: name, location }, + }) } + // One entry per declaration, reported once per pair: a 4000-element + // array declared twice is one mistake, not four thousand errors. + declared.push({ at: parsed, slots: slotCount, scope, variableName: name, location }) + declaredOutputs.set(prefix, declared) } if (directionOf(prefix) === 'M') { @@ -685,12 +710,23 @@ export function describeDuplicateOutput(issue: DuplicateOutput): string { issue.first.scope === issue.second.scope ? `both in ${issue.first.scope}` : `${issue.first.scope} and ${issue.second.scope}` + + /* The ADDRESS, not the slot number. `slot` is linear within the prefix + * space, so for a bit class it counts bits and two variables at %QX3.2 would + * be reported as "slot 26" — leaving the user to divide by eight to get back + * to what they typed, in the commonest duplicate-output case there is. */ + const at = formatAddress(issue.cls, issue.slot) + const addresses = + issue.first.location === issue.second.location + ? `both at ${issue.first.location}` + : `${issue.first.location} and ${issue.second.location}, which overlap at ${at}` + return ( `Two variables drive the same output: "${issue.first.variableName}" and ` + - `"${issue.second.variableName}" (${where}) both cover slot ${issue.slot} of ` + - `${issue.prefix}. IEC located addresses are global, so the last write in the scan ` + - 'would win and which one that is depends on POU order — give one of them another ' + - 'address, or have one read the other rather than both writing.' + `"${issue.second.variableName}" (${where}), ${addresses}. IEC located addresses ` + + 'are global, so the last write in the scan would win and which one that is ' + + 'depends on POU order — give one of them another address, or have one read the ' + + 'other rather than both writing.' ) } diff --git a/src/frontend/components/_molecules/variables-table/editable-cell.tsx b/src/frontend/components/_molecules/variables-table/editable-cell.tsx index ca4562bdf..3b5811827 100644 --- a/src/frontend/components/_molecules/variables-table/editable-cell.tsx +++ b/src/frontend/components/_molecules/variables-table/editable-cell.tsx @@ -560,15 +560,20 @@ const EditableLocationCell = ({ // the variable is alias-bound, the literal address when manual. The // combobox `value` is the same string, so picking an alias option (whose // value is the alias name) or typing a literal both operate on `location`. + /* Excluded by SCOPE AND NAME together. Filtering by name alone removed the + * other writer too whenever both were called the same thing -- which is the + * common case, because the two declarations are usually in different POUs + * and `run` or `out` is an ordinary name in each. The tooltip then rendered + * with an empty list while the glyph still showed. */ const otherWriters = (duplicateOutputs.get(locationValue) ?? []).filter( - (name) => name !== variable?.name, + (writer) => !(writer.name === variable?.name && writer.scope === editor.meta.name), ) const warningTooltip = isOrphaned ? `Alias "${cellValue}" is not declared by any active I/O source — this variable is unlocated at compile time.` : locationConflict ? `Address ${cellValue} conflicts with alias "${locationConflict.aliasName}" assigned to "${locationConflict.variableName}". Two variables cannot share a location.` : isDuplicateOutput - ? `Output ${cellValue} is also driven by ${otherWriters.map((name) => `"${name}"`).join(', ')}. IEC located addresses are global, so the last write in the scan would win — the compiler refuses this.` + ? `Output ${cellValue} is also driven by ${otherWriters.map((writer) => `"${writer.name}" in ${writer.scope}`).join(', ')}. IEC located addresses are global, so the last write in the scan would win — the compiler refuses this.` : undefined // The warning glyph must stay visible whether or not the row is selected. diff --git a/src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts b/src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts new file mode 100644 index 000000000..468f546a8 --- /dev/null +++ b/src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts @@ -0,0 +1,99 @@ +/** + * The edit-time duplicate-output scan (DOPE-615, B6). + * + * Two declarations of one output address are refused at compile time; this is + * the same fact answered while the user is still typing. It carries SCOPE as + * well as name for a reason that is easy to get wrong: the two declarations + * are usually in different POUs, where the same name is entirely ordinary. A + * cell excluding itself by name alone removed the other writer too, and the + * warning rendered with an empty list while the glyph still showed. + */ + +import { renderHook } from '@testing-library/react' + +import { useOpenPLCStore } from '@root/frontend/store' +import { useDuplicateOutputLocations } from '../use-duplicate-output-locations' + +const variable = (name: string, location: string) => ({ + name, + location, + documentation: '', + type: { definition: 'base-type' as const, value: 'BOOL' as const }, +}) + +function withProject(pous: Array<{ name: string; variables: unknown[] }>, globals: unknown[] = []) { + const state = useOpenPLCStore.getState() + useOpenPLCStore.setState({ + ...state, + project: { + ...state.project, + data: { + ...state.project.data, + pous: pous.map((p) => ({ name: p.name, interface: { variables: p.variables } })), + configurations: { resource: { globalVariables: globals } }, + }, + }, + } as never) +} + +describe('useDuplicateOutputLocations', () => { + it('reports both declarations of one output, with their POUs', () => { + withProject([ + { name: 'motor', variables: [variable('run', '%QX0.0')] }, + { name: 'pump', variables: [variable('start', '%QX0.0')] }, + ]) + const { result } = renderHook(() => useDuplicateOutputLocations()) + expect(result.current.get('%QX0.0')).toEqual([ + { scope: 'motor', name: 'run' }, + { scope: 'pump', name: 'start' }, + ]) + }) + + it('keeps both when the two variables share a NAME', () => { + // The case that emptied the tooltip. `run` in two POUs is ordinary. + withProject([ + { name: 'motor', variables: [variable('run', '%QX0.0')] }, + { name: 'pump', variables: [variable('run', '%QX0.0')] }, + ]) + const { result } = renderHook(() => useDuplicateOutputLocations()) + const writers = result.current.get('%QX0.0') ?? [] + expect(writers).toHaveLength(2) + // Excluding "myself in motor" still leaves the other one. + expect(writers.filter((w) => !(w.name === 'run' && w.scope === 'motor'))).toEqual([ + { scope: 'pump', name: 'run' }, + ]) + }) + + it('names the configuration global scope', () => { + withProject([{ name: 'main', variables: [variable('local', '%QW3')] }], [variable('shared', '%QW3')]) + const { result } = renderHook(() => useDuplicateOutputLocations()) + expect(result.current.get('%QW3')).toEqual([ + { scope: 'main', name: 'local' }, + { scope: 'Global Variables', name: 'shared' }, + ]) + }) + + it('ignores inputs and memory', () => { + // Two POUs reading one input is ordinary, and sharing a memory address is + // what memory is for. + withProject([ + { name: 'a', variables: [variable('x', '%IX0.0'), variable('y', '%MW7')] }, + { name: 'b', variables: [variable('x', '%IX0.0'), variable('y', '%MW7')] }, + ]) + const { result } = renderHook(() => useDuplicateOutputLocations()) + expect(result.current.get('%IX0.0')).toBeUndefined() + expect(result.current.get('%MW7')).toBeUndefined() + }) + + it('ignores an alias, which is not a literal address', () => { + withProject([{ name: 'a', variables: [variable('x', 'MOTOR_RUN')] }]) + const { result } = renderHook(() => useDuplicateOutputLocations()) + expect(result.current.size).toBe(0) + }) + + it('reports a single declaration too, so the caller decides what is a clash', () => { + withProject([{ name: 'a', variables: [variable('x', '%QW0')] }]) + const { result } = renderHook(() => useDuplicateOutputLocations()) + expect(result.current.get('%QW0')).toHaveLength(1) + }) +}) diff --git a/src/frontend/hooks/use-duplicate-output-locations.ts b/src/frontend/hooks/use-duplicate-output-locations.ts index 25c88b249..17dc9ad81 100644 --- a/src/frontend/hooks/use-duplicate-output-locations.ts +++ b/src/frontend/hooks/use-duplicate-output-locations.ts @@ -25,8 +25,22 @@ import { useOpenPLCStore } from '@root/frontend/store' import type { PLCVariable } from '@root/middleware/shared/ports/types' import { isLiteralLocation } from '@root/middleware/shared/utils/iec-address/registry' -/** `'%QX0.0'` -> the names of every variable declaring exactly that address. */ -export type DuplicateOutputMap = ReadonlyMap +/** One declaration of an output address: which POU, and what it is called. */ +export interface OutputWriter { + /** The POU, or `'Global Variables'` for a configuration global. */ + scope: string + name: string +} + +/** `'%QX0.0'` -> every declaration of exactly that address. + * + * SCOPE AND NAME, not the name alone. The whole point of the warning is that + * the two declarations are usually in DIFFERENT POUs, where the same name is + * entirely ordinary -- `run`, `motor_on`, `out`. Keyed by name only, a cell + * excluding itself removed the other writer too and the tooltip rendered with + * an empty list while the glyph still showed. The scope is also the thing the + * user needs in order to go and fix it. */ +export type DuplicateOutputMap = ReadonlyMap interface Cache { pous: unknown @@ -50,18 +64,18 @@ export function useDuplicateOutputLocations(): DuplicateOutputMap { // nothing changed. if (cache && cache.pous === pous && cache.globals === globals) return cache.map - const map = new Map() - const collect = (variables: PLCVariable[] | undefined): void => { + const map = new Map() + const collect = (scope: string, variables: PLCVariable[] | undefined): void => { for (const variable of variables ?? []) { const location = variable.location ?? '' if (!isOutputLocation(location)) continue - const names = map.get(location) - if (names) names.push(variable.name) - else map.set(location, [variable.name]) + const writers = map.get(location) + if (writers) writers.push({ scope, name: variable.name }) + else map.set(location, [{ scope, name: variable.name }]) } } - for (const pou of pous) collect(pou.interface?.variables) - collect(globals) + for (const pou of pous) collect(pou.name, pou.interface?.variables) + collect('Global Variables', globals) cache = { pous, globals, map } return map diff --git a/src/middleware/shared/utils/io-image/__tests__/tables.test.ts b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts index 89ea01be4..269f9a1e6 100644 --- a/src/middleware/shared/utils/io-image/__tests__/tables.test.ts +++ b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts @@ -143,42 +143,55 @@ describe('extentForDataBlock', () => { // opposite ways and a single-direction test passes with either one inverted. it('turns wire bytes into elements: 128 bytes of a word table is 64 words', () => { - expect(extentForDataBlock({ unit: 'words' }, 0, 128)).toBe(64) + expect(extentForDataBlock({ unit: 'words' }, 0, 128).end).toBe(64) }) it('and the other way: 64 words of a word table is 128 bytes on the wire', () => { // The inverse, stated as the size a block must declare to reach 64 words. - expect(extentForDataBlock({ unit: 'words' }, 0, 64 * 2)).toBe(64) - expect(extentForDataBlock({ unit: 'words' }, 0, 63 * 2)).toBe(63) + expect(extentForDataBlock({ unit: 'words' }, 0, 64 * 2).end).toBe(64) + expect(extentForDataBlock({ unit: 'words' }, 0, 63 * 2).end).toBe(63) }) it('converts each width by its own byte count', () => { - expect(extentForDataBlock({ unit: 'bytes' }, 0, 8)).toBe(8) - expect(extentForDataBlock({ unit: 'words' }, 0, 8)).toBe(4) - expect(extentForDataBlock({ unit: 'dwords' }, 0, 8)).toBe(2) - expect(extentForDataBlock({ unit: 'lwords' }, 0, 8)).toBe(1) + expect(extentForDataBlock({ unit: 'bytes' }, 0, 8).end).toBe(8) + expect(extentForDataBlock({ unit: 'words' }, 0, 8).end).toBe(4) + expect(extentForDataBlock({ unit: 'dwords' }, 0, 8).end).toBe(2) + expect(extentForDataBlock({ unit: 'lwords' }, 0, 8).end).toBe(1) }) it('drops a partial element rather than rounding it up', () => { // Three bytes of a word table is one addressable word. Rounding up would // size storage for a word the block does not actually carry. - expect(extentForDataBlock({ unit: 'words' }, 0, 3)).toBe(1) - expect(extentForDataBlock({ unit: 'lwords' }, 0, 7)).toBe(0) + expect(extentForDataBlock({ unit: 'words' }, 0, 3).end).toBe(1) + expect(extentForDataBlock({ unit: 'lwords' }, 0, 7).end).toBe(0) }) it('reports a BOOL block in bits while taking its start in elements', () => { // The one table where the two units differ. A block at element 2, four // bytes long, covers bytes 2..5 — bits 16..47 — so it needs 48 bits. - expect(extentForDataBlock({ unit: 'bits' }, 2, 4)).toBe(48) - expect(extentForDataBlock({ unit: 'bits' }, 0, 1)).toBe(8) + expect(extentForDataBlock({ unit: 'bits' }, 2, 4).end).toBe(48) + expect(extentForDataBlock({ unit: 'bits' }, 0, 1).end).toBe(8) }) it('and the other way for BOOL: 48 bits is six bytes from element zero', () => { - expect(extentForDataBlock({ unit: 'bits' }, 0, 6)).toBe(48) + expect(extentForDataBlock({ unit: 'bits' }, 0, 6).end).toBe(48) + }) + + it('reports where the block STARTS, not only where it ends', () => { + // The half that used to be missing. A block at element 100 produces + // nothing below 100, and backing from zero would vouch for a hundred + // addresses the plugin never writes. + expect(extentForDataBlock({ unit: 'words' }, 100, 8)).toEqual({ start: 100, end: 104 }) + expect(extentForDataBlock({ unit: 'words' }, 0, 8)).toEqual({ start: 0, end: 4 }) + }) + + it('scales the start the same way it scales the end, for a BOOL table', () => { + // Element 2 of a bool table is bit 16, not bit 2. + expect(extentForDataBlock({ unit: 'bits' }, 2, 4)).toEqual({ start: 16, end: 48 }) }) it('adds the start buffer, because the image is contiguous', () => { - expect(extentForDataBlock({ unit: 'words' }, 100, 8)).toBe(104) - expect(extentForDataBlock({ unit: 'words' }, 0, 0)).toBe(0) + expect(extentForDataBlock({ unit: 'words' }, 100, 8).end).toBe(104) + expect(extentForDataBlock({ unit: 'words' }, 0, 0).end).toBe(0) }) }) diff --git a/src/middleware/shared/utils/io-image/tables.ts b/src/middleware/shared/utils/io-image/tables.ts index fc9d6790b..ff154f6d5 100644 --- a/src/middleware/shared/utils/io-image/tables.ts +++ b/src/middleware/shared/utils/io-image/tables.ts @@ -138,14 +138,22 @@ const ADDRESSES_PER_ELEMENT: Record = { * * Getting either backwards sizes an area by a factor of two, four or eight, * with no diagnostic: the block simply stops answering partway through. + * + * RETURNS BOTH ENDS, and the caller needs both for different things. `end` is + * the high-water mark, which is what SIZES the area, because the image is a + * contiguous buffer and a block reaching address 103 needs 104 of them. + * `start` is where the block's coverage actually begins, which is what BACKS: + * a block at `startBuffer` 100 produces nothing at all below 100, and saying + * otherwise would vouch for addresses the plugin never writes. */ export function extentForDataBlock( table: Pick, startBuffer: number, sizeBytes: number, -): number { +): { start: number; end: number } { const elements = Math.floor(sizeBytes / BYTES_PER_ELEMENT[table.unit]) - return (startBuffer + elements) * ADDRESSES_PER_ELEMENT[table.unit] + const scale = ADDRESSES_PER_ELEMENT[table.unit] + return { start: startBuffer * scale, end: (startBuffer + elements) * scale } } /** Look a table up by the prefix it stores, or `undefined` for a prefix no From 54c5d84ee849b2bfca94749a526ae8451471ded1 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 17:23:49 -0300 Subject: [PATCH 3/6] fix(compile): a disabled server backs nothing, and the module stops exporting dead code Marcone's nits on #1114, and an answer to the question he raised with them. A DISABLED S7COMM SERVER DOES NOT BACK. `serverExposure` never consulted `server.enabled`, which was harmless for Modbus because that path only backs outputs. With S7comm blocks backing inputs too, a server switched off -- one the runtime will not serve -- made `AT %IW7 : INT` compile clean with genuinely nothing producing it, which is the declaration BR14 exists to refuse. It still SIZES, deliberately: `generateS7commConfig` ships the config regardless of `enabled`, so the storage that file describes has to exist. That the emitter ignores `enabled` looks like a defect of its own and the Modbus emitter has the same shape; settling that is what would let a disabled server stop sizing as well. Until then, sizing follows the file that ships and backing follows what will actually run, which is written down where the decision is made. THE MODULE STOPS EXPORTING WHAT NOTHING USES. `elementsFor` and `tableForPrefix` had no production callers. `elementsFor` is dropped outright: it converts bits to elements in a codebase whose whole point is that the editor stopped converting, so keeping it is an invitation to reintroduce the factor of eight. `tableForPrefix` becomes `tableForKey`, which is the lookup the sizer actually performs -- it was doing it inline with `IMAGE_TABLES.find` on the key -- so the exported surface is now the surface something depends on. THE MANIFEST WARNING FIRES ONCE PER CHANNEL. `resolveModuleChannels` is a pure resolver called per slot and per render, not a one-shot load step, so a manifest with one bad channel produced an unbounded stream of identical lines -- which makes the log less useful rather than more, working against the reason the line exists. Deduped on name and prefix, with a test that calls the resolver three times and expects one line. And the cache fields are typed rather than `unknown`, matching the sibling hook. `unknown` costs nothing at runtime and gives up the one check keeping the cache correct: that the identity compared is the identity read. 4708 tests, 100% on compute-io-image.ts, tsc and validate:arch clean, mirror byte-identical. Co-Authored-By: Claude Opus 5 --- .../__tests__/compute-io-image.test.ts | 18 +++++++++++++ .../shared/compile/steps/compute-io-image.ts | 23 ++++++++++++++--- .../hooks/use-duplicate-output-locations.ts | 10 +++++--- .../__tests__/resolve-module-channels.test.ts | 12 +++++++++ .../utils/vpp/resolve-module-channels.ts | 25 +++++++++++++++---- .../utils/io-image/__tests__/tables.test.ts | 23 +++-------------- .../shared/utils/io-image/tables.ts | 15 +++-------- 7 files changed, 84 insertions(+), 42 deletions(-) diff --git a/src/backend/shared/compile/__tests__/compute-io-image.test.ts b/src/backend/shared/compile/__tests__/compute-io-image.test.ts index 328678833..4f7ccdc74 100644 --- a/src/backend/shared/compile/__tests__/compute-io-image.test.ts +++ b/src/backend/shared/compile/__tests__/compute-io-image.test.ts @@ -604,6 +604,24 @@ describe('computeIoImage — S7comm exposure', () => { expect(image.sizes['%IW']).toBe(104) }) + it('does NOT back when the server is switched off', () => { + // A server the runtime will not serve gives no address meaning. It still + // sizes, because generateS7commConfig ships the config regardless of + // `enabled`, so the storage that file describes has to exist. + const servers = [ + { + name: 's7', + protocol: 's7comm', + s7commSlaveConfig: { server: { enabled: false }, dataBlocks: [block('int_input', 0, 16)] }, + }, + ] + const image = compute( + makeProject({ pous: [{ name: 'main', variables: [variable('v', '%IW2')] }], servers }), + ) + expect(image.unbacked).toHaveLength(1) + expect(image.sizes['%IW']).toBe(8) + }) + it('backs an address the block does cover', () => { // The control, so the refusal above is not simply "nothing is ever backed". const image = compute( diff --git a/src/backend/shared/compile/steps/compute-io-image.ts b/src/backend/shared/compile/steps/compute-io-image.ts index bf1dbd381..58f2975ed 100644 --- a/src/backend/shared/compile/steps/compute-io-image.ts +++ b/src/backend/shared/compile/steps/compute-io-image.ts @@ -65,7 +65,7 @@ import { extentForDataBlock, IMAGE_AREAS_BAREMETAL, IMAGE_AREAS_RUNTIME_V4, - IMAGE_TABLES, + tableForKey, } from '../../../../middleware/shared/utils/io-image/tables' import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' @@ -452,11 +452,28 @@ function s7commExposure( .map((area) => ({ mapping: area.mapping, sizeBytes: area.sizeBytes })), ] + /* A DISABLED SERVER SIZES BUT DOES NOT BACK. + * + * Backing is the claim that something external gives the address meaning, + * and a server the runtime will not serve gives nothing meaning. With inputs + * backed (see above), skipping this check would let `AT %IW7 : INT` compile + * clean against a server that is switched off -- genuinely nothing producing + * it, which is the declaration BR14 exists to refuse. + * + * It still SIZES, deliberately, because `generateS7commConfig` ships the + * config regardless of `enabled` -- so the storage the file describes has to + * exist. That the emitter ignores `enabled` looks like a defect of its own, + * and the Modbus emitter has the same shape; settling it is what would let + * a disabled server stop sizing too. Until then, sizing follows the file + * that ships and backing follows what will actually run. + */ + const serving = config.server?.enabled !== false + for (const block of blocks) { // A system area may be enabled with no mapping yet, which publishes // nothing and sizes nothing. if (!block.mapping) continue - const table = IMAGE_TABLES.find((entry) => entry.key === block.mapping?.type) + const table = tableForKey(block.mapping.type) /* istanbul ignore next -- the schema admits only table names, so a block naming something else cannot reach here today. Sizing nothing for it is the safe reading if that ever changes. */ @@ -478,7 +495,7 @@ function s7commExposure( // always start at IEC index 0. S7comm blocks do not, which is what // startBuffer is for. claim(sizes, table.prefix, end) - markBacked(backed, table.prefix, start, end - start) + if (serving) markBacked(backed, table.prefix, start, end - start) } } diff --git a/src/frontend/hooks/use-duplicate-output-locations.ts b/src/frontend/hooks/use-duplicate-output-locations.ts index 17dc9ad81..92bb60e4c 100644 --- a/src/frontend/hooks/use-duplicate-output-locations.ts +++ b/src/frontend/hooks/use-duplicate-output-locations.ts @@ -22,7 +22,7 @@ */ import { useOpenPLCStore } from '@root/frontend/store' -import type { PLCVariable } from '@root/middleware/shared/ports/types' +import type { PLCPou, PLCVariable } from '@root/middleware/shared/ports/types' import { isLiteralLocation } from '@root/middleware/shared/utils/iec-address/registry' /** One declaration of an output address: which POU, and what it is called. */ @@ -43,8 +43,12 @@ export interface OutputWriter { export type DuplicateOutputMap = ReadonlyMap interface Cache { - pous: unknown - globals: unknown + /* Typed, not `unknown`. The sibling hook types the same fields, and the + * check the compiler gives up on with `unknown` is the only thing keeping + * this cache correct: that the identity being compared is the identity + * being read. */ + pous: PLCPou[] + globals: PLCVariable[] | undefined map: DuplicateOutputMap } diff --git a/src/frontend/utils/vpp/__tests__/resolve-module-channels.test.ts b/src/frontend/utils/vpp/__tests__/resolve-module-channels.test.ts index 2985b4542..e9da25efa 100644 --- a/src/frontend/utils/vpp/__tests__/resolve-module-channels.test.ts +++ b/src/frontend/utils/vpp/__tests__/resolve-module-channels.test.ts @@ -195,6 +195,18 @@ describe('manifest address prefixes (DOPE-615, B7)', () => { expect(isValidManifestPrefix('%ZZ')).toBe(false) }) + it('warns once per channel, not once per render', () => { + // A pure resolver called per slot and per render: an unbounded stream of + // identical lines makes the log less useful rather than more. + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}) + const channels = [{ name: 'repeat', type: 'analogInput', dataType: 'UINT', addressPrefix: '%MW' }] + withChannels(channels) + withChannels(channels) + withChannels(channels) + expect(warn).toHaveBeenCalledTimes(1) + warn.mockRestore() + }) + it('drops the offending channel and keeps the rest', () => { // One bad channel must not take down the whole device screen. const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}) diff --git a/src/frontend/utils/vpp/resolve-module-channels.ts b/src/frontend/utils/vpp/resolve-module-channels.ts index 913c91ff8..8204c691a 100644 --- a/src/frontend/utils/vpp/resolve-module-channels.ts +++ b/src/frontend/utils/vpp/resolve-module-channels.ts @@ -113,6 +113,11 @@ const MANIFEST_ADDRESS_PREFIXES: ReadonlySet = new Set([ '%QL', ]) +/** Channels already reported, so the warning does not repeat every render. + * Module-level and never cleared: a manifest does not change within a + * session, and the point is to say it once. */ +const warnedChannels = new Set() + /** Whether a manifest channel names a prefix the schema allows. */ export function isValidManifestPrefix(prefix: string): boolean { return MANIFEST_ADDRESS_PREFIXES.has(prefix) @@ -164,11 +169,21 @@ export function resolveModuleChannels( if (isValidManifestPrefix(channel.addressPrefix)) return true // Warned rather than silent: the channel disappears from the screen, and // without this line there is nothing anywhere saying why. - console.warn( - `VPP manifest: channel "${channel.name}" declares address prefix ` + - `"${channel.addressPrefix}", which is not one the manifest schema allows ` + - `(${[...MANIFEST_ADDRESS_PREFIXES].join(', ')}). The channel is ignored.`, - ) + // + // ONCE PER CHANNEL, not once per call. This is a pure resolver run per + // slot and per render, not a one-shot load step, so a manifest with one + // bad channel produced an unbounded stream of identical lines — which + // makes the log less useful rather than more, working against the reason + // the line exists at all. + const seen = `${channel.name}:${channel.addressPrefix}` + if (!warnedChannels.has(seen)) { + warnedChannels.add(seen) + console.warn( + `VPP manifest: channel "${channel.name}" declares address prefix ` + + `"${channel.addressPrefix}", which is not one the manifest schema allows ` + + `(${[...MANIFEST_ADDRESS_PREFIXES].join(', ')}). The channel is ignored.`, + ) + } return false }) } diff --git a/src/middleware/shared/utils/io-image/__tests__/tables.test.ts b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts index 269f9a1e6..5538061cc 100644 --- a/src/middleware/shared/utils/io-image/__tests__/tables.test.ts +++ b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts @@ -15,12 +15,11 @@ */ import { - elementsFor, extentForDataBlock, IMAGE_AREAS_BAREMETAL, IMAGE_AREAS_RUNTIME_V4, IMAGE_TABLES, - tableForPrefix, + tableForKey, } from '../tables' describe('IMAGE_TABLES', () => { @@ -50,14 +49,14 @@ describe('IMAGE_TABLES', () => { // Not an oversight to be tidied: the runtime declares byte_input and // byte_output but no byte_memory, so `%MB` has no storage anywhere. expect(IMAGE_TABLES.map((table) => table.key)).not.toContain('byte_memory') - expect(tableForPrefix('%MB')).toBeUndefined() + expect(tableForKey('byte_memory')).toBeUndefined() }) it('gives every table exactly one prefix, and every prefix one table', () => { const prefixes = IMAGE_TABLES.map((table) => table.prefix) expect(new Set(prefixes).size).toBe(prefixes.length) for (const table of IMAGE_TABLES) { - expect(tableForPrefix(table.prefix)).toBe(table) + expect(tableForKey(table.key)).toBe(table) } }) @@ -121,22 +120,6 @@ describe('the area sets derived from it', () => { }) }) -describe('elementsFor', () => { - it('rounds a bit count up to whole bytes', () => { - // Rounding down would make the slots of the partial byte unaddressable. - expect(elementsFor({ unit: 'bits' }, 0)).toBe(0) - expect(elementsFor({ unit: 'bits' }, 1)).toBe(1) - expect(elementsFor({ unit: 'bits' }, 8)).toBe(1) - expect(elementsFor({ unit: 'bits' }, 9)).toBe(2) - }) - - it('leaves every other unit alone', () => { - expect(elementsFor({ unit: 'words' }, 9)).toBe(9) - expect(elementsFor({ unit: 'dwords' }, 9)).toBe(9) - expect(elementsFor({ unit: 'lwords' }, 9)).toBe(9) - expect(elementsFor({ unit: 'bytes' }, 9)).toBe(9) - }) -}) describe('extentForDataBlock', () => { // Asserted from BOTH directions, because the two conversions inside it pull diff --git a/src/middleware/shared/utils/io-image/tables.ts b/src/middleware/shared/utils/io-image/tables.ts index ff154f6d5..b92f57dbc 100644 --- a/src/middleware/shared/utils/io-image/tables.ts +++ b/src/middleware/shared/utils/io-image/tables.ts @@ -89,13 +89,6 @@ export const IMAGE_AREAS_BAREMETAL: ReadonlySet = new Set( IMAGE_TABLES.filter((table) => table.macro).map((table) => table.prefix), ) -/** How many elements of `table` a count in the file's unit amounts to. - * The BOOL tables are the only ones whose address unit and storage unit - * differ, and rounding UP is what keeps the slots of a partial byte - * addressable. */ -export function elementsFor(table: Pick, count: number): number { - return table.unit === 'bits' ? Math.ceil(count / 8) : count -} /** * How many BYTES one element of this table occupies. @@ -156,8 +149,8 @@ export function extentForDataBlock( return { start: startBuffer * scale, end: (startBuffer + elements) * scale } } -/** Look a table up by the prefix it stores, or `undefined` for a prefix no - * runtime has storage for (`%MB`). */ -export function tableForPrefix(prefix: string): (typeof IMAGE_TABLES)[number] | undefined { - return IMAGE_TABLES.find((table) => table.prefix === prefix) +/** Look a table up by its name — the `image.conf` key, and what an S7comm + * data block names. `undefined` for a name no runtime has a table for. */ +export function tableForKey(key: string): (typeof IMAGE_TABLES)[number] | undefined { + return IMAGE_TABLES.find((table) => table.key === key) } From 7198d4df5b4b6133dda25ea3548654d2fee82630 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Mon, 14 Sep 2026 17:48:29 -0300 Subject: [PATCH 4/6] feat(compile): say where each sized area's number came from The build log reported how MANY areas were sized and never which contributor set any of them. That is the one question the number cannot answer on its own: producers, a Modbus slave config and an S7comm one can each size the same area and the image takes the largest, so `%QW = 1024` might be the program's own I/O or a `bufferMapping` nobody has opened in a year -- and which it is decides what the user changes. Without it the only way forward is to guess. It bites hardest on a project that already exists on disk. A new project grows its producers under the user's eye; an imported one arrives with both server configs already in it and nothing anywhere says so. `computeIoImage` now returns `origins` beside `sizes`, written by the same `claim` that writes the size, so the two cannot disagree. Four sources: `producers`, `modbus-server`, `s7comm-server`, and `declarations` -- the last one memory-only, because memory is its own producer (BR14/FR24) while an input or output declaration is checked against the image and never grows it (FR02). DETERMINISTIC, like the sizes it describes (FR07). `claim` overwrites only on a strictly larger number and the contributors run in a fixed order, so a tie always names the same one. The lines come out in IMAGE_TABLES order -- the order `image.conf` is written in and the runtime header declares -- so a reader goes down the log, the file and the header in step. Areas that came out at zero are left out: they have no number to attribute, and all fourteen every build would bury the handful that carry something. Emitted once, before the branch, so it serves bare metal and v4 alike for the same reason the sizer is called there. Gated on `sizesTheImage`: v3 and the simulator keep their firmware defaults, and a line there would describe an image neither receives. Twelve tests on the sizer and two on the pipeline. Each new invariant was confirmed to fail when broken -- tie order, table order, the origin filter, the gate. The gate test needed `%MW7` rather than an output address to mean anything: with no producer in the project an output sizes nothing, so the first version passed whether the gate was there or not. Co-Authored-By: Claude Opus 5 --- .../__tests__/compute-io-image.test.ts | 132 +++++++++++++++- .../shared/compile/__tests__/pipeline.test.ts | 54 +++++++ src/backend/shared/compile/pipeline.ts | 23 +++ .../shared/compile/steps/compute-io-image.ts | 143 +++++++++++++++--- 4 files changed, 327 insertions(+), 25 deletions(-) diff --git a/src/backend/shared/compile/__tests__/compute-io-image.test.ts b/src/backend/shared/compile/__tests__/compute-io-image.test.ts index 269ac340e..3025046bf 100644 --- a/src/backend/shared/compile/__tests__/compute-io-image.test.ts +++ b/src/backend/shared/compile/__tests__/compute-io-image.test.ts @@ -16,6 +16,7 @@ import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' import { computeIoImage, describeDuplicateOutput, + describeIoImageSizes, describeUnbackedLocation, describeUnsupportedArea, IMAGE_AREAS_BAREMETAL, @@ -130,7 +131,7 @@ const compute = (projectData: PLCProjectData, extra: Partial { it('sizes nothing for an empty project', () => { // FR21 / BR12: the floor is zero, and zero is expressed by absence. - expect(compute(makeProject({}))).toEqual({ sizes: {}, unbacked: [], unsupported: [], duplicateOutputs: [] }) + expect(compute(makeProject({}))).toEqual({ sizes: {}, origins: {}, unbacked: [], unsupported: [], duplicateOutputs: [] }) }) it('sizes an area from the pins that claim it', () => { @@ -937,7 +938,7 @@ describe('computeIoImage — BR14, an address with no producer', () => { }, ], }) - expect(compute(project)).toEqual({ sizes: {}, unbacked: [], unsupported: [], duplicateOutputs: [] }) + expect(compute(project)).toEqual({ sizes: {}, origins: {}, unbacked: [], unsupported: [], duplicateOutputs: [] }) }) }) @@ -1090,3 +1091,130 @@ describe('computeIoImage — array extents that cannot be read', () => { it('falls back to one slot for a dimension entry with no dimension', () => oneSlot({ definition: 'array', data: { dimensions: [{}] } })) }) + +describe('computeIoImage — where each number came from', () => { + const s7Block = (type: string, startBuffer: number, sizeBytes: number) => [ + { + name: 's7', + protocol: 's7comm', + s7commSlaveConfig: { + server: { enabled: true }, + dataBlocks: [{ dbNumber: 1, description: '', sizeBytes, mapping: { type, startBuffer, bitAddressing: false } }], + }, + }, + ] + + const modbusServer = (bufferMapping: unknown) => [ + { + name: 'mb', + protocol: 'modbus-tcp', + modbusSlaveConfig: { enabled: true, networkInterface: '', port: 502, bufferMapping }, + }, + ] + + it('names the producers when a pin set the number', () => { + const image = compute(makeProject({}), { devicePinMapping: pins('%IX0.0', '%IX0.1') }) + expect(image.origins).toEqual({ '%IX': 'producers' }) + }) + + it('names the Modbus server when its exposure set the number', () => { + const image = compute(makeProject({ servers: modbusServer({ holdingRegisters: { qwCount: 40 } }) })) + expect(image.origins).toEqual({ '%QW': 'modbus-server' }) + }) + + it('names the S7comm server when a data block set the number', () => { + const image = compute(makeProject({ servers: s7Block('int_output', 0, 128) })) + expect(image.origins).toEqual({ '%QW': 's7comm-server' }) + }) + + it('names the program when a memory declaration set the number', () => { + // Memory is its own producer (BR14/FR24), so unlike an input or an output + // its declaration SIZES the area — the one case where the program itself + // is the origin. + const image = compute(makeProject({ pous: [{ name: 'main', variables: [variable('m', '%MW7')] }] })) + expect(image.sizes).toEqual({ '%MW': 8 }) + expect(image.origins).toEqual({ '%MW': 'declarations' }) + }) + + it('names the LARGER claimant when two contributors size the same area', () => { + const image = compute( + makeProject({ servers: modbusServer({ holdingRegisters: { qwCount: 40 } }) }), + { devicePinMapping: pins('%QW0') }, + ) + expect(image.sizes).toEqual({ '%QW': 40 }) + expect(image.origins).toEqual({ '%QW': 'modbus-server' }) + }) + + it('leaves the EARLIER claimant named on a tie', () => { + // `claim` only overwrites on a strictly larger number and the contributors + // run in a fixed order, so equal claims always resolve the same way — the + // log has to be as deterministic as the sizes are (FR07). + const image = compute( + makeProject({ servers: modbusServer({ holdingRegisters: { qwCount: 1 } }) }), + { devicePinMapping: pins('%QW0') }, + ) + expect(image.sizes).toEqual({ '%QW': 1 }) + expect(image.origins).toEqual({ '%QW': 'producers' }) + }) + + it('drops the origin with the area when the target has no such buffer', () => { + // The two records are filtered together: an origin left behind for an area + // that was removed would be named in a log line for a size that is gone. + const image = compute(makeProject({ servers: modbusServer({ coils: { mxBits: 16 } }) }), { + areas: IMAGE_AREAS_BAREMETAL, + }) + expect(image.sizes).toEqual({}) + expect(image.origins).toEqual({}) + }) +}) + +describe('describeIoImageSizes', () => { + const modbusServer = (bufferMapping: unknown) => [ + { + name: 'mb', + protocol: 'modbus-tcp', + modbusSlaveConfig: { enabled: true, networkInterface: '', port: 502, bufferMapping }, + }, + ] + + it('says nothing about a project that sizes nothing', () => { + expect(describeIoImageSizes(compute(makeProject({})))).toEqual([]) + }) + + it('names the area, the size, the unit and the source', () => { + const image = compute(makeProject({ servers: modbusServer({ holdingRegisters: { qwCount: 40 } }) })) + expect(describeIoImageSizes(image)).toEqual(['%QW sized to 40 words from Modbus server exposure']) + }) + + it('drops the plural for a single element', () => { + const image = compute(makeProject({}), { devicePinMapping: pins('%QW0') }) + expect(describeIoImageSizes(image)).toEqual(['%QW sized to 1 word from address producers']) + }) + + it('follows the image.conf order rather than insertion order', () => { + // A reader goes down the log, the file and the runtime header in step, so + // the order here is IMAGE_TABLES'. %MW is claimed FIRST below and must + // still come last: it is the eleventh table and %IX is the first. + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('m', '%MW0')] }], + servers: modbusServer({ discreteInputs: { ixBits: 8 }, holdingRegisters: { qwCount: 2 } }), + }), + ) + expect(describeIoImageSizes(image)).toEqual([ + '%IX sized to 8 bits from Modbus server exposure', + '%QW sized to 2 words from Modbus server exposure', + '%MW sized to 1 word from memory declarations in the program', + ]) + }) + + it('is stable across two runs of the same project', () => { + // FR07 reaches the log too: the same project must produce the same lines, + // or a user comparing two builds sees a difference that is not one. + const project = makeProject({ + pous: [{ name: 'main', variables: [variable('m', '%MW3')] }], + servers: modbusServer({ coils: { qxBits: 24 } }), + }) + expect(describeIoImageSizes(compute(project))).toEqual(describeIoImageSizes(compute(project))) + }) +}) diff --git a/src/backend/shared/compile/__tests__/pipeline.test.ts b/src/backend/shared/compile/__tests__/pipeline.test.ts index 89da72866..fcf4a70e4 100644 --- a/src/backend/shared/compile/__tests__/pipeline.test.ts +++ b/src/backend/shared/compile/__tests__/pipeline.test.ts @@ -510,6 +510,60 @@ describe('runCompilePipeline — I/O image gate', () => { expect(defines).toContain('#define MAX_MEMORY_WORD 0') }) + it('says in the build log where each sized area came from', async () => { + // The size on its own is untraceable: three contributors can size an area + // and the image takes the largest, so %QW = 4 might be the program's + // producers or a Modbus config nobody has opened in a year. Naming the + // source is what stops the user guessing which one to change. + const port = makePort() + const { events, emit } = captureEvents() + + await runCompilePipeline( + arduinoArgs({ + projectData: withPou('%QW3'), + devicePinMapping: [ + { pin: '3', pinType: 'analogOutput', address: '%QW3' }, + { pin: '2', pinType: 'digitalInput', address: '%IX0.0' }, + ] as DevicePin[], + }), + port, + emit, + ) + + const lines = events.filter((e) => e.message.includes('sized to')).map((e) => e.message) + expect(lines).toEqual([ + '%IX sized to 1 bit from address producers', + '%QW sized to 4 words from address producers', + ]) + }) + + it('says nothing about sizes for a target that keeps its firmware defaults', async () => { + // v3 and the simulator are not sized, so a line here would describe an + // image neither of them receives. + // + // %MW7 and not an output address: memory is its own producer, so it sizes + // an area with no pins and no server in the project. An output would size + // nothing here whatever the gate did, and the test would pass without + // exercising it — which is exactly what it did before this comment. + const port = makePort() + const { events, emit } = captureEvents() + + await runCompilePipeline( + makeArgs({ + projectData: withPou('%MW7'), + isSimulator: false, + isRuntimeV3: true, + boardRuntime: 'openplc-compiler', + boardTarget: 'OpenPLC Runtime v3', + compileOnly: true, + }), + port, + emit, + ) + + expect(events.filter((e) => e.message.includes('sized to'))).toEqual([]) + }) + it('leaves the simulator defines.h without a process image block', async () => { // It keeps openplc.h's own fallbacks, so its defines.h is byte-identical // to what it was before any of this existed. diff --git a/src/backend/shared/compile/pipeline.ts b/src/backend/shared/compile/pipeline.ts index 751848e97..412016be8 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -55,6 +55,7 @@ import { computeIoImage, describeDuplicateOutput, describeUnbackedLocation, + describeIoImageSizes, describeUnsupportedArea, IMAGE_AREAS_BAREMETAL, IMAGE_AREAS_RUNTIME_V4, @@ -480,6 +481,28 @@ async function runCompilePipelineInner( return bailError(emit, 'validate', 'Compilation aborted: every located variable needs an address that exists.') } + // WHERE EACH NUMBER CAME FROM, not just what it is. + // + // Both emitters are downstream of here — the `#define` block for bare metal + // and `image.conf` for v4 — so this is the one place that serves both, the + // same reason the sizer itself is called here rather than in each branch. + // + // The size alone is untraceable. Three contributors can size an area and the + // image takes the largest, so `%QW = 1024` might be the program's producers + // or might be a Modbus slave config nobody has opened in a year, and which + // one it is decides what the user changes. That is sharpest for a project + // that came from somewhere else: it arrives with a `bufferMapping` and an + // `s7commSlaveConfig` already in it, and nothing in the editor says so. + // + // Info level and only for the targets that actually size: v3 and the + // simulator keep their firmware defaults, so a line here would describe an + // image neither of them receives. + if (sizesTheImage) { + for (const line of describeIoImageSizes(ioImage)) { + emit({ stage: 'validate', message: line, level: 'info' }) + } + } + // --------------------------------------------------------------------- // Step 0: Use the already-preprocessed project data. // diff --git a/src/backend/shared/compile/steps/compute-io-image.ts b/src/backend/shared/compile/steps/compute-io-image.ts index 547a52af1..9c4fedd22 100644 --- a/src/backend/shared/compile/steps/compute-io-image.ts +++ b/src/backend/shared/compile/steps/compute-io-image.ts @@ -68,6 +68,7 @@ import { extentForDataBlock, IMAGE_AREAS_BAREMETAL, IMAGE_AREAS_RUNTIME_V4, + IMAGE_TABLES, tableForKey, } from '../../../../middleware/shared/utils/io-image/tables' import { parseDimensionRange } from '../../../../frontend/utils/PLC/dimension-range' @@ -95,6 +96,29 @@ import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' */ export type IoImageSizes = Readonly> +/** + * WHICH contributor set an area's number. + * + * Recorded because the number alone cannot be traced back. A project on disk + * carries producers, a Modbus slave config and an S7comm one all at once, and + * the image takes the largest of them per area — so "why is `%QW` 1024 here + * and 8 in the other project?" has three possible answers and today's build + * log gives none of them. Guessing is the failure mode this removes: the log + * says where each number came from, so the user changes the right thing. + * + * `'declarations'` is a MEMORY-ONLY origin, and the asymmetry is the rule + * itself. Memory is its own producer (BR14/FR24), so `AT %MW0 : ARRAY [0..9]` + * sizes `%MW` — without that, a program using scratch memory and no server + * would be handed zero memory words (BR15). An input or output declaration is + * checked AGAINST the image and never grows it (FR02), so it can never be the + * origin of a size. + */ +export type IoImageOrigin = 'producers' | 'modbus-server' | 's7comm-server' | 'declarations' + +/** What each sized area's number came from. A prefix absent from `sizes` is + * absent here too — zero has no origin to name. */ +export type IoImageOrigins = Readonly> + /** * A located input or output declaration with no producer at its address — * a BR14 violation, which fails the compile (FR20). @@ -169,6 +193,8 @@ export interface DuplicateOutput { export interface IoImage { sizes: IoImageSizes + /** Where each sized area's number came from. Same keys as `sizes`. */ + origins: IoImageOrigins /** Empty when every input and output declaration is backed. */ unbacked: UnbackedLocation[] /** Empty when every declaration names an area the target actually has. */ @@ -236,16 +262,35 @@ function directionOf(prefix: string): string { return prefix.charAt(1) } +/** The running sizes and, for each one, who put it there. */ +interface SizeTally { + sizes: Record + origins: Record +} + /** - * Raise `sizes[prefix]` to `slots` if it is not already at least that. + * Raise `tally.sizes[prefix]` to `slots` if it is not already at least that, + * recording `origin` when it wins. * * No guard against a non-positive `slots`: the comparison against a default * of zero already declines it, so a zero or (hand-edited) negative count * leaves the prefix absent, which is how zero is expressed here anyway. + * + * STRICTLY greater, so a TIE leaves the earlier claimant named. That is a + * choice and not an accident: contributors are applied in a fixed order + * (producers, then Modbus, then S7comm, then memory declarations), so equal + * claims always resolve the same way and the log is as deterministic as the + * sizes are (FR07). Naming one + * of several equal claimants is honest -- it says which one the number is at + * least as large as -- and naming all of them would make the common case read + * like a conflict. */ -function claim(sizes: Record, prefix: string, slots: number): void { - const current = sizes[prefix] ?? 0 - if (slots > current) sizes[prefix] = slots +function claim(tally: SizeTally, prefix: string, slots: number, origin: IoImageOrigin): void { + const current = tally.sizes[prefix] ?? 0 + if (slots > current) { + tally.sizes[prefix] = slots + tally.origins[prefix] = origin + } } /** Mark slots `[from, from + count)` of `prefix` as having a producer. */ @@ -369,9 +414,9 @@ function producerClaims(input: ComputeIoImageInput, backed: Map>, -): Record { - const sizes: Record = {} +): void { // EVERY PROTOCOL, but still the FIRST server of each one. // @@ -411,10 +456,8 @@ function serverExposure( ? list.find((server) => server.protocol === 's7comm' && server.s7commSlaveConfig) : undefined - if (modbus?.modbusSlaveConfig) modbusExposure(modbus.modbusSlaveConfig.bufferMapping, sizes, backed) - if (s7comm?.s7commSlaveConfig) s7commExposure(s7comm.s7commSlaveConfig, sizes, backed) - - return sizes + if (modbus?.modbusSlaveConfig) modbusExposure(modbus.modbusSlaveConfig.bufferMapping, tally, backed) + if (s7comm?.s7commSlaveConfig) s7commExposure(s7comm.s7commSlaveConfig, tally, backed) } /** @@ -440,7 +483,7 @@ function serverExposure( */ function modbusExposure( mapping: ModbusBufferMapping | undefined, - sizes: Record, + tally: SizeTally, backed: Map>, ): void { if (!mapping) return @@ -465,7 +508,7 @@ function modbusExposure( // segment exists and is switched off, so it exposes nothing and sizes // nothing. if (count === undefined || count <= 0) continue - claim(sizes, prefix, count) + claim(tally, prefix, count, 'modbus-server') if (WRITABLE_BY_THE_MASTER.has(prefix)) markBacked(backed, prefix, 0, count) } } @@ -497,7 +540,7 @@ function modbusExposure( */ function s7commExposure( config: NonNullable, - sizes: Record, + tally: SizeTally, backed: Map>, ): void { const blocks: Array<{ mapping?: S7CommMappingLike; sizeBytes: number }> = [ @@ -549,7 +592,7 @@ function s7commExposure( // The Modbus path above may legitimately mark from zero: its segments // always start at IEC index 0. S7comm blocks do not, which is what // startBuffer is for. - claim(sizes, table.prefix, end) + claim(tally, table.prefix, end, 's7comm-server') if (serving) markBacked(backed, table.prefix, start, end - start) } } @@ -634,20 +677,28 @@ function declaredSlotCount(variableType: PLCVariable['type'] | undefined): numbe */ export function computeIoImage(input: ComputeIoImageInput): IoImage { const backed = new Map>() - const sizes: Record = producerClaims(input, backed) - - for (const [prefix, count] of Object.entries( - serverExposure(input.projectData.servers, input.serverCapabilities, backed), - )) { - claim(sizes, prefix, count) + // ORDER IS THE TIE-BREAK, and it is fixed here rather than anywhere else: + // producers, then the servers, then the memory declarations further down. + // `claim` only overwrites on a strictly larger number, so equal claims leave + // the earlier contributor named and the log never changes between two runs + // of the same project (FR07). + const tally: SizeTally = { sizes: {}, origins: {} } + for (const [prefix, count] of Object.entries(producerClaims(input, backed))) { + claim(tally, prefix, count, 'producers') } + serverExposure(input.projectData.servers, input.serverCapabilities, tally, backed) + + const sizes = tally.sizes // A producer or a server can only have claimed an area the target has, but // project.json is a file on disk and a target switch moves the goalposts, so // never SIZE an area the runtime declares no buffer for — the emitters would // otherwise be asked for a macro or a table key that does not exist. for (const prefix of Object.keys(sizes)) { - if (!input.areas.has(prefix)) delete sizes[prefix] + if (!input.areas.has(prefix)) { + delete sizes[prefix] + delete tally.origins[prefix] + } } const unbacked: UnbackedLocation[] = [] @@ -722,7 +773,7 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { // `AT %MW0 : ARRAY [0..10000000] OF WORD` inserted ten million Set // entries in the main process before the platform compiler ever got to // refuse the size. - claim(sizes, prefix, parsed.linear + slotCount) + claim(tally, prefix, parsed.linear + slotCount, 'declarations') continue } @@ -750,7 +801,7 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { }) } - return { sizes, unbacked, unsupported, duplicateOutputs } + return { sizes, origins: tally.origins, unbacked, unsupported, duplicateOutputs } } /** @@ -823,3 +874,49 @@ export function describeUnsupportedArea(issue: UnsupportedArea, boardTarget: str 'kind, so no address in it can be read or written. Use a different address area.' ) } + +/** How each origin reads in the build log. Written out rather than derived + * from the union member, because "modbus-server" is an identifier and the + * log is read by someone who did not write it. */ +const ORIGIN_LABELS: Record = { + producers: 'address producers', + 'modbus-server': 'Modbus server exposure', + 's7comm-server': 'S7comm server exposure', + declarations: 'memory declarations in the program', +} + +/** + * The sized areas, one line each, saying WHERE the number came from. + * + * This exists because the size on its own is untraceable. Three contributors + * can size an area and the image takes the largest, so a user looking at + * `%QW = 1024` cannot tell whether the program's producers need that much or + * whether a Modbus slave config nobody has opened in a year is holding the + * floor up — and the difference decides what they change. The number alone + * makes them guess; this says it. + * + * It matters most for a project that already exists on disk. A new project + * grows its producers under the user's eye, but an imported one arrives with a + * `bufferMapping` and an `s7commSlaveConfig` already in it, and that is + * exactly when "why is this area this big?" has no answer in the editor. + * + * IMAGE_TABLES ORDER, not insertion order and not alphabetical: the same + * order `image.conf` is written in and the runtime header declares, so a + * reader can go down the log, the file and the header in step. Areas that came + * out at zero are left out — they have no number to attribute, and listing all + * fourteen every build would bury the handful that carry something. + */ +export function describeIoImageSizes(image: IoImage): string[] { + return IMAGE_TABLES.filter((table) => (image.sizes[table.prefix] ?? 0) > 0).map((table) => { + const size = image.sizes[table.prefix] + const origin = image.origins[table.prefix] + /* istanbul ignore next -- `origins` is written by the same `claim` that + writes `sizes`, so a sized area always has one. Defensive because the + two are separate records: a future contributor that sets a size without + going through `claim` must not make the log lie about its source. */ + const from = origin ? ORIGIN_LABELS[origin] : 'an unrecorded source' + // Every unit name is a plural noun, so one of anything drops the final s. + const unit = size === 1 ? table.unit.slice(0, -1) : table.unit + return `${table.prefix} sized to ${size} ${unit} from ${from}` + }) +} From 4f4a39e947839029f5c877ca431c03da1ffca664 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Tue, 15 Sep 2026 08:25:31 -0300 Subject: [PATCH 5/6] style(compile): prettier and import order across the branch `ci-format` and `ci-lint` never ran on this PR -- the workflows are gated on a `development` base and this targets the epic branch, so the only check reported is CodeRabbit, which skips. Running the gate locally found nine files failing `prettier --check` and four `simple-import-sort/imports` ERRORS, which fail CI rather than warn. Formatting and import order only; `--fix` output, nothing rewritten by hand. Behaviour is unchanged and the tests confirm it: 450 pass in the editor, 149 in the web mirror. Surfaces re-verified byte-identical after the reformat, 1117 files, zero diffs. Co-Authored-By: Claude Opus 5 --- .../__tests__/compute-io-image.test.ts | 56 +++++++++++-------- .../shared/compile/__tests__/pipeline.test.ts | 5 +- src/backend/shared/compile/pipeline.ts | 2 +- .../shared/compile/steps/compute-io-image.ts | 19 ++++--- .../shared/compile/steps/generate-defines.ts | 2 +- src/backend/shared/types/PLC/open-plc.ts | 9 +-- .../variables-table/editable-cell.tsx | 3 +- .../use-duplicate-output-locations.test.ts | 4 +- .../utils/vpp/resolve-module-channels.ts | 11 +--- .../utils/io-image/__tests__/tables.test.ts | 9 +-- .../shared/utils/io-image/tables.ts | 5 +- 11 files changed, 54 insertions(+), 71 deletions(-) diff --git a/src/backend/shared/compile/__tests__/compute-io-image.test.ts b/src/backend/shared/compile/__tests__/compute-io-image.test.ts index 3025046bf..8cf12994d 100644 --- a/src/backend/shared/compile/__tests__/compute-io-image.test.ts +++ b/src/backend/shared/compile/__tests__/compute-io-image.test.ts @@ -131,7 +131,13 @@ const compute = (projectData: PLCProjectData, extra: Partial { it('sizes nothing for an empty project', () => { // FR21 / BR12: the floor is zero, and zero is expressed by absence. - expect(compute(makeProject({}))).toEqual({ sizes: {}, origins: {}, unbacked: [], unsupported: [], duplicateOutputs: [] }) + expect(compute(makeProject({}))).toEqual({ + sizes: {}, + origins: {}, + unbacked: [], + unsupported: [], + duplicateOutputs: [], + }) }) it('sizes an area from the pins that claim it', () => { @@ -613,9 +619,7 @@ describe('computeIoImage — S7comm exposure', () => { }) it('takes the largest extent when blocks overlap a table', () => { - const image = compute( - makeProject({ servers: s7Server([block('int_output', 0, 8), block('int_output', 50, 8)]) }), - ) + const image = compute(makeProject({ servers: s7Server([block('int_output', 0, 8), block('int_output', 50, 8)]) })) expect(image.sizes).toEqual({ '%QW': 54 }) }) @@ -707,9 +711,7 @@ describe('computeIoImage — S7comm exposure', () => { s7commSlaveConfig: { server: { enabled: false }, dataBlocks: [block('int_input', 0, 16)] }, }, ] - const image = compute( - makeProject({ pous: [{ name: 'main', variables: [variable('v', '%IW2')] }], servers }), - ) + const image = compute(makeProject({ pous: [{ name: 'main', variables: [variable('v', '%IW2')] }], servers })) expect(image.unbacked).toHaveLength(1) expect(image.sizes['%IW']).toBe(8) }) @@ -743,7 +745,11 @@ describe('computeIoImage — S7comm exposure', () => { const image = compute( makeProject({ servers: s7Server([], { - paArea: { enabled: true, sizeBytes: 16, mapping: { type: 'int_output', startBuffer: 0, bitAddressing: false } }, + paArea: { + enabled: true, + sizeBytes: 16, + mapping: { type: 'int_output', startBuffer: 0, bitAddressing: false }, + }, }), }), ) @@ -762,7 +768,11 @@ describe('computeIoImage — S7comm exposure', () => { const image = compute( makeProject({ servers: s7Server([], { - paArea: { enabled: false, sizeBytes: 16, mapping: { type: 'int_output', startBuffer: 0, bitAddressing: false } }, + paArea: { + enabled: false, + sizeBytes: 16, + mapping: { type: 'int_output', startBuffer: 0, bitAddressing: false }, + }, }), }), ) @@ -770,9 +780,7 @@ describe('computeIoImage — S7comm exposure', () => { }) it('ignores an enabled system area with no mapping yet', () => { - const image = compute( - makeProject({ servers: s7Server([], { mkArea: { enabled: true, sizeBytes: 16 } }) }), - ) + const image = compute(makeProject({ servers: s7Server([], { mkArea: { enabled: true, sizeBytes: 16 } }) })) expect(image.sizes).toEqual({}) }) @@ -781,7 +789,12 @@ describe('computeIoImage — S7comm exposure', () => { { name: 'mb', protocol: 'modbus-tcp', - modbusSlaveConfig: { enabled: true, networkInterface: '', port: 502, bufferMapping: { holdingRegisters: { qwCount: 10 } } }, + modbusSlaveConfig: { + enabled: true, + networkInterface: '', + port: 502, + bufferMapping: { holdingRegisters: { qwCount: 10 } }, + }, }, ...s7Server([block('int_memory', 0, 40)]), ] @@ -1026,8 +1039,7 @@ describe('computeIoImage — an array whose lower bound is negative', () => { // editor, one word sized in the image, and eleven written into it. /** A project whose one POU declares `array`. */ - const withArray = (array: PLCVariable) => - compute(makeProject({ pous: [{ name: 'main', variables: [array] }] })) + const withArray = (array: PLCVariable) => compute(makeProject({ pous: [{ name: 'main', variables: [array] }] })) it('counts every element of ARRAY [-5..5]', () => { expect(withArray(arrayVar('v', '%MW0', -5, 5)).sizes).toEqual({ '%MW': 11 }) @@ -1137,10 +1149,9 @@ describe('computeIoImage — where each number came from', () => { }) it('names the LARGER claimant when two contributors size the same area', () => { - const image = compute( - makeProject({ servers: modbusServer({ holdingRegisters: { qwCount: 40 } }) }), - { devicePinMapping: pins('%QW0') }, - ) + const image = compute(makeProject({ servers: modbusServer({ holdingRegisters: { qwCount: 40 } }) }), { + devicePinMapping: pins('%QW0'), + }) expect(image.sizes).toEqual({ '%QW': 40 }) expect(image.origins).toEqual({ '%QW': 'modbus-server' }) }) @@ -1149,10 +1160,9 @@ describe('computeIoImage — where each number came from', () => { // `claim` only overwrites on a strictly larger number and the contributors // run in a fixed order, so equal claims always resolve the same way — the // log has to be as deterministic as the sizes are (FR07). - const image = compute( - makeProject({ servers: modbusServer({ holdingRegisters: { qwCount: 1 } }) }), - { devicePinMapping: pins('%QW0') }, - ) + const image = compute(makeProject({ servers: modbusServer({ holdingRegisters: { qwCount: 1 } }) }), { + devicePinMapping: pins('%QW0'), + }) expect(image.sizes).toEqual({ '%QW': 1 }) expect(image.origins).toEqual({ '%QW': 'producers' }) }) diff --git a/src/backend/shared/compile/__tests__/pipeline.test.ts b/src/backend/shared/compile/__tests__/pipeline.test.ts index fcf4a70e4..5dd084306 100644 --- a/src/backend/shared/compile/__tests__/pipeline.test.ts +++ b/src/backend/shared/compile/__tests__/pipeline.test.ts @@ -531,10 +531,7 @@ describe('runCompilePipeline — I/O image gate', () => { ) const lines = events.filter((e) => e.message.includes('sized to')).map((e) => e.message) - expect(lines).toEqual([ - '%IX sized to 1 bit from address producers', - '%QW sized to 4 words from address producers', - ]) + expect(lines).toEqual(['%IX sized to 1 bit from address producers', '%QW sized to 4 words from address producers']) }) it('says nothing about sizes for a target that keeps its firmware defaults', async () => { diff --git a/src/backend/shared/compile/pipeline.ts b/src/backend/shared/compile/pipeline.ts index 412016be8..34cd6da0c 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -54,8 +54,8 @@ import { buildCBlocksFromPous, composeFirmwareBundle } from './steps/compose-fir import { computeIoImage, describeDuplicateOutput, - describeUnbackedLocation, describeIoImageSizes, + describeUnbackedLocation, describeUnsupportedArea, IMAGE_AREAS_BAREMETAL, IMAGE_AREAS_RUNTIME_V4, diff --git a/src/backend/shared/compile/steps/compute-io-image.ts b/src/backend/shared/compile/steps/compute-io-image.ts index 9c4fedd22..9f7f5806a 100644 --- a/src/backend/shared/compile/steps/compute-io-image.ts +++ b/src/backend/shared/compile/steps/compute-io-image.ts @@ -48,22 +48,19 @@ * Pure function: no fs I/O, no store, no platform coupling. */ +import { parseDimensionRange } from '../../../../frontend/utils/PLC/dimension-range' import type { DevicePin, ModbusBufferMapping, PLCServer } from '../../../../middleware/shared/ports/types' import type { PoolVppIoInput } from '../../../../middleware/shared/utils/iec-address' import type { AddressClass, ParsedAddress } from '../../../../middleware/shared/utils/iec-address/registry' import { activeKindsFor, allocateAddresses, - migrateToRegistry, formatAddress, + migrateToRegistry, parseAddress, prefixOf, slotRangesOverlap, } from '../../../../middleware/shared/utils/iec-address/registry' -import type { - AddressProducerCapabilities, - ServerCapabilities, -} from '../../../../middleware/shared/utils/target-capabilities' import { extentForDataBlock, IMAGE_AREAS_BAREMETAL, @@ -71,7 +68,10 @@ import { IMAGE_TABLES, tableForKey, } from '../../../../middleware/shared/utils/io-image/tables' -import { parseDimensionRange } from '../../../../frontend/utils/PLC/dimension-range' +import type { + AddressProducerCapabilities, + ServerCapabilities, +} from '../../../../middleware/shared/utils/target-capabilities' import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' /** @@ -209,7 +209,6 @@ export interface IoImage { * pipeline keeps importing them from the step that uses them. */ export { IMAGE_AREAS_BAREMETAL, IMAGE_AREAS_RUNTIME_V4 } - export interface ComputeIoImageInput { /** Compile-ready project data — locations already resolved from aliases to * literal `%…` addresses by `getCompileReadyProjectData`. */ @@ -417,7 +416,6 @@ function serverExposure( tally: SizeTally, backed: Map>, ): void { - // EVERY PROTOCOL, but still the FIRST server of each one. // // The generalisation that was missing is across protocols: a project with an @@ -715,7 +713,10 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { * branch below carries a comment about having removed, and `slotRangesOverlap` * is the primitive the registry already owns for exactly this. */ - const declaredOutputs = new Map>() + const declaredOutputs = new Map< + string, + Array<{ at: ParsedAddress; slots: number; scope: string; variableName: string; location: string }> + >() for (const { scope, name, location, slotCount } of locatedVariables(input.projectData)) { const parsed = parseAddress(location) diff --git a/src/backend/shared/compile/steps/generate-defines.ts b/src/backend/shared/compile/steps/generate-defines.ts index be15fe7ca..caf426870 100644 --- a/src/backend/shared/compile/steps/generate-defines.ts +++ b/src/backend/shared/compile/steps/generate-defines.ts @@ -17,8 +17,8 @@ * into the in-memory file map sent to `/compile-arduino`). */ -import type { DevicePin } from '../../types/PLC/devices' import { IMAGE_TABLES } from '../../../../middleware/shared/utils/io-image/tables' +import type { DevicePin } from '../../types/PLC/devices' import type { IoImageSizes } from './compute-io-image' import { generateModbusDefines, resolveDebugBaud, resolveDebugSlave, type VppModbusScreenState } from './modbus-defines' diff --git a/src/backend/shared/types/PLC/open-plc.ts b/src/backend/shared/types/PLC/open-plc.ts index 527794097..48ee034a3 100644 --- a/src/backend/shared/types/PLC/open-plc.ts +++ b/src/backend/shared/types/PLC/open-plc.ts @@ -1,8 +1,5 @@ import { z } from 'zod' -import type { ImageTableKey } from '../../../../middleware/shared/utils/io-image/tables' -import { IMAGE_TABLES } from '../../../../middleware/shared/utils/io-image/tables' - import { zodFBDFlowSchema, zodLadderFlowSchema } from '../../../../middleware/shared/ports/flow-schemas' // One source of truth for the IEC base-type list: the canonical // schema lives in `middleware/shared/ports/plc-schemas` and is @@ -17,6 +14,8 @@ import { zodFBDFlowSchema, zodLadderFlowSchema } from '../../../../middleware/sh // drifted from the runtime uppercase one and caused projects to // fail validation on open. import { baseTypeSchema } from '../../../../middleware/shared/ports/plc-schemas' +import type { ImageTableKey } from '../../../../middleware/shared/utils/io-image/tables' +import { IMAGE_TABLES } from '../../../../middleware/shared/utils/io-image/tables' type BaseType = z.infer @@ -327,9 +326,7 @@ type ModbusSlaveConfig = z.infer * one removed and forgotten would be a block that sizes storage that is gone. * * `z.enum` needs a non-empty tuple literal, hence the cast on the spread. */ -const S7CommBufferTypeSchema = z.enum( - IMAGE_TABLES.map((table) => table.key) as [ImageTableKey, ...ImageTableKey[]], -) +const S7CommBufferTypeSchema = z.enum(IMAGE_TABLES.map((table) => table.key) as [ImageTableKey, ...ImageTableKey[]]) type S7CommBufferType = z.infer // S7Comm Server Settings Schema diff --git a/src/frontend/components/_molecules/variables-table/editable-cell.tsx b/src/frontend/components/_molecules/variables-table/editable-cell.tsx index 3b5811827..5b49cd9c8 100644 --- a/src/frontend/components/_molecules/variables-table/editable-cell.tsx +++ b/src/frontend/components/_molecules/variables-table/editable-cell.tsx @@ -511,8 +511,7 @@ const EditableLocationCell = ({ // saying so while the user is still typing is cheaper than saying it at // build time. Inputs and memory are excluded: only two WRITERS contradict. const duplicateOutputs = useDuplicateOutputLocations() - const isDuplicateOutput = - isLocationCell && (duplicateOutputs.get(locationValue)?.length ?? 0) > 1 + const isDuplicateOutput = isLocationCell && (duplicateOutputs.get(locationValue)?.length ?? 0) > 1 const hasLocationWarning = isOrphaned || isManualConflict || isDuplicateOutput // When the input is blurred, we'll call our table meta's updateData function diff --git a/src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts b/src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts index 468f546a8..f22b812a0 100644 --- a/src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts +++ b/src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts @@ -59,9 +59,7 @@ describe('useDuplicateOutputLocations', () => { const writers = result.current.get('%QX0.0') ?? [] expect(writers).toHaveLength(2) // Excluding "myself in motor" still leaves the other one. - expect(writers.filter((w) => !(w.name === 'run' && w.scope === 'motor'))).toEqual([ - { scope: 'pump', name: 'run' }, - ]) + expect(writers.filter((w) => !(w.name === 'run' && w.scope === 'motor'))).toEqual([{ scope: 'pump', name: 'run' }]) }) it('names the configuration global scope', () => { diff --git a/src/frontend/utils/vpp/resolve-module-channels.ts b/src/frontend/utils/vpp/resolve-module-channels.ts index 8204c691a..1c8bf3f2a 100644 --- a/src/frontend/utils/vpp/resolve-module-channels.ts +++ b/src/frontend/utils/vpp/resolve-module-channels.ts @@ -102,16 +102,7 @@ export type SlotFieldValue = string | number | boolean * manifest must not take down the whole device screen, and a channel that * allocates nothing is visibly missing in a way the user can report. */ -const MANIFEST_ADDRESS_PREFIXES: ReadonlySet = new Set([ - '%IX', - '%QX', - '%IW', - '%QW', - '%ID', - '%QD', - '%IL', - '%QL', -]) +const MANIFEST_ADDRESS_PREFIXES: ReadonlySet = new Set(['%IX', '%QX', '%IW', '%QW', '%ID', '%QD', '%IL', '%QL']) /** Channels already reported, so the warning does not repeat every render. * Module-level and never cleared: a manifest does not change within a diff --git a/src/middleware/shared/utils/io-image/__tests__/tables.test.ts b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts index 5538061cc..f0327b29e 100644 --- a/src/middleware/shared/utils/io-image/__tests__/tables.test.ts +++ b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts @@ -14,13 +14,7 @@ * declaration order of `core/src/plc_app/image_tables.h`. */ -import { - extentForDataBlock, - IMAGE_AREAS_BAREMETAL, - IMAGE_AREAS_RUNTIME_V4, - IMAGE_TABLES, - tableForKey, -} from '../tables' +import { extentForDataBlock, IMAGE_AREAS_BAREMETAL, IMAGE_AREAS_RUNTIME_V4, IMAGE_TABLES, tableForKey } from '../tables' describe('IMAGE_TABLES', () => { it('lists the fourteen tables in the header declaration order', () => { @@ -120,7 +114,6 @@ describe('the area sets derived from it', () => { }) }) - describe('extentForDataBlock', () => { // Asserted from BOTH directions, because the two conversions inside it pull // opposite ways and a single-direction test passes with either one inverted. diff --git a/src/middleware/shared/utils/io-image/tables.ts b/src/middleware/shared/utils/io-image/tables.ts index b92f57dbc..7ac91cfad 100644 --- a/src/middleware/shared/utils/io-image/tables.ts +++ b/src/middleware/shared/utils/io-image/tables.ts @@ -74,9 +74,7 @@ export type ImageTableKey = (typeof IMAGE_TABLES)[number]['key'] * Note the gap this makes visible: `byte_input` and `byte_output` exist but * there is no `byte_memory`, so `%MB` has no storage on v4 at all. */ -export const IMAGE_AREAS_RUNTIME_V4: ReadonlySet = new Set( - IMAGE_TABLES.map((table) => table.prefix), -) +export const IMAGE_AREAS_RUNTIME_V4: ReadonlySet = new Set(IMAGE_TABLES.map((table) => table.prefix)) /** * The areas bare metal declares buffers for — the ones with a macro. @@ -89,7 +87,6 @@ export const IMAGE_AREAS_BAREMETAL: ReadonlySet = new Set( IMAGE_TABLES.filter((table) => table.macro).map((table) => table.prefix), ) - /** * How many BYTES one element of this table occupies. * From 1a48a2d4f80f395cf9ea2ed834001a02adc95205 Mon Sep 17 00:00:00 2001 From: JulioSergioFS Date: Tue, 15 Sep 2026 09:51:55 -0300 Subject: [PATCH 6/6] fix(compile): two writers on one output warns, and no longer refuses Marcone, on the workplan: IEC 61131-3 does not forbid declaring the same located variable in two POUs, so the editor will not forbid it either. Where the standard does not restrict, neither do we -- which write survives is the programmer's call. B5 said "fail the compile naming both"; that half is dropped. B6, the amber glyph at edit time, stays exactly as it is. So `duplicateOutputs` leaves the gate that aborts the build and gets its own block at `level: warning`. The detection is untouched: the glyph needs it, and it is the same computation. The wording had to move with it, in four places that all asserted a refusal that no longer happens -- including the tooltip, which ended in "the compiler refuses this" and would have been a plain lie on screen. The compile message now states the consequence instead of issuing an instruction: the addresses are global, so the last write wins and POU order decides which, and the fix is offered conditionally ("if that is not what you meant"). ONE NEW TEST, because nothing covered this at all -- the review had already noted the existing ones only check that both names appear. It asserts the build SUCCEEDS, that exactly one warning comes out carrying both names, and that the sentence never says "refuse". Confirmed to fail on both ways this could regress: putting duplicateOutputs back in the abort condition, and putting the level back to error. Editor: prettier, eslint (0 errors), tsc, 414 tests. Web mirror: 321 tests. Surfaces byte-identical, 1117 files, zero diffs. Co-Authored-By: Claude Opus 5 --- .../shared/compile/__tests__/pipeline.test.ts | 46 +++++++++++++++++++ src/backend/shared/compile/pipeline.ts | 22 ++++++--- .../shared/compile/steps/compute-io-image.ts | 12 +++-- .../variables-table/editable-cell.tsx | 11 +++-- .../hooks/use-duplicate-output-locations.ts | 16 +++++-- 5 files changed, 89 insertions(+), 18 deletions(-) diff --git a/src/backend/shared/compile/__tests__/pipeline.test.ts b/src/backend/shared/compile/__tests__/pipeline.test.ts index 5dd084306..b19c45884 100644 --- a/src/backend/shared/compile/__tests__/pipeline.test.ts +++ b/src/backend/shared/compile/__tests__/pipeline.test.ts @@ -394,6 +394,52 @@ describe('runCompilePipeline — I/O image gate', () => { ...overrides, }) + it('WARNS about two writers on one output and still compiles', async () => { + // IEC 61131-3 does not forbid declaring one located variable in two POUs, + // so the editor does not either -- which write survives is the + // programmer's business. What the compile owes them is the fact that the + // addresses are global and POU order decides the winner, which neither + // declaration shows on its own. So: a warning, and the build carries on. + const port = makePort() + const { events, emit } = captureEvents() + + const twoWriters = { + ...projectDataFixture, + pous: [ + pouLocating('%QW3'), + { + type: 'program', + data: { + name: 'second', + language: 'st', + documentation: '', + body: { language: 'st', value: '' }, + variables: [{ name: 'pump', location: '%QW3' }], + }, + }, + ], + } as unknown as PLCProjectData + + const result = await runCompilePipeline( + arduinoArgs({ + projectData: twoWriters, + devicePinMapping: [{ pin: '3', pinType: 'analogOutput', address: '%QW3' }] as DevicePin[], + }), + port, + emit, + ) + + expect(result.success).toBe(true) + const warned = events.filter((e) => e.message.includes('drive the same output')) + expect(warned).toHaveLength(1) + expect(warned[0].level).toBe('warning') + // Both names, because either one may be the mistake. + expect(warned[0].message).toContain('valve') + expect(warned[0].message).toContain('pump') + // And it never says the compiler refuses it, because it does not. + expect(warned[0].message).not.toContain('refuse') + }) + it('bails before transpilation when an output declaration has no producer', async () => { const port = makePort() const { events, emit } = captureEvents() diff --git a/src/backend/shared/compile/pipeline.ts b/src/backend/shared/compile/pipeline.ts index 34cd6da0c..60d09bd0d 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -462,10 +462,7 @@ async function runCompilePipelineInner( areas: isRuntimeV4 ? IMAGE_AREAS_RUNTIME_V4 : IMAGE_AREAS_BAREMETAL, }) - if ( - sizesTheImage && - (ioImage.unsupported.length > 0 || ioImage.unbacked.length > 0 || ioImage.duplicateOutputs.length > 0) - ) { + if (sizesTheImage && (ioImage.unsupported.length > 0 || ioImage.unbacked.length > 0)) { // Both lists, not the first non-empty one: a project can carry each kind of // mistake, and reporting one round at a time turns a single fix into // several compile attempts. @@ -475,10 +472,23 @@ async function runCompilePipelineInner( for (const issue of ioImage.unbacked) { emit({ stage: 'validate', message: describeUnbackedLocation(issue), level: 'error' }) } + return bailError(emit, 'validate', 'Compilation aborted: every located variable needs an address that exists.') + } + + // TWO WRITERS ON ONE OUTPUT IS A WARNING, NOT A REFUSAL. + // + // IEC 61131-3 does not forbid it: a located variable may be declared in more + // than one POU, and which write survives is then the programmer's business, + // not the editor's. Where the standard does not restrict, neither do we. + // + // It is still worth saying. The addresses are global, so the last write in + // the scan wins and which one that is depends on POU order — a fact that is + // invisible in either declaration on its own. So the compile reports it and + // continues, and the amber glyph says the same thing at edit time. + if (sizesTheImage) { for (const issue of ioImage.duplicateOutputs) { - emit({ stage: 'validate', message: describeDuplicateOutput(issue), level: 'error' }) + emit({ stage: 'validate', message: describeDuplicateOutput(issue), level: 'warning' }) } - return bailError(emit, 'validate', 'Compilation aborted: every located variable needs an address that exists.') } // WHERE EACH NUMBER CAME FROM, not just what it is. diff --git a/src/backend/shared/compile/steps/compute-io-image.ts b/src/backend/shared/compile/steps/compute-io-image.ts index 9f7f5806a..679d394bc 100644 --- a/src/backend/shared/compile/steps/compute-io-image.ts +++ b/src/backend/shared/compile/steps/compute-io-image.ts @@ -835,6 +835,12 @@ export function describeUnbackedLocation(issue: UnbackedLocation): string { * Names BOTH, because either one may be the mistake and the user cannot tell * which from an address alone -- and because the two are usually in different * POUs, which is the whole reason the editor's per-list check missed it. + * + * WORDED AS A WARNING, not a refusal. IEC 61131-3 does not forbid declaring one + * located variable in two POUs, so the compile reports this and carries on: + * which write survives is the programmer's call. The sentence therefore states + * the consequence -- last write wins, and POU order decides which -- and offers + * the fix conditionally, rather than telling them to change something. */ export function describeDuplicateOutput(issue: DuplicateOutput): string { const where = @@ -855,9 +861,9 @@ export function describeDuplicateOutput(issue: DuplicateOutput): string { return ( `Two variables drive the same output: "${issue.first.variableName}" and ` + `"${issue.second.variableName}" (${where}), ${addresses}. IEC located addresses ` + - 'are global, so the last write in the scan would win and which one that is ' + - 'depends on POU order — give one of them another address, or have one read the ' + - 'other rather than both writing.' + 'are global, so the last write in the scan wins and which one that is depends ' + + 'on POU order. If that is not what you meant, give one of them another address, ' + + 'or have one read the other rather than both writing.' ) } diff --git a/src/frontend/components/_molecules/variables-table/editable-cell.tsx b/src/frontend/components/_molecules/variables-table/editable-cell.tsx index 5b49cd9c8..a83cd836c 100644 --- a/src/frontend/components/_molecules/variables-table/editable-cell.tsx +++ b/src/frontend/components/_molecules/variables-table/editable-cell.tsx @@ -507,9 +507,12 @@ const EditableLocationCell = ({ // Two literals on one OUTPUT address, which the per-list duplicate check // cannot see: it reads this POU's variables, and the other declaration is // usually in another POU or the global scope. Same glyph, same reasoning as - // the alias conflict above — the compiler refuses this (DOPE-615, B5), and - // saying so while the user is still typing is cheaper than saying it at - // build time. Inputs and memory are excluded: only two WRITERS contradict. + // the alias conflict above, and the same verdict as the compile: a WARNING, + // never a refusal. IEC 61131-3 permits it, so the editor permits it too — + // what it owes the user is the fact that the last write wins and which one + // that is depends on POU order. Saying it while they are still typing is + // cheaper than saying it at build time. Inputs and memory are excluded: + // only two WRITERS contradict. const duplicateOutputs = useDuplicateOutputLocations() const isDuplicateOutput = isLocationCell && (duplicateOutputs.get(locationValue)?.length ?? 0) > 1 const hasLocationWarning = isOrphaned || isManualConflict || isDuplicateOutput @@ -572,7 +575,7 @@ const EditableLocationCell = ({ : locationConflict ? `Address ${cellValue} conflicts with alias "${locationConflict.aliasName}" assigned to "${locationConflict.variableName}". Two variables cannot share a location.` : isDuplicateOutput - ? `Output ${cellValue} is also driven by ${otherWriters.map((writer) => `"${writer.name}" in ${writer.scope}`).join(', ')}. IEC located addresses are global, so the last write in the scan would win — the compiler refuses this.` + ? `Output ${cellValue} is also driven by ${otherWriters.map((writer) => `"${writer.name}" in ${writer.scope}`).join(', ')}. IEC located addresses are global, so the last write in the scan would win, and which one that is depends on POU order.` : undefined // The warning glyph must stay visible whether or not the row is selected. diff --git a/src/frontend/hooks/use-duplicate-output-locations.ts b/src/frontend/hooks/use-duplicate-output-locations.ts index 92bb60e4c..b4c50cda0 100644 --- a/src/frontend/hooks/use-duplicate-output-locations.ts +++ b/src/frontend/hooks/use-duplicate-output-locations.ts @@ -4,12 +4,18 @@ * * IEC located addresses are GLOBAL. The variables table's own duplicate check * reads one variable list at a time, so two POUs can each declare `AT %QX0.0` - * and both pass; the compiler refuses it (`computeIoImage`), but only once the - * user has finished and pressed build. + * and both pass, and nothing anywhere says so until the build warns about it. * - * This is the same fact answered while editing. It is the literal-against- - * literal counterpart of the alias scan `useProjectAliasBindings` already - * does, and it is project-wide for the same reason that one is. + * A WARNING AND NOT A REFUSAL, on both sides. IEC 61131-3 does not forbid the + * same located variable in two POUs, so the editor does not either: which + * write survives is the programmer's business. What the editor owes them is + * the fact that the addresses are global and the surviving write therefore + * depends on POU order, which neither declaration shows on its own. + * + * This is that fact answered while editing rather than at build time. It is + * the literal-against-literal counterpart of the alias scan + * `useProjectAliasBindings` already does, and it is project-wide for the same + * reason that one is. * * OUTPUTS ONLY, matching the compile-time rule: two POUs reading one input is * ordinary, and sharing a memory address is what memory is for. An output is