-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor for readability and stability (Phase 1+2) #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
uist1idrju3i
merged 2 commits into
main
from
devin/1783819432-refactor-stability-readability
Aug 16, 2026
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| // Connection session - owns the transport and every DAP-layer object created | ||
| // on top of it for one Flash/Recover/RTT session, so cleanup is a single | ||
| // dispose() call instead of scattered nullable globals. | ||
|
|
||
| import { WebUSBTransport } from '../transport/webusb-transport.js'; | ||
| import { withTimeout } from '../core/async-utils.js'; | ||
| import { DAP_DISCONNECT_TIMEOUT_MS } from '../core/constants.js'; | ||
|
|
||
| export class ConnectionSession { | ||
| constructor() { | ||
| this._transport = null; | ||
| this._disconnectables = []; | ||
| } | ||
|
|
||
| /** | ||
| * Prompt for a device and open the WebUSB transport | ||
| * @param {Array<{vendorId: number}>} usbFilters - Probe USB filters | ||
| * @param {object} options - selectDevice options (e.g. skipProbeCheck) | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async open(usbFilters, options) { | ||
| this._transport = new WebUSBTransport(); | ||
| await this._transport.selectDevice(usbFilters, options); | ||
| } | ||
|
|
||
| /** | ||
| * Get the underlying DAPjs transport object | ||
| * @returns {object} DAPjs.WebUSB transport | ||
| */ | ||
| getTransport() { | ||
| return this._transport.getTransport(); | ||
| } | ||
|
|
||
| /** | ||
| * Get a human-readable device name | ||
| * @returns {string} | ||
| */ | ||
| getDeviceName() { | ||
| return this._transport ? this._transport.getDeviceName() : 'No device'; | ||
| } | ||
|
|
||
| /** | ||
| * Check whether this session owns a given USBDevice (used to match | ||
| * navigator.usb 'disconnect' events to the active session) | ||
| * @param {USBDevice} usbDevice - Device from a WebUSB event | ||
| * @returns {boolean} | ||
| */ | ||
| ownsDevice(usbDevice) { | ||
| return !!this._transport && this._transport.getDevice() === usbDevice; | ||
| } | ||
|
|
||
| /** | ||
| * Register a DAP-layer object (DAPjs.ADI, DAPjs.CortexM, ...) whose | ||
| * disconnect() must run when the session is disposed | ||
| * @param {object} dapObject - Object with an async disconnect() method | ||
| * @returns {object} The same object, for chaining | ||
| */ | ||
| track(dapObject) { | ||
| this._disconnectables.push(dapObject); | ||
| return dapObject; | ||
| } | ||
|
|
||
| /** | ||
| * Stop tracking a DAP object that the caller has already disconnected, | ||
| * so dispose() does not disconnect it a second time | ||
| * @param {object} dapObject - Object previously passed to track() | ||
| */ | ||
| untrack(dapObject) { | ||
| this._disconnectables = this._disconnectables.filter(o => o !== dapObject); | ||
| } | ||
|
|
||
| /** | ||
| * Disconnect all tracked DAP objects (bounded by a timeout each) and drop | ||
| * the transport. Safe to call multiple times; errors are swallowed because | ||
| * dispose runs in cleanup paths where the device may already be gone. | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async dispose() { | ||
| // Detach the tracked list up front so a concurrent dispose() sees an | ||
| // empty session instead of iterating an array being mutated here. | ||
| const disconnectables = this._disconnectables; | ||
| this._disconnectables = []; | ||
| this._transport = null; | ||
| for (const obj of disconnectables.reverse()) { | ||
| try { | ||
| await withTimeout(obj.disconnect(), DAP_DISCONNECT_TIMEOUT_MS, 'DAP disconnect'); | ||
| } catch (_) { /* ignore: device may already be gone */ } | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,222 @@ | ||
| // Flash / Recover controller. | ||
| // Both operations share the same orchestration (lock, RTT teardown, connect, | ||
| // mass erase, reset, cleanup); runOperation() implements it once and the | ||
| // flash-specific middle steps are injected as a callback. | ||
|
|
||
| import { sleep, withTimeout } from '../core/async-utils.js'; | ||
| import { DAP_CONNECT_TIMEOUT_MS, DAP_RECONNECT_DELAY_MS } from '../core/constants.js'; | ||
| import { log, clearLog } from '../ui/logger.js'; | ||
| import { | ||
| initStepProgress, | ||
| activateStep, | ||
| updateStepProgress, | ||
| completeStep, | ||
| failStep, | ||
| resetStepProgress, | ||
| scheduleStepReset, | ||
| cancelScheduledStepReset, | ||
| isStepProgressVisible | ||
| } from '../ui/step-progress.js'; | ||
| import { operationLock } from './operation-lock.js'; | ||
| import { ConnectionSession } from './connection-session.js'; | ||
|
|
||
| // Step definitions for each operation mode | ||
| export const FLASH_STEPS_VERIFY = ['🔌 Connect', '🗑️ Mass Erase', '📤 Flash', '✅ Verify', '🔄 Reset']; | ||
| export const FLASH_STEPS_NO_VERIFY = ['🔌 Connect', '🗑️ Mass Erase', '📤 Flash', '🔄 Reset']; | ||
| export const RECOVER_STEPS = ['🔌 Connect', '🗑️ Mass Erase', '🔄 Reset']; | ||
|
|
||
| /** | ||
| * Create the flash/recover controller | ||
| * @param {object} ctx - Dependencies injected by main.js: | ||
| * - dom {object} - DOM references (verifyCheckbox) | ||
| * - targetManager {TargetManager} | ||
| * - stateManager {StateManager} | ||
| * - rttController {object} - For disconnecting RTT before an operation | ||
| * - getSelectDeviceOptions {function} | ||
| * - getParsedFirmware {function} | ||
| * - updateStatus {function} | ||
| * - setButtonsEnabled {function} | ||
| * @returns {object} Controller API: { runFlash, runRecover } | ||
| */ | ||
| export function createFlashController(ctx) { | ||
| const { targetManager, stateManager, rttController } = ctx; | ||
|
|
||
| /** | ||
| * Shared runner for Flash and Recover. | ||
| * | ||
| * Sequence: acquire lock -> disconnect RTT -> connect -> mass erase -> | ||
| * [middle steps] -> reset -> disconnect -> cleanup. | ||
| * @param {object} config - Operation configuration | ||
| * @param {string} config.type - 'FLASH' or 'RECOVER' (lock type) | ||
| * @param {string} config.title - Log banner title | ||
| * @param {Array<string>} config.steps - Step names for the progress UI | ||
| * @param {function} [config.middle] - Optional async callback executed | ||
| * between mass erase and reset. Receives ({ handler, dap, session, | ||
| * stepper }) and must return the DAP instance to use for the remaining | ||
| * steps (it may create a fresh one). | ||
| */ | ||
| async function runOperation({ type, title, steps, middle }) { | ||
| if (!operationLock.tryAcquire(type, `run${title}`)) { | ||
| log(`Cannot start ${title}: ${operationLock.getCurrentLock()} operation is in progress`, 'warning'); | ||
| return; | ||
| } | ||
|
|
||
| // If step progress is already visible, clear it immediately | ||
| if (isStepProgressVisible()) { | ||
| log('Clearing previous operation progress...', 'info'); | ||
| cancelScheduledStepReset(); | ||
| resetStepProgress(); | ||
| } | ||
|
|
||
| // Disconnect RTT if connected | ||
| const wasRttConnected = stateManager.getState().isRttConnected; | ||
| if (wasRttConnected) { | ||
| log(`RTT is connected, disconnecting for ${title.toLowerCase()} operation...`, 'info'); | ||
| await rttController.disconnectRtt(); | ||
| if (!operationLock.tryAcquire(type, `run${title}`)) { | ||
| log(`Cannot start ${title}: ${operationLock.getCurrentLock()} operation is in progress`, 'warning'); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Stop StateManager polling during the operation | ||
| stateManager.stopPolling(); | ||
|
|
||
| clearLog(); | ||
| ctx.setButtonsEnabled(false); | ||
|
|
||
| initStepProgress(steps); | ||
|
|
||
| const session = new ConnectionSession(); | ||
|
devin-ai-integration[bot] marked this conversation as resolved.
|
||
| let dap; | ||
| let stepIdx = 0; | ||
|
|
||
| // Small helper so step bookkeeping cannot drift between operations | ||
| const stepper = { | ||
| get index() { return stepIdx; }, | ||
| begin() { activateStep(stepIdx); }, | ||
| done() { completeStep(stepIdx); stepIdx++; }, | ||
| progress(p, text) { updateStepProgress(stepIdx, p, text); } | ||
| }; | ||
|
|
||
| try { | ||
| // Step: Connect | ||
| stepper.begin(); | ||
| log(`=== ${title} Operation ===`, 'info'); | ||
|
|
||
| ctx.updateStatus('Selecting device...', false, true, 'Connecting'); | ||
| await session.open(targetManager.getUsbFilters(), ctx.getSelectDeviceOptions()); | ||
|
|
||
| const deviceName = session.getDeviceName(); | ||
| log(`Device selected: ${deviceName}`, 'success'); | ||
| ctx.updateStatus(`Connected: ${deviceName}`, true, true, 'Mass Erasing'); | ||
|
|
||
| const handler = targetManager.createHandler(log); | ||
| dap = session.track(new DAPjs.ADI(session.getTransport())); | ||
| await withTimeout(dap.connect(), DAP_CONNECT_TIMEOUT_MS, 'DAP connect'); | ||
| log('DAP connected', 'success'); | ||
| stepper.done(); | ||
|
|
||
| // Step: Mass Erase | ||
| stepper.begin(); | ||
| dap = await handler.recover(dap, (p) => stepper.progress(p)); | ||
| stepper.done(); | ||
|
|
||
| // Middle steps (Flash / Verify) | ||
| if (middle) { | ||
| dap = await middle({ handler, dap, session, stepper }); | ||
| } | ||
|
|
||
| // Step: Reset | ||
| stepper.begin(); | ||
| await handler.reset(dap); | ||
| stepper.done(); | ||
|
|
||
| log('Disconnecting...', 'info'); | ||
| await session.dispose(); | ||
| ctx.updateStatus('Operation completed', true, false); | ||
| log(`=== ${title} Completed Successfully ===`, 'success'); | ||
|
|
||
| // Notify user to manually reconnect RTT if it was connected before | ||
| if (wasRttConnected) { | ||
| log(`RTT was disconnected for ${title.toLowerCase()} operation. Click "Connect RTT" to reconnect.`, 'info'); | ||
| } | ||
|
|
||
| } catch (error) { | ||
| log(`Error: ${error.message}`, 'error'); | ||
| failStep(stepIdx); | ||
| ctx.updateStatus('Operation failed', false, false); | ||
| } finally { | ||
| await session.dispose(); | ||
|
|
||
| ctx.setButtonsEnabled(true); | ||
|
|
||
| operationLock.release(type); | ||
|
|
||
| // Ensure StateManager is properly cleaned up | ||
| stateManager.setRttConnected(false); | ||
| stateManager.setDeviceConnected(false); | ||
| stateManager.setRttComponents(null, null); | ||
| stateManager.stopPolling(); | ||
|
|
||
| scheduleStepReset(); | ||
| } | ||
| } | ||
|
|
||
| async function runFlash() { | ||
| const parsedFirmware = ctx.getParsedFirmware(); | ||
| if (!parsedFirmware) { | ||
| log('Please select a firmware file first', 'warning'); | ||
| return; | ||
| } | ||
|
|
||
| const verify = ctx.dom.verifyCheckbox.checked && targetManager.hasCapability('verify'); | ||
| const steps = verify ? [...FLASH_STEPS_VERIFY] : [...FLASH_STEPS_NO_VERIFY]; | ||
|
|
||
| await runOperation({ | ||
| type: 'FLASH', | ||
| title: 'Flash', | ||
| steps, | ||
| middle: async ({ handler, dap, session, stepper }) => { | ||
| log(`Firmware: ${parsedFirmware.size} bytes at 0x${parsedFirmware.startAddress.toString(16)}`, 'info'); | ||
|
|
||
| // Step: Flash | ||
| stepper.begin(); | ||
| log('Creating fresh DAP connection for flashing...', 'info'); | ||
| const transport = session.getTransport(); | ||
| await dap.disconnect(); | ||
| session.untrack(dap); | ||
| await sleep(DAP_RECONNECT_DELAY_MS); | ||
| const flashDap = session.track(await handler.createFreshDap(transport)); | ||
| await sleep(DAP_RECONNECT_DELAY_MS); | ||
|
|
||
| await handler.flash(flashDap, parsedFirmware.data, parsedFirmware.startAddress, | ||
| (p) => stepper.progress(p, `Flashing: ${Math.round(p)}%`)); | ||
| stepper.done(); | ||
|
|
||
| // Step: Verify (optional) | ||
| if (verify) { | ||
| stepper.begin(); | ||
| const result = await handler.verify(flashDap, parsedFirmware.data, parsedFirmware.startAddress, | ||
| (p) => stepper.progress(p, `Verifying: ${Math.round(p)}%`)); | ||
| if (!result.success) { | ||
| throw new Error(`Verification failed: ${result.mismatches} mismatches`); | ||
| } | ||
| stepper.done(); | ||
| } | ||
|
|
||
| return flashDap; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| async function runRecover() { | ||
| await runOperation({ | ||
| type: 'RECOVER', | ||
| title: 'Recover', | ||
| steps: [...RECOVER_STEPS] | ||
| }); | ||
| } | ||
|
|
||
| return { runFlash, runRecover }; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Flash and Recover operations are blocked when RTT is connected, instead of auto-disconnecting RTT first
The operation lock is acquired (
tryAcquire(type, ...)atpublic/js/app/flash-controller.js:59) before the code that auto-disconnects RTT (lines 72-79), so when RTT holds the lock the function exits early with a warning instead of tearing down RTT and proceeding.Impact: Users must manually disconnect RTT before flashing or recovering; clicking Flash/Recover while RTT is connected shows "Cannot start Flash: RTT operation is in progress" instead of auto-disconnecting.
Lock ordering prevents reaching the RTT disconnect code
When RTT is connected,
connectRtt()inpublic/js/app/rtt-controller.js:86acquires the lock as'RTT'. When the user clicks Flash,runOperation()is called withtype = 'FLASH'.At
public/js/app/flash-controller.js:59:Since the lock is
'RTT'and the requested type is'FLASH',tryAcquirereturnsfalse(public/js/app/operation-lock.js:25-28). The function returns before reaching the RTT disconnect code at lines 72-79.The author clearly intended the auto-disconnect to work, as evidenced by the re-acquire logic at line 76. The fix is to move the RTT disconnect check before the initial lock acquisition, or to check for RTT specifically and disconnect it before acquiring the flash lock.
Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pre-existing behavior preserved by the refactor, not a regression: the original
runFlash/runRecoveralso calledoperationLock.tryAcquire()before the RTT-disconnect block (seemain.jsonmain, wheretryAcquire('FLASH', ...)returns before thedisconnectRtt()call when RTT holds the lock).In practice the path is unreachable from the UI: while RTT is connected,
setButtonsEnabled(false)disables the Flash/Recover buttons (btnFlash.disabled = ... || currentLock === 'RTT'), so the user must disconnect RTT first by design. The RTT-disconnect block insiderunOperation()is a defensive fallback (e.g. if RTT was connected without the lock being held), and the re-acquire after it guards that case.Changing this to auto-disconnect RTT on Flash would be a deliberate UX change rather than a bug fix, so I'm leaving it as-is unless the maintainer wants that behavior.