Refactor for readability and stability (Phase 1+2) - #10
Conversation
…nify concurrency, harden parsers Co-Authored-By: Yoshihiro Yamashiro <me@xn--uist1idrju3i.jp>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…dempotent, adopt session tracking in flash controller, update AI_REVIEW.md module list Co-Authored-By: Yoshihiro Yamashiro <me@xn--uist1idrju3i.jp>
| 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; | ||
| } |
There was a problem hiding this comment.
🔴 Flash and Recover operations are blocked when RTT is connected, instead of auto-disconnecting RTT first
The operation lock is acquired (tryAcquire(type, ...) at public/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() in public/js/app/rtt-controller.js:86 acquires the lock as 'RTT'. When the user clicks Flash, runOperation() is called with type = 'FLASH'.
At public/js/app/flash-controller.js:59:
if (!operationLock.tryAcquire(type, `run${title}`)) {
log(`Cannot start ${title}: ...`, 'warning');
return; // <-- exits here
}Since the lock is 'RTT' and the requested type is 'FLASH', tryAcquire returns false (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
In flash-controller.js runOperation(), the lock acquisition at line 59 prevents reaching the RTT auto-disconnect code at lines 72-79. When RTT is connected, the RTT controller holds the operation lock as 'RTT', so tryAcquire('FLASH') fails and the function returns early.
The fix is to restructure the flow so that RTT disconnection happens before the lock is acquired for the flash/recover operation. One approach:
1. Before the lock acquisition, check if RTT is connected.
2. If so, disconnect RTT first (which releases the 'RTT' lock).
3. Then acquire the flash/recover lock.
Alternatively, the initial lock check could be made RTT-aware: if the current lock is 'RTT' and the requested operation is 'FLASH' or 'RECOVER', auto-disconnect RTT first, then acquire the new lock.
The re-acquire logic at line 76 can remain as a safety check after the disconnect, but the primary lock acquisition should come after the RTT teardown.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is pre-existing behavior preserved by the refactor, not a regression: the original runFlash/runRecover also called operationLock.tryAcquire() before the RTT-disconnect block (see main.js on main, where tryAcquire('FLASH', ...) returns before the disconnectRtt() 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 inside runOperation() 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.
E2E UI test resultsServed this branch locally ( Limitation: no physical CMSIS-DAP probe available, so real-device Flash/Recover/RTT flows are untested — hardware validation before merge is recommended.
Key evidenceGraceful failure with no device (lock released, UI usable)Settings restored after reloadTested by Devin session |
1 participant
Summary
Phase 1 (readability) + Phase 2 (stability) of the refactoring plan. Behavior-preserving; no protocol/performance changes (Phase 3 deferred).
Phase 1 — module split.
main.js(1,774 lines) is reduced to a wiring layer (~530 lines):ui/logger.js— timestamped log, now bounded toMAX_LOG_LINES(2000) DOM nodesui/step-progress.js— step preview/progress state + reset timer, previously module-level globals inmain.jsui/settings.js—bindPersistedSettings([{element, key, kind, onChange}])replaces ~20 copies of restore/savetry/catchboilerplate;core/storage.jswrapslocalStoragesafelyapp/flash-controller.js—runFlash/runRecovernow share onerunOperation({type, title, steps, middle}); the flash/verify steps are injected as amiddlecallback, eliminating the duplicated lock/RTT-teardown/connect/erase/reset/cleanup sequenceapp/rtt-controller.js— RTT lifecycle + terminal + utility ops; utilities share awithRttConnection(name, fn)guard (also fixes thestatevariable shadowing ingetCoreState())Phase 2 — stability.
OperationLock(app/operation-lock.js); the redundantisOperationInProgressflag andsetExternalOperationInProgresscalls are removed (polling is stopped during operations instead)app/connection-session.jsowns the transport and every DAP object created on it;dispose()disconnects them all, each bounded byDAP_DISCONNECT_TIMEOUT_MSnavigator.usbdisconnectevents now tear down the active RTT session immediately (ConnectionSession.ownsDevice()matches the unpluggedUSBDevice;WebUSBTransport.getDevice()added)DAPjs.ADI.connect()/CortexM.connect()are bounded bywithTimeout(..., DAP_CONNECT_TIMEOUT_MS); timing magic numbers centralized incore/constants.jsStateManager: poll loop captures the abort signal locally (astopPolling()during an await could null_abortControllermid-loop), and_handleErrornow awaits_onCleanup()so polling can't race a teardown in flighthex-parser.js: rejects non-hex characters, record-length mismatches, and images spanning >32 MB; builds the buffer from record segments instead of one object per bytertt-handler.js: control-block sanity checks (buffer counts ≤16, descriptors within the scanned block, byte-aligned signature match) and rejects corruptWrOff/RdOff≥SizeOfBufferbefore dereferencing themdap-operations.js: proxy discovery consolidated into cachedgetTransferProxy()(WeakMap per ADI instance);rawDapTransferWriteretries bounded times on ACK WAIT;sleepmoved tocore/async-utils.js(re-exported for compatibility)Testing
npm run lint,npm run lint:html,npm run lint:jsonall passLink to Devin session: https://app.devin.ai/sessions/97470cdb2191400699ca1fe8a40cb0f1
Requested by: @uist1idrju3i