diff --git a/src/backend/editor/compiler/editor-compiler-platform-port.ts b/src/backend/editor/compiler/editor-compiler-platform-port.ts index 8b689d18d..8c78488b2 100644 --- a/src/backend/editor/compiler/editor-compiler-platform-port.ts +++ b/src/backend/editor/compiler/editor-compiler-platform-port.ts @@ -24,7 +24,7 @@ * `middleware/adapters/web/`. */ -import { deployRuntimeProgram } from '@root/backend/shared/library/deploy-runtime-program' +import { deployReachedDevice, deployRuntimeProgram } from '@root/backend/shared/library/deploy-runtime-program' import { probeRuntimeVersion } from '@root/backend/shared/library/probe-runtime-version' import { fromSchemaShape, @@ -387,7 +387,11 @@ export function createEditorCompilerPlatformPort( startIntervalMs: context.startIntervalMs, }) - return { ok: deployOutcome === 'STARTED' } + // 'STARTED' is not the only success: a runtime that refused to start + // because its hardware mode switch reads STOP has still taken the + // program. deployReachedDevice keeps that judgement in one place, shared + // with the web adapter. + return { ok: deployReachedDevice(deployOutcome) } } catch (error) { const message = error instanceof Error ? error.message : String(error) log(`Runtime v4 upload failed: ${message}`, 'error') @@ -486,7 +490,11 @@ export function createEditorCompilerPlatformPort( startTimeoutMs: context.startTimeoutMs, startIntervalMs: context.startIntervalMs, }) - return { ok: deployOutcome === 'STARTED' } + // 'STARTED' is not the only success: a runtime that refused to start + // because its hardware mode switch reads STOP has still taken the + // program. deployReachedDevice keeps that judgement in one place, shared + // with the web adapter. + return { ok: deployReachedDevice(deployOutcome) } } catch (error) { const message = error instanceof Error ? error.message : String(error) log(`Runtime v3 upload failed: ${message}`, 'error') diff --git a/src/backend/shared/library/__tests__/deploy-runtime-program.test.ts b/src/backend/shared/library/__tests__/deploy-runtime-program.test.ts index e19623923..78486b345 100644 --- a/src/backend/shared/library/__tests__/deploy-runtime-program.test.ts +++ b/src/backend/shared/library/__tests__/deploy-runtime-program.test.ts @@ -1,4 +1,4 @@ -import { deployRuntimeProgram } from '../deploy-runtime-program' +import { deployReachedDevice, type DeployRuntimeProgramOutcome, deployRuntimeProgram } from '../deploy-runtime-program' import type { RuntimeCompilationStatus } from '../poll-runtime-compilation' function makeStatusFetcher(...responses: Array) { @@ -133,4 +133,84 @@ describe('deployRuntimeProgram', () => { }) expect(outcome).toBe('START_TIMEOUT') }) + + it('returns UPLOADED_NOT_STARTED when the hardware mode switch declines the start', async () => { + const start = jest.fn(makeStartFetcher('START:ERROR_SWITCH_STOP')) + const logs: Array<{ level: string; message: string }> = [] + + const outcome = await deployRuntimeProgram({ + uploadProgram: async () => ({ success: true }), + fetchCompilationStatus: makeStatusFetcher({ status: 'SUCCESS', logs: [], exit_code: 0 }), + fetchStartResponse: start, + onLog: (level, message) => logs.push({ level, message }), + pollIntervalMs: 1, + startIntervalMs: 1, + }) + + expect(outcome).toBe('UPLOADED_NOT_STARTED') + // Asked once and accepted the answer: nothing changes until someone moves + // the switch, so retrying until the deadline would only stall the deploy. + expect(start).toHaveBeenCalledTimes(1) + // Reported as a warning, never an error — the program is on the device. + expect(logs.some((l) => l.level === 'warning')).toBe(true) + expect(logs.some((l) => l.level === 'error')).toBe(false) + }) +}) + +describe('deployReachedDevice', () => { + /** + * Exhaustive by construction: a new member of `DeployRuntimeProgramOutcome` + * fails to type-check here until someone decides which side of the + * "did the program reach the device?" line it falls on. + */ + const reachedDeviceByOutcome: Record = { + STARTED: true, + UPLOADED_NOT_STARTED: true, + UPLOAD_FAILED: false, + BUILD_FAILED: false, + BUILD_TIMEOUT: false, + BUILD_ERROR: false, + START_FAILED: false, + START_TIMEOUT: false, + } + + /** Typed walk over the table above — the length check below keeps the two in step. */ + const allOutcomes: DeployRuntimeProgramOutcome[] = [ + 'STARTED', + 'UPLOADED_NOT_STARTED', + 'UPLOAD_FAILED', + 'BUILD_FAILED', + 'BUILD_TIMEOUT', + 'BUILD_ERROR', + 'START_FAILED', + 'START_TIMEOUT', + ] + + it('classifies every outcome the deploy can produce', () => { + expect(allOutcomes).toHaveLength(Object.keys(reachedDeviceByOutcome).length) + }) + + it('is true when the runtime took the program and ran it', () => { + expect(deployReachedDevice('STARTED')).toBe(true) + }) + + it('is true when the runtime took the program but declined to run it', () => { + // The mode switch reads STOP. Reporting this as a failed upload sends the + // user looking for a problem that does not exist. + expect(deployReachedDevice('UPLOADED_NOT_STARTED')).toBe(true) + }) + + it('is false for every outcome where the program never landed', () => { + const failures = allOutcomes.filter((outcome) => !reachedDeviceByOutcome[outcome]) + expect(failures).toHaveLength(6) + for (const outcome of failures) { + expect(deployReachedDevice(outcome)).toBe(false) + } + }) + + it('agrees with the full outcome table', () => { + for (const outcome of allOutcomes) { + expect(deployReachedDevice(outcome)).toBe(reachedDeviceByOutcome[outcome]) + } + }) }) diff --git a/src/backend/shared/library/__tests__/start-plc-after-build.test.ts b/src/backend/shared/library/__tests__/start-plc-after-build.test.ts index 15d97ab21..6829b160a 100644 --- a/src/backend/shared/library/__tests__/start-plc-after-build.test.ts +++ b/src/backend/shared/library/__tests__/start-plc-after-build.test.ts @@ -54,6 +54,25 @@ describe('startPlcAfterBuild', () => { expect(calls()).toBe(3) }) + it('treats a mode switch in STOP as a warning, not a failure', async () => { + // The runtime refuses to start while a hardware switch reads STOP. The + // program is on the device, so this must NOT read as a failed upload -- that + // sent users hunting for a problem that did not exist. + const { fetch, calls } = scriptedFetch(['START:ERROR_SWITCH_STOP']) + const logs: Array<{ level: string; message: string }> = [] + const outcome = await startPlcAfterBuild({ + fetchStart: fetch, + onLog: (level, message) => logs.push({ level, message }), + pollIntervalMs: 1, + }) + expect(outcome).toBe('SWITCH_IN_STOP') + // Not retried: nothing changes until a human moves the switch. + expect(calls()).toBe(1) + expect(logs[logs.length - 1].level).toBe('warning') + expect(logs[logs.length - 1].message).toMatch(/uploaded/i) + expect(logs[logs.length - 1].message).toMatch(/switch is in STOP/i) + }) + it('bails with FAILED on a non-BUSY error reply', async () => { const { fetch, calls } = scriptedFetch(['ERROR:INVALID_PROGRAM']) const logs: Array<{ level: string; message: string }> = [] diff --git a/src/backend/shared/library/deploy-runtime-program.ts b/src/backend/shared/library/deploy-runtime-program.ts index 783ae5676..2ae037970 100644 --- a/src/backend/shared/library/deploy-runtime-program.ts +++ b/src/backend/shared/library/deploy-runtime-program.ts @@ -23,6 +23,7 @@ import { startPlcAfterBuild, type StartPlcAfterBuildOptions } from './start-plc- export type DeployRuntimeProgramOutcome = | 'STARTED' + | 'UPLOADED_NOT_STARTED' | 'UPLOAD_FAILED' | 'BUILD_FAILED' | 'BUILD_TIMEOUT' @@ -30,6 +31,23 @@ export type DeployRuntimeProgramOutcome = | 'START_FAILED' | 'START_TIMEOUT' +/** + * Did the program reach the device? + * + * Distinct from "did it start", because those are different questions and the + * upload step must only fail for the first one. A runtime that refuses to start + * while its hardware mode switch reads STOP has still taken the program, and + * reporting that as a failed upload sends the user looking for a problem that + * does not exist. + * + * Shared rather than re-derived per platform: the editor and web adapters both + * turn this outcome into `UploadResult.ok`, and they must not drift on which + * outcomes count. + */ +export function deployReachedDevice(outcome: DeployRuntimeProgramOutcome): boolean { + return outcome === 'STARTED' || outcome === 'UPLOADED_NOT_STARTED' +} + export type DeployRuntimeProgramLogLevel = 'info' | 'error' | 'warning' | 'debug' export interface DeployRuntimeProgramOptions { @@ -109,6 +127,9 @@ export async function deployRuntimeProgram(opts: DeployRuntimeProgramOptions): P pollIntervalMs: opts.startIntervalMs, }) if (startOutcome === 'STARTED') return 'STARTED' + // Refused by the hardware mode switch: uploaded, deliberately not running. + // startPlcAfterBuild has already explained it as a warning. + if (startOutcome === 'SWITCH_IN_STOP') return 'UPLOADED_NOT_STARTED' if (startOutcome === 'TIMEOUT') return 'START_TIMEOUT' return 'START_FAILED' } diff --git a/src/backend/shared/library/start-plc-after-build.ts b/src/backend/shared/library/start-plc-after-build.ts index 583b60a51..fda248154 100644 --- a/src/backend/shared/library/start-plc-after-build.ts +++ b/src/backend/shared/library/start-plc-after-build.ts @@ -21,12 +21,17 @@ * - 150-ms interval between attempts. * - `START:OK` or `ALREADY_RUNNING` in the runtime's reply → SUCCESS. * - `BUSY` in the reply → keep retrying. + * - `ERROR_SWITCH_STOP` → SWITCH_IN_STOP: not an error. The runtime + * refuses to start while a hardware mode switch reads STOP, and + * leaving the switch there is a normal thing for someone to do + * while uploading. The upload succeeded; only the start was + * declined, and the user decides when to allow it. * - Any other reply → terminal failure (e.g. invalid program). * - Network error on the fetch → terminal failure. * - Deadline elapsed → TIMEOUT (a warning, not a fatal error). */ -export type StartPlcAfterBuildOutcome = 'STARTED' | 'FAILED' | 'TIMEOUT' +export type StartPlcAfterBuildOutcome = 'STARTED' | 'FAILED' | 'TIMEOUT' | 'SWITCH_IN_STOP' export type StartPlcAfterBuildLogLevel = 'info' | 'error' | 'warning' @@ -74,6 +79,18 @@ export async function startPlcAfterBuild(opts: StartPlcAfterBuildOptions): Promi return 'STARTED' } + // A hardware mode switch in STOP is a refusal, not a failure: the + // program is on the device, and the runtime is doing exactly what it + // should by declining to run it until the switch says so. Retrying + // would be wrong too — nothing changes until someone moves it. + if (status.includes('ERROR_SWITCH_STOP')) { + opts.onLog( + 'warning', + 'Program uploaded. The PLC was not started because the mode switch is in STOP — move it to RUN to start.', + ) + return 'SWITCH_IN_STOP' + } + // Only BUSY is retryable — everything else is a real error // from the runtime (invalid program, compile error, …). if (!status.includes('BUSY')) { diff --git a/src/frontend/components/_features/[workspace]/editor/device/configuration/board.tsx b/src/frontend/components/_features/[workspace]/editor/device/configuration/board.tsx index 6a14c8240..51aaf936c 100644 --- a/src/frontend/components/_features/[workspace]/editor/device/configuration/board.tsx +++ b/src/frontend/components/_features/[workspace]/editor/device/configuration/board.tsx @@ -27,15 +27,6 @@ import { ScanCycleStats } from '../../../../../_molecules/scan-cycle-stats' import { DeviceEditorSlot } from '../../../../../_templates/[editors]/device-editor-slot' import { PinMappingTable } from './components/pin-mapping-table' -/** - * Confirms the held device link on the device screen: a quiet, monochrome line - * that appears once Connect has settled on a channel a firmware answered. - */ -function DeviceConnectedIndicator({ isConnected }: { isConnected: boolean }) { - if (!isConnected) return null - return Connected -} - const Board = memo(function () { const capabilities = useCapabilities() const device = useDevice() @@ -634,13 +625,8 @@ const Board = memo(function () { onConnect={handleConnectToRuntime} onDisconnect={handleConnectToRuntime} > - {connectionStatus === 'connected' && ( - <> - {plcStatus && ( - | PLC: {plcStatus} - )} - - + {connectionStatus === 'connected' && plcStatus && ( + PLC: {plcStatus} )} @@ -718,9 +704,7 @@ const Board = memo(function () { {...(!isConnected && !communicationPort && !modbusTcpConfigured ? { blockedReason: 'Select a communication port first' } : {})} - > - - + /> ) : null} {!isOpenPLCRuntimeTarget(currentBoardInfo) && !isSimulatorTarget(currentBoardInfo) && ( diff --git a/src/frontend/components/_molecules/device-connect-button/__tests__/device-connect-button.test.tsx b/src/frontend/components/_molecules/device-connect-button/__tests__/device-connect-button.test.tsx index 6fc9bdc76..e34c4ff6c 100644 --- a/src/frontend/components/_molecules/device-connect-button/__tests__/device-connect-button.test.tsx +++ b/src/frontend/components/_molecules/device-connect-button/__tests__/device-connect-button.test.tsx @@ -30,11 +30,12 @@ describe('DeviceConnectButton', () => { expect(onConnect).not.toHaveBeenCalled() }) - it('confirms a live connection on screen', () => { - // The baremetal copy never showed this, so a connected device looked the same - // as a disconnected one apart from the button label. + it('says Disconnect when connected, and nothing else', () => { + // The label IS the status: "Disconnect" can only appear while connected, so a + // separate "Connected" badge beside it was saying the same thing twice. render() - expect(screen.getByText('● Connected')).not.toBeNull() + expect(screen.getByRole('button').textContent).toBe('Disconnect') + expect(screen.queryByText(/Connected/)).toBeNull() }) it('reports a failed attempt', () => { diff --git a/src/frontend/components/_molecules/device-connect-button/index.tsx b/src/frontend/components/_molecules/device-connect-button/index.tsx index 796c59ac4..f2006a0c3 100644 --- a/src/frontend/components/_molecules/device-connect-button/index.tsx +++ b/src/frontend/components/_molecules/device-connect-button/index.tsx @@ -51,6 +51,10 @@ const DeviceConnectButton = ({ onClick={isConnected ? onDisconnect : onConnect} disabled={disabled} title={blockedReason ?? (isConnected ? 'Disconnect from the device' : 'Connect to the device')} + // Same classes main used for this button. They render correctly again now + // that cn() knows the cp-* font scale -- before that, twMerge dropped + // `text-cp-sm` as a conflict with `text-white` and the button jumped to the + // browser default size. className={cn( 'h-[30px] rounded-md bg-brand px-4 py-1 font-caption text-cp-sm font-medium text-white', 'hover:bg-brand-medium-dark disabled:opacity-50', @@ -59,7 +63,6 @@ const DeviceConnectButton = ({ {isConnecting ? 'Connecting...' : isConnected ? 'Disconnect' : 'Connect'} - {isConnected && ● Connected} {status === 'error' && ● Connection failed} {children} diff --git a/src/frontend/components/_organisms/workspace-activity-bar/default.tsx b/src/frontend/components/_organisms/workspace-activity-bar/default.tsx index f50a79eac..47a547008 100644 --- a/src/frontend/components/_organisms/workspace-activity-bar/default.tsx +++ b/src/frontend/components/_organisms/workspace-activity-bar/default.tsx @@ -104,11 +104,29 @@ export const DefaultWorkspaceActivityBar = ({ zoom }: DefaultWorkspaceActivityBa // reason, and says so. `handlePlcControl` refuses such a target anyway, so // without this the button looked live and the click did nothing at all — no // command, no error, no log line. + // + // A transition already in flight blocks it for a third reason. TRANSITIONING + // means the runtime has a start or stop underway: it answers COMMAND:BUSY to + // everything except PING and STATUS, and the state it will settle on is not + // decided yet, so the icon is drawn from a state that is about to change. + // Clicking then cannot do what it appears to. + // + // The reason chain runs in the same order as the blocks it explains, most + // fundamental first: a target that cannot do run/stop at all, then no session + // to send over, then a transition in flight. Asking about the transition first + // would answer "PLC is changing state..." to someone who is not connected, + // reporting a state we last saw rather than the reason the button is inert — + // `plcStatus` is polled and survives the drop. Reached only when blocked, so + // the tail needs no test of its own: supported and connected and still blocked + // leaves exactly one reason. const plcStateControlSupported = resolveTargetCapabilities(currentBoardInfo).plcStateControl - const plcControlBlocked = !plcStateControlSupported || deviceConnectionStatus !== 'connected' - const plcControlBlockedReason = plcStateControlSupported - ? 'Connect to the target first' - : 'This target does not support Start/Stop from the editor' + const plcTransitioning = plcStatus === 'TRANSITIONING' + const plcControlBlocked = !plcStateControlSupported || deviceConnectionStatus !== 'connected' || plcTransitioning + const plcControlBlockedReason = !plcStateControlSupported + ? 'This target does not support Start/Stop from the editor' + : deviceConnectionStatus !== 'connected' + ? 'Connect to the target first' + : 'PLC is changing state...' // The emulator stopping is a session ending, and a debug session riding it ends // with it — which the drop handler below already does for every target. This @@ -547,6 +565,11 @@ export const DefaultWorkspaceActivityBar = ({ zoom }: DefaultWorkspaceActivityBa // `switchPosition` in the store, so the pre-check is a store lookup rather than // another round trip over a medium the poll is already using. try { + // The button is disabled while a transition is in flight; this covers the + // window before the next status poll catches up, and any caller that is not + // the click. + if (plcStatus === 'TRANSITIONING') return + const wantRun = plcStatus !== 'RUNNING' // Never send a start to a device whose switch reads STOP. `null` means diff --git a/src/frontend/utils/__tests__/cn.test.ts b/src/frontend/utils/__tests__/cn.test.ts index 087bbc300..e19d522a6 100644 --- a/src/frontend/utils/__tests__/cn.test.ts +++ b/src/frontend/utils/__tests__/cn.test.ts @@ -1,5 +1,25 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' + import { cn } from '../cn' +/** + * The `cp-*` font scale is declared twice: once in `tailwind.config.ts`, where + * it generates the classes, and once inside `cn()`, where twMerge has to be + * told those names are sizes. Read the scale out of the config rather than + * restating it here, so the "keep this list in step" comment on `cn()` is + * enforced by this suite instead of hoped for — a fourth size added to + * Tailwind and forgotten in `cn()` fails below rather than silently + * evaporating at runtime, which is exactly how the Connect button lost its + * `text-cp-sm` with no warning and no build error. + */ +function tailwindFontSizeNames(): string[] { + const config = readFileSync(join(process.cwd(), 'tailwind.config.ts'), 'utf8') + const block = /fontSize:\s*\{([\s\S]*?)\n\s*\},/.exec(config) + if (!block) throw new Error('No `fontSize` block found in tailwind.config.ts') + return [...block[1].matchAll(/'([\w-]+)':/g)].map(([, name]) => name) +} + describe('cn', () => { it('merges class names', () => { expect(cn('foo', 'bar')).toBe('foo bar') @@ -26,3 +46,38 @@ describe('cn', () => { expect(cn(['foo', 'bar'])).toBe('foo bar') }) }) + +describe('cn — the custom cp-* font scale', () => { + const fontSizes = tailwindFontSizeNames() + + it('reads the scale out of tailwind.config.ts', () => { + expect(fontSizes).toEqual(['cp-xs', 'cp-sm', 'cp-base']) + }) + + it('pins the reported regression: text-cp-sm survives beside text-white', () => { + // Plain twMerge reads both as colours and drops the size, leaving the + // Connect button at the browser default. + expect(cn('text-cp-sm', 'text-white')).toBe('text-cp-sm text-white') + }) + + it('pins the other half: two cp-* sizes conflict, and the last one wins', () => { + expect(cn('text-cp-xs', 'text-cp-base')).toBe('text-cp-base') + }) + + it.each(fontSizes)('keeps text-%s beside a text colour', (size) => { + expect(cn(`text-${size}`, 'text-white')).toBe(`text-${size} text-white`) + }) + + it('resolves any pair of cp-* sizes to the later one', () => { + for (const first of fontSizes) { + for (const second of fontSizes) { + if (first === second) continue + expect(cn(`text-${first}`, `text-${second}`)).toBe(`text-${second}`) + } + } + }) + + it('still lets a cp-* size override a stock Tailwind size', () => { + expect(cn('text-sm', 'text-cp-base')).toBe('text-cp-base') + }) +}) diff --git a/src/frontend/utils/cn.ts b/src/frontend/utils/cn.ts index c8498c564..7916262b7 100644 --- a/src/frontend/utils/cn.ts +++ b/src/frontend/utils/cn.ts @@ -1,4 +1,26 @@ import { ClassValue, clsx } from 'clsx' -import { twMerge } from 'tailwind-merge' +import { extendTailwindMerge } from 'tailwind-merge' + +/** + * Class merger that knows this project's custom `cp-*` font scale. + * + * Plain `twMerge` does not. Faced with `text-cp-sm text-white` it cannot tell that + * the first is a size, assumes both are text colours, and keeps only the last -- + * silently dropping the size. Anything styled that way rendered at the browser + * default instead: the Connect button came out visibly larger than its + * neighbours, with no warning and no build error. Buttons that escaped it did so + * only by passing a plain string instead of calling this. + * + * Declaring the scale here fixes every call site at once, rather than each caller + * having to know that its size class might evaporate. Keep this list in step with + * `fontSize` in tailwind.config.ts. + */ +const twMerge = extendTailwindMerge({ + extend: { + classGroups: { + 'font-size': [{ text: ['cp-xs', 'cp-sm', 'cp-base'] }], + }, + }, +}) export const cn = (...inputs: ClassValue[]) => twMerge(clsx(inputs))