diff --git a/.claude/skills/audit/SKILL.md b/.claude/skills/audit/SKILL.md index 47877ed4..0cfaafdb 100644 --- a/.claude/skills/audit/SKILL.md +++ b/.claude/skills/audit/SKILL.md @@ -26,7 +26,7 @@ Split by what the code shares, not by directory size. One area per run. Read `CLAUDE.md`, `CONTRIBUTING.md` and `src/__tests__/conformance.test.ts`. -The suite already asserts nine conventions. **Do not report what it asserts** — +The suite already asserts thirteen conventions. **Do not report what it asserts** — it is green, so those are closed. Audit what a test cannot see. ## The eight criteria diff --git a/CHANGELOG.md b/CHANGELOG.md index fe749986..df0f987a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,60 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The transaction log now shows the address of a coil read and of a write.** + The Addr column was blank for every coil read, every discrete input read and + every write Modbux sends, so a log of coil traffic said nothing about which + address each request went to. +- **The unit ID scan no longer asks for more than a device can answer.** Its + Length field took anything up to 65535 and sent it as the quantity, which is + past what the protocol allows. Every device on the bus then refused the + request or said nothing, so what the scan told you was about the request + rather than about the device. The field now stops at 125 registers, or 2000 + bits when only bit types are selected. +- **A 64-bit server register now shows the value a master wrote.** A master + writing an INT64, UINT64 or DATETIME register sends four words, and the server + folded them through a number that carries 53 bits, so anything above that came + back rounded. Writing one more word to that register afterwards collapsed it + to the last word alone. A DATETIME at the far end of the range read back one + millisecond late for the same reason. +- **A configuration from a newer Modbux is now checked before it is loaded.** It + was the one file that went in unread: the fields were taken as they came, so a + register map a newer version writes differently was written straight into the + app. The warning said some features may not work correctly. The file is now + read against what this version understands, what matches is kept, and the + message says which parts did not come across. +- **A file that is not a configuration is now refused instead of opened.** + Picking a JSON file holding a number, a string or a list emptied the server + and reported that the configuration had been updated from an older format. A + file holding nothing at all emptied it too and then failed with a message + about a property of null. A file claiming a version that is not a number got + one of those or the newer-version warning, depending on the value. +- **Opening the wrong file no longer empties the server.** Picking a client + config, or any file Modbux cannot read as a server config, cleared the setup + that was on screen before anything had read the file, and then reported the + failure. The file is read first now, and a refused one costs nothing. +- **The server view no longer comes up blank and stays that way.** One thing main + refused while the setup was being handed over left the whole view faded out, + on that launch and on every launch after it, with no message and nothing on + screen to clear the configuration with. A refusal now costs the one server it + belonged to. +- **A server added in the split out window can be changed when that window + closes.** Its port, unit id and byte order each refused every change with no + message until Modbux was restarted. +- **A server register the encoder cannot serve is now refused by name.** A + config file could put a fixed value outside the range of its own data type, a + string wider than the register map, a register running past the last address, + a generator drawing between two numbers its data type cannot hold, or a + generator whose interval was so long it fired every millisecond instead. + Opening it left the server view blank for that launch and every launch after + it, with no message and nothing on screen to clear it with. The file is now + refused with the register and the field named, and one already saved is + dropped on the next launch with the rest of the setup kept. +- **Splitting the server into its own window no longer turns Read + Configuration off.** The toggle stayed on while the reading went back to the + address and length in the toolbar, so every configured register above the + tenth stopped being read and its value left the grid. The split out window now + leaves the client alone. - **A DATETIME write now carries the date you typed.** The write dialog took a value field whose range was stated in seconds while the register is encoded from milliseconds, so every value it accepted fell before the year 2000 and diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2a1fcfc7..0f2117ca 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -145,6 +145,21 @@ like a guard. The caller has to be in `src/renderer`, because a channel only the e2e suite drives is one the app does not use, and that is a decision to take rather than to let happen. +**Every channel that reaches main's client is placed.** `main/index.ts` +constructs one `ModbusClient` and no client channel carries an addressee, so any +window can aim one at it, and both windows load the same renderer bundle. +`CLIENT_CHANNELS` in `main/ipc.ts` is the list `createIpcHandle` refuses from a +window that is not `windows.main`, and it is written by hand. A channel added +later is a name, a spec entry and a handler, and every other rule here stays +green while it escapes that list. The signal the test reads is the handler's own +body: a listener naming `client` or `state` reaches what one window owns. +Everything else drives a server, which is addressed by uuid, or asks `app` or a +Linux helper. Three reach the client and are deliberately not refused, and the +test names them with the reason: `list_serial_ports` and `validate_serial_port` +enumerate hardware and the RTU server's COM field asks for the first of the two +from the server window, and `get_client_state` answers a value rather than +`undefined`, so a refusal would have nothing to hand back. + **Every configured path alias is imported through.** `@renderer/*` and `@shared` are the two, in the tsconfigs and in `electron.vite.config.ts` alike. An alias nobody imports through resolves whatever it points at, including a directory diff --git a/e2e/specs/01-main/05-file-io.spec.ts b/e2e/specs/01-main/05-file-io.spec.ts index 5a525fcc..0004b62c 100644 --- a/e2e/specs/01-main/05-file-io.spec.ts +++ b/e2e/specs/01-main/05-file-io.spec.ts @@ -48,6 +48,36 @@ test.describe.serial('File I/O — open, save, clear server and client configs', await mainPage.waitForTimeout(300) }) + // ─── A file the server cannot read ──────────────────────────────────── + + // `useOpen` awaited `resetServer` above its `try`, and `resetServer` reaches + // main and then clears the store. `migrateServerConfig` throws on both shapes + // a user picks by accident, a client config file and malformed JSON, and the + // `catch` only enqueues a snackbar, so the configuration on screen was gone + // before anything had read the file. + test('opening a client config into the server says so and keeps the config', async ({ + mainPage + }) => { + const fileInput = mainPage.getByTestId('server-open-file-input') + await fileInput.setInputFiles(CONFIG_FILES.clientBasic) + + await expect(mainPage.locator('.notistack-SnackbarContainer')).toContainText( + 'Failed to load config', + { timeout: 5000 } + ) + + await expect(mainPage.getByTestId('server-name-input').locator('input')).toHaveValue( + 'Basic Server' + ) + await expect(mainPage.getByTestId('section-holding_registers')).toContainText('(2)') + await expect(mainPage.getByTestId('section-input_registers')).toContainText('(1)') + }) + + test('the buttons come back after a file it could not read', async ({ mainPage }) => { + await expect(mainPage.getByTestId('server-clear-btn')).toBeEnabled() + await expect(mainPage.getByTestId('server-save-btn')).toBeEnabled() + }) + // ─── Server config clear ────────────────────────────────────────────── test('clear server config — verify all sections show (0)', async ({ mainPage }) => { @@ -138,7 +168,10 @@ test.describe.serial('File I/O — open, save, clear server and client configs', const content = await fs.readFile(savePath, 'utf-8') const config = JSON.parse(content) - expect(config.version).toBe(2) + // `CURRENT_SERVER_CONFIG_VERSION`, written out because importing `@shared` + // here pulls the barrel and with it modules that need Electron. It moved to + // 3 when a 64 bit value became a decimal string. + expect(config.version).toBe(3) expect(config.name).toBe('Main Server') expect(config.littleEndian).toBe(false) expect(config.serverRegistersPerUnit).toBeDefined() diff --git a/e2e/specs/01-main/12-scan-unitids.spec.ts b/e2e/specs/01-main/12-scan-unitids.spec.ts index 84941075..bb4b237d 100644 --- a/e2e/specs/01-main/12-scan-unitids.spec.ts +++ b/e2e/specs/01-main/12-scan-unitids.spec.ts @@ -95,6 +95,46 @@ test.describe.serial('Scan Unit IDs', () => { await expect(input).toHaveValue('500') }) + // ─── Length field ────────────────────────────────────────────────── + + // The field passed no `max`, so `UintInput`'s default of 65535 was typeable + // and `modbus-serial` wrote it into the quantity field. A device answers + // illegal-data-value or says nothing, and the answer reads as a fact about + // the bus rather than about the request. FC03 answers at most 125 registers. + test('the length field stops at what one read of a register answers', async ({ mainPage }) => { + const input = mainPage.getByTestId('scan-unitid-length-input').locator('input') + await input.click() + await input.selectText() + await input.pressSequentially('65535') + await input.blur() + + await expect(input).toHaveValue('125') + }) + + // FC01 answers 2000 bits, so selecting a bit type widens the same field. + test('and at 2000 once a bit type is selected on its own', async ({ mainPage }) => { + const modal = mainPage.locator('.MuiModal-root') + await modal.getByTestId('scan-unitid-type-coils').click() + await modal.getByTestId('scan-unitid-type-holding-registers').click() + + const input = mainPage.getByTestId('scan-unitid-length-input').locator('input') + await input.click() + await input.selectText() + await input.pressSequentially('65535') + await input.blur() + + await expect(input).toHaveValue('2000') + + // Back to the defaults the tests below read. + await modal.getByTestId('scan-unitid-type-holding-registers').click() + await modal.getByTestId('scan-unitid-type-coils').click() + await input.click() + await input.selectText() + await input.pressSequentially('2') + await input.blur() + await expect(input).toHaveValue('2') + }) + // ─── Register type toggle buttons ────────────────────────────────── test('Holding Registers is selected by default', async ({ mainPage }) => { diff --git a/e2e/specs/01-main/15-client-toolbar.spec.ts b/e2e/specs/01-main/15-client-toolbar.spec.ts index 276a7124..e637a923 100644 --- a/e2e/specs/01-main/15-client-toolbar.spec.ts +++ b/e2e/specs/01-main/15-client-toolbar.spec.ts @@ -255,6 +255,29 @@ test.describe.serial('Client toolbar — display options and utilities', () => { await expect(logPanel).not.toBeVisible() }) + // modbus-serial files `nextDataAddress` on two of its twelve transaction + // records, so FC3, FC4 and FC6 carried an address and FC1, FC2, FC5, FC15 and + // FC16 did not: the Addr cell was blank for every coil read, every discrete + // input read and every write Modbux sends. + test('the log shows the address of a coil read, which FC1 files none for', async ({ + mainPage + }) => { + await selectRegisterType(mainPage, 'Coils') + await mainPage.getByTestId('show-log-btn').click() + + await readRegisters(mainPage, '4', '2') + + const logPanel = mainPage.getByTestId('transaction-log-panel') + const addressCell = logPanel + .locator('.MuiDataGrid-row') + .first() + .locator('[data-field="address"]') + await expect(addressCell).toHaveText('4', { timeout: 5000 }) + + await mainPage.getByTestId('show-log-btn').click() + await selectRegisterType(mainPage, 'Holding Registers') + }) + // ─── Register read config toggle ────────────────────────────────── for (const regType of ['Holding Registers', 'Input Registers']) { diff --git a/e2e/specs/01-main/31-split-view-client-state.spec.ts b/e2e/specs/01-main/31-split-view-client-state.spec.ts new file mode 100644 index 00000000..60f9026e --- /dev/null +++ b/e2e/specs/01-main/31-split-view-client-state.spec.ts @@ -0,0 +1,85 @@ +import { test, expect } from '../../fixtures/electron-app' +import { + navigateToClient, + navigateToHome, + connectClient, + disconnectClient, + enableAdvancedMode, + enableReadConfiguration, + disableReadConfiguration, + cleanServerState, + clearData, + loadServerConfig, + loadClientConfig, + selectRegisterType, + splitOutServerWindow, + expectCell +} from '../../fixtures/helpers' +import { resolve } from 'path' +import { type Page } from '@playwright/test' + +const CONFIG_DIR = resolve(__dirname, '../../fixtures/config-files') +const SERVER_CONFIG = resolve(CONFIG_DIR, 'server-large-config.json') +const CLIENT_CONFIG = resolve(CONFIG_DIR, 'client-large-config.json') + +let serverPage: Page + +// Both windows load the one renderer bundle, so both evaluate +// `client.zustand`'s module scope, and `init` there called +// `setReadConfiguration(false)` with no addressee. The main window's toggle +// still read on, and the next read went out as one flat `[address, length]` +// block instead of the mapping's groups: the length field holds 10, so every +// configured register above address 9 stopped being read. Only two windows can +// see it. +test.describe.serial('Read configuration survives the split out server window', () => { + test.afterAll(async ({ electronApp }) => { + await electronApp.evaluate(({ BrowserWindow }) => { + BrowserWindow.getAllWindows() + .filter((w) => w.getTitle() === 'Server') + .forEach((w) => w.close()) + }) + }) + + test('clean server state', async ({ mainPage }) => { + await cleanServerState(mainPage) + }) + + test('load 120 holding registers into the server and the matching mapping', async ({ + mainPage + }) => { + await loadServerConfig(mainPage, SERVER_CONFIG) + await navigateToClient(mainPage) + await enableAdvancedMode(mainPage) + await selectRegisterType(mainPage, 'Holding Registers') + await loadClientConfig(mainPage, CLIENT_CONFIG) + }) + + test('connected, read configuration fills every configured register', async ({ mainPage }) => { + await connectClient(mainPage, '127.0.0.1', '502', '0') + await enableReadConfiguration(mainPage) + + await expectCell(mainPage, 0, 'word_uint16', '1') + await expectCell(mainPage, 50, 'word_uint16', '51') + }) + + test('the split out window leaves the mapping reading', async ({ electronApp, mainPage }) => { + await navigateToHome(mainPage) + serverPage = await splitOutServerWindow(electronApp, mainPage) + await expect(serverPage.getByTestId('section-holding_registers')).toBeVisible() + + // The rows the previous test read already satisfy the assertion below, and + // the read that would empty them lands after it. Clear them first, so the + // grid has to be filled again rather than left alone. + await clearData(mainPage) + await expect(mainPage.locator('.MuiDataGrid-row[data-id="50"]')).toHaveCount(0) + + await mainPage.getByTestId('read-btn').click() + + await expectCell(mainPage, 50, 'word_uint16', '51') + }) + + test('cleanup', async ({ mainPage }) => { + await disableReadConfiguration(mainPage) + await disconnectClient(mainPage) + }) +}) diff --git a/src/__tests__/conformance.test.ts b/src/__tests__/conformance.test.ts index 120894b0..0667faf8 100644 --- a/src/__tests__/conformance.test.ts +++ b/src/__tests__/conformance.test.ts @@ -698,6 +698,83 @@ describe('the lists that name a channel agree', () => { }) }) +// +// ─── Every channel that reaches main's client is placed ────────────────────── +// +// `main/index.ts` constructs one `ModbusClient`, and no client channel carries +// an addressee, so any window can aim one at it. `CLIENT_CHANNELS` in +// `main/ipc.ts` is the list `createIpcHandle` refuses from a window that is not +// `windows.main`, and it is hand written. A channel added later is three lines, +// a name, a spec entry and a handler, and every one of the rules above stays +// green while it escapes the guard, which is the defect this list was added to +// close. +// +// The signal is the handler's own body: a listener naming `client` or `state` +// reaches what one window owns. Everything else drives a server, which is +// addressed by uuid, or asks `app` or a Linux helper. + +describe('every channel that reaches main’s client is placed', () => { + /** + * The channels that reach the client and are deliberately not refused. + * + * The two serial ones enumerate hardware rather than touch the client, and + * `server.zustand.ts refreshSerialPorts` calls the first of the two for the + * RTU server's COM field. `get_client_state` answers a `ClientState` rather + * than `undefined`, so a refusal has nothing to hand back, and reading what + * main is doing changes nothing about it. + */ + const ALLOWED_FROM_EITHER_WINDOW = [ + 'list_serial_ports', + 'validate_serial_port', + 'get_client_state' + ] + + const ipc = parse(join(repoRoot, 'src/main/ipc.ts')) + + /** Every channel whose listener body names `client` or `state`. */ + const reachesTheClient = new Set() + eachNode(ipc, (node) => { + if (!ts.isCallExpression(node) || !ts.isIdentifier(node.expression)) return + if (node.expression.text !== 'ipcHandle') return + const [channel, listener] = node.arguments + if (!channel || !ts.isStringLiteral(channel) || !listener) return + if (/\b(client|state)\./.test(listener.getText(ipc))) reachesTheClient.add(channel.text) + }) + + /** The names `CLIENT_CHANNELS` holds. */ + const refused = new Set() + eachNode(ipc, (node) => { + if (!ts.isVariableDeclaration(node)) return + if (node.name.getText(ipc) !== 'CLIENT_CHANNELS') return + eachNode(node as unknown as ts.SourceFile, (child) => { + if (ts.isStringLiteral(child)) refused.add(child.text) + }) + }) + + it('finds channels to check', () => { + expect(reachesTheClient.size).toBeGreaterThan(10) + expect(refused.size).toBeGreaterThan(10) + }) + + it('refuses every one of them from the server window, or says why not', () => { + const unplaced = [...reachesTheClient] + .filter((channel) => !refused.has(channel)) + .filter((channel) => !ALLOWED_FROM_EITHER_WINDOW.includes(channel)) + .sort() + expect(unplaced).toEqual([]) + }) + + it('names no channel the handlers do not reach the client through', () => { + const stale = [...refused].filter((channel) => !reachesTheClient.has(channel)).sort() + expect(stale).toEqual([]) + }) + + it('allows only channels that are there', () => { + const gone = ALLOWED_FROM_EITHER_WINDOW.filter((channel) => !reachesTheClient.has(channel)) + expect(gone).toEqual([]) + }) +}) + // // ─── Every channel carrying an object declares a schema ────────────────────── // diff --git a/src/main/__tests__/ipc.test.ts b/src/main/__tests__/ipc.test.ts index 812bdb6c..c6beb6f2 100644 --- a/src/main/__tests__/ipc.test.ts +++ b/src/main/__tests__/ipc.test.ts @@ -36,18 +36,25 @@ interface SentMessage { message: BackendMessage } +/** Stands in for the `WebContents` that `ipcMain.handle` hands every listener. */ +const SENDER = { id: 'the window that asked' } + +/** + * `windows`, with `SENDER` standing in for the main window's contents. + * + * `isMain` is what the client channels are refused on, so a stub answering + * false for everything would refuse the fourteen of them in every test here. + */ const createWindows = (): { windows: Windows; sent: SentMessage[] } => { const sent: SentMessage[] = [] const windows = { send: (_event: string, message: BackendMessage, to: unknown = 'all') => - sent.push({ to, message }) + sent.push({ to, message }), + isMain: (contents: unknown) => contents === SENDER } as unknown as Windows return { windows, sent } } -/** Stands in for the `WebContents` that `ipcMain.handle` hands every listener. */ -const SENDER = { id: 'the window that asked' } - /** Invokes the listener that was registered for `channel`. */ const invoke = async ( channel: string, @@ -172,6 +179,81 @@ describe('createIpcHandle', () => { ipcHandle('get_privileged_port_status', vi.fn(), PortSchema) }) + // `main/index.ts:23` constructs one `ModbusClient` and none of these channels + // carries an addressee, so before this any window could aim them at it. The + // split out server window did, from `client.zustand`'s module scope. + it('refuses a client channel from a window that is not the main one', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + const serverWindow = { id: 'the split out server window' } + + ipcHandle('set_read_configuration', listener) + const returned = await invoke('set_read_configuration', false, serverWindow) + + expect(listener).not.toHaveBeenCalled() + expect(returned).toBeUndefined() + expect(sent).toHaveLength(1) + expect(sent[0]?.to).toBe(serverWindow) + expect(String(sent[0]?.message.error)).toContain('set_read_configuration') + }) + + // A payload the schema also refuses is the input the two orders answer + // differently on: schema first reports the Zod failure and says nothing about + // the window. The asker is the question worth answering, so it goes first. + it('refuses a guarded client channel before it reads the payload', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + + ipcHandle('scan_unit_ids', listener, ScanUnitIDParametersSchema) + await invoke('scan_unit_ids', 'not a payload', { id: 'the split out server window' }) + + expect(listener).not.toHaveBeenCalled() + expect(sent).toHaveLength(1) + expect(String(sent[0]?.message.error)).toContain('not the main one') + expect(String(sent[0]?.message.error)).not.toContain('Expected') + }) + + it('reads the payload of a guarded client channel from the main window', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + + ipcHandle('scan_unit_ids', listener, ScanUnitIDParametersSchema) + await invoke('scan_unit_ids', 'not a payload') + + expect(listener).not.toHaveBeenCalled() + expect(String(sent[0]?.message.error)).toContain('scan_unit_ids') + expect(String(sent[0]?.message.error)).not.toContain('not the main one') + }) + + it('takes the same channel from the main window', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + + ipcHandle('set_read_configuration', listener) + await invoke('set_read_configuration', true) + + expect(listener).toHaveBeenCalledWith({ sender: SENDER }, true) + expect(sent).toEqual([]) + }) + + // The RTU server's COM field reads the port list from the server window, so a + // rule over every channel main's client happens to own would break it. + it('takes serial discovery from either window', async () => { + const { windows, sent } = createWindows() + const ipcHandle = createIpcHandle(windows) + const listener = vi.fn() + + ipcHandle('list_serial_ports', listener) + await invoke('list_serial_ports', undefined, { id: 'the split out server window' }) + + expect(listener).toHaveBeenCalledOnce() + expect(sent).toEqual([]) + }) + it('accepts one where the answer admits undefined', () => { const { windows } = createWindows() const ipcHandle = createIpcHandle(windows) diff --git a/src/main/__tests__/windows.test.ts b/src/main/__tests__/windows.test.ts index d7b13bf7..5fc04bb7 100644 --- a/src/main/__tests__/windows.test.ts +++ b/src/main/__tests__/windows.test.ts @@ -242,6 +242,37 @@ describe('Windows', () => { }) }) + // `createIpcHandle` refuses the fourteen channels that drive main's one + // Modbus client on this answer, so a window it cannot place has to answer + // false rather than throw. + describe('isMain', () => { + it('answers for the main window and for no other', () => { + const mainWindow = createMockWindow() + const serverWindow = createMockWindow() + windows.main = mainWindow as never + windows.server = serverWindow as never + + expect(windows.isMain(mainWindow.webContents as never)).toBe(true) + expect(windows.isMain(serverWindow.webContents as never)).toBe(false) + }) + + it('answers false with no main window', () => { + const orphan = createMockWindow() + + expect(windows.isMain(orphan.webContents as never)).toBe(false) + }) + + // On macos the app outlives its windows, so a destroyed main window is a + // state the handle reaches rather than a hypothetical. + it('answers false for a destroyed main window', () => { + const mainWindow = createMockWindow() + windows.main = mainWindow as never + mainWindow.isDestroyed.mockReturnValue(true) + + expect(windows.isMain(mainWindow.webContents as never)).toBe(false) + }) + }) + describe('main getter/setter', () => { // ! Coverage-only: trivial initial state it('returns null initially', () => { diff --git a/src/main/ipc.ts b/src/main/ipc.ts index d0f51d0b..e3888c49 100644 --- a/src/main/ipc.ts +++ b/src/main/ipc.ts @@ -51,6 +51,47 @@ type PayloadSchema = ? ZodType : never +/** A channel a refusal has an answer for, which is the same rule as above. */ +type RefusableChannel = { + [C in keyof IpcHandlerMap]: undefined extends Awaited ? C : never +}[keyof IpcHandlerMap] + +/** + * The channels that drive main's one Modbus client, answered for `windows.main` + * alone. + * + * `main/index.ts` constructs one `ModbusClient`, and not one of these channels + * carries an addressee, so any window could aim them at it. Both windows load + * the same renderer bundle, so both hold the client store; the near-term cost + * was its module scope calling `set_read_configuration` and + * `stop_scanning_unit_ids` from the split out server window, which + * `client.zustand.ts` now gates on `isServerWindow`. This side is the rule + * rather than the instance: a caller added to that module later, or a client + * component mounted in the server window, reaches the same single client. + * + * `list_serial_ports` and `validate_serial_port` are not here. They enumerate + * hardware rather than touch the client, and `server.zustand.ts:467` calls the + * first of the two for the RTU server's COM field. `get_client_state` is not + * here either: it answers a `ClientState` rather than `undefined`, and reading + * what main is doing changes nothing about it. + */ +const CLIENT_CHANNELS: readonly RefusableChannel[] = [ + 'connect', + 'disconnect', + 'read', + 'start_polling', + 'stop_polling', + 'write', + 'scan_unit_ids', + 'stop_scanning_unit_ids', + 'scan_registers', + 'stop_scanning_registers', + 'update_connection_config', + 'update_register_config', + 'set_register_mapping', + 'set_read_configuration' +] + /** * Builds the `ipcHandle` used below, bound to the windows it reports through. * @@ -67,6 +108,10 @@ type PayloadSchema = * windows were listening, which in split view was the main one, so a payload * refused on a channel the server window owns reported into a window the user * was not looking at. + * + * A channel in `CLIENT_CHANNELS` is refused the same way when it comes from a + * window that is not `windows.main`, and that question is asked before the + * schema, because the payload is beside the point once the asker is wrong. */ export const createIpcHandle = (windows: Windows) => @@ -75,12 +120,24 @@ export const createIpcHandle = listener: IpcListener, schema?: PayloadSchema ): void => { - if (!schema) { - ipcMain.handle(channel, listener) - return - } + const drivesTheClient = (CLIENT_CHANNELS as readonly string[]).includes(channel) ipcMain.handle(channel, (event: IpcMainInvokeEvent, ...args: unknown[]) => { + if (drivesTheClient && !windows.isMain(event.sender)) { + windows.send( + 'backend_message', + { + message: 'The Modbus client is driven from the main window', + variant: 'error', + error: `${channel}: refused, the asking window is not the main one` + }, + event.sender + ) + return undefined + } + + if (!schema) return listener(event, ...(args as IpcHandlerMap[C]['args'])) + const result = schema.safeParse(args[0]) if (!result.success) { diff --git a/src/main/modules/__tests__/modbusClient.test.ts b/src/main/modules/__tests__/modbusClient.test.ts index 5386c24e..7d2f375b 100644 --- a/src/main/modules/__tests__/modbusClient.test.ts +++ b/src/main/modules/__tests__/modbusClient.test.ts @@ -1484,6 +1484,54 @@ describe('ModbusClient', () => { expect(tx.errorMessage).toBeUndefined() }) + // modbus-serial files `nextDataAddress` on two of its twelve transaction + // records, inside `writeFC4` and `writeFC6`, and `writeFC1` delegates to + // `writeFC2` while `writeFC3` delegates to `writeFC4`. So FC3, FC4 and FC6 + // carry one and FC1, FC2, FC5, FC15 and FC16 do not, and the Addr column + // was blank for every coil read, every discrete input read and every write. + // Every `writeFCx` puts the address at bytes 2 and 3 of the frame. + it('reads the address off the request frame when the library files none', async () => { + await connectClient() + appState.updateRegisterConfig({ type: 'coils', address: 300, length: 4 }) + mockModbusRTU.readCoils.mockImplementation(async () => { + fileTransaction({ + nextAddress: 1, + nextCode: 1, + nextLength: 6, + _timeoutFired: false, + // Unit id, function code, then the data address as a big-endian word. + request: Buffer.from([0x01, 0x01, 0x01, 0x2c, 0x00, 0x04]), + responses: [Buffer.from([0x01, 0x01, 0x01, 0x00])] + }) + return { data: [false, false, false, false], buffer: Buffer.from([0x00]) } + }) + + await client.read() + + const tx = getWindowCalls('transaction')[0]?.[1] + expect(tx.code).toBe(1) + expect(tx.address).toBe(300) + }) + + // A frame that never went out has neither, and the cell is blank rather + // than reading 0, which is an address. + it('answers no address for a request that carries neither', async () => { + await connectClient() + mockModbusRTU.readHoldingRegisters.mockImplementation(async () => { + fileTransaction({ + nextAddress: 1, + nextCode: 3, + nextLength: 10, + _timeoutFired: false + }) + return { data: new Array(10).fill(0), buffer: Buffer.alloc(20) } + }) + + await client.read() + + expect(getWindowCalls('transaction')[0]?.[1].address).toBeUndefined() + }) + it('removes the transaction it logged', async () => { await connectClient() setupHoldingRegisterReadMock([0]) diff --git a/src/main/modules/__tests__/modbusServer.test.ts b/src/main/modules/__tests__/modbusServer.test.ts index fa3470be..5621d936 100644 --- a/src/main/modules/__tests__/modbusServer.test.ts +++ b/src/main/modules/__tests__/modbusServer.test.ts @@ -183,6 +183,17 @@ const lastSerialPortOptions = (): unknown => { const createMockWindows = (): Windows => ({ send: vi.fn() }) as unknown as Windows +/** + * The words `addRegister` answered, or a failure naming the refusal. + * + * `addRegister` answers `undefined` for a register it cannot encode, and a + * helper passing that on fails at whatever reads it next with nothing to say. + */ +const addedOrThrow = (words: number[] | undefined): number[] => { + if (!words) throw new Error('addRegister refused the register') + return words +} + describe('ModbusServer', () => { let server: ModbusServer let windows: Windows @@ -190,22 +201,28 @@ describe('ModbusServer', () => { const unitId: UnitIdString = '1' const unitIdNumber = Number(unitId) - /** A register the unit hosts, which is what makes the server answer for it at all. */ + /** + * A register the unit hosts, which is what makes the server answer for it at + * all. Throws by name on a refusal, because a helper answering `undefined` + * here fails somewhere else with nothing to say. + */ const hostUnit = (): number[] => - server.addRegister({ - uuid, - unitId, - params: { - address: 100, - registerType: 'holding_registers', - dataType: 'uint16', - comment: '', - value: 1, - min: undefined, - max: undefined, - interval: undefined - } - }) + addedOrThrow( + server.addRegister({ + uuid, + unitId, + params: { + address: 100, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 1, + min: undefined, + max: undefined, + interval: undefined + } + }) + ) beforeEach(() => { vi.useFakeTimers() @@ -263,6 +280,85 @@ describe('ModbusServer', () => { ) }) + // `undefined` is what the store reads as nothing changed, so the refusal + // reuses the answer a refused payload already has. It used to throw, and + // `createIpcHandle` puts no try around a listener. + it('refuses a value its data type cannot encode and writes nothing', () => { + const answer = server.addRegister({ + uuid, + unitId, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 70000, + min: undefined, + max: undefined, + interval: undefined + } + }) + + expect(answer).toBeUndefined() + expect(windows.send).not.toHaveBeenCalledWith( + 'register_value', + expect.anything(), + expect.anything() + ) + expect(windows.send).toHaveBeenCalledWith( + 'backend_message', + expect.objectContaining({ + variant: 'error', + message: 'The uint16 register at 0 was not added: main cannot encode that value' + }), + 'serverView' + ) + }) + + // The replacement was taken apart before the encode was tried, so a + // refusal left the address zeroed and its generator gone while the store, + // reading `undefined` as nothing changed, went on drawing it. + it('leaves the register it was replacing running when it refuses', () => { + server.addRegister({ + uuid, + unitId, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: undefined, + min: 300, + max: 300, + interval: 5000 + } + }) + + const answer = server.addRegister({ + uuid, + unitId, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 70000, + min: undefined, + max: undefined, + interval: undefined + } + }) + expect(answer).toBeUndefined() + + const sent = (windows.send as ReturnType).mock.calls.length + vi.advanceTimersByTime(6000) + const stillGenerating = (windows.send as ReturnType).mock.calls + .slice(sent) + .filter((call) => call[0] === 'register_value') + expect(stillGenerating.length).toBeGreaterThan(0) + expect(stillGenerating[0]?.[1]).toMatchObject({ address: 0, value: 300 }) + }) + // The `register_value` events above go out before this answer does, so the // renderer's store has no entry to put them in and drops them. The answer // is what that store folds instead. @@ -305,8 +401,8 @@ describe('ModbusServer', () => { }) expect(answer).toHaveLength(1) - expect(answer[0]).toBeGreaterThanOrEqual(100) - expect(answer[0]).toBeLessThanOrEqual(200) + expect(answer?.[0]).toBeGreaterThanOrEqual(100) + expect(answer?.[0]).toBeLessThanOrEqual(200) }) // `none` holds its address open and writes nothing, so there is no word to @@ -1003,6 +1099,51 @@ describe('ModbusServer', () => { ) }) + // `createRegisters` threw straight out of `addRegister`, and this loop is a + // bare `for`, so one register the encoder refused took every register after + // it in the unit with it and rejected the invoke the renderer's `init` + // awaits. `RegisterParamsSchema` refuses the payload before it gets here + // now, so this is the class answering for its own input. + it('keeps the registers after one the encoder refuses', () => { + server.syncServerRegisters({ + uuid, + unitId, + registerValues: [ + { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: -1, + min: undefined, + max: undefined, + interval: undefined + }, + { + address: 5, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 7, + min: undefined, + max: undefined, + interval: undefined + } + ] + }) + + expect(windows.send).toHaveBeenCalledWith( + 'register_value', + expect.objectContaining({ address: 5, value: 7 }), + 'serverView' + ) + expect(windows.send).toHaveBeenCalledWith( + 'backend_message', + expect.objectContaining({ variant: 'error' }), + 'serverView' + ) + }) + it('disposes existing generators before syncing', () => { // Add generators for both register types server.addRegister({ @@ -2185,20 +2326,22 @@ describe('ModbusServer', () => { } const hostUnit = (id: UnitIdString, address: number, value: number): number[] => - server.addRegister({ - uuid, - unitId: id, - params: { - address, - registerType: 'holding_registers', - dataType: 'uint16', - comment: '', - value, - min: undefined, - max: undefined, - interval: undefined - } - }) + addedOrThrow( + server.addRegister({ + uuid, + unitId: id, + params: { + address, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value, + min: undefined, + max: undefined, + interval: undefined + } + }) + ) const tcpVector = async (): Promise => { await server.createServer({ uuid, port: 5020 }) diff --git a/src/main/modules/__tests__/unencodableRegister.test.ts b/src/main/modules/__tests__/unencodableRegister.test.ts new file mode 100644 index 00000000..be89f4b6 --- /dev/null +++ b/src/main/modules/__tests__/unencodableRegister.test.ts @@ -0,0 +1,55 @@ +// The three doors `AUDIT-clusters.md` names as still reaching `addRegister` +// after the schema was tightened: the add dialog, a repaired persisted store, +// and the width overflow. All three arrive on a guarded channel, so this asks +// the two schemas `main/ipc.ts` guards with whether they still let the input +// through that `createRegisters` and `createStringRegisters` throw on. +import { describe, expect, it } from 'vitest' +import { AddRegisterParamsSchema, SyncRegisterValueParamsSchema } from '@shared' + +const params = (overrides: Record): Record => ({ + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 1, + ...overrides +}) + +const addTakes = (overrides: Record): boolean => + AddRegisterParamsSchema.safeParse({ uuid: 'u', unitId: '1', params: params(overrides) }).success + +const syncTakes = (overrides: Record): boolean => + SyncRegisterValueParamsSchema.safeParse({ + uuid: 'u', + unitId: '1', + registerValues: [params(overrides)] + }).success + +describe('the register main cannot encode', () => { + const unencodable: [string, Record][] = [ + ['a uint16 of 70000', { value: 70000 }], + ['an int16 of 40000', { dataType: 'int16', value: 40000 }], + ['an int64 of 1.5', { dataType: 'int64', value: 1.5 }], + ['a string of 1e12 words', { dataType: 'utf8', length: 1e12, stringValue: 'x', value: 0 }], + ['a string of 1e9 words', { dataType: 'utf8', length: 1e9, stringValue: 'x', value: 0 }], + [ + 'a string of ten at 65530', + { address: 65530, dataType: 'utf8', length: 10, stringValue: 'x', value: 0 } + ], + ['an interval of 1e12', { interval: 1e12, min: 0, max: 10, value: undefined }] + ] + + it.each(unencodable)('is refused on add before main sees it: %s', (_name, overrides) => { + expect(addTakes(overrides)).toBe(false) + }) + + it.each(unencodable)('is refused on sync before main sees it: %s', (_name, overrides) => { + expect(syncTakes(overrides)).toBe(false) + }) + + it('still takes the registers the app sends', () => { + expect(addTakes({})).toBe(true) + expect(addTakes({ dataType: 'utf8', length: 10, stringValue: 'hello', value: 0 })).toBe(true) + expect(addTakes({ interval: 1000, min: 0, max: 10, value: undefined })).toBe(true) + }) +}) diff --git a/src/main/modules/modbusClient.ts b/src/main/modules/modbusClient.ts index 3ba8a08b..684b352b 100644 --- a/src/main/modules/modbusClient.ts +++ b/src/main/modules/modbusClient.ts @@ -680,6 +680,30 @@ export class ModbusClient { */ private _nextTransactionIdKey = (): string => String(this._internals()._port?._transactionIdWrite) + /** + * The data address the request asked for. + * + * Read off the request frame, not off `nextDataAddress`. modbus-serial files + * that field on two of its twelve transaction records, inside `writeFC4` and + * `writeFC6`, and `writeFC1` delegates to `writeFC2` while `writeFC3` + * delegates to `writeFC4`. So FC3, FC4 and FC6 carry one and FC1, FC2, FC5, + * FC15 and FC16 do not, and the Addr column was blank for every coil read, + * every discrete input read and every write Modbux sends. + * + * Every `writeFCx` builds its frame as unit id, function code, then the data + * address at offset 2 as a big-endian word: measured over FC2, FC4, FC5, FC6, + * FC15 and FC16, which is every code Modbux sends. `request` is stashed by + * `_writeBufferToPort` while debug mode is on, and `connect` sets + * `isDebugEnabled` on every client it opens, including the replacement the + * disconnect timeout builds, so a frame that went out over a connection this + * file made has one. `nextDataAddress` is the fallback either way. + */ + private _requestedAddress = (rawTransaction: RawTransaction): number | undefined => { + const request = rawTransaction.request + if (request && request.length >= 4) return request.readUInt16BE(2) + return rawTransaction.nextDataAddress + } + /** * Log the request filed under `transactionIdKey`, and only that one. * @@ -703,7 +727,7 @@ export class ModbusClient { id: `${transactionIdKey}__${v4()}`, timestamp: DateTime.now().toMillis(), unitId: rawTransaction.nextAddress, - address: rawTransaction.nextDataAddress, + address: this._requestedAddress(rawTransaction), code: rawTransaction.nextCode, responseLength: rawTransaction.nextLength, timeout: rawTransaction._timeoutFired, diff --git a/src/main/modules/modbusServer.ts b/src/main/modules/modbusServer.ts index 3de84d75..247ff822 100644 --- a/src/main/modules/modbusServer.ts +++ b/src/main/modules/modbusServer.ts @@ -17,7 +17,8 @@ import { RegisterType, RegisterValue, DataBits, - StopBits + StopBits, + ValuedDataType } from '@shared' import { ServerTCP, ServerSerial } from 'modbus-serial' import { ServerEndianness } from '@shared' @@ -455,6 +456,37 @@ export class ModbusServer { this._littleEndian.set(uuid, littleEndian) } + /** + * The words a fixed register holds, or nothing at all when it cannot be + * encoded. + * + * `RegisterParamsSchema` bounds the pair of `dataType` and `value` at the IPC + * boundary, and `getValueRangeError` there reads the same table this asks + * about. This is the class answering for its own input: `addRegister` is + * public, and a caller inside main reaches it without crossing that boundary. + */ + private _encode = ({ + dataType, + value, + littleEndian, + stringValue, + length + }: { + dataType: ValuedDataType + value: number + littleEndian: boolean + stringValue?: string + length?: number + }): number[] | undefined => { + try { + return dataType === 'utf8' + ? createStringRegisters(stringValue ?? '', length ?? DEFAULT_UTF8_LENGTH) + : createRegisters(dataType, value, littleEndian) + } catch { + return undefined + } + } + private _unitData = (uuid: string, unitId: UnitIdString): ServerData => { const perUnitMap = this._ensureInnerMap(this._serverData, uuid) const serverData = perUnitMap.get(unitId) ?? getDefaultServerData() @@ -467,14 +499,26 @@ export class ModbusServer { * If a generator already exists at the address, it is disposed and replaced. * If a fixed value is provided, sets the register directly. * - * Answers the words now held from `address` on, which is nothing for `none` - * because that writes none. The renderer's store waits for this answer before - * it writes, and every `register_value` below goes out before the answer - * does, so those words would otherwise reach a store with no entry to put - * them in and be dropped. The store folds what comes back through the same + * Answers the words now held from `address` on, which is an empty list for + * `none` because that writes none. The renderer's store waits for this answer + * before it writes, and every `register_value` below goes out before the + * answer does, so those words would otherwise reach a store with no entry to + * put them in and be dropped. The store folds what comes back through the same * merge the event feeds, which is where a word becomes a value. + * + * `undefined` is the refusal, which is what the store already reads as + * nothing changed. An encoder that cannot take the register used to throw + * straight out of here: `createIpcHandle` puts no try around a listener, so + * the invoke rejected rather than answering, and `syncServerRegisters` adds + * in a bare loop, so the throw took every register after it in that unit + * with it. One register is refused now and the rest of the unit stands. + * + * The generator branch cannot be answered for here. `ValueGenerator` encodes + * inside an unawaited async tick, so a range its data type cannot take writes + * nothing and warns once per interval instead of throwing, which is `M-02` + * and is a change to that class. */ - public addRegister = ({ uuid, unitId, params }: AddRegisterParams): number[] => { + public addRegister = ({ uuid, unitId, params }: AddRegisterParams): number[] | undefined => { const littleEndian = this._littleEndian.get(uuid) ?? false const { address, @@ -501,26 +545,41 @@ export class ModbusServer { } const generators = serverGenerators[registerType] - const generator = generators.get(address) - generator?.dispose() - generators?.delete(address) - // Ensure server data map for this server and unitId - const serverData = this._unitData(uuid, unitId) - this._setServerData(uuid, unitId, serverData) + /** Frees the register this call is replacing, and answers its data map. */ + const takeTheAddress = (): ServerData => { + generators.get(address)?.dispose() + generators.delete(address) + const serverData = this._unitData(uuid, unitId) + this._setServerData(uuid, unitId, serverData) + return serverData + } // `none` is an address held open with nothing in it, so there is nothing to - // write and nothing to generate. The generator above is disposed either way, + // write and nothing to generate. The generator is disposed either way, // which is what editing a register to `none` has to do. - if (dataType === 'none') return [] + if (dataType === 'none') { + takeTheAddress() + return [] + } // If a fixed value is provided, set the register directly const fixedValue = !interval && value !== undefined if (fixedValue) { - const registers = - dataType === 'utf8' - ? createStringRegisters(stringValue ?? '', length ?? DEFAULT_UTF8_LENGTH) - : createRegisters(dataType, value, littleEndian) + // Encoded before the address is taken, because a refusal answers + // `undefined` and the store reads that as nothing changed. Disposing + // first zeroed the words of the generator being replaced and dropped it, + // so the grid went on drawing a generator that no longer ran. + const registers = this._encode({ dataType, value, littleEndian, stringValue, length }) + if (!registers) { + this._emitMessage({ + message: `The ${dataType} register at ${address} was not added: main cannot encode that value`, + variant: 'error' + }) + return undefined + } + + const serverData = takeTheAddress() registers.forEach((register, index) => { const registerAddress = address + index serverData[registerType][registerAddress] = register @@ -541,6 +600,7 @@ export class ModbusServer { } // Otherwise, add a value generator for this register + const serverData = takeTheAddress() generators.set( address, new ValueGenerator({ diff --git a/src/main/windows.ts b/src/main/windows.ts index 51051e56..06dc31e2 100644 --- a/src/main/windows.ts +++ b/src/main/windows.ts @@ -72,6 +72,15 @@ export class Windows { return [to] } + /** + * Whether `contents` is the main window's, which is where client work happens. + * + * Reuses the same liveness question `send` asks, so a main window that is + * gone or destroyed answers false rather than throwing on `webContents`. + */ + public isMain = (contents: WebContents): boolean => + this._liveContents(this._windows.main).includes(contents) + private _liveContents(window: BrowserWindow | null): WebContents[] { if (!window || window.isDestroyed() || !window.webContents) return [] return [window.webContents] diff --git a/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx b/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx index e6e9efdc..ab66cac4 100644 --- a/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx +++ b/src/renderer/src/components/client/ClientGrids/RegisterGrid/RegisterGridToolbar/LoadButton.tsx @@ -2,7 +2,7 @@ import FileOpen from '@mui/icons-material/FileOpen' import Box from '@mui/material/Box' import IconButton from '@mui/material/IconButton' import { useClientZustand } from '@renderer/context/client.zustand' -import { migrateClientConfig } from '@shared' +import { migrateClientConfig, resetMessage } from '@shared' import { useSnackbar } from 'notistack' import { useRef, useState, useCallback } from 'react' import { showMapping } from '@renderer/context/data.zustand' @@ -28,7 +28,7 @@ const LoadButton = meme((): JSX.Element => { try { // Use migration framework to handle all config versions const migrationResult = migrateClientConfig(content) - const { config, migrated, warning } = migrationResult + const { config, migrated, futureVersion } = migrationResult // Set name, endianness and register mapping if (config.name) clientZustand.setName(config.name) @@ -49,13 +49,18 @@ const LoadButton = meme((): JSX.Element => { }) } - // Show warning for future version - if (warning === 'FUTURE_VERSION') { + // A config from a newer Modbux is parsed against the current schema + // and keeps what matches, so the warning says which fields did not come + // across rather than that some feature may not work. + if (futureVersion) { enqueueSnackbar({ variant: 'warning', - message: - 'This config was created with a newer version of Modbux. Some features may not work correctly.', - persist: true + message: resetMessage('Client', futureVersion), + // A notice that everything came across is not one the user has to + // dismiss, and it sits on top of "Configuration opened + // successfully" either way. + persist: futureVersion.fields.length > 0, + autoHideDuration: 8000 }) } } catch (error) { diff --git a/src/renderer/src/components/client/ScanRegisters/ScanRegisters.tsx b/src/renderer/src/components/client/ScanRegisters/ScanRegisters.tsx index af359d66..d9e112f2 100644 --- a/src/renderer/src/components/client/ScanRegisters/ScanRegisters.tsx +++ b/src/renderer/src/components/client/ScanRegisters/ScanRegisters.tsx @@ -20,6 +20,7 @@ import { ScanTimeoutField } from '../ScanProgress/ScanProgress' import { meme } from '@renderer/components/shared/inputs/meme' +import { maxReadQuantity } from '@shared' import { useScanRegistersZustand } from './scanRegisters.zustand' // @@ -105,8 +106,9 @@ const ChunkSizeField = meme((): JSX.Element => { const scanning = useClientZustand((z) => z.clientState.scanningRegisters) const chunkSize = useScanRegistersZustand((z) => String(z.chunkSize)) const type = useClientZustand((z) => z.registerConfig.type) - const isCoilType = ['coils', 'discrete_inputs'].includes(type) - const max = isCoilType ? 2000 : 125 + // The protocol's pair, stated once in `ranges.ts`: this field computed it by + // hand and the unit id scan's Length field computed nothing at all. + const max = maxReadQuantity([type]) const setChunkSize = useScanRegistersZustand.getState().setChunkSize diff --git a/src/renderer/src/components/client/ScanUnitIds/ScanUnitIds.tsx b/src/renderer/src/components/client/ScanUnitIds/ScanUnitIds.tsx index dc9ba4af..757fec95 100644 --- a/src/renderer/src/components/client/ScanUnitIds/ScanUnitIds.tsx +++ b/src/renderer/src/components/client/ScanUnitIds/ScanUnitIds.tsx @@ -12,7 +12,7 @@ import AddressBaseInput from '@renderer/components/shared/inputs/AddressBaseInpu import { maskInputProps } from '@renderer/components/shared/inputs/types' import UIntInput from '@renderer/components/shared/inputs/UintInput' import { useClientZustand } from '@renderer/context/client.zustand' -import { RegisterType } from '@shared' +import { maxReadQuantity, RegisterType } from '@shared' import { ElementType, useCallback, useMemo } from 'react' import useScanUnitIdColumns from './_columns' import { useScanUnitIdZustand } from './scanUnitIds.zustand' @@ -102,6 +102,13 @@ const LengthField = meme((): JSX.Element => { const scanning = useClientZustand((z) => z.clientState.scanningUnitIds) const length = useScanUnitIdZustand((z) => String(z.length)) + // The field passed no `max`, so `UintInput`'s default of 65535 was typeable + // and went to the socket as the quantity. `maxReadQuantity` is the protocol's + // own pair, and it reads the types selected because one length goes out for + // every one of them. + const registerTypes = useScanUnitIdZustand((z) => z.registerTypes) + const max = maxReadQuantity(registerTypes) + const setLength = useScanUnitIdZustand.getState().setLength return ( @@ -116,7 +123,7 @@ const LengthField = meme((): JSX.Element => { slotProps={{ input: { inputComponent: UIntInput as unknown as ElementType, - inputProps: maskInputProps({ set: setLength }) + inputProps: maskInputProps({ set: setLength, max }) } }} /> @@ -213,9 +220,14 @@ const ScanButton = meme((): JSX.Element => { const { address, length, startUnitId, count, registerTypes, timeout } = scanUnitIdZustand + // Clamped where the request is built rather than left to the boundary. The + // field masks to the same ceiling, and a length typed under one set of + // register types stays in the store when another is selected, so this is + // the moment the pair is finally known. The boundary refuses what it is + // given and the results are already cleared by then. window.api.scanUnitIds({ address, - length, + length: Math.min(length, maxReadQuantity(registerTypes)), range: [startUnitId, startUnitId + count - 1], registerTypes, timeout diff --git a/src/renderer/src/components/server/OpenSaveClear.tsx b/src/renderer/src/components/server/OpenSaveClear.tsx index 26f7817d..35fd816a 100644 --- a/src/renderer/src/components/server/OpenSaveClear.tsx +++ b/src/renderer/src/components/server/OpenSaveClear.tsx @@ -7,7 +7,7 @@ import { downloadJson } from '@renderer/components/shared/downloadJson' import { meme } from '@renderer/components/shared/inputs/meme' import { useLayoutZustand } from '@renderer/context/layout.zustand' import { useServerZustand } from '@renderer/context/server.zustand' -import { checkHasConfig, migrateServerConfig } from '@shared' +import { checkHasConfig, migrateServerConfig, resetMessage } from '@shared' import { CURRENT_SERVER_CONFIG_VERSION, ServerConfig, @@ -42,17 +42,23 @@ const useOpen: UseOpenHook = () => { const serverZustand = useServerZustand.getState() - // A unit the file does not name is not written over on the way in, so - // whatever the previous config left on it would answer a master after - // this one is loaded. Reset takes both sides down to nothing first. - await serverZustand.resetServer(serverZustand.selectedUuid) - - const content = await file.text() - try { + const content = await file.text() + // Use migration framework to handle all config versions const migrationResult = migrateServerConfig(content) - const { config, migrated, warning, wasMixedEndianness } = migrationResult + const { config, migrated, wasMixedEndianness, futureVersion } = migrationResult + + // A unit the file does not name is not written over on the way in, so + // whatever the previous config left on it would answer a master after + // this one is loaded. Reset takes both sides down to nothing first. + // + // After the migration rather than before it, because `migrateServerConfig` + // needs nothing from the store and throws on both shapes a user picks by + // accident, a client config file and malformed JSON. Reset first, and the + // configuration on screen was gone in main and in the store before + // anything had read the file. + await serverZustand.resetServer(serverZustand.selectedUuid) // Set name and littleEndian serverZustand.setName(config.name) @@ -90,26 +96,36 @@ const useOpen: UseOpenHook = () => { }) } - // Show warning for future version - if (warning === 'FUTURE_VERSION') { + // A config from a newer Modbux is parsed against the current schema and + // keeps what matches, so the warning says which fields did not come + // across rather than that some feature may not work. + if (futureVersion) { enqueueSnackbar({ variant: 'warning', - message: - 'This config was created with a newer version of Modbux. Some features may not work correctly.', - persist: true + message: resetMessage('Server', futureVersion), + // A notice that everything came across is not one the user has to + // dismiss, and it sits on top of "Configuration opened + // successfully" either way. + persist: futureVersion.fields.length > 0, + autoHideDuration: 8000 }) } } catch (error) { const tError = error as Error enqueueSnackbar({ variant: 'error', message: `Failed to load config: ${tError.message}` }) console.error('Config load error:', error) + } finally { + // In the `finally` because `file.text()` is inside the `try` now, and a + // file that is gone by the time it is read left `opening` true and + // every server button disabled until the app was restarted. + // + // `init` runs either way: on a refused file it hands main the + // configuration that is still there, which is the one on screen. + await serverZustand.init(serverZustand.selectedUuid) + + openingRef.current = false + setOpening(false) } - - // Synchronize only the selected server after opening the configuration - await serverZustand.init(serverZustand.selectedUuid) - - openingRef.current = false - setOpening(false) }, [enqueueSnackbar] ) diff --git a/src/renderer/src/components/server/ServerGrid/ServerBooleans.tsx b/src/renderer/src/components/server/ServerGrid/ServerBooleans.tsx index 72f2302f..8dec2b86 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerBooleans.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerBooleans.tsx @@ -28,8 +28,17 @@ interface ServerBoolRowProps { type: BooleanRegisters } +/** + * One coil or discrete input, with its comment and its remove button. + * + * It read `collapse` and passed it on as `readOnly`, and `ServerBooleans` + * renders this list only under `{!collapse && ...}`. Both read the same store + * and React renders the parent first, so no committed render carried + * `readOnly: true`, and the hover styles and the remove button were guarded on + * the same unreachable half. Two tests in `ServerBit.test.tsx` asserted + * behaviour nothing in the app reached. + */ const ServerBoolRow = meme(({ address, type }: ServerBoolRowProps) => { - const collapse = useServerGridZustand((z) => z.collapse[type]) const entry = useServerZustand((z) => { const uuid = z.selectedUuid const unitId = z.getUnitId(uuid) @@ -63,11 +72,9 @@ const ServerBoolRow = meme(({ address, type }: ServerBoolRowProps) => { alignItems: 'center', borderRadius: 1, transition: 'background-color 0.15s', - ...(!collapse && { - '&:hover .remove-btn': { opacity: 1 }, - '&:hover': { backgroundColor: alpha(theme.palette.primary.dark, 0.1) }, - '&:has(.remove-btn:hover)': { backgroundColor: alpha(theme.palette.error.main, 0.1) } - }) + '&:hover .remove-btn': { opacity: 1 }, + '&:hover': { backgroundColor: alpha(theme.palette.primary.dark, 0.1) }, + '&:has(.remove-btn:hover)': { backgroundColor: alpha(theme.palette.error.main, 0.1) } })} > @@ -81,26 +88,23 @@ const ServerBoolRow = meme(({ address, type }: ServerBoolRowProps) => { padDigits={5} dimUnmapped={false} hoverHighlight={false} - readOnly={collapse} /> - {!collapse && ( - ({ - opacity: 0, - transition: 'opacity 0.15s', - p: 0.25, - color: theme.palette.text.secondary, - '&:hover': { color: theme.palette.error.main } - })} - > - - - )} + ({ + opacity: 0, + transition: 'opacity 0.15s', + p: 0.25, + color: theme.palette.text.secondary, + '&:hover': { color: theme.palette.error.main } + })} + > + + ) }) diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/AddRegister.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/AddRegister.tsx index 4da52abe..8aac1d04 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/AddRegister.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/AddRegister.tsx @@ -78,9 +78,10 @@ const AddRegister = meme(() => { addRegisterZustand.setValue(FIELD_DEFAULTS.value, true) } else if (value !== undefined && isTimestampType(dataType)) { // The picker works in milliseconds and a unix register stores seconds. - // `RegisterParamsSchema` bounds no value, so a config file can hold one - // outside the window, and the field says so rather than the picker having - // to be touched first. + // `getValueRangeError` leaves `unix` and `datetime` alone, because one + // goes through `value >>> 0` and the other through a clamp rather than + // throwing, so a config file can hold one outside the window and the + // field says so rather than the picker having to be touched first. const milliseconds = dataType === 'unix' ? Number(value) * 1000 : Number(value) addRegisterZustand.setValue(String(milliseconds), inTimestampWindow(dataType, milliseconds)) } else if (value !== undefined) { diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/__tests__/AddRegister.test.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/__tests__/AddRegister.test.tsx index 1a019f1f..299f7080 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/__tests__/AddRegister.test.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/__tests__/AddRegister.test.tsx @@ -316,7 +316,9 @@ describe('the edit dialog buttons', () => { expect(Number(useAddRegisterZustand.getState().value)).toBeGreaterThanOrEqual(openedAt) }) - // `RegisterParamsSchema` bounds no value, so this arrives from a config file. + // `getValueRangeError` leaves `datetime` alone, because + // `encodeIEC870DateTime` clamps rather than throws, so this arrives from a + // config file and the field is where it is said. it('marks a stored datetime outside the window wrong before anything is typed', () => { renderEditing(timestampAt100('datetime', Date.UTC(2200, 0, 1))) diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/maskedInputs.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/maskedInputs.tsx index 306eac5c..77aaec7c 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/maskedInputs.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/AddRegister/maskedInputs.tsx @@ -11,7 +11,7 @@ import { meme } from '@renderer/components/shared/inputs/meme' import { MaskInputProps } from '@renderer/components/shared/inputs/types' import { forwardRef } from 'react' import { IMask, IMaskInput } from 'react-imask' -import { notEmpty, registerWidth } from '@shared' +import { MAX_UTF8_LENGTH, notEmpty, registerWidth } from '@shared' import { useMinMaxInteger } from '@renderer/hooks' const AddressInputForward = forwardRef((props, ref) => { @@ -159,7 +159,7 @@ const RegisterLengthForward = forwardRef((prop {...other} {...integerMask} min={1} - max={124} + max={MAX_UTF8_LENGTH} autofix inputRef={ref} onAccept={(value) => set(value, notEmpty(value))} diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/ServerBitMapDetail.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/ServerBitMapDetail.tsx index 214e5ff5..22bca36d 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/ServerBitMapDetail.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/ServerBitMapDetail.tsx @@ -105,7 +105,7 @@ const ServerBitMapDetail = meme(({ register }: ServerBitMapDetailProps): JSX.Ele void handleToggle(bitIndex)} onCommentChange={(c) => handleCommentChange(bitIndex, c)} diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/__tests__/ServerBitMapDetail.test.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/__tests__/ServerBitMapDetail.test.tsx index 61a676ad..8a6b5cea 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/__tests__/ServerBitMapDetail.test.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerBitMapDetail/__tests__/ServerBitMapDetail.test.tsx @@ -19,7 +19,7 @@ const serverState = { /** Answers the entry's own value, which is what the real one does with an empty batcher. */ const mockPendingRegisterValue = vi.fn( - (_uuid: string, _unitId: string, entry: ServerRegisterEntry): number => entry.value + (_uuid: string, _unitId: string, entry: ServerRegisterEntry): number => Number(entry.value) ) vi.mock('@renderer/context/server.zustand', () => ({ @@ -64,7 +64,7 @@ const writtenParams = (call = 0): ServerRegisterEntry['params'] => { beforeEach(() => { mockAddRegister.mockReset() mockPendingRegisterValue.mockReset() - mockPendingRegisterValue.mockImplementation((_uuid, _unitId, entry) => entry.value) + mockPendingRegisterValue.mockImplementation((_uuid, _unitId, entry) => Number(entry.value)) }) describe('which bits the panel shows as on', () => { diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx index 7078d369..77dc693e 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/ServerRegisters.tsx @@ -9,6 +9,7 @@ import { formatUnixSeconds, NumberRegisters, parseIEC870DateTimeValue, + toExact64Bits, ServerRegister } from '@shared' import { useServerZustand } from '@renderer/context/server.zustand' @@ -46,10 +47,15 @@ const RowEdit = meme(({ register }: RowProps) => { const getDisplayValue = (register: ServerRegister[number]): string | number => { const { dataType } = register.params if (dataType === 'utf8') return register.params.stringValue ?? '' - if (dataType === 'unix') return formatUnixSeconds(register.value) + // A `unix` register is a uint32, so its composite is a number. The three + // types that hold a string are read through `toExact64Bits`. + if (dataType === 'unix') return formatUnixSeconds(Number(register.value)) // The shared decoder answers '' for a register no date can be read out of, // which is what a register the server has not written yet holds. - if (dataType === 'datetime') return parseIEC870DateTimeValue(register.value) || '—' + if (dataType === 'datetime') { + const packed = toExact64Bits(register.value) + return (packed !== undefined && parseIEC870DateTimeValue(packed)) || '—' + } return register.value } diff --git a/src/renderer/src/components/server/ServerGrid/ServerRegisters/__tests__/ServerRegisters.test.tsx b/src/renderer/src/components/server/ServerGrid/ServerRegisters/__tests__/ServerRegisters.test.tsx index 9a3a71b7..2c11e371 100644 --- a/src/renderer/src/components/server/ServerGrid/ServerRegisters/__tests__/ServerRegisters.test.tsx +++ b/src/renderer/src/components/server/ServerGrid/ServerRegisters/__tests__/ServerRegisters.test.tsx @@ -2,7 +2,7 @@ /// import { render, screen } from '@testing-library/react' import { describe, it, expect, vi } from 'vitest' -import type { ServerRegister, ServerRegisterEntry } from '@shared' +import type { ServerRegister, ServerRegisterEntry, ServerRegisterValue } from '@shared' // ─── Store stub ────────────────────────────────────────────────────── // The real server store persists through window.api on import, and the rows @@ -25,15 +25,26 @@ vi.mock('@renderer/context/server.zustand', () => ({ import ServerRegisters from '../ServerRegisters' -/** A register of `dataType` at address 6, holding the composite the store folds. */ -const registerAt6 = (dataType: 'unix' | 'datetime', value: number): ServerRegisterEntry => ({ +/** + * A register of `dataType` at address 6, holding the composite the store folds. + * + * A `datetime` entry holds that composite as a decimal string, because it fills + * 64 bits as an integer and a JS number carries 53 of them. `unix` is a uint32 + * and holds a number. + */ +const registerAt6 = ( + dataType: 'unix' | 'datetime', + value: ServerRegisterValue +): ServerRegisterEntry => ({ value, params: { address: 6, registerType: 'holding_registers', dataType, comment: 'timestamp', - value, + // `params.value` is the fixed value the dialog stored and stays a number; + // the entry's own `value` is the composite the store folds. + value: 0, min: undefined, max: undefined, interval: undefined @@ -47,9 +58,14 @@ const valueCellFor = async (entry: ServerRegisterEntry): Promise => return screen.findByTestId('server-reg-value-holding_registers-6') } -/** One composite value out of the four IEC 870-5 words, the way the store folds them. */ -const packed = (word1: number, word2: number, word3: number, word4: number): number => - word1 * 2 ** 48 + word2 * 2 ** 32 + word3 * 2 ** 16 + word4 +/** One composite value out of the four IEC 870-5 words, as the store holds it. */ +const packed = (word1: number, word2: number, word3: number, word4: number): string => + ( + BigInt(word1) * 2n ** 48n + + BigInt(word2) * 2n ** 32n + + BigInt(word3) * 2n ** 16n + + BigInt(word4) + ).toString() // 2024/06/01 12:34:56.789 UTC const JUNE_2024 = packed(0x0018, 0x0601, 0x0c22, 0xddd5) @@ -61,7 +77,7 @@ describe('what the server row shows for a timestamp', () => { }) it('shows a dash for a datetime register the server has not written', async () => { - const cell = await valueCellFor(registerAt6('datetime', 0)) + const cell = await valueCellFor(registerAt6('datetime', '0')) expect(cell).toHaveTextContent('—') }) @@ -89,4 +105,20 @@ describe('what the server row shows for a timestamp', () => { const cell = await valueCellFor(registerAt6('unix', 1700000000)) expect(cell).toHaveTextContent('2023/11/14 22:13:20') }) + + // The end of the window the format carries, which rounded up by one + // millisecond through `Number` and read 2100/01/01 00:00:00. + it('reads the last second the format holds', async () => { + const cell = await valueCellFor( + registerAt6('datetime', packed(99, (12 << 8) | 31, (23 << 8) | 59, 59999)) + ) + expect(cell).toHaveTextContent('2099/12/31 23:59:59') + }) + + // `ServerRegisterEntrySchema` takes a fractional number for a 64 bit type, so + // a hand-edited config reaches a value no composite can be read out of. + it('shows a dash for a datetime value that is not a whole number', async () => { + const cell = await valueCellFor(registerAt6('datetime', 0.5)) + expect(cell).toHaveTextContent('—') + }) }) diff --git a/src/renderer/src/components/server/ServerGrid/__tests__/collapse.test.tsx b/src/renderer/src/components/server/ServerGrid/__tests__/collapse.test.tsx new file mode 100644 index 00000000..c8b94fce --- /dev/null +++ b/src/renderer/src/components/server/ServerGrid/__tests__/collapse.test.tsx @@ -0,0 +1,55 @@ +// @vitest-environment happy-dom +/// +// +// `ServerBit` carried a `readOnly` prop, set from one place, +// `readOnly={collapse}` in `ServerBoolRow`. `ServerBooleans` renders that list +// only under `{!collapse && ...}`, and both read the same store, so React +// renders the parent first and no committed render could carry `readOnly: true`. +// The same half guarded `ServerBoolRow`'s hover styles and its remove button, +// and two tests in `ServerBit.test.tsx` asserted behaviour nothing reached. +import { render, screen } from '@testing-library/react' +import { describe, it, expect, vi } from 'vitest' +import type { ServerBool } from '@shared' + +const coils: ServerBool = { 0: { value: true, comment: 'run' } } +const serverState = { + selectedUuid: 'main', + getUnitId: (): string => '0', + unitId: { main: '0' }, + serverRegisters: { main: { '0': { coils } } } +} + +vi.mock('@renderer/context/server.zustand', () => ({ + useServerZustand: Object.assign( + (selector: (state: typeof serverState) => unknown) => selector(serverState), + { getState: () => serverState } + ) +})) + +import ServerBooleans from '../ServerBooleans' +import useServerGridZustand from '../serverGrid.zustand' + +const allOpen = { + coils: false, + discrete_inputs: false, + input_registers: false, + holding_registers: false +} + +describe('a collapsed boolean section', () => { + it('shows its bits while it is open', () => { + useServerGridZustand.setState({ collapse: allOpen }) + render() + + expect(screen.getByTestId('server-bool-coils-circle-0')).toBeInTheDocument() + expect(screen.getByTestId('remove-bool-coils-0')).toBeInTheDocument() + }) + + it('renders no bit at all once collapsed', () => { + useServerGridZustand.setState({ collapse: { ...allOpen, coils: true } }) + render() + + expect(screen.queryByTestId('server-bool-coils-circle-0')).toBeNull() + expect(screen.queryByTestId('server-bool-row-coils-0')).toBeNull() + }) +}) diff --git a/src/renderer/src/components/server/ServerGrid/shared/ServerBit.tsx b/src/renderer/src/components/server/ServerGrid/shared/ServerBit.tsx index 5f37e1bf..d948b621 100644 --- a/src/renderer/src/components/server/ServerGrid/shared/ServerBit.tsx +++ b/src/renderer/src/components/server/ServerGrid/shared/ServerBit.tsx @@ -18,8 +18,6 @@ export interface ServerBitProps { dimUnmapped?: boolean /** Show hover background on this row (default: true) */ hoverHighlight?: boolean - /** Disable toggle & comment editing (default: false) */ - readOnly?: boolean } const ServerBit = meme( @@ -32,8 +30,7 @@ const ServerBit = meme( testIdPrefix = 'server-bit', padDigits = 2, dimUnmapped = true, - hoverHighlight = true, - readOnly = false + hoverHighlight = true }: ServerBitProps): JSX.Element => { const [editing, setEditing] = useState(false) const [draft, setDraft] = useState(comment ?? '') @@ -75,19 +72,17 @@ const ServerBit = meme( // On and off differ only in background and shadow, so the circle // renders identically to a test either way. This is the state itself. data-active={active} - onClick={readOnly ? undefined : onToggle} + onClick={onToggle} sx={(theme) => ({ width: 12, height: 12, borderRadius: '50%', flexShrink: 0, - cursor: readOnly ? 'default' : 'pointer', + cursor: 'pointer', background: active ? theme.palette.success.main : theme.palette.action.disabled, boxShadow: active ? `0 0 5px ${theme.palette.success.main}` : 'none', transition: 'background 0.15s, box-shadow 0.15s', - ...(!readOnly && { - '&:hover': { outline: `2px solid ${theme.palette.primary.main}`, outlineOffset: 1 } - }) + '&:hover': { outline: `2px solid ${theme.palette.primary.main}`, outlineOffset: 1 } })} /> @@ -107,7 +102,7 @@ const ServerBit = meme( {/* Comment — inline editable */} - {!readOnly && editing ? ( + {editing ? ( { - setDraft(comment ?? '') - setEditing(true) - } - } + onClick={(): void => { + setDraft(comment ?? '') + setEditing(true) + }} sx={{ flexGrow: 1, minWidth: 0, lineHeight: '18px', - cursor: readOnly ? 'default' : 'text', - ...(!readOnly && { '&:hover': { textDecoration: 'underline dotted' } }) + cursor: 'text', + '&:hover': { textDecoration: 'underline dotted' } }} > {hasMapped ? comment : '...'} diff --git a/src/renderer/src/components/server/ServerGrid/shared/__tests__/ServerBit.test.tsx b/src/renderer/src/components/server/ServerGrid/shared/__tests__/ServerBit.test.tsx index cf720171..ce413c45 100644 --- a/src/renderer/src/components/server/ServerGrid/shared/__tests__/ServerBit.test.tsx +++ b/src/renderer/src/components/server/ServerGrid/shared/__tests__/ServerBit.test.tsx @@ -38,15 +38,6 @@ describe('the toggle circle', () => { expect(onToggle).toHaveBeenCalledOnce() }) - - it('does not toggle when the row is read only', async () => { - const onToggle = vi.fn() - renderBit({ onToggle, readOnly: true }) - - await userEvent.click(screen.getByTestId('server-bit-circle-3')) - - expect(onToggle).not.toHaveBeenCalled() - }) }) describe('the comment', () => { @@ -60,13 +51,4 @@ describe('the comment', () => { expect(onCommentChange).toHaveBeenCalledWith('motor running') }) - - it('is not editable when the row is read only', async () => { - const onCommentChange = vi.fn() - renderBit({ comment: 'run', onCommentChange, readOnly: true }) - - await userEvent.click(screen.getByTestId('server-bit-comment-3')) - - expect(screen.queryByRole('textbox')).not.toBeInTheDocument() - }) }) diff --git a/src/renderer/src/containers/Server.tsx b/src/renderer/src/containers/Server.tsx index 0a7d5978..d2fde6cb 100644 --- a/src/renderer/src/containers/Server.tsx +++ b/src/renderer/src/containers/Server.tsx @@ -35,9 +35,12 @@ const ServerName = meme(() => { }) const Server = meme(() => { - const ready = useServerZustand((z) => Object.values(z.ready).some((ready) => ready)) + // `initialized` rather than `ready`, which is per uuid and written only on a + // sync that got through: one rejected invoke in `init`'s loop left this view + // blank on that launch and every one after it. + const initialized = useServerZustand((z) => z.initialized) return ( - + => { + await Promise.resolve() + await Promise.resolve() +} + +beforeEach(() => { + vi.resetModules() + localStorage.clear() + calls.length = 0 +}) + +describe('client.zustand in the split out server window', () => { + it('tells main nothing at module scope', async () => { + stubRenderer({ isServerWindow: true }) + recordApiCalls(calls) + + await import('../client.zustand') + await settle() + + expect(calls.map(({ method }) => method)).toEqual(['getAppVersion']) + }) + + // persist wraps `setState`, so `init`'s own `set` wrote the whole partialized + // state under the shared key on every open of the split out window. + it('does not write the shared storage key', async () => { + stubRenderer({ isServerWindow: true }) + + await import('../client.zustand') + await settle() + + expect(localStorage.getItem(CLIENT_ZUSTAND_STORAGE_KEY)).toBeNull() + }) + + it('still calls main from the main window', async () => { + stubRenderer() + recordApiCalls(calls) + + await import('../client.zustand') + await settle() + + const methods = calls.map(({ method }) => method) + expect(methods).toContain('updateConnectionConfig') + expect(methods).toContain('updateRegisterConfig') + expect(methods).toContain('stopScanningUnitIds') + expect(calls.find(({ method }) => method === 'setReadConfiguration')?.payload).toBe(false) + }) +}) diff --git a/src/renderer/src/context/__tests__/exact64BitValue.test.ts b/src/renderer/src/context/__tests__/exact64BitValue.test.ts new file mode 100644 index 00000000..cdd17334 --- /dev/null +++ b/src/renderer/src/context/__tests__/exact64BitValue.test.ts @@ -0,0 +1,121 @@ +// @vitest-environment happy-dom +// +// `ServerRegisterEntrySchema.value` was `z.number()`. `applyRegisterValue` read +// the composite back with `getBigUint64` and stored `Number(...)`, so four +// words of 0xFFFF came out 18446744073709552000 rather than +// 18446744073709551615. The next single word write read that back through +// `BigInt`, which `setBigUint64` takes modulo 2 ** 64 rather than throwing, so +// the entry collapsed to the low word alone. `ServerRegisters.tsx` renders +// `register.value`, and the four `register_value` senders in main send one +// event per word, so a client writing a 64-bit holding register reaches it. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + MAIN_SERVER_UUID, + ServerRegisterEntry, + ServerRegisterValue, + ServerRegisters +} from '@shared' +import { stubRenderer } from './stubRenderer' + +type Store = typeof import('../server.zustand') + +beforeEach(() => { + vi.resetModules() + vi.useFakeTimers() + localStorage.clear() + stubRenderer() +}) +afterEach(() => vi.useRealTimers()) + +/** One 64 bit register at address 10, holding `value`. */ +const entry = (dataType: 'uint64' | 'int64', value: ServerRegisterValue): ServerRegisterEntry => ({ + value, + params: { + address: 10, + registerType: 'holding_registers' as const, + dataType, + comment: '', + value: 0, + min: undefined, + max: undefined, + interval: undefined + } +}) + +const seeded = async (dataType: 'uint64' | 'int64'): Promise => { + const store = await import('../server.zustand') + store.useServerZustand.getState().clean(MAIN_SERVER_UUID) + store.useServerZustand.getState().replaceServerRegisters('0', { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { 10: entry(dataType, 0) } + }) + return store +} + +const stored = (store: Store): ServerRegisterValue => { + const registers: ServerRegisters | undefined = + store.useServerZustand.getState().serverRegisters[MAIN_SERVER_UUID]?.['0'] + const found = registers?.holding_registers[10] + if (!found) throw new Error('the register at address 10 is not there') + return found.value +} + +const write = (store: Store, offset: number, word: number): void => + store.applyRegisterValue({ + uuid: MAIN_SERVER_UUID, + unitId: '0', + registerType: 'holding_registers', + address: 10 + offset, + value: word + }) + +describe('a uint64 register a client writes', () => { + it('holds every one of its 64 bits', async () => { + const store = await seeded('uint64') + for (const [offset, word] of [0xffff, 0xffff, 0xffff, 0xffff].entries()) { + write(store, offset, word) + } + await vi.advanceTimersByTimeAsync(100) + + expect(stored(store)).toBe('18446744073709551615') + }) + + // The flush clears the batcher, so the next word write reads the entry. That + // read used to be the rounded number. + it('keeps the other three words when one more word is written after a flush', async () => { + const store = await seeded('uint64') + for (const [offset, word] of [0xffff, 0xffff, 0xffff, 0xffff].entries()) { + write(store, offset, word) + } + await vi.advanceTimersByTimeAsync(100) + + write(store, 3, 0x0001) + await vi.advanceTimersByTimeAsync(100) + + expect(stored(store)).toBe('18446744073709486081') + }) + + it('holds a value above 2 ** 53 exactly', async () => { + const store = await seeded('uint64') + for (const [offset, word] of [0x0102, 0x0304, 0x0506, 0x0708].entries()) { + write(store, offset, word) + } + await vi.advanceTimersByTimeAsync(100) + + expect(stored(store)).toBe('72623859790382856') + }) +}) + +describe('an int64 register a client writes', () => { + it('holds minus one', async () => { + const store = await seeded('int64') + for (const [offset, word] of [0xffff, 0xffff, 0xffff, 0xffff].entries()) { + write(store, offset, word) + } + await vi.advanceTimersByTimeAsync(100) + + expect(stored(store)).toBe('-1') + }) +}) diff --git a/src/renderer/src/context/__tests__/serverStoreRehydrate.test.ts b/src/renderer/src/context/__tests__/serverStoreRehydrate.test.ts index 25eea06d..bf61a7bb 100644 --- a/src/renderer/src/context/__tests__/serverStoreRehydrate.test.ts +++ b/src/renderer/src/context/__tests__/serverStoreRehydrate.test.ts @@ -45,6 +45,26 @@ const store = (addresses: number[]): string => version: CURRENT_SERVER_ZUSTAND_VERSION }) +/** The same blob with a second server in it, as the split out window writes it. */ +const withSecondServer = (raw: string, uuid: string): string => { + const blob = JSON.parse(raw) + const { state } = blob + state.uuids.push(uuid) + state.selectedUuid = uuid + state.port[uuid] = '503' + state.unitId[uuid] = '0' + state.littleEndian[uuid] = false + state.serverRegisters[uuid] = {} + return JSON.stringify(blob) +} + +/** Waits out the rehydrate, which persist answers with a promise. */ +const settle = async (): Promise => { + await Promise.resolve() + await Promise.resolve() + await Promise.resolve() +} + const held = async (): Promise => { const { useServerZustand } = await import('../server.zustand') return Object.keys(useServerZustand.getState().serverRegisters.u?.['0']?.holding_registers ?? {}) @@ -84,6 +104,48 @@ describe('the server window closing', () => { expect(await held()).toEqual(['0']) }) + // `ready` is not persisted either, so the rehydrate brought a uuid back with + // no entry, and `setPort`, `setUnitId` and `setLittleEndian` each refuse on + // that with no message. The window that made the server ran `createServer` + // for it, so main does know it. + it('marks a server the split out window made as one main knows', async () => { + localStorage.setItem(SERVER_ZUSTAND_STORAGE_KEY, store([0])) + const { useServerZustand } = await import('../server.zustand') + await settle() + + localStorage.setItem( + SERVER_ZUSTAND_STORAGE_KEY, + withSecondServer(store([0]), 'made-in-the-split-window') + ) + + fireEvent('window_update', { main: true, server: true }) + fireEvent('window_update', { main: true, server: false }) + await settle() + + expect(useServerZustand.getState().uuids).toContain('made-in-the-split-window') + expect(useServerZustand.getState().ready['made-in-the-split-window']).toBe(true) + + useServerZustand.getState().setUnitId('7') + expect(useServerZustand.getState().unitId['made-in-the-split-window']).toBe('7') + }) + + // A uuid this window's own `init` wrote `false` for is one main refused, and + // writing `true` over it hands the three setters back for a server main has + // no listener for. + it('leaves a uuid this window already knows about alone', async () => { + localStorage.setItem(SERVER_ZUSTAND_STORAGE_KEY, store([0])) + const { useServerZustand } = await import('../server.zustand') + await settle() + + useServerZustand.setState({ ready: { u: false } }) + + fireEvent('window_update', { main: true, server: true }) + fireEvent('window_update', { main: true, server: false }) + await settle() + + expect(useServerZustand.getState().ready.u).toBe(false) + }) + // `rtuServerActive` is not persisted, so re-reading the key leaves it where // it was, and the split out window is the one that heard the last change. it('asks main for the RTU status, at load and at the close', async () => { diff --git a/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts b/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts new file mode 100644 index 00000000..207547d7 --- /dev/null +++ b/src/renderer/src/context/__tests__/serverViewDrawsAfterInit.test.ts @@ -0,0 +1,173 @@ +// @vitest-environment happy-dom +// +// `containers/Server.tsx` fades the whole server view in on one flag. It read +// `ready`, which `syncUuidToBackend` writes only for a uuid whose sync got +// through, and `init` sets every uuid false before its loop. So a rejected +// invoke anywhere in that loop left the view blank on that launch and on every +// one after it, with nothing on screen to clear the config with. The four +// verdicts in `tmp/audit-2` that end in a blank server view all end here. +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + CURRENT_SERVER_ZUSTAND_VERSION, + MAIN_SERVER_UUID, + SERVER_ZUSTAND_STORAGE_KEY +} from '@shared' +import { stubRenderer } from './stubRenderer' + +/** A blob with one server holding one register, so `init` has a sync to make. */ +const persisted = (): string => + JSON.stringify({ + state: { + selectedUuid: MAIN_SERVER_UUID, + uuids: [MAIN_SERVER_UUID], + port: { [MAIN_SERVER_UUID]: '502' }, + name: { [MAIN_SERVER_UUID]: 'bench' }, + unitId: { [MAIN_SERVER_UUID]: '0' }, + littleEndian: { [MAIN_SERVER_UUID]: false }, + usedAddresses: {}, + serverRegisters: { + [MAIN_SERVER_UUID]: { + '0': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': { + value: 1, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + value: 1 + } + } + } + } + } + } + }, + version: CURRENT_SERVER_ZUSTAND_VERSION + }) + +/** Main answering the port it bound, which is what `init` continues past. */ +const stubCreateServer = (port: number): void => { + const boundary = window.api as unknown as Record + window.api = new Proxy(boundary, { + get: (target, method: string): unknown => + method === 'createServer' + ? (): Promise => Promise.resolve(port) + : Reflect.get(target, method) + }) as never +} + +/** Makes one channel reject, the way a handler that throws does. */ +const rejectChannel = (method: string): void => { + const boundary = window.api as unknown as Record + window.api = new Proxy( + {}, + { + get: (_target, asked: string): unknown => { + if (asked === 'isServerWindow') return boundary[asked] + if (asked !== method) return boundary[asked] + return (): Promise => Promise.reject(new Error(`${method} threw`)) + } + } + ) as never +} + +/** Waits out `init`, which runs from module scope with nothing awaiting it. */ +const settle = async (): Promise => { + for (let i = 0; i < 20; i++) await Promise.resolve() +} + +beforeEach(() => { + vi.resetModules() + localStorage.clear() + stubRenderer() + localStorage.setItem(SERVER_ZUSTAND_STORAGE_KEY, persisted()) +}) + +describe('the flag the server view draws on', () => { + it('is set once init has run', async () => { + stubCreateServer(502) + const { useServerZustand } = await import('../server.zustand') + await settle() + + expect(useServerZustand.getState().initialized).toBe(true) + expect(useServerZustand.getState().ready[MAIN_SERVER_UUID]).toBe(true) + }) + + it('is set even when a register main refuses rejects the sync', async () => { + stubCreateServer(502) + rejectChannel('syncServerRegister') + const { useServerZustand } = await import('../server.zustand') + await settle() + + expect(useServerZustand.getState().initialized).toBe(true) + // The uuid is the one that is not ready, which is what the three setters + // refuse on, and it is the only thing the failure costs. + expect(useServerZustand.getState().ready[MAIN_SERVER_UUID]).toBe(false) + }) + + it('is set when creating the server fails outright', async () => { + rejectChannel('createServer') + const { useServerZustand } = await import('../server.zustand') + await settle() + + expect(useServerZustand.getState().initialized).toBe(true) + }) +}) + +// The `try` stood around the whole loop, so a refusal for the first uuid threw +// out of it and `createServer` was never called for the second: no listener on +// its port, none of its registers in main, and `ready` false with no message. +describe('one server main refuses', () => { + const SECOND = 'a-second-server' + + /** Records which uuids reached `createServer`, and refuses the first one. */ + const refuseTheFirstSync = (created: string[]): void => { + const boundary = window.api as unknown as Record + window.api = new Proxy( + {}, + { + get: (_target, method: string): unknown => { + if (method === 'isServerWindow') return boundary[method] + if (method === 'createServer') { + return (params: { uuid: string; port: number }): Promise => { + created.push(params.uuid) + return Promise.resolve(params.port) + } + } + if (method === 'setServerEndianness') { + return (params: { uuid: string }): Promise => + params.uuid === MAIN_SERVER_UUID + ? Promise.reject(new Error('setServerEndianness threw')) + : Promise.resolve() + } + return boundary[method] + } + } + ) as never + } + + it('costs that server and no other', async () => { + const blob = JSON.parse(persisted()) + blob.state.uuids.push(SECOND) + blob.state.port[SECOND] = '503' + blob.state.unitId[SECOND] = '0' + blob.state.littleEndian[SECOND] = false + blob.state.serverRegisters[SECOND] = {} + localStorage.setItem(SERVER_ZUSTAND_STORAGE_KEY, JSON.stringify(blob)) + + const created: string[] = [] + refuseTheFirstSync(created) + const { useServerZustand } = await import('../server.zustand') + await settle() + + expect(created).toEqual([MAIN_SERVER_UUID, SECOND]) + expect(useServerZustand.getState().ready[MAIN_SERVER_UUID]).toBe(false) + expect(useServerZustand.getState().ready[SECOND]).toBe(true) + expect(useServerZustand.getState().initialized).toBe(true) + }) +}) diff --git a/src/renderer/src/context/__tests__/stubRenderer.ts b/src/renderer/src/context/__tests__/stubRenderer.ts index 5a1ca32a..617b413b 100644 --- a/src/renderer/src/context/__tests__/stubRenderer.ts +++ b/src/renderer/src/context/__tests__/stubRenderer.ts @@ -62,7 +62,17 @@ export const fireEvent = (event: string, ...args: unknown[]): void => { for (const listener of listeners.get(event) ?? []) listener(...args) } -export const stubRenderer = (): void => { +/** + * Which window the stub answers as. + * + * `isServerWindow` is the flag three stores read at module scope to decide what + * they may do, so a test about the split out window has to be able to say so. + */ +export interface StubOptions { + isServerWindow?: boolean +} + +export const stubRenderer = ({ isServerWindow = false }: StubOptions = {}): void => { listeners.clear() const w = window as unknown as { electron: unknown; api: unknown } w.electron = { @@ -86,7 +96,7 @@ export const stubRenderer = (): void => { // and would make every window look like the server window. get: (_target, method: string): unknown => method === 'isServerWindow' - ? false + ? isServerWindow : (answers[method] ?? ((): Promise => Promise.resolve(undefined))) } ) diff --git a/src/renderer/src/context/client.zustand.ts b/src/renderer/src/context/client.zustand.ts index d2e88404..95f6c908 100644 --- a/src/renderer/src/context/client.zustand.ts +++ b/src/renderer/src/context/client.zustand.ts @@ -15,7 +15,7 @@ import { migrateClientState, carryFormerClientState, CLIENT_ZUSTAND_STORAGE_KEY, - RegisterMapping + emptyRegisterMapping } from '@shared' import { useDataZustand } from './data.zustand' import { loadSerialPorts } from './serialPorts' @@ -52,14 +52,6 @@ export const flushRegisterMappingToMain = (): void => { window.api.setRegisterMapping(useClientZustand.getState().registerMapping) } -/** A mapping with nothing configured, one empty record per register type. */ -const emptyRegisterMapping = (): RegisterMapping => ({ - coils: {}, - discrete_inputs: {}, - holding_registers: {}, - input_registers: {} -}) - /** * Drop the rows on screen, unless something is about to replace them. * @@ -528,6 +520,25 @@ export const useClientZustand = create< const clientZustand = useClientZustand.getState() +/** + * The window this module may call main from, the question its two siblings ask. + * + * `App.tsx` imports `containers/Client` statically and `Client.tsx:11` imports + * this file, so `out/renderer/assets/` holds one js file and both windows + * evaluate this module scope. Without the guard, opening the split out server + * window ran `init`, which hands main the config this window loaded and calls + * `setReadConfiguration(false)`. `modbusClient.ts:586` reads that flag, and off + * it polls one flat `[address, length]` block instead of the configured groups, + * while the main window's toggle still reads on. `init`'s own `set` writes + * `CLIENT_ZUSTAND_STORAGE_KEY` as well, because persist wraps `setState`, so + * the split window overwrote the shared key on every open. + * + * The version fetch below stays outside it. `get_app_version` asks `app` rather + * than the client, and `containers/Home.tsx` is the only reader of what it + * answers, which `App.tsx` does not draw in this window. + */ +const isServerWindow = window.api.isServerWindow + // Keep the fields that parsed and default the rest, then say which went. const repair = repairPersistedStore(useClientZustand, PersistedClientZustandSchema, { storageKey: CLIENT_ZUSTAND_STORAGE_KEY, @@ -538,7 +549,7 @@ const repair = repairPersistedStore(useClientZustand, PersistedClientZustandSche if (repair) useClientZustand.setState({ ...repair.state, configReset: repair.reset }) // Sync the main process state with the front end -clientZustand.init() +if (!isServerWindow) clientZustand.init() // // @@ -575,7 +586,7 @@ onEvent('scan_progress', (scanProgress) => { // // Stop scanning when reloaded, shouldn't be a problem with the build app, // but just in case and for development, stop scanning when the frontend is reloaded -window.api.stopScanningUnitIds() +if (!isServerWindow) window.api.stopScanningUnitIds() window.api.getAppVersion().then((version) => { useLayoutZustand.getState().setVersion(version) diff --git a/src/renderer/src/context/server.zustand.ts b/src/renderer/src/context/server.zustand.ts index 3580373f..c15abe42 100644 --- a/src/renderer/src/context/server.zustand.ts +++ b/src/renderer/src/context/server.zustand.ts @@ -22,7 +22,10 @@ import { registerWidth, ModbusBaudRate, RegisterType, - RegisterValue + RegisterValue, + ServerRegisterValue, + holdsExact64Bits, + toExact64Bits } from '@shared' import { onEvent } from '@renderer/events' import { round } from 'lodash' @@ -61,6 +64,7 @@ export const useServerZustand = create< state.configReset = undefined }), ready: { [MAIN_SERVER_UUID]: false }, + initialized: false, selectedUuid: MAIN_SERVER_UUID, uuids: [MAIN_SERVER_UUID], port: { [MAIN_SERVER_UUID]: '502' }, @@ -139,6 +143,17 @@ export const useServerZustand = create< await window.api.resetServer(uuid) get().clean(uuid) }, + /** + * Hands main what the store holds, one server at a time, and marks the + * store initialized once it has been through them all. + * + * `containers/Server.tsx` draws nothing until the flag is set, so a + * rejected invoke used to cost the whole server view rather than the one + * uuid it belonged to, on that launch and on every one after it. The uuid + * keeps `ready` false, which is what its three setters refuse on, and + * that is the whole cost now. `openServer` is where the catch sits, so + * the servers after it still get opened. + */ init: async (uuid) => { set((state) => { if (uuid) state.ready[uuid] = false @@ -159,6 +174,37 @@ export const useServerZustand = create< } }) + /** + * Opens one server and hands main what it holds, and answers whether + * that got through. + * + * The `catch` is per uuid, because that is the unit of the cost: it + * stood around the loop, and a refusal for the first uuid then meant + * `createServer` was never called for the second at all, so it had no + * listener on its port, none of its registers in main, and `ready` + * false with no message. + * + * Empty, because this runs from module scope with nothing awaiting it + * and a rejection there is an unhandled one. Main reports its own + * failures through `backend_message`; a rejected invoke carries the + * channel name and nothing the user can act on. `console.error` is what + * says the other kind happened, a throw out of the store's own recipes. + */ + const openServer = async (syncUuid: string, port: number): Promise => { + try { + const actualPort = await window.api.createServer({ uuid: syncUuid, port }) + if (actualPort === undefined) return + + set((state) => { + state.port[syncUuid] = String(actualPort) + }) + + await syncUuidToBackend(set, get, syncUuid) + } catch (error) { + console.error(`Server ${syncUuid} was not opened:`, error) + } + } + if (mode === 'rtu') { // RTU serves the main server's registers, so that is the only uuid // there is anything to open for. @@ -173,7 +219,11 @@ export const useServerZustand = create< } } - await syncUuidToBackend(set, get, MAIN_SERVER_UUID) + try { + await syncUuidToBackend(set, get, MAIN_SERVER_UUID) + } catch (error) { + console.error(`Server ${MAIN_SERVER_UUID} was not opened:`, error) + } set((state) => { state.selectedUuid = MAIN_SERVER_UUID @@ -183,15 +233,7 @@ export const useServerZustand = create< const uuidsToSync = uuid ? [uuid] : state.uuids for (const syncUuid of uuidsToSync) { - const port = Number(state.port[syncUuid]) - const actualPort = await window.api.createServer({ uuid: syncUuid, port }) - if (actualPort === undefined) continue - - set((state) => { - state.port[syncUuid] = String(actualPort) - }) - - await syncUuidToBackend(set, get, syncUuid) + await openServer(syncUuid, Number(state.port[syncUuid])) } if (state.uuids.length === 0) { @@ -203,6 +245,10 @@ export const useServerZustand = create< } get().cleanOrphanedServerState() + + set((state) => { + state.initialized = true + }) }, setSelectedUuid: (uuid) => set((state) => { @@ -658,13 +704,24 @@ export const applyRegisterValue = (payload: RegisterValue): void => { case 'float': view.setFloat32(0, Number(currentValue) || 0, littleEndian) break - case 'int64': - view.setBigInt64(0, BigInt(currentValue) || 0n, littleEndian) + // `undefined` is a stored value no composite can be read out of, which a + // hand-edited config reaches because `ServerRegisterEntrySchema` takes a + // fractional number for these types. Aborting leaves the entry alone; + // merging the one word into a composite of zero would cost the other + // three registers. + case 'int64': { + const composite = toExact64Bits(currentValue) + if (composite === undefined) return + view.setBigInt64(0, composite, littleEndian) break + } case 'uint64': - case 'datetime': - view.setBigUint64(0, BigInt(currentValue) || 0n, littleEndian) + case 'datetime': { + const composite = toExact64Bits(currentValue) + if (composite === undefined) return + view.setBigUint64(0, composite, littleEndian) break + } case 'double': view.setFloat64(0, Number(currentValue) || 0, littleEndian) break @@ -723,7 +780,17 @@ export const applyRegisterValue = (payload: RegisterValue): void => { return } - const value = round(Number(newComposite), ['float', 'double'].includes(dataType) ? 3 : 0) + // A decimal string where the composite fills 64 bits as an integer, because + // `Number` carries 53 of them: four words of 0xFFFF came out + // 18446744073709552000 rather than 18446744073709551615, and the next single + // word write read that back through `BigInt`, which `setBigUint64` took + // modulo 2 ** 64, leaving the entry holding the low word alone. + // + // The cache above held the exact composite already, so the loss only showed + // after a flush cleared it and the fallback was the entry. + const value: ServerRegisterValue = holdsExact64Bits(dataType) + ? newComposite.toString() + : round(Number(newComposite), ['float', 'double'].includes(dataType) ? 3 : 0) delayedRegister.setValue(cacheKey, newComposite) @@ -763,6 +830,30 @@ readRtuServerStatus() /** Whether a split out server window has been the one writing the key. */ let serverWindowOwnsTheKey = false +/** + * Mark the uuids the rehydrate introduced as ones main knows. + * + * `ready` is not in `partialize`, so a rehydrate brings back `uuids`, `port` + * and `selectedUuid` and leaves this window's own `ready` where `init` left it. + * A server made in the split out window therefore came back with no entry, and + * `setPort`, `setUnitId` and `setLittleEndian` each refuse on that with no + * message, until a restart. The window that made it ran `createServer` for it, + * so main does know it. + * + * Only where the entry is missing. A uuid this window's own `init` wrote + * `false` for is one main refused, and writing `true` over that would hand the + * three setters back for a server main has no listener for, and flip the flag + * `PrivilegedPortModal` runs its check on. + */ +const markRehydratedUuidsReady = (): void => { + const { uuids, ready } = useServerZustand.getState() + const introduced = uuids.filter((uuid) => ready[uuid] === undefined) + if (introduced.length === 0) return + useServerZustand.setState({ + ready: { ...ready, ...Object.fromEntries(introduced.map((uuid) => [uuid, true])) } + }) +} + /** * Re-read the key the split out window has been writing. * @@ -781,6 +872,11 @@ onEvent('window_update', ({ server }) => { } if (!serverWindowOwnsTheKey) return serverWindowOwnsTheKey = false - useServerZustand.persist.rehydrate() + void Promise.resolve(useServerZustand.persist.rehydrate()) + .then(markRehydratedUuidsReady) + // `rehydrate` reads the key and runs `migrateServerState` over what it + // finds, and either can throw on a blob a hand edit left there. Unhandled, + // that is the window's only sign that the re-read did not happen. + .catch((error) => console.error('Re-reading the server store failed:', error)) readRtuServerStatus() }) diff --git a/src/renderer/src/context/server.zustand.types.ts b/src/renderer/src/context/server.zustand.types.ts index 5acd5170..739f907b 100644 --- a/src/renderer/src/context/server.zustand.types.ts +++ b/src/renderer/src/context/server.zustand.types.ts @@ -16,7 +16,8 @@ import { Parity, ConfigReset, DataBits, - StopBits + StopBits, + ServerRegisterValue } from '@shared' import { AsyncMaskSetFn, MaskSetFn } from './client.zustand.types' import { z } from 'zod' @@ -53,7 +54,7 @@ export interface SetBoolParameters { export interface SetRegisterValueParameters { registerType: NumberRegisters address: number - value: number + value: ServerRegisterValue optionalUuid?: string optionalUnitId?: UnitIdString } @@ -63,7 +64,28 @@ export type ServerZustand = { configReset: ConfigReset | undefined /** Called once the reset has been reported, so it is reported once. */ acknowledgeConfigReset: () => void + /** + * Whether main knows this uuid, which is what `setPort`, `setUnitId` and + * `setLittleEndian` refuse on: a port main has not bound is a port nothing + * can be changed about. + * + * Written by `syncUuidToBackend`, by `createServer`, and by the + * `window_update` rehydrate for a uuid the split out window made and synced + * itself. `init` sets every uuid false before its loop, so a sync that fails + * leaves that one uuid refusing its three setters and no other. + */ ready: { [uuid: string]: boolean } + /** + * Whether `init` has run to its end, whatever it managed. + * + * `containers/Server.tsx` fades the whole server view in on this. It used to + * read `ready`, which is per uuid and written only on a sync that got + * through, so a rejected invoke anywhere in `init`'s loop left the view blank + * on that launch and on every one after it, with nothing on screen to clear + * the config with. The two questions are separate: this one is about the + * store, `ready` is about a server. + */ + initialized: boolean clean: (uuid: string) => void /** * Remove all state entries for uuids that are not present in the uuids array. diff --git a/src/shared/__tests__/conversion.test.ts b/src/shared/__tests__/conversion.test.ts index 76f91be8..b028384a 100644 --- a/src/shared/__tests__/conversion.test.ts +++ b/src/shared/__tests__/conversion.test.ts @@ -142,7 +142,10 @@ describe('parseIEC870DateTimeValue', () => { words.forEach((word, i) => buf.writeUInt16BE(word, i * 2)) return buf } - const composite = words.reduce((total, word, i) => total + word * 2 ** (48 - i * 16), 0) + const composite = words.reduce( + (total, word, i) => total + BigInt(word) * 2n ** BigInt(48 - i * 16), + 0n + ) it('reads the same date out of the composite as out of the buffer', () => { expect(parseIEC870DateTimeValue(composite)).toBe(parseIEC870DateTime(buffer())) @@ -150,14 +153,31 @@ describe('parseIEC870DateTimeValue', () => { }) it('returns empty string for a composite of zero', () => { - expect(parseIEC870DateTimeValue(0)).toBe('') + expect(parseIEC870DateTimeValue(0n)).toBe('') }) - // The all-0xFFFF sentinel is 2 ** 64 - 1, which no Number holds: the store - // narrows the composite with Number() and it arrives here as 2 ** 64. The - // sentinel branch is unreachable from this side, and the range gate answers. - it('returns empty string for the sentinel the store rounded past 64 bits', () => { - expect(parseIEC870DateTimeValue(2 ** 64)).toBe('') + // The all-0xFFFF sentinel is 2 ** 64 - 1, which no Number holds. The store + // kept the composite as one and it arrived here as 2 ** 64, so the sentinel + // branch was unreachable from this side and the range gate answered instead. + // A bigint reaches it. + it('reads the sentinel as the sentinel', () => { + expect(parseIEC870DateTimeValue(2n ** 64n - 1n)).toBe('') + }) + + // The end of the window the format carries, which is what rounded up by one + // millisecond through `Number`. + it('reads the last millisecond the format holds', () => { + const buf = Buffer.alloc(8) + buf.writeUInt16BE(99, 0) + buf.writeUInt16BE((12 << 8) | 31, 2) + buf.writeUInt16BE((23 << 8) | 59, 4) + buf.writeUInt16BE(59999, 6) + const exact = buf.readBigUInt64BE(0) + + expect(parseIEC870DateTimeValue(exact)).toBe(parseIEC870DateTime(buf)) + expect(parseIEC870DateTimeValue(BigInt(Number(exact)))).not.toBe( + parseIEC870DateTimeValue(exact) + ) }) }) diff --git a/src/shared/__tests__/migrations.test.ts b/src/shared/__tests__/migrations.test.ts index cc14ca49..9e5459e2 100644 --- a/src/shared/__tests__/migrations.test.ts +++ b/src/shared/__tests__/migrations.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest' -import { migrateServerConfig } from '../migrations/server/config' +import { CURRENT_SERVER_CONFIG_VERSION, migrateServerConfig } from '../migrations/server/config' import { migrateClientConfig } from '../migrations/client/config' +import { resetMessage } from '../repairPersisted' import { migrateServerRegistersState, migrateBoolShape } from '../migrations/server/zustand' import { readFileSync } from 'fs' import { join } from 'path' @@ -19,7 +20,7 @@ describe('configMigration', () => { expect(result.migrated).toBe(true) expect(result.fromVersion).toBe(1) - expect(result.config.version).toBe(2) + expect(result.config.version).toBe(CURRENT_SERVER_CONFIG_VERSION) expect(result.config.littleEndian).toBe(false) expect(result.config.name).toBe('Test Server Big Endian') expect(result.warning).toBeUndefined() @@ -136,34 +137,193 @@ describe('configMigration', () => { }) }) - describe('v2 pass-through', () => { - it('does not migrate v2 config (pass-through)', () => { + // The v2 fixture is a migration now: a 64 bit value is a decimal string, + // and a file this build writes has to say so, or v2.3.0 takes the + // `detectedVersion === CURRENT` branch and refuses the whole file on + // `z.number()` rather than the one register. + describe('v2 to v3 migration', () => { + it('migrates a v2 config to the current version', () => { const v2Config = loadFixture('server-config-v2-current.json') const result = migrateServerConfig(v2Config) - expect(result.migrated).toBe(false) + expect(result.migrated).toBe(true) expect(result.fromVersion).toBe(2) - expect(result.config.version).toBe(2) + expect(result.config.version).toBe(CURRENT_SERVER_CONFIG_VERSION) expect(result.config.littleEndian).toBe(false) expect(result.warning).toBeUndefined() }) + + // The persisted store's own step is gated on the store version and does + // not run on a config load, so without this the first single word write + // after opening a v3 file folds from the rounded value. + it('rewrites a 64 bit value a file of the current version holds as a number', () => { + const result = migrateServerConfig( + JSON.stringify({ + version: CURRENT_SERVER_CONFIG_VERSION, + modbuxVersion: '3.0.0', + name: 'hand edited', + littleEndian: false, + serverRegistersPerUnit: { + '1': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': { + value: 72623859790382850, + params: { + address: 0, + registerType: 'holding_registers', + dataType: 'uint64', + comment: '', + value: 0 + } + } + } + } + } + }) + ) + + expect(result.migrated).toBe(false) + expect(result.config.serverRegistersPerUnit['1']?.holding_registers['0']?.value).toBe( + '72623859790382850' + ) + }) + + it('rewrites a 64 bit value a v2 file holds as a number', () => { + const register = (dataType: string, value: number): unknown => ({ + value, + params: { address: 0, registerType: 'holding_registers', dataType, comment: '', value: 0 } + }) + const result = migrateServerConfig( + JSON.stringify({ + version: 2, + modbuxVersion: '2.3.0', + name: 'v2', + littleEndian: false, + serverRegistersPerUnit: { + '1': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': register('uint64', 72623859790382850), + '10': register('uint16', 7) + } + } + } + }) + ) + + const holding = result.config.serverRegistersPerUnit['1']?.holding_registers + expect(holding?.['0']?.value).toBe('72623859790382850') + expect(holding?.['10']?.value).toBe(7) + }) }) describe('Future version handling', () => { - it('handles future version (v3) with warning', () => { - const v3Config = JSON.stringify({ - version: 3, - modbuxVersion: '2.0.0', + it('handles a future version with warning', () => { + const futureConfig = JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', name: 'Future', littleEndian: false, newField: 'something', serverRegistersPerUnit: {} }) - const result = migrateServerConfig(v3Config) + const result = migrateServerConfig(futureConfig) expect(result.migrated).toBe(false) - expect(result.fromVersion).toBe(3) - expect(result.warning).toBe('FUTURE_VERSION') + expect(result.fromVersion).toBe(9) + expect(result.futureVersion).toBeDefined() + expect(result.futureVersion?.savedByNewerVersion).toBe(true) + expect(result.futureVersion?.fields).toEqual([]) + }) + + // The branch cast the parsed JSON straight to `ServerConfig` and returned + // it, so this was the one door into the app no schema stood in, and the + // snackbar read as a compatibility notice rather than "this was not + // checked". Both fixtures the suite had were well formed, so it was green + // on the half that works. + it('keeps the fields a future config still shares and names the rest', () => { + const result = migrateServerConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + name: 'Future', + littleEndian: true, + serverRegistersPerUnit: 'not a unit' + }) + ) + + expect(result.futureVersion).toBeDefined() + expect(result.config.name).toBe('Future') + expect(result.config.littleEndian).toBe(true) + expect(result.config.serverRegistersPerUnit).toEqual({}) + expect(result.futureVersion?.fields).toEqual(['serverRegistersPerUnit']) + }) + + // `repairPersisted` reads a field whole, and `serverRegistersPerUnit` is + // the one field a server config is about. `DataTypeSchema` is a closed + // enum, so one register with a type this version does not name failed the + // field and cost every register on every unit, after `useOpen` had + // already emptied the store and main. That is the cost + // `dropUnservableRegisters` exists to avoid on the persisted side. + it('keeps the registers a future config shares and drops the one it does not', () => { + const register = (address: number, dataType: string): unknown => ({ + value: 1, + params: { + address, + registerType: 'holding_registers', + dataType, + comment: '', + value: 1 + } + }) + const result = migrateServerConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + name: 'Future', + littleEndian: false, + serverRegistersPerUnit: { + '1': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': register(0, 'uint16'), + '10': register(10, 'a_type_from_3_0'), + '20': register(20, 'int32') + } + } + } + }) + ) + + expect( + Object.keys(result.config.serverRegistersPerUnit['1']?.holding_registers ?? {}) + ).toEqual(['0', '20']) + expect(result.futureVersion?.fields).toEqual([]) + }) + + it('says so in a sentence naming the field', () => { + const result = migrateServerConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + name: 'Future', + littleEndian: false, + serverRegistersPerUnit: 'not a unit' + }) + ) + const reset = result.futureVersion + if (!reset) throw new Error('a future config always answers a reset') + + expect(resetMessage('Server', reset)).toBe( + 'Server configuration was saved by a newer version of Modbux, and the registers did not come across. Everything else was kept.' + ) }) }) @@ -197,6 +357,45 @@ describe('configMigration', () => { }) }) + // `parsed.version ?? 1` on any of these is `undefined`, so 1, so the v1 to + // v2 step ran, and every field the migration needs sits behind a `??`. Each + // came out a valid empty v2 config, reported as updated from an older + // format, and `useOpen` had already emptied the server by then. + describe('A file that is not a configuration', () => { + it.each(['5', '[]', '"hello"', 'true', 'null'])('refuses %s', (raw) => { + expect(() => migrateServerConfig(raw)).toThrow( + 'This file does not hold a Modbux configuration' + ) + expect(() => migrateClientConfig(raw)).toThrow( + 'This file does not hold a Modbux configuration' + ) + }) + + // `JSON.parse` answers `any`, so nothing asked what was in this field, + // and a relational operator coerces: `"3" > 2` and `2.5 > 2` are both + // true, so those two took the future version branch and came back + // `migrated: false` with the newer-version warning, while `true` and + // `null` compared false against both and took the v1 path. + it.each(['"3"', 'true', '2.5', 'null'])('refuses a version of %s', (version) => { + const raw = `{"version": ${version}, "serverRegistersPerUnit": {}}` + expect(() => migrateServerConfig(raw)).toThrow('which is not a version') + }) + + it.each(['"3"', 'true', '2.5', 'null'])( + 'refuses a client config claiming version %s', + (version) => { + const raw = `{"version": ${version}, "registerMapping": {}}` + expect(() => migrateClientConfig(raw)).toThrow('which is not a version') + } + ) + + it('still takes a file with no version at all', () => { + const result = migrateServerConfig(JSON.stringify({ name: 'v1', coils: {} })) + expect(result.fromVersion).toBe(1) + expect(result.migrated).toBe(true) + }) + }) + // A refused file is refused whole, so the message is the only thing that // says which register to go and fix. describe('What a refused config names', () => { @@ -360,7 +559,59 @@ describe('configMigration', () => { const result = migrateClientConfig(v3Config) expect(result.fromVersion).toBe(3) - expect(result.warning).toBe('FUTURE_VERSION') + expect(result.futureVersion).toBeDefined() + }) + + // `LoadButton` calls `replaceRegisterMapping(config.registerMapping)` + // with no gate at all, so `'nope'` went into the persisted store and was + // flushed to main. + // `registerMapping` is one field too, and it is the one thing in the + // client store built by hand, so one address this version cannot read + // cost the whole mapping. `LoadButton` then flushed that nothing to main. + it('keeps the mapping entries a future config shares and drops the one it does not', () => { + const result = migrateClientConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + littleEndian: false, + registerMapping: { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: { + '0': { dataType: 'uint16', comment: 'kept' }, + '10': { dataType: 'a_type_from_3_0' }, + '20': { dataType: 'int32' } + } + } + }) + ) + + expect(Object.keys(result.config.registerMapping.holding_registers)).toEqual(['0', '20']) + expect(result.futureVersion?.fields).toEqual([]) + }) + + it('keeps the fields a future config still shares and names the rest', () => { + const result = migrateClientConfig( + JSON.stringify({ + version: 9, + modbuxVersion: '9.0.0', + name: 'Future', + littleEndian: true, + registerMapping: 'nope' + }) + ) + + expect(result.futureVersion).toBeDefined() + expect(result.config.name).toBe('Future') + expect(result.config.littleEndian).toBe(true) + expect(result.config.registerMapping).toEqual({ + coils: {}, + discrete_inputs: {}, + holding_registers: {}, + input_registers: {} + }) + expect(result.futureVersion?.fields).toEqual(['registerMapping']) }) }) }) diff --git a/src/shared/__tests__/schemas.test.ts b/src/shared/__tests__/schemas.test.ts index 9c26639a..410be37c 100644 --- a/src/shared/__tests__/schemas.test.ts +++ b/src/shared/__tests__/schemas.test.ts @@ -6,8 +6,12 @@ import { } from '../types/client' import { defaultConnectionConfig, defaultRegisterConfig } from '../default' import { BitColorSchema, BitMapEntrySchema, BitMapConfigSchema } from '../types/bitmap' -import { RegisterParamsSchema } from '../types/server' +import { RegisterParamsSchema, RemoveRegisterParamsSchema } from '../types/server' import { DataBitsSchema, SerialPortOptionsSchema, StopBitsSchema } from '../types/serial' +import { MAX_UTF8_LENGTH } from '../utils' +import { ScanUnitIDParametersSchema } from '../types/scan' +import { MAX_READ_BITS, MAX_READ_REGISTERS, maxReadQuantity } from '../types/ranges' +import type { RegisterType } from '../types/register' describe('RegisterMapObjectSchema', () => { it('accepts numeric string keys', () => { @@ -309,10 +313,18 @@ describe('Server RegisterParamsSchema — generator', () => { expect(RegisterParamsSchema.safeParse({ ...generator, min: 0, max: 0 }).success).toBe(true) }) - it.each([0, -5, 500, 1000.5])('refuses interval %s', (interval) => { + // Above 2147483647 `setInterval` warns `TimeoutOverflowWarning` and sets the + // duration to 1, so 1e12 fires every millisecond: the ceiling is Node's. + it.each([0, -5, 500, 1000.5, 2147483648, 1e12])('refuses interval %s', (interval) => { expect(RegisterParamsSchema.safeParse({ ...generator, interval }).success).toBe(false) }) + it('accepts the highest interval setInterval holds', () => { + expect(RegisterParamsSchema.safeParse({ ...generator, interval: 2147483647 }).success).toBe( + true + ) + }) + // The dialog lets one through and the generator covers the same range either // way, so a rule here would refuse what the Add button sends. it('accepts a min above its max', () => { @@ -333,6 +345,219 @@ describe('Server RegisterParamsSchema — generator', () => { }) }) +// `RegisterParamsSchema` bounded the address and left `length` and `value` +// bare, so a config file reached the code that cannot take them: +// `createStringRegisters` is `Buffer.alloc(length * 2)`, and `addRegister`'s +// fixed branch hands the value straight to `createRegisters`. +describe('Server RegisterParamsSchema — the width and the value', () => { + const fixed = (params: Record): unknown => ({ + address: 0, + registerType: 'holding_registers', + comment: '', + ...params + }) + + it.each([0, -1, 10.5, 1e9, 1e12])('refuses a string of length %s', (length) => { + expect( + RegisterParamsSchema.safeParse( + fixed({ dataType: 'utf8', length, stringValue: 'x', value: 0 }) + ).success + ).toBe(false) + }) + + it('accepts the widest string the dialog offers', () => { + expect( + RegisterParamsSchema.safeParse( + fixed({ dataType: 'utf8', length: MAX_UTF8_LENGTH, stringValue: 'x', value: 0 }) + ).success + ).toBe(true) + }) + + it('refuses a register that runs past address 65535', () => { + expect( + RegisterParamsSchema.safeParse(fixed({ address: 65534, dataType: 'uint64', value: 1 })) + .success + ).toBe(false) + expect( + RegisterParamsSchema.safeParse(fixed({ address: 65532, dataType: 'uint64', value: 1 })) + .success + ).toBe(true) + }) + + // `writeUInt16BE(70000)`, `writeInt16BE(40000)` and `BigInt(1.5)` all throw, + // and each of the three passed this schema. + it.each([ + ['uint16', 70000], + ['uint16', -1], + ['int16', 40000], + ['int32', 2147483648], + ['uint32', -1], + ['int64', 1.5], + ['uint64', -1], + ['bitmap', 65536] + ])('refuses %s value %s', (dataType, value) => { + expect(RegisterParamsSchema.safeParse(fixed({ dataType, value })).success).toBe(false) + }) + + // The encoder takes these, so a rule refusing them would cost a working + // config. `float` and `double` write anything, `unix` goes through + // `value >>> 0` and `datetime` through the clamp in `encodeIEC870DateTime`. + it.each([ + ['float', 1e300], + ['double', -1e300], + ['unix', -1], + ['datetime', 0], + ['uint16', 65535], + ['int16', -32768] + ])('accepts %s value %s', (dataType, value) => { + expect(RegisterParamsSchema.safeParse(fixed({ dataType, value })).success).toBe(true) + }) + + it('names the field it refused', () => { + const result = RegisterParamsSchema.safeParse(fixed({ dataType: 'uint16', value: 70000 })) + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.path).toEqual(['value']) + }) + + // `registerWidth` reads `length` for `utf8` alone, so naming it for a + // `uint64` that still carries one from an earlier edit points at a field with + // no bearing on the width. + it('names the address when the width is not the length', () => { + const result = RegisterParamsSchema.safeParse( + fixed({ address: 65534, dataType: 'uint64', value: 1, length: 4 }) + ) + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.path).toEqual(['address']) + }) + + // `removeRegister` loops `registerWidth(dataType, length)` times writing into + // the register array, so the remove channel needs the bound add and sync have. + it('holds a string to the same width on add, sync and remove', () => { + const tooWide = { dataType: 'utf8', length: 125, stringValue: 'x', value: 0 } + expect(RegisterParamsSchema.safeParse(fixed(tooWide)).success).toBe(false) + expect( + RemoveRegisterParamsSchema.safeParse({ + uuid: 'u', + unitId: '1', + registerType: 'holding_registers', + address: 0, + dataType: 'utf8', + length: 125 + }).success + ).toBe(false) + expect( + RegisterParamsSchema.safeParse(fixed({ ...tooWide, length: MAX_UTF8_LENGTH })).success + ).toBe(true) + }) +}) + +// The generator draws between `min` and `max` and hands the draw to +// `createRegisters`, inside an `async` tick nobody awaits, so a range the data +// type cannot take answers 0 for as long as the generator runs and warns once +// per interval. The dialog masks both fields to `getMinMaxValues(dataType)` +// already, so the rule refuses nothing the Add button sends. +describe('Server RegisterParamsSchema — what a generator draws between', () => { + const generator = (overrides: Record): unknown => ({ + address: 10, + registerType: 'holding_registers', + dataType: 'uint16', + comment: '', + min: 0, + max: 100, + interval: 1000, + ...overrides + }) + + it.each([ + ['max', 1e9], + ['max', 65536], + ['min', -1] + ])('refuses %s of %s on a uint16', (field, bound) => { + const result = RegisterParamsSchema.safeParse(generator({ [field]: bound })) + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.path).toEqual([field]) + }) + + it('takes a range the data type holds', () => { + expect(RegisterParamsSchema.safeParse(generator({ min: 0, max: 65535 })).success).toBe(true) + expect( + RegisterParamsSchema.safeParse(generator({ dataType: 'uint32', max: 1e9 })).success + ).toBe(true) + }) + + // `float` and `double` write anything, so the pair stays open there. + it('leaves a float generator alone', () => { + expect( + RegisterParamsSchema.safeParse(generator({ dataType: 'float', min: -1e300, max: 1e300 })) + .success + ).toBe(true) + }) +}) + +// Nothing between the keyboard and the wire clamped: `LengthField` passed no +// `max`, `UintInput` defaults to 65535, `setLength` stores the number as given, +// `length` was `z.number().int().positive()`, and `_scanUnitIds` hands it to +// `this._readers[registerType](address, length)`, which writes it into the +// quantity field. A device answers illegal-data-value or says nothing, and the +// answer the user reads is about the request rather than about the bus. +describe('ScanUnitIDParametersSchema — the quantity that reaches the wire', () => { + const parameters = (over: Record): unknown => ({ + range: [1, 5], + address: 0, + length: 2, + registerTypes: ['holding_registers'], + timeout: 500, + ...over + }) + + const takes = (over: Record): boolean => + ScanUnitIDParametersSchema.safeParse(parameters(over)).success + + it('refuses more registers than one read answers', () => { + expect(takes({ length: 126 })).toBe(false) + expect(takes({ length: 65535 })).toBe(false) + expect(takes({ length: MAX_READ_REGISTERS })).toBe(true) + }) + + it('refuses more bits than one read answers', () => { + expect(takes({ registerTypes: ['coils'], length: 2001 })).toBe(false) + expect(takes({ registerTypes: ['coils'], length: MAX_READ_BITS })).toBe(true) + }) + + // One length goes out for every type selected, so the strictest of them is + // the one the request has to fit. + it('holds a mixed selection to the stricter of the two', () => { + expect(takes({ registerTypes: ['coils', 'holding_registers'], length: 2000 })).toBe(false) + expect(takes({ registerTypes: ['coils', 'holding_registers'], length: 125 })).toBe(true) + }) + + it('names the length', () => { + const result = ScanUnitIDParametersSchema.safeParse(parameters({ length: 65535 })) + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.path).toEqual(['length']) + }) +}) + +describe('maxReadQuantity', () => { + // MODBUS Application Protocol Specification V1.1b3 section 6: FC01 and FC02 + // answer at most 2000 bits, FC03 and FC04 at most 125 registers. + it.each([ + [['coils'], MAX_READ_BITS], + [['discrete_inputs'], MAX_READ_BITS], + [['coils', 'discrete_inputs'], MAX_READ_BITS], + [['input_registers'], MAX_READ_REGISTERS], + [['holding_registers'], MAX_READ_REGISTERS], + [['coils', 'holding_registers'], MAX_READ_REGISTERS] + ] as [RegisterType[], number][])('answers %j with %i', (registerTypes, expected) => { + expect(maxReadQuantity(registerTypes)).toBe(expected) + }) + + it('states the protocol rather than a habit', () => { + expect(MAX_READ_BITS).toBe(2000) + expect(MAX_READ_REGISTERS).toBe(125) + }) +}) + describe('SerialPortOptionsSchema', () => { const withOptions = (options: Record): unknown => ({ baudRate: '9600', diff --git a/src/shared/conversion.ts b/src/shared/conversion.ts index 4f772dc0..cb28076c 100644 --- a/src/shared/conversion.ts +++ b/src/shared/conversion.ts @@ -70,20 +70,22 @@ export const parseIEC870DateTime = (buf: Buffer): string => { } /** - * The same datetime, read off the composite number the server store keeps. + * The same datetime, read off the composite the server store keeps. * * The store folds the four registers into one `value`, so the row had its own * copy of the layout above, without the invalid flag and without the range gate. - * The number carries all 64 bits up to `Number.MAX_SAFE_INTEGER` and no further: - * the encoding of 2099/12/31 23:59:59.999 rounds up by one millisecond on the - * way through `Number`, and the year offset is what pushes it that far. + * + * A `bigint` because the composite fills all 64 bits and a JS number carries 53 + * of them. Through `Number` the encoding of 2099/12/31 23:59:59.999 rounded up + * by one millisecond, and the all-0xFFFF sentinel came out as 2 ** 64, which is + * out of range rather than the sentinel. */ -export const parseIEC870DateTimeValue = (packed: number): string => +export const parseIEC870DateTimeValue = (packed: bigint): string => parseIEC870Words( - Math.floor(packed / 2 ** 48) & 0xffff, - Math.floor(packed / 2 ** 32) & 0xffff, - Math.floor(packed / 2 ** 16) & 0xffff, - packed & 0xffff + Number((packed >> 48n) & 0xffffn), + Number((packed >> 32n) & 0xffffn), + Number((packed >> 16n) & 0xffffn), + Number(packed & 0xffffn) ) /** The `yyyy/MM/dd HH:mm:ss` a unix register's seconds stand for. */ diff --git a/src/shared/default.ts b/src/shared/default.ts index 6dc5ec6e..095c33ae 100644 --- a/src/shared/default.ts +++ b/src/shared/default.ts @@ -4,6 +4,7 @@ import { RegisterConfig, RegisterData, RegisterDataWords, + RegisterMapping, SerialPortOptions } from './types' @@ -46,6 +47,20 @@ export const defaultRegisterConfig: RegisterConfig = { addressBase: '0' } +/** + * A mapping with nothing configured, one empty record per register type. + * + * Built fresh at each call, because the store writes into it. Two callers: the + * client store's Clear, and the repair a config from a newer Modbux goes + * through, which needs something to default the field to when it fails. + */ +export const emptyRegisterMapping = (): RegisterMapping => ({ + coils: {}, + discrete_inputs: {}, + holding_registers: {}, + input_registers: {} +}) + export const defaultClientState: ClientState = { connectState: 'disconnected', polling: false, diff --git a/src/shared/migrations/__tests__/legacyRegisterTypeKeys.test.ts b/src/shared/migrations/__tests__/legacyRegisterTypeKeys.test.ts index 9e879247..423849e4 100644 --- a/src/shared/migrations/__tests__/legacyRegisterTypeKeys.test.ts +++ b/src/shared/migrations/__tests__/legacyRegisterTypeKeys.test.ts @@ -53,7 +53,6 @@ describe('the text a config carries', () => { it.each(legacyNames)('keeps a server config named after %s', (name) => { const result = migrateServerConfig(serverConfigNamed(`${name} bank A`)) - expect(result.migrated).toBe(false) expect(result.config.name).toBe(`${name} bank A`) }) diff --git a/src/shared/migrations/__tests__/registerAddress.test.ts b/src/shared/migrations/__tests__/registerAddress.test.ts index bc68ccda..e566b6a1 100644 --- a/src/shared/migrations/__tests__/registerAddress.test.ts +++ b/src/shared/migrations/__tests__/registerAddress.test.ts @@ -390,6 +390,154 @@ describe('the used addresses the drop writes back', () => { }) }) +describe('a persisted register the encoder cannot serve', () => { + /** A fixed register at `address`, with `params` over the defaults. */ + const register = ( + address: number, + overrides: Record + ): Record => ({ + value: 1, + params: { ...params(address), ...overrides } + }) + + const persisted = ( + registers: Record> + ): Record => ({ + serverRegisters: { + u: { + '1': { + coils: {}, + discrete_inputs: {}, + input_registers: {}, + holding_registers: registers + } + } + } + }) + + const holding = (state: Record): Record => { + const perUuid = state.serverRegisters as Record> + const unit = perUuid.u?.['1'] as Record + return unit.holding_registers as Record + } + + // `Buffer.alloc(2e12)` answers ERR_OUT_OF_RANGE and `getUsedAddresses` loops + // the same number, so the register that went is the one that hung the launch. + it('drops a string wider than the map and keeps its neighbours', () => { + const state = persisted({ + '0': register(0, { dataType: 'utf8', length: 10, stringValue: 'x', value: 0 }), + '20': register(20, { dataType: 'utf8', length: 1e12, stringValue: 'x', value: 0 }), + '40': register(40, {}) + }) + dropUnservableRegisters(state) + + expect(Object.keys(holding(state))).toEqual(['0', '40']) + }) + + it('drops a fixed value its data type cannot encode', () => { + const state = persisted({ + '0': register(0, { value: 70000 }), + '10': register(10, { dataType: 'int64', value: 1.5 }), + '20': register(20, { value: 65535 }) + }) + dropUnservableRegisters(state) + + expect(Object.keys(holding(state))).toEqual(['20']) + }) + + it('drops a register that runs past address 65535', () => { + const state = persisted({ + '65534': register(65534, { dataType: 'uint64', value: 1 }), + '65532': register(65532, { dataType: 'uint64', value: 1 }) + }) + dropUnservableRegisters(state) + + expect(Object.keys(holding(state))).toEqual(['65532']) + }) + + // The drop on its own is not the store's behaviour; the step in `migrate` is. + it('is dropped by the migration a v6 blob runs', () => { + const state = migrateServerState( + persisted({ '0': register(0, { value: 70000 }), '10': register(10, {}) }), + 6 + ) + + expect(Object.keys(holding(state))).toEqual(['10']) + }) + + it('is behind a version the store has moved past', () => { + expect(CURRENT_SERVER_ZUSTAND_VERSION).toBeGreaterThan(6) + }) +}) + +// `ServerRegisterEntrySchema.value` was `z.number()`, so every blob on disk +// carries a number for the three types whose composite fills 64 bits as an +// integer. The first word write after a launch reads that value, and a number +// carries 53 bits. +describe('a persisted 64 bit value', () => { + const persisted = (dataType: string, value: unknown): Record => ({ + serverRegisters: { + u: { + '1': { + holding_registers: { + '10': { + value, + params: { + address: 10, + registerType: 'holding_registers', + dataType, + comment: '', + value: 0 + } + } + } + } + } + } + }) + + const heldValue = (state: Record): unknown => { + const perUuid = state.serverRegisters as Record> + const unit = perUuid.u?.['1'] as Record + const holding = unit.holding_registers as Record> + return holding['10']?.value + } + + it.each(['uint64', 'int64', 'datetime'])('becomes a string for %s', (dataType) => { + const state = migrateServerState(persisted(dataType, 72623859790382850), 7) + + expect(heldValue(state)).toBe('72623859790382850') + }) + + it.each(['uint16', 'int32', 'double', 'float', 'unix'])('stays a number for %s', (dataType) => { + const state = migrateServerState(persisted(dataType, 1234), 7) + + expect(heldValue(state)).toBe(1234) + }) + + // `toExact64Bits` answers nothing for a value no composite can be read out + // of, and `applyRegisterValue` then leaves the entry where it is, so the grid + // would show that value for every write from then on while main served the + // new one. `String(0.5)` is `"0.5"` and `String(1e21)` is `"1e+21"`, and the + // schema takes either as a number and neither as a string, so neither can be + // written back as it is. + it.each([0.5, 1e21, -0.25])('repairs a value of %p that no composite reads', (value) => { + const state = migrateServerState(persisted('uint64', value), 7) + + expect(heldValue(state)).toBe('0') + }) + + it('leaves a value already stored as a string alone', () => { + const state = migrateServerState(persisted('uint64', '18446744073709551615'), 7) + + expect(heldValue(state)).toBe('18446744073709551615') + }) + + it('is behind a version the store has moved past', () => { + expect(CURRENT_SERVER_ZUSTAND_VERSION).toBeGreaterThan(7) + }) +}) + describe('a persisted mapping entry outside the map', () => { /** The last client store version whose blobs can carry any numeric key. */ const LAST_VERSION_ACCEPTING_ANY_KEY = 3 diff --git a/src/shared/migrations/client/config.ts b/src/shared/migrations/client/config.ts index ac41eb35..0604b08e 100644 --- a/src/shared/migrations/client/config.ts +++ b/src/shared/migrations/client/config.ts @@ -1,6 +1,13 @@ import { RegisterMapConfigSchema, RegisterMapConfig, RegisterMapping } from '../../types/client' +import { emptyRegisterMapping } from '../../default' +import { repairPersisted } from '../../repairPersisted' import { MigrationResult, Migration } from '../types' -import { formatZodError, renameLegacyRegisterTypeKeys } from '../shared' +import { + dropUnreadableConfigMapping, + formatZodError, + parseConfigFile, + renameLegacyRegisterTypeKeys +} from '../shared' /** * The version the Save button writes into a client config file. @@ -42,8 +49,7 @@ function migrateClientV1toV2(v1Config: unknown): RegisterMapConfig { * Migrate client config to current version */ export function migrateClientConfig(raw: string): MigrationResult { - const parsed = JSON.parse(raw) - const detectedVersion = parsed.version ?? 1 + const { parsed, detectedVersion } = parseConfigFile(raw) // Current version - no migration needed if (detectedVersion === CURRENT_CLIENT_CONFIG_VERSION) { @@ -60,13 +66,37 @@ export function migrateClientConfig(raw: string): MigrationResult CURRENT_CLIENT_CONFIG_VERSION) { + // Entry by entry first. `registerMapping` is one field, and it is the one + // thing in the client store built by hand, so one address this version + // cannot read would cost the whole mapping. + dropUnreadableConfigMapping(parsed) + + const repair = repairPersisted( + RegisterMapConfigSchema, + parsed, + { + version: detectedVersion, + modbuxVersion: '', + name: undefined, + littleEndian: false, + registerMapping: emptyRegisterMapping() + } satisfies RegisterMapConfig, + true + ) + return { - config: parsed as RegisterMapConfig, + config: repair.state, migrated: false, fromVersion: detectedVersion, - warning: 'FUTURE_VERSION' + futureVersion: repair.reset } } diff --git a/src/shared/migrations/server/config.ts b/src/shared/migrations/server/config.ts index 070b2504..bfeb4aea 100644 --- a/src/shared/migrations/server/config.ts +++ b/src/shared/migrations/server/config.ts @@ -11,9 +11,13 @@ import { formatZodError, migrateBoolShapeForUnit, objectValues, - renameLegacyRegisterTypeKeys + dropUnservableConfigRegisters, + parseConfigFile, + renameLegacyRegisterTypeKeys, + stringifyExact64BitValues } from '../shared' import { V1ServerConfig, extractGlobalEndianness } from './shared' +import { repairPersisted } from '../../repairPersisted' /** * The version the Save button writes into a server config file. @@ -22,11 +26,12 @@ import { V1ServerConfig, extractGlobalEndianness } from './shared' * the loop below counts the steps and this number only says where to stop, so * v1 to v2 writes 2 whatever this becomes. */ -export const CURRENT_SERVER_CONFIG_VERSION = 2 +export const CURRENT_SERVER_CONFIG_VERSION = 3 const SERVER_CONFIG_MIGRATIONS: Record> = { - 1: migrateServerV1toV2 - // Future: 2: migrateServerV2toV3 + 1: migrateServerV1toV2, + 2: migrateServerV2toV3 + // Future: 3: migrateServerV3toV4 } /** @@ -104,6 +109,30 @@ function migrateServerV1toV2(v1Config: unknown): ServerConfig & { wasMixedEndian return v2Config } +/** + * Migrate server config from v2 to v3: a 64 bit value is a decimal string. + * + * The three types whose composite fills 64 bits as an integer held a number, + * and a number carries 53 bits. The version moves because the file shape moved: + * without it a config this build writes still claims v2, and v2.3.0 takes the + * `detectedVersion === CURRENT` branch, fails `ServerConfigSchema` on + * `z.number()`, and refuses the whole file rather than the one register. + * + * `stringifyExact64BitValues` walks a persisted store, `serverRegisters[uuid]`, + * and a config is keyed one level up, so the blob is wrapped in the shape that + * helper reads. That is the same difference `migrateBoolShapeForUnit` carries. + * + * The bool shape goes with it. `migrateBoolShapeInConfig` ran on the + * `detectedVersion === CURRENT` branch alone, and a v2 file carrying the old + * `boolean` entries now takes this step instead of that branch. + */ +function migrateServerV2toV3(v2Config: unknown): ServerConfig { + const config = v2Config as ServerConfig & Record + migrateBoolShapeInConfig(config) + stringifyExact64BitValues({ serverRegisters: { config: config.serverRegistersPerUnit } }) + return { ...config, version: 3 } +} + /** * Convert old `boolean` bool entries to `{ value: boolean }` in a parsed config * object. Mutates in place. Safe to call on already-migrated data. @@ -118,12 +147,18 @@ function migrateBoolShapeInConfig(config: Record): void { * Migrate server config to current version */ export function migrateServerConfig(raw: string): MigrationResult { - const parsed = JSON.parse(raw) - const detectedVersion = parsed.version ?? 1 + const { parsed, detectedVersion } = parseConfigFile(raw) // Current version - migrate bool shape if needed, then validate if (detectedVersion === CURRENT_SERVER_CONFIG_VERSION) { migrateBoolShapeInConfig(parsed) + // A file this version writes carries a decimal string for a 64 bit value, + // and a hand-edited one can carry the number: `ServerConfigSchema` takes + // either. The persisted store's own step is gated on the store version and + // does not run on a config load, so without this the first single word + // write after opening such a file folds from the rounded value, which is + // the defect the v2 to v3 step removes for a v2 file. + stringifyExact64BitValues({ serverRegisters: { config: parsed.serverRegistersPerUnit } }) const result = ServerConfigSchema.safeParse(parsed) if (!result.success) { throw new Error( @@ -137,13 +172,38 @@ export function migrateServerConfig(raw: string): MigrationResult } } - // Future version + // A config from a newer Modbux keeps the fields it still shares with this + // one. The branch cast the parsed JSON straight to `ServerConfig` and + // returned it, so `{ version: 9, serverRegistersPerUnit: 'not a unit' }` went + // into the store with the snackbar reading "Some features may not work + // correctly", which is a compatibility notice for something no schema had + // looked at. `repairPersisted` is the same answer a persisted store already + // gets, and `reset` is what the warning now says. if (detectedVersion > CURRENT_SERVER_CONFIG_VERSION) { + // Register by register first, because `repairPersisted` reads a field + // whole and `serverRegistersPerUnit` is the one field a server config is + // about: one register with a data type this version does not name would + // cost every register on every unit. + dropUnservableConfigRegisters(parsed) + + const repair = repairPersisted( + ServerConfigSchema, + parsed, + { + version: detectedVersion, + modbuxVersion: '', + name: '', + littleEndian: false, + serverRegistersPerUnit: {} + } satisfies ServerConfig, + true + ) + return { - config: parsed as ServerConfig, + config: repair.state, migrated: false, fromVersion: detectedVersion, - warning: 'FUTURE_VERSION' + futureVersion: repair.reset } } @@ -166,11 +226,16 @@ export function migrateServerConfig(raw: string): MigrationResult throw new Error(`Migration produced invalid config: ${formatZodError(result.error)}`) } + // `migrateServerV1toV2` sets this flag, and `parseConfigFile` types the blob + // it reads as a record of `unknown` rather than the `any` `JSON.parse` gave, + // so the flag has to be read as the boolean it is. + const wasMixedEndianness = config.wasMixedEndianness === true + return { config: result.data, migrated: true, fromVersion: detectedVersion, - warning: config.wasMixedEndianness ? 'MIXED_ENDIANNESS' : undefined, - wasMixedEndianness: config.wasMixedEndianness + warning: wasMixedEndianness ? 'MIXED_ENDIANNESS' : undefined, + wasMixedEndianness } } diff --git a/src/shared/migrations/server/zustand.ts b/src/shared/migrations/server/zustand.ts index f544f36c..59068d4c 100644 --- a/src/shared/migrations/server/zustand.ts +++ b/src/shared/migrations/server/zustand.ts @@ -1,7 +1,12 @@ import { V1RegisterParams, V1ServerRegistersPerUnit, extractGlobalEndianness } from './shared' -import { dropUnservableRegisters, migrateBoolShapeForUnit, repairPersistedParity } from '../shared' +import { + dropUnservableRegisters, + migrateBoolShapeForUnit, + repairPersistedParity, + stringifyExact64BitValues +} from '../shared' -export const CURRENT_SERVER_ZUSTAND_VERSION = 6 +export const CURRENT_SERVER_ZUSTAND_VERSION = 8 /** Where the server store keeps its state. */ export const SERVER_ZUSTAND_STORAGE_KEY = 'server.zustand' @@ -145,9 +150,17 @@ export function migrateServerState( // v4→v5: registers at an address outside the 16 bit map // v5→v6: and generators the interval floor now refuses - if (version < 6) { + // v6→v7: and a string width, a fixed value or an interval outside what the + // encoder, the map or `setInterval` can take + if (version < 7) { dropUnservableRegisters(state) } + // v7→v8: a 64 bit composite is a decimal string, so the first word write + // after a launch reads it exactly rather than rounded + if (version < 8) { + stringifyExact64BitValues(state) + } + return state } diff --git a/src/shared/migrations/shared.ts b/src/shared/migrations/shared.ts index 62a03f34..dd27823c 100644 --- a/src/shared/migrations/shared.ts +++ b/src/shared/migrations/shared.ts @@ -1,8 +1,9 @@ import type { ZodError, ZodIssue } from 'zod' import { RegisterAddressKeySchema } from '../types/ranges' +import { RegisterMapValueSchema } from '../types/client' import { RegisterParamsSchema } from '../types/server' -import { NumberRegistersSchema, ParitySchema, UnitIdStringSchema } from '../types' -import { getUsedAddresses } from '../utils' +import { DataType, NumberRegistersSchema, ParitySchema, UnitIdStringSchema } from '../types' +import { getUsedAddresses, holdsExact64Bits } from '../utils' /** * Replace a stored parity that `ParitySchema` no longer names, at `path` from @@ -31,6 +32,47 @@ export function repairPersistedParity(state: Record, ...path: s const isRecord = (value: unknown): value is Record => typeof value === 'object' && value !== null +/** + * What a config file has to be, and the version it claims, for either migration. + * + * Both opened with `JSON.parse(raw)` and then `parsed.version ?? 1`. On a + * number, a string, a boolean or an array that is `undefined`, so 1, so the v1 + * to v2 step ran, and every field the server migration needs sits behind a + * `??`, so `5`, `[]`, `"hello"` and `true` each came out a valid empty v2 + * config and the user was told the configuration had been updated from an older + * format. `null` reached them as + * `Cannot read properties of null (reading 'version')` instead. The client + * migration refuses the same input, but only because `migrateClientV1toV2` + * feeds the whole blob into `registerMapping` and the final `safeParse` catches + * it, which is an accident of that one field rather than a rule. + * + * An array is refused with the rest: `Array.isArray` is the half `typeof` does + * not cover, and a config is a map of named fields either way. + * + * The version is checked here too. `JSON.parse` answers `any`, so nothing had + * asked what was in that field, and a relational operator coerces: `"3" > 2` + * and `2.5 > 2` are both true, so those two took the future version branch and + * came back with `migrated: false` and the newer-version warning, while `true` + * and `null` compared false against both and took the v1 path. Four values, + * three outcomes, none of them the one the file asked for. + */ +export function parseConfigFile(raw: string): { + parsed: Record + detectedVersion: number +} { + const parsed: unknown = JSON.parse(raw) + if (!isRecord(parsed) || Array.isArray(parsed)) { + throw new Error('This file does not hold a Modbux configuration') + } + + const version = parsed.version + if (version !== undefined && (typeof version !== 'number' || !Number.isInteger(version))) { + throw new Error(`This file claims version ${JSON.stringify(version)}, which is not a version`) + } + + return { parsed, detectedVersion: version ?? 1 } +} + /** The object entries of `value`, and nothing at all when it is not an object. */ const recordEntries = (value: unknown): [string, Record][] => isRecord(value) @@ -63,18 +105,6 @@ const recordAt = ( return made } -/** - * A register map is keyed by address, and a register entry repeats its whole - * parameter set, so both have to hold for the entry to be servable. A boolean - * entry carries the key alone. - */ -const isServable = (address: string, entry: Record): boolean => { - if (!RegisterAddressKeySchema.safeParse(address).success) return false - const params = entry.params - if (!isRecord(params)) return true - return RegisterParamsSchema.safeParse(params).success -} - /** The addresses one unit's surviving registers occupy, by register type. */ const usedAddressesOfUnit = ( registersByType: Record @@ -93,12 +123,17 @@ const usedAddressesOfUnit = ( * Drop persisted registers the current `RegisterParamsSchema` no longer names, * and rewrite the used addresses of every unit walked. * - * Two rules arrived after registers had already been persisted against looser - * ones. `RegisterParamsBasePartSchema.address` was a bare number, so a config - * file could put a register at 70000. `interval` was a bare number, so one - * could carry a generator that fires every millisecond. `repairPersisted` - * works a top level field at a time, and without this one such register costs - * every register on every server and every unit. + * Every rule here arrived after registers had already been persisted against a + * looser one. `address` was a bare number, so a config file could put a + * register at 70000. `interval` had no floor, so one could carry a generator + * that fires every millisecond, and then no ceiling, where `setInterval` sets + * the duration to 1 and does the same thing. `length` was bare, so a string + * could ask for `Buffer.alloc(2e12)`, and a fixed `value` was bare, so a + * `uint16` could carry 70000 and throw out of `createRegisters`. A register + * running past address 65535 is the pair of `address` and `length` and was + * refused by neither. `repairPersisted` works a top level field at a time, and + * without this one such register costs every register on every server and every + * unit. * * `usedAddresses` is persisted beside the registers, `isAddressInUse` refuses an * address against it, and the only thing recomputing it on launch is @@ -135,6 +170,112 @@ export function dropUnservableRegisters(state: Record): void { } } +/** + * A register map is keyed by address, and a register entry repeats its whole + * parameter set, so both have to hold for the entry to be servable. A boolean + * entry carries the key alone. + */ +const isServable = (address: string, entry: Record): boolean => { + if (!RegisterAddressKeySchema.safeParse(address).success) return false + const params = entry.params + if (!isRecord(params)) return true + return RegisterParamsSchema.safeParse(params).success +} + +/** + * Drop the registers a config file from a newer Modbux carries that this one + * cannot serve, so the ones it can survive. + * + * `repairPersisted` reads a field whole, and `serverRegistersPerUnit` is the + * one field a server config is about, so without this a single register with a + * data type this version does not name cost every register on every unit. That + * is the cost `dropUnservableRegisters` exists to avoid on the persisted store, + * and a config from a newer version is where a register the enum does not name + * actually comes from. + * + * Walks `serverRegistersPerUnit[unit]` where the store walks + * `serverRegisters[uuid][unit]`, which is the same difference + * `migrateBoolShapeForUnit` already carries. + */ +export function dropUnservableConfigRegisters(parsed: Record): void { + for (const [, registersByType] of recordEntries(parsed.serverRegistersPerUnit)) { + for (const entriesByAddress of objectValues(registersByType)) { + for (const [address, entry] of Object.entries(entriesByAddress)) { + if (isRecord(entry) && isServable(address, entry)) continue + delete entriesByAddress[address] + } + } + } +} + +/** + * Drop the mapping entries a client config from a newer Modbux carries that + * this one cannot read. + * + * `registerMapping` is one field too, and it is the one thing in the client + * store built by hand, which is the reason `dropUnmappableRegisters` gives for + * doing the same on the persisted side. `LoadButton` calls + * `replaceRegisterMapping` on whatever comes back and flushes it to main, so a + * mapping of thousands of rows was replaced by nothing on account of one entry. + */ +export function dropUnreadableConfigMapping(parsed: Record): void { + for (const entriesByAddress of objectValues(parsed.registerMapping)) { + for (const [address, entry] of Object.entries(entriesByAddress)) { + if (RegisterAddressKeySchema.safeParse(address).success) { + if (RegisterMapValueSchema.safeParse(entry).success) continue + } + delete entriesByAddress[address] + } + } +} + +/** + * Rewrite a persisted 64 bit register value as the decimal string it is now. + * + * `ServerRegisterEntrySchema.value` was `z.number()`, so every blob on disk + * carries a number for the three types whose composite fills 64 bits as an + * integer. The schema takes a string as well now, and a number still parses, + * so nothing is dropped by leaving them: this is so the first word write after + * a launch reads an exact value rather than the rounded one. A value already + * past 2 ** 53 cannot be recovered, and the string says what was stored rather + * than what the device holds. + */ +export function stringifyExact64BitValues(state: Record): void { + for (const [, registersPerUnit] of recordEntries(state.serverRegisters)) { + for (const [, registersByType] of recordEntries(registersPerUnit)) { + for (const entriesByAddress of objectValues(registersByType)) { + for (const entry of Object.values(entriesByAddress)) { + if (!isRecord(entry)) continue + if (typeof entry.value !== 'number') continue + const params = entry.params + if (!isRecord(params)) continue + const dataType = params.dataType + if (typeof dataType !== 'string') continue + if (!holdsExact64Bits(dataType as DataType)) continue + + // The digits, or `'0'` where there are none. `String(0.5)` is + // `"0.5"` and `String(1e21)` is `"1e+21"`, and the union takes + // either as a number and neither as a string, so writing one back + // would turn a blob that parsed into one that does not, and + // `repairPersisted` reads `serverRegisters` whole: every server, + // every unit. An integer above 2 ** 53 is the case this step is for, + // so the test is the digits rather than `Number.isSafeInteger`. + // + // `'0'` rather than left alone, because `toExact64Bits` answers + // nothing for a value no composite can be read out of, and + // `applyRegisterValue` then leaves the entry where it is: the grid + // would show that value for every write from then on while main + // served the new one. Zero is what a register the server has not + // written holds, and a fraction on a 64 bit type is not a value any + // device wrote. + const asString = String(entry.value) + entry.value = /^-?\d+$/.test(asString) ? asString : '0' + } + } + } + } +} + /** * Drop persisted mapping entries at an address outside the 16 bit map. * diff --git a/src/shared/migrations/types.ts b/src/shared/migrations/types.ts index b1f16cbd..b7d53d59 100644 --- a/src/shared/migrations/types.ts +++ b/src/shared/migrations/types.ts @@ -1,9 +1,24 @@ +import type { ConfigReset } from '../repairPersisted' + export interface MigrationResult { config: T migrated: boolean fromVersion: number - warning?: 'FUTURE_VERSION' | 'MIXED_ENDIANNESS' + warning?: 'MIXED_ENDIANNESS' wasMixedEndianness?: boolean + /** + * Set when the file claims a version newer than this Modbux writes, and says + * what it did not bring across. + * + * One field rather than a `warning` of `'FUTURE_VERSION'` beside a `reset` + * that is only ever set with it: the pair made the caller ask twice, and the + * second question is one no input answers no. + * + * That branch used to cast the parsed JSON straight to the config type, so it + * was the one door into the app no schema stood in, and the warning the user + * read was a compatibility notice for something that had not been checked. + */ + futureVersion?: ConfigReset } export type Migration = (config: unknown) => T diff --git a/src/shared/repairPersisted.ts b/src/shared/repairPersisted.ts index 7c3ddfc8..d1c56afd 100644 --- a/src/shared/repairPersisted.ts +++ b/src/shared/repairPersisted.ts @@ -62,10 +62,17 @@ export function repairPersisted( } /** - * What each persisted field is called on screen. A field with no entry is named - * as it is stored, which is worth more in a bug report than a guess. + * What each field is called on screen. A field with no entry is named as it is + * stored, which is worth more in a bug report than a guess. + * + * Both a persisted store and a config file from a newer Modbux are read + * through here, and the two name some of the same things differently: the store + * keeps `serverRegisters`, the file `serverRegistersPerUnit`, and a file also + * carries the two version fields. */ const FIELD_LABELS: Record = { + serverRegistersPerUnit: 'the registers', + modbuxVersion: 'the version it was saved by', connectionConfig: 'the connection settings', registerConfig: 'the register settings', registerMapping: 'the register mapping', @@ -107,9 +114,9 @@ export function resetMessage(store: 'Client' | 'Server', reset: ConfigReset): st return `${store} configuration was saved by a newer version of Modbux and was read in full.` } - return `${store} configuration was saved by a newer version of Modbux. ${listFields( + return `${store} configuration was saved by a newer version of Modbux, and ${listFields( reset.fields - )} did not come across and ${reset.fields.length === 1 ? 'was' : 'were'} reset. ${kept}` + )} did not come across. ${kept}` } /** diff --git a/src/shared/types/client.ts b/src/shared/types/client.ts index 390f88eb..b9f77c02 100644 --- a/src/shared/types/client.ts +++ b/src/shared/types/client.ts @@ -64,7 +64,17 @@ export const TransactionSchema = z.object({ id: z.string(), timestamp: z.number(), unitId: UnitIdSchema, - address: RegisterAddressSchema, + /** + * The data address the request asked for, and undefined where it cannot be + * read. + * + * `_requestedAddress` takes it off bytes 2 and 3 of the request frame, which + * every function code Modbux sends puts it at, and falls back on + * `nextDataAddress`, which modbus-serial files for three of them. Both are + * gone only for a frame that never went out, and `_columns.tsx` binds + * `field: 'address'` with no formatter, so the cell is blank there. + */ + address: RegisterAddressSchema.optional(), code: z.number(), responseLength: z.number(), timeout: z.boolean(), @@ -247,7 +257,17 @@ export interface RegisterDataWords { export interface RawTransaction { nextAddress: number - nextDataAddress: number + /** + * The data address, filed by two of modbus-serial's twelve transaction + * records. + * + * `writeFC4` at index.js:880 and `writeFC6` at 983 set it, and `writeFC1` + * delegates to `writeFC2` and `writeFC3` to `writeFC4`, so FC3, FC4 and FC6 + * carry one and FC1, FC2, FC5, FC15 and FC16 do not. This said `number` and + * `_logTransaction` read it, so the Addr cell was blank for every coil read, + * every discrete input read and every write Modbux sends. + */ + nextDataAddress?: number nextCode: number nextLength: number // next: [Function: cb], diff --git a/src/shared/types/ranges.ts b/src/shared/types/ranges.ts index ee5d1db0..9941649e 100644 --- a/src/shared/types/ranges.ts +++ b/src/shared/types/ranges.ts @@ -1,4 +1,5 @@ import z from 'zod' +import { NumberRegistersSchema, RegisterType } from './register' /** * Ranges the protocol and the socket fix, so a schema states them once. @@ -20,6 +21,37 @@ export const RegisterAddressSchema = z.number().int().min(0).max(65535) export const UnitIdSchema = z.number().int().min(0).max(255) export const PortSchema = z.number().int().min(0).max(65535) +/** + * The most a single read can ask for, by what it is reading. + * + * MODBUS Application Protocol Specification V1.1b3, section 6: FC01 and FC02 + * answer at most 2000 bits, FC03 and FC04 at most 125 registers. The PDU is 253 + * bytes, and a read response is the function code, a byte count and the data, + * so 251 bytes are left: 125 registers of two bytes, and 250 bytes of bits. + * + * A write is a different pair, 1968 bits and 123 registers, because the request + * carries the data as well as the address and the quantity. Nothing reads that + * here, because every field these bound builds a read. + */ +export const MAX_READ_BITS = 2000 +export const MAX_READ_REGISTERS = 125 + +/** + * The ceiling one read of these register types has. + * + * A unit id scan asks for several types with one length, so the strictest of + * them is the one the request has to fit. `ScanRegisters.tsx ChunkSizeField` + * computed this pair by hand; the scan's own Length field computed nothing, so + * `UintInput`'s default of 65535 was typeable and `modbus-serial` wrote it + * into the quantity field unchecked. A device answers illegal-data-value or + * says nothing, and the answer the user reads is about the request rather than + * about the bus. + */ +export const maxReadQuantity = (registerTypes: readonly RegisterType[]): number => + registerTypes.some((registerType) => NumberRegistersSchema.safeParse(registerType).success) + ? MAX_READ_REGISTERS + : MAX_READ_BITS + /** * The same range for a register map keyed by address. * diff --git a/src/shared/types/scan.ts b/src/shared/types/scan.ts index bbb67788..00964585 100644 --- a/src/shared/types/scan.ts +++ b/src/shared/types/scan.ts @@ -1,5 +1,5 @@ import { RegisterType, RegisterTypeSchema } from './register' -import { RegisterAddressSchema, UnitIdSchema } from './ranges' +import { maxReadQuantity, RegisterAddressSchema, UnitIdSchema } from './ranges' import z from 'zod' // Scan Registers @@ -13,13 +13,32 @@ export type ScanRegistersParameters = z.infer { + const ceiling = maxReadQuantity(parameters.registerTypes) + if (parameters.length <= ceiling) return + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['length'], + message: `One read of these register types answers at most ${ceiling}` + }) + }) export type ScanUnitIDParameters = z.infer const ScanUnitIdErrorMessageSchema = z.object({ diff --git a/src/shared/types/server.ts b/src/shared/types/server.ts index 395a01db..90271429 100644 --- a/src/shared/types/server.ts +++ b/src/shared/types/server.ts @@ -4,6 +4,7 @@ import { BitMapConfigSchema } from './bitmap' import { PortSchema, RegisterAddressKeySchema, RegisterAddressSchema } from './ranges' import { SerialPortOptionsSchema } from './serial' import { UnitIdString, UnitIdStringSchema } from './unitid' +import { getAddressFitError, getValueRangeError, MAX_UTF8_LENGTH } from '../utils' import { BooleanRegisters, BooleanRegistersSchema, @@ -45,19 +46,35 @@ export type StartRtuServerParams = z.infer * stored in milliseconds, so anything below a second reaches this only from a * config file. An interval of 0 is the one that costs: `valueGenerator` hands * it to `setInterval`, and an interval of 0 fired 78 times in 100 ms when - * measured. The ceiling is the mask's rather than the generator's, so it is - * not stated here. + * measured. The ceiling is Node's rather than the mask's: above 2147483647 it + * warns `TimeoutOverflowWarning` and sets the duration to 1, so an interval of + * 1e12 fires every millisecond, which is the flood the floor was written to + * prevent. * - * `min` and `max` are each a bare number and stay one. The dialog lets a min - * above a max through, and `Math.random() * (max - min) + min` covers the same - * range either way: ten thousand draws of min 100 max 10 ran 10 to 100, the - * same as min 10 max 100. A rule here would refuse a payload the Add button - * sends and cost the whole persisted register map on the next launch. + * `min` and `max` are each held to their own data type, in the refine below, + * because the generator draws between them and hands the draw to + * `createRegisters`. `ValueGenerator._updateValue` is `async` and nobody awaits + * it, so a draw the type cannot encode is not a throw the caller sees: the + * register answers 0 for as long as the generator runs and main takes an + * unhandled rejection every interval. Electron 43 stays alive through that, + * which is why it is quiet rather than loud. + * + * The reason they were bare was that a rule here would refuse what the Add + * button sends and cost the whole persisted register map, and both halves were + * measured false. `useMinMaxInteger` already masks both fields to + * `getMinMaxValues(dataType)` over the same seven types, so switching a + * generator from `uint32` to `uint16` rewrites a max of 100000 to 65535 and + * reports it valid. And one register the schema refuses costs that register + * now, because store version 7 drops it and keeps the rest. + * + * A min above a max is not a rule here. The dialog lets one through, and + * `Math.random() * (max - min) + min` covers the same range either way: ten + * thousand draws of min 100 max 10 ran 10 to 100, the same as min 10 max 100. */ const RegisterParamsGeneratorPartSchema = z.object({ min: z.number(), max: z.number(), - interval: z.number().int().min(1000), + interval: z.number().int().min(1000).max(2147483647), value: z.undefined() // Explicitly forbid 'value' }) export type RegisterParamsGeneratorPart = z.infer @@ -88,22 +105,68 @@ export type RegisterValue = { * `sync_server_register` take. `remove_server_register` takes * `RegisterAddressSchema`, so while this field was a bare number an address * outside the map went in and could not come back out. + * + * `length` is the width of a string, and it is the one field the width of the + * register is read off. Left bare, `length: 1e12` passed here and reached + * `createStringRegisters`, which is `Buffer.alloc(2e12)`: inside Electron 43 + * that answers ERR_OUT_OF_RANGE, and `length: 1e9` allocates and then builds a + * billion words instead. `getUsedAddresses` loops the same number. The ceiling + * is `MAX_UTF8_LENGTH`, which `RegisterLengthInput` masks to. */ export const RegisterParamsBasePartSchema = z.object({ address: RegisterAddressSchema, registerType: NumberRegistersSchema, dataType: BaseDataTypeSchema, comment: z.string(), - length: z.number().optional(), + length: z.number().int().min(1).max(MAX_UTF8_LENGTH).optional(), stringValue: z.string().optional(), bitMap: BitMapConfigSchema.optional() }) export type RegisterParamsBasePart = z.infer -// Final RegisterValueParameters schema with conditional fields +/** + * Two rules over fields that bound each other, which is why they sit here + * rather than beside the fields. + * + * The register has to fit the map. `address` is 16 bit and `length` is bounded + * above, and a register can still run off the end: the add dialog refuses that + * with `getAddressFitError`, and a config file did not ask. + * + * A fixed value has to be one its own `dataType` can encode, which is + * `getValueRangeError`. The pair is the rule, not the field: 70000 is a + * `uint32` and not a `uint16`, and `addRegister`'s fixed branch hands whatever + * arrives straight to `createRegisters`. + * + * A generator's `min` and `max` go through the same rule as a fixed value, for + * the reason `RegisterParamsGeneratorPartSchema` states: the draw between them + * is what reaches the encoder. + * + * All three are stated where the add dialog states them, so the file and the + * field answer the same question. + */ export const RegisterParamsSchema = RegisterParamsBasePartSchema.and( z.union([RegisterParamsGeneratorPartSchema, RegisterParamsStaticPartSchema]) -) +).superRefine((params, ctx) => { + if (getAddressFitError(params.dataType, params.address, params.length)) { + // `registerWidth` reads `length` for `utf8` alone, so naming it for any + // other type points at a field with no bearing on the width. A `uint64` + // that still carries a `length` from an earlier edit is one of those. + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [params.dataType === 'utf8' ? 'length' : 'address'], + message: `A ${params.dataType} register at ${params.address} runs past address 65535` + }) + } + + for (const field of ['value', 'min', 'max'] as const) { + const bound = params[field] + if (bound === undefined) continue + const rangeError = getValueRangeError(params.dataType, bound) + if (rangeError) { + ctx.addIssue({ code: z.ZodIssueCode.custom, path: [field], message: rangeError }) + } + } +}) export type RegisterParams = z.infer // Schema for a single boolean entry with optional comment @@ -117,9 +180,16 @@ export type ServerBoolEntry = z.infer export const ServerBoolSchema = z.record(RegisterAddressKeySchema, ServerBoolEntrySchema) export type ServerBool = z.infer -// Schema for a single register entry +/** + * One register entry: what it holds, and the parameters that say what it is. + * + * `value` takes a string as well as a number, for the three types whose + * composite fills 64 bits as an integer. See `ServerRegisterValue`. A string + * that is not a decimal integer is refused here, so `toExact64Bits` has a + * number or digits to read. + */ export const ServerRegisterEntrySchema = z.object({ - value: z.number(), + value: z.union([z.number(), z.string().regex(/^-?\d+$/)]), params: RegisterParamsSchema }) export type ServerRegisterEntry = z.infer @@ -187,7 +257,9 @@ export const RemoveRegisterParamsSchema = z.object({ dataType: BaseDataTypeSchema, // Only a string has a width the user chose, and without it the server has to // guess how much of the map the register occupied and erases the guess. - length: z.number().optional() + // Bounded the way add and sync bound it: `removeRegister` loops + // `registerWidth(dataType, length)` times writing into the register array. + length: z.number().int().min(1).max(MAX_UTF8_LENGTH).optional() }) export type RemoveRegisterParams = z.infer diff --git a/src/shared/utils.ts b/src/shared/utils.ts index e8fce3ce..f31097d7 100644 --- a/src/shared/utils.ts +++ b/src/shared/utils.ts @@ -1,4 +1,10 @@ -import { CamelCase, DataType, EncodableDataType, RegisterParams, ServerRegisters } from './types' +// `types/server.ts` imports `getAddressFitError` and `getValueRangeError` from +// here, so this reads the barrel no longer: it would be a cycle through +// `types/index.ts`, and one that costs nothing only while every name below is a +// type the compiler erases. `import type` says which of the two these are. +import type { CamelCase } from './types/utils' +import type { DataType, EncodableDataType } from './types/datatype' +import type { RegisterParams, ServerRegisters } from './types/server' /** * Whether bit `bit` of `word` is set. @@ -13,6 +19,19 @@ export const getBit = (word: number, bit: number): boolean => Math.floor(word / /** The width the add dialog offers for a string when the field is left alone. */ export const DEFAULT_UTF8_LENGTH = 10 +/** + * The widest string a register map holds, which is the add dialog's own mask. + * + * The protocol puts a ceiling under it either way: FC3 and FC4 answer at most + * 125 registers, so a string wider than that cannot be read in one request. + * The number here is the one `RegisterLengthInput` has always masked to, and + * `RegisterParamsBasePartSchema` states it too, because a config file is the + * other door and the map is the only thing that bounded it there: one string of + * 65536 made `addRegister` send 65536 `register_value` messages in one + * synchronous loop. + */ +export const MAX_UTF8_LENGTH = 124 + /** * How many registers a value of this type occupies. * @@ -209,6 +228,101 @@ export const getMinMaxValues = (dataType: DataType): { min: number; max: number } } +/** + * What a server register entry holds, which is not always a number. + * + * A decimal string for the three types whose composite fills 64 bits as an + * integer. It was `z.number()` for all of them: `applyRegisterValue` read the + * composite back with `getBigUint64` and stored `Number(...)`, so four words of + * 0xFFFF came out 18446744073709552000 rather than 18446744073709551615, and + * `setBigUint64` takes its argument modulo 2 ** 64 rather than throwing, so the + * next single word write on that entry collapsed it to the low word alone. + * Measured: one more word after the flush left the entry holding 1. + * + * A string rather than a `bigint` because the store is persisted through + * `JSON.stringify`, which refuses a bigint outright. + */ +export type ServerRegisterValue = number | string + +/** + * The types whose composite fills 64 bits as an integer. + * + * `applyRegisterValue` reads these three back with `getBigInt64` or + * `getBigUint64`. `double` is the fourth type four registers wide and is not + * here: its composite is a float64, which is exactly what a JS number is. + */ +export const holdsExact64Bits = (dataType: DataType): boolean => + dataType === 'int64' || dataType === 'uint64' || dataType === 'datetime' + +/** + * The exact composite a stored value stands for, or nothing when it stands for + * none. + * + * Takes a `bigint` as well, because `ServerDelayedSetter` holds the composite + * it last folded and that is the value the next word write reads first. + * + * `undefined` rather than `0n`, so a caller merging one word into a composite + * aborts rather than rebuilding it from zero: the other three registers are + * what zero would cost. `BigInt` throws on a fraction and on anything that is + * not digits, and `ServerRegisterEntrySchema` takes a fractional number for a + * 64 bit type, so a hand-edited config reaches this. + */ +export const toExact64Bits = (value: ServerRegisterValue | bigint): bigint | undefined => { + try { + return BigInt(value) + } catch { + return undefined + } +} + +/** + * The types `createRegisters` throws on for a value outside their range. + * + * Measured against the writers it calls. `writeUInt16BE(1.5)` and + * `writeInt32BE(1.5)` truncate, `writeFloatBE(1e300)` and `writeDoubleBE` take + * anything, `unix` is written through `value >>> 0` and `datetime` through + * `encodeIEC870DateTime`, which clamps to the format's own window. What is left + * is the seven below, where `writeUInt32BE(-1)` and + * `writeBigUInt64BE(2n ** 64n)` both answer ERR_OUT_OF_RANGE. `none` and `utf8` + * are not `EncodableDataType` and never reach the encoder at all. + */ +const RANGE_CHECKED_TYPES: readonly DataType[] = [ + 'int16', + 'uint16', + 'bitmap', + 'int32', + 'uint32', + 'int64', + 'uint64' +] + +/** + * Why `createRegisters` cannot encode `value` as `dataType`, or nothing. + * + * `RegisterParamsSchema` bounded the address and left the value bare, so a + * config file carrying `{ dataType: 'uint16', value: 70000 }` reached + * `writeUInt16BE` and threw out of `addRegister`. The ranges are + * `getMinMaxValues`, which the add dialog's mask already reads, so the file and + * the field now answer the same question. + * + * A 64 bit value also has to be a whole number, because `BigInt(1.5)` throws + * where `writeUInt16BE(1.5)` truncates. + */ +export const getValueRangeError = (dataType: DataType, value: number): string | undefined => { + if (!RANGE_CHECKED_TYPES.includes(dataType)) return undefined + + if ((dataType === 'int64' || dataType === 'uint64') && !Number.isInteger(value)) { + return `A ${dataType} value has to be a whole number` + } + + const { min, max } = getMinMaxValues(dataType) + if (value < min || value > max) { + return `A ${dataType} value has to be between ${min} and ${max}` + } + + return undefined +} + /** * Whether the mask has taken anything a number could be made of. *