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 c32852689..8cf12994d 100644 --- a/src/backend/shared/compile/__tests__/compute-io-image.test.ts +++ b/src/backend/shared/compile/__tests__/compute-io-image.test.ts @@ -15,6 +15,8 @@ import { getArrayTotalElements } from '@root/frontend/utils/PLC/array-codegen-he import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' import { computeIoImage, + describeDuplicateOutput, + describeIoImageSizes, describeUnbackedLocation, describeUnsupportedArea, IMAGE_AREAS_BAREMETAL, @@ -129,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: {}, unbacked: [], unsupported: [] }) + expect(compute(makeProject({}))).toEqual({ + sizes: {}, + origins: {}, + unbacked: [], + unsupported: [], + duplicateOutputs: [], + }) }) it('sizes an area from the pins that claim it', () => { @@ -410,6 +418,399 @@ 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('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]) + 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('sizes and backs NOTHING when the target does not run an S7 server', () => { + // The same scoping the Modbus path gets, and for the same reason: an + // s7commSlaveConfig outlives a target change and is never removed from + // project.json, so a bare-metal board would otherwise be sized from a + // config it will never receive — and the block would VOUCH for %IW2, + // letting the declaration past the BR14 gate with no producer anywhere. + const image = compute( + makeProject({ + pous: [{ name: 'main', variables: [variable('v', '%IW2')] }], + servers: s7Server([block('int_input', 0, 16)]), + }), + { serverCapabilities: NO_SERVERS, areas: IMAGE_AREAS_BAREMETAL }, + ) + expect(image.sizes).toEqual({}) + expect(image.unbacked).toHaveLength(1) + expect(image.unbacked[0].location).toBe('%IW2') + }) + + it('scopes the two protocols independently', () => { + // One flag per protocol, not one flag for "servers": a Runtime v4 target + // that speaks Modbus but not S7 must size from the Modbus config and + // ignore the S7 one, rather than all-or-nothing on either. + const image = compute( + makeProject({ + servers: [ + { + name: 'mb', + protocol: 'modbus-tcp', + modbusSlaveConfig: { + enabled: true, + networkInterface: '', + port: 502, + bufferMapping: { holdingRegisters: { qwCount: 16 } }, + }, + }, + ...s7Server([block('int_input', 0, 16)]), + ], + }), + { serverCapabilities: { modbusTcpServer: true, opcuaServer: false, s7Server: false } }, + ) + expect(image.sizes).toEqual({ '%QW': 16 }) + }) + + 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('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('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( + 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. + 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 @@ -550,7 +951,7 @@ describe('computeIoImage — BR14, an address with no producer', () => { }, ], }) - expect(compute(project)).toEqual({ sizes: {}, unbacked: [], unsupported: [] }) + expect(compute(project)).toEqual({ sizes: {}, origins: {}, unbacked: [], unsupported: [], duplicateOutputs: [] }) }) }) @@ -638,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 }) @@ -703,3 +1103,128 @@ 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..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() @@ -510,6 +556,57 @@ 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 60d2a9672..60d09bd0d 100644 --- a/src/backend/shared/compile/pipeline.ts +++ b/src/backend/shared/compile/pipeline.ts @@ -53,6 +53,8 @@ import type { PLCProjectData } from '../types/PLC/open-plc' import { buildCBlocksFromPous, composeFirmwareBundle } from './steps/compose-firmware-bundle' import { computeIoImage, + describeDuplicateOutput, + describeIoImageSizes, describeUnbackedLocation, describeUnsupportedArea, IMAGE_AREAS_BAREMETAL, @@ -473,6 +475,44 @@ async function runCompilePipelineInner( 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: 'warning' }) + } + } + + // 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 380cf0196..679d394bc 100644 --- a/src/backend/shared/compile/steps/compute-io-image.ts +++ b/src/backend/shared/compile/steps/compute-io-image.ts @@ -48,20 +48,30 @@ * 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, + formatAddress, migrateToRegistry, parseAddress, prefixOf, + slotRangesOverlap, } from '../../../../middleware/shared/utils/iec-address/registry' +import { + extentForDataBlock, + IMAGE_AREAS_BAREMETAL, + IMAGE_AREAS_RUNTIME_V4, + IMAGE_TABLES, + tableForKey, +} from '../../../../middleware/shared/utils/io-image/tables' import type { AddressProducerCapabilities, ServerCapabilities, } from '../../../../middleware/shared/utils/target-capabilities' -import { parseDimensionRange } from '../../../../frontend/utils/PLC/dimension-range' import type { PLCProjectData, PLCVariable } from '../../types/PLC/open-plc' /** @@ -86,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). @@ -123,67 +156,58 @@ 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 { + prefix: string + /** 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, each with the address as WRITTEN. */ + first: { scope: string; variableName: string; location: string } + second: { scope: string; variableName: string; location: string } +} + 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. */ 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 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', -]) +/* 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 } export interface ComputeIoImageInput { /** Compile-ready project data — locations already resolved from aliases to @@ -237,16 +261,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. */ @@ -370,75 +413,186 @@ function producerClaims(input: ComputeIoImageInput, 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 + // 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. + // + // SCOPED TO THE TARGET, exactly as the producer claims are, and now per + // protocol because there are two of them. + // + // A server config outlives a target change: retarget a project from Runtime + // v4 to a bare-metal board and `servers` stays in project.json, hidden in + // the UI rather than removed, while `generateRuntimeConfs` -- the only route + // by which a slave config reaches a device -- runs under `isRuntimeV4` + // alone. Sizing from it anyway hands the firmware `MAX_*` macros derived + // from a config that board will never run; with the materialised Modbus + // defaults that is 8192 bits and four areas at 1024, which most MCU targets + // will not even link. + // + // The backing half is worse than the sizing half. The exposure VOUCHES for + // the addresses it covers, so `AT %QX0.0 : BOOL` would pass the BR14 gate on + // a target where nothing whatsoever produces it -- "inside the image" and + // "has a producer" both answered by a file the target never receives. + const list = servers ?? [] + const modbus = serverCapabilities.modbusTcpServer + ? list.find((server) => server.protocol === 'modbus-tcp' && server.modbusSlaveConfig) + : undefined + const s7comm = serverCapabilities.s7Server + ? list.find((server) => server.protocol === 's7comm' && server.s7commSlaveConfig) + : undefined + + if (modbus?.modbusSlaveConfig) modbusExposure(modbus.modbusSlaveConfig.bufferMapping, tally, backed) + if (s7comm?.s7commSlaveConfig) s7commExposure(s7comm.s7commSlaveConfig, tally, backed) +} + +/** + * 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, + tally: SizeTally, backed: Map>, -): Record { - const sizes: Record = {} +): 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(tally, prefix, count, 'modbus-server') + if (WRITABLE_BY_THE_MASTER.has(prefix)) markBacked(backed, prefix, 0, count) + } +} - /* SCOPED TO THE TARGET, exactly as the producer claims above are. +/** + * 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, + tally: SizeTally, + 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 })), + ] + + /* A DISABLED SERVER SIZES BUT DOES NOT BACK. * - * A server config outlives a target change: retarget a project from Runtime - * v4 to a bare-metal board and `servers` stays in project.json, hidden in - * the UI rather than removed, while `generateRuntimeConfs` -- the only route - * by which a `bufferMapping` reaches a device -- runs under `isRuntimeV4` - * alone. Sizing from it anyway hands the firmware `MAX_*` macros derived - * from a slave config that board will never run; with the materialised - * defaults that is 8192 bits and four areas at 1024, which most MCU targets - * will not even link. + * 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. * - * The backing half is worse than the sizing half. `markBacked` below makes - * the exposure VOUCH for those addresses, so `AT %QX0.0 : BOOL` would pass - * the BR14 gate on a target where nothing whatsoever produces it -- "inside - * the image" and "has a producer" both answered by a file the target never - * receives. */ - if (!serverCapabilities.modbusTcpServer) return sizes - - 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. + * 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 = 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. */ + if (!table) continue + + 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. // - // 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) - } + // 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(tally, table.prefix, end, 's7comm-server') + if (serving) markBacked(backed, table.prefix, start, end - start) } - - return sizes } /** @@ -521,24 +675,48 @@ 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[] = [] const unsupported: UnsupportedArea[] = [] + const duplicateOutputs: DuplicateOutput[] = [] + /** + * 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< + 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) @@ -561,6 +739,30 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { continue } + if (directionOf(prefix) === 'Q') { + // 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') { // Memory is its own producer (BR14), so the declaration SIZES the area // and is never reported unbacked (FR24). Without this a program using @@ -572,7 +774,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 } @@ -600,7 +802,7 @@ export function computeIoImage(input: ComputeIoImageInput): IoImage { }) } - return { sizes, unbacked, unsupported } + return { sizes, origins: tally.origins, unbacked, unsupported, duplicateOutputs } } /** @@ -627,6 +829,44 @@ 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. + * + * 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 = + 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}), ${addresses}. IEC located addresses ` + + '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.' + ) +} + /** * One-line rendering of a declaration in an area the target does not have. * @@ -641,3 +881,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}` + }) +} diff --git a/src/backend/shared/compile/steps/generate-defines.ts b/src/backend/shared/compile/steps/generate-defines.ts index 92569ea67..caf426870 100644 --- a/src/backend/shared/compile/steps/generate-defines.ts +++ b/src/backend/shared/compile/steps/generate-defines.ts @@ -17,6 +17,7 @@ * into the in-memory file map sent to `/compile-arduino`). */ +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' @@ -50,17 +51,13 @@ export type { VppModbusScreenState } from './modbus-defines' * prevent: a comment claiming a unit difference where only a padding * difference exists reads as an instruction to convert. */ -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 cfaf6befd..bc8b28cdd 100644 --- a/src/backend/shared/compile/steps/generate-image-conf.ts +++ b/src/backend/shared/compile/steps/generate-image-conf.ts @@ -52,6 +52,7 @@ * string into the upload bundle. */ +import { IMAGE_TABLES } from '../../../../middleware/shared/utils/io-image/tables' import type { IoImageSizes } from './compute-io-image' /** @@ -63,22 +64,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 @@ -116,7 +101,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..48ee034a3 100644 --- a/src/backend/shared/types/PLC/open-plc.ts +++ b/src/backend/shared/types/PLC/open-plc.ts @@ -14,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 @@ -318,22 +320,13 @@ 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..a83cd836c 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,18 @@ 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, 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 // When the input is blurred, we'll call our table meta's updateData function const onBlur = (value: string) => { @@ -550,11 +562,21 @@ 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( + (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.` - : undefined + : 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, and which one that is depends on POU order.` + : 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 +585,13 @@ const EditableLocationCell = ({ const warningGlyph = hasLocationWarning && warningTooltip ? ( ) : null 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..f22b812a0 --- /dev/null +++ b/src/frontend/hooks/__tests__/use-duplicate-output-locations.test.ts @@ -0,0 +1,97 @@ +/** + * 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 new file mode 100644 index 000000000..b4c50cda0 --- /dev/null +++ b/src/frontend/hooks/use-duplicate-output-locations.ts @@ -0,0 +1,92 @@ +/** + * 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, and nothing anywhere says so until the build warns about it. + * + * 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 + * 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 { 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. */ +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 { + /* 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 +} + +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 = (scope: string, variables: PLCVariable[] | undefined): void => { + for (const variable of variables ?? []) { + const location = variable.location ?? '' + if (!isOutputLocation(location)) continue + 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.name, pou.interface?.variables) + collect('Global Variables', 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..e9da25efa 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,57 @@ 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('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(() => {}) + 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..1c8bf3f2a 100644 --- a/src/frontend/utils/vpp/resolve-module-channels.ts +++ b/src/frontend/utils/vpp/resolve-module-channels.ts @@ -82,6 +82,38 @@ 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']) + +/** 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) +} + export function resolveModuleChannels( moduleDef: ResolverModuleDef | undefined, slotConfig: Record | undefined, @@ -124,5 +156,25 @@ 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. + // + // 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 new file mode 100644 index 000000000..f0327b29e --- /dev/null +++ b/src/middleware/shared/utils/io-image/__tests__/tables.test.ts @@ -0,0 +1,173 @@ +/** + * 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 { 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', () => { + // 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(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(tableForKey(table.key)).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('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).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).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).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).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).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).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).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 new file mode 100644 index 000000000..7ac91cfad --- /dev/null +++ b/src/middleware/shared/utils/io-image/tables.ts @@ -0,0 +1,153 @@ +/** + * 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 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. + * + * 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, +): { start: number; end: number } { + const elements = Math.floor(sizeBytes / BYTES_PER_ELEMENT[table.unit]) + const scale = ADDRESSES_PER_ELEMENT[table.unit] + return { start: startBuffer * scale, end: (startBuffer + elements) * scale } +} + +/** 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) +}