Fix port event handling #4661
Fix port event handling #4661
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMultiple modules updated: MSP callbacks now run even on CRC errors; firmware_flasher gained debounced port-change handlers and public APIs; WebSerial uses a logger and adds recovery on InvalidStateError; Serial adds reconnection/wait logic; AutoDetect.verifyBoard became async with centralized cleanup; tests added/updated. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Firmware Flasher UI
participant EB as EventBus
participant AD as AutoDetect
note over UI,AD: Debounced port-change verification (PORT_CHANGE_DEBOUNCE_MS)
EB-->>UI: ports-input:change(port)
UI->>UI: clear pending portChangeTimer
UI->>UI: schedule verifyBoard() after debounce
UI->>AD: verifyBoard() (after debounce)
AD-->>UI: verification result
UI-->>User: update selection/logs
sequenceDiagram
participant App
participant WS as WebSerial
participant Port as navigator.serial.Port
note over WS,Port: Recovery when open() throws InvalidStateError
App->>WS: connect(path, options)
WS->>Port: open()
Port-->>WS: throws InvalidStateError
WS->>Port: attempt acquire reader/writer & getInfo()
alt reader & writer acquired
WS->>WS: set connected, start read loop
WS-->>App: dispatch connect(true)
else acquisition failed
WS-->>App: dispatch connect(false)
end
sequenceDiagram
participant Caller1
participant Caller2
participant Serial
participant Protocol
note over Serial: Concurrent connect handling + reconnection short-circuit
Caller1->>Serial: connect(path)
Serial->>Protocol: connect()
par while in-flight
Caller2->>Serial: connect(path)
Serial->>Serial: wait for connect/disconnect (timeout ~5s)
end
alt connected to requested path
Serial-->>Caller1: true
Serial-->>Caller2: true
else failure/path-mismatch
Serial->>Protocol: disconnect/reconnect as needed
Serial-->>Caller1: false
Serial-->>Caller2: false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/js/msp/MSPHelper.js(1 hunks)src/js/tabs/firmware_flasher.js(5 hunks)test/js/MSPHelper.test.js(3 hunks)test/js/firmware_flasher.test.js(1 hunks)test/js/msp_helpers.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
test/js/firmware_flasher.test.js (1)
src/js/tabs/firmware_flasher.js (1)
firmware_flasher(24-50)
src/js/tabs/firmware_flasher.js (3)
src/js/protocols/webstm32.js (1)
STM32(1007-1007)src/js/gui.js (1)
GUI(473-473)src/js/port_handler.js (2)
devicePath(110-110)PortHandler(16-50)
test/js/MSPHelper.test.js (1)
src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)
src/js/msp/MSPHelper.js (1)
src/js/msp.js (2)
data(74-74)data(355-355)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/js/msp/MSPHelper.js (1)
1782-1793: LGTM - Callback behavior change is intentional and well-structured.The refactoring to always invoke callbacks (regardless of
crcError) is a significant but intentional change, as noted in the comment. The try/catch wrapper provides good protection against callback errors.One minor observation: The warning at line 1792 is redundant with the warning at line 1768 (both log essentially the same CRC failure message). The warning at line 1768 is sufficient. However, if the intent is to emphasize that a callback was invoked despite the CRC error, consider making the message at line 1792 more specific, e.g.,
console.warn(\code: ${code} - callback invoked despite crc failure`);`src/js/tabs/firmware_flasher.js (1)
53-135: LGTM - Handler implementations are well-structured.The module-scoped handlers provide good separation of concerns:
- Device detection logic is centralized in
handleDetectedDevice- Port change debouncing prevents rapid re-entry
- Debounce timer is properly cleared before starting a new one
- The debounced callback re-checks
GUI.connect_lockto handle race conditionsThe logic at line 75 (
GUI.connect_lock &&= !wasReboot;) is correct but subtle. The comment helps, but consider expanding it slightly for future maintainers:- // Only clear the global connect lock when we are resuming from a reboot sequence. + // Only clear the global connect lock when we are resuming from a reboot sequence. + // If wasReboot is true, this clears connect_lock; otherwise, it leaves it unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/js/msp/MSPHelper.js(1 hunks)src/js/tabs/firmware_flasher.js(5 hunks)test/js/MSPHelper.test.js(5 hunks)test/js/firmware_flasher.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/js/MSPHelper.test.js
- test/js/firmware_flasher.test.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T20:41:42.451Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.
Applied to files:
src/js/msp/MSPHelper.js
🧬 Code graph analysis (1)
src/js/tabs/firmware_flasher.js (3)
src/js/protocols/webstm32.js (1)
STM32(1007-1007)src/js/gui.js (1)
GUI(473-473)src/js/port_handler.js (1)
PortHandler(16-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (5)
src/js/tabs/firmware_flasher.js (5)
43-51: LGTM - Clean module-scoped handler organization.Adding handler references to the firmware_flasher object is a good design choice that enables proper cleanup and testability.
835-844: LGTM - Handler wiring is properly sequenced.The wiring of module-scoped handlers to EventBus after DOM readiness is the correct approach. Exposing
clearBufferedFirmwareat line 837 provides the necessary access for both the module handlers and cleanup routines.
1478-1481: LGTM - startFlashing registration is clean.Binding
startFlashingto the instance and exposing it on the firmware_flasher object allows it to be invoked directly by the module-scoped handlers without indirection.
1566-1575: Verify if the 1-second delay is necessary.The auto-detect logic for already-connected devices is a good addition. However, the 1000ms delay at line 1574 seems arbitrary.
Consider whether a smaller delay (e.g., 100-300ms) would suffice, or if there's a specific reason for 1000ms. If the delay is necessary to wait for DOM elements or other initialization, the comment should be more specific about what is being waited for.
// If a specific initialization sequence requires this delay, document it: setTimeout(() => { AutoDetect.verifyBoard(); }, 1000); // Wait for port picker and select2 widgets to fully initialize
1595-1611: LGTM - Cleanup properly handles handlers and timers.The cleanup implementation correctly:
- Unregisters EventBus listeners without nulling handler references (preventing re-initialization issues as noted in past reviews)
- Clears the pending debounce timer to avoid callbacks firing after cleanup
This ensures resources are released while preserving the handler functions for future tab activations.
There was a problem hiding this comment.
All critical issues have been addressed! 🎉
The PR successfully fixes port event handling with:
✅ Comprehensive test coverage (4 new test suites)
✅ Proper error handling and state management
✅ Debouncing for port changes
✅ WebSerial recovery logic
✅ Serial connection concurrency handling
✅ All blocking issues resolved
Excellent work on this complex refactor!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/js/protocols/WebSerial.js (1)
241-261: Consider logging recovery attempt start for clarity.While warnings are logged when writer/reader acquisition fails (lines 251, 259), adding a log at the start of the recovery attempt would make the flow clearer in debug scenarios.
Add at line 241:
try { + logger.info(`${logHead} Attempting to recover already-open port`); const connectionInfo = this.port?.getInfo ? this.port.getInfo() : null;src/js/serial.js (2)
1-443: Consider migrating to centralized logger for consistency.
WebSerial.jshas been migrated to use the centralizedloggermodule (imported at line 3 of WebSerial.js), butserial.jsstill usesconsole.log/warn/errordirectly (e.g., lines 48, 177, 214, 294). For consistency, testability, and alignment with the PR's logging improvements, consider importing and using theloggermodule here as well.Example:
+import logger from "./logger"; import WebSerial from "./protocols/WebSerial.js"; import WebBluetooth from "./protocols/WebBluetooth.js";Then replace console calls:
- console.log(`${this.logHead} Already connected to the requested port`); + logger.info(`${this.logHead} Already connected to the requested port`);
234-237: Consider making timeout value configurable.The 5-second timeout is hardcoded. For different environments or use cases (e.g., slower devices, testing), a configurable timeout might be beneficial.
Example approach:
+ const CONNECT_WAIT_TIMEOUT_MS = 5000; // or make it a class property/config const timer = setTimeout(() => { cleanup(); resolve(null); - }, 5000); + }, CONNECT_WAIT_TIMEOUT_MS);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/js/logger.js(1 hunks)src/js/main.js(0 hunks)src/js/msp/MSPHelper.js(1 hunks)src/js/protocols/WebSerial.js(19 hunks)src/js/serial.js(4 hunks)src/js/tabs/firmware_flasher.js(5 hunks)src/js/utils/AutoDetect.js(3 hunks)test/js/MSPHelper.test.js(5 hunks)test/js/firmware_flasher.test.js(1 hunks)test/js/serial_connect.test.js(1 hunks)test/js/webserial.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/js/main.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/utils/AutoDetect.js
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.
Applied to files:
src/js/serial.jssrc/js/protocols/WebSerial.js
📚 Learning: 2025-09-19T20:41:42.451Z
Learnt from: haslinghuis
PR: betaflight/betaflight-configurator#4510
File: src/js/msp.js:377-382
Timestamp: 2025-09-19T20:41:42.451Z
Learning: In the MSP.send_message function in src/js/msp.js, the doCallbackOnError parameter was removed as it was never used in production code throughout the entire codebase.
Applied to files:
src/js/msp/MSPHelper.js
📚 Learning: 2025-09-02T07:45:48.606Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#4583
File: src/js/tabs/firmware_flasher.js:949-961
Timestamp: 2025-09-02T07:45:48.606Z
Learning: In src/js/tabs/firmware_flasher.js, the config file loading code path after firmware loading (in the load_file click handler) cannot be reached when UF2 firmware is loaded, according to the maintainer blckmn. This code is maintained for backward compatibility with unified target settings and will be refactored in a future PR.
Applied to files:
src/js/tabs/firmware_flasher.js
🧬 Code graph analysis (6)
src/js/msp/MSPHelper.js (2)
src/js/tabs/cli.js (1)
data(411-411)src/js/msp.js (2)
data(74-74)data(355-355)
test/js/serial_connect.test.js (1)
src/js/serial.js (2)
serial(443-443)serial(443-443)
test/js/firmware_flasher.test.js (3)
src/js/tabs/firmware_flasher.js (1)
firmware_flasher(27-145)src/js/gui.js (1)
GUI(473-473)src/js/protocols/webstm32.js (1)
STM32(1007-1007)
test/js/MSPHelper.test.js (2)
src/js/fc.js (1)
FC(132-1006)src/js/msp/MSPCodes.js (1)
MSPCodes(2-211)
src/js/protocols/WebSerial.js (1)
src/js/logger.js (1)
logger(3-7)
src/js/tabs/firmware_flasher.js (3)
src/js/protocols/webstm32.js (1)
STM32(1007-1007)src/js/gui.js (1)
GUI(473-473)src/js/port_handler.js (2)
devicePath(110-110)PortHandler(16-50)
🔇 Additional comments (6)
src/js/protocols/WebSerial.js (2)
3-3: LGTM: Centralized logging improves testability.The migration from console methods to the centralized logger module is clean and consistent throughout the file. This enhances testability and aligns with the PR's goal of introducing a test-friendly logger.
Also applies to: 61-61, 96-96, 123-123, 139-141, 154-154, 164-164, 177-177, 194-194, 206-206, 218-218, 223-223
225-312: LGTM: Recovery logic properly handles already-open ports.The recovery path for
InvalidStateErroris well-implemented with appropriate safeguards:
- Validates both reader and writer before declaring success (lines 263-264)
- Releases partial locks if recovery fails (lines 266-281), preventing port lockup
- Proper state initialization including event listeners, connection state, and read loop
- Graceful fallback to the normal failure path if recovery cannot proceed
This addresses Chromium 140+ compatibility issues where concurrent open attempts throw
InvalidStateErroron already-open ports. The defensive programming (optional chaining at line 242, try-catch around lock operations) ensures robust error handling.Based on past review feedback that has been addressed in commits 2719276 and 45422a5.
src/js/serial.js (4)
36-79: LGTM: Reconnection helper handles port switching correctly.The
_handleReconnectionmethod properly handles three scenarios:
- Already connected to requested port → returns
trueimmediately- Connected to different port → disconnects, then reconnects to new port
- Neither case → returns
undefinedto continue normal flowError handling is appropriate with try-catch wrapping and proper callback invocation. The use of
undefinedas a sentinel value to signal "continue normal flow" is clear and allows the mainconnectmethod to proceed with its standard logic.
161-161: LGTM: Simplified selectProtocol signature.Removing the default parameter and
forceDisconnectparameter simplifies the API. The breaking change is documented in the PR summary.
213-279: LGTM: Comprehensive in-flight wait logic prevents race conditions.The wait logic thoroughly handles concurrent connection attempts:
- Timeout protection (5s) prevents indefinite hangs (lines 234-237)
- State validation after timeout checks for lingering
openRequested(lines 254-258)- Port mismatch handling after wait disconnects if needed (lines 271-278)
- Proper cleanup of event listeners in all paths (lines 226-233)
This addresses race conditions where multiple callers attempt to connect simultaneously, ensuring they wait for in-flight attempts to complete rather than colliding.
Based on past review feedback that has been addressed in commits 97c806d, bcc1200, 0bb260e, c00a971, and 35d44cd.
283-317: LGTM: Robust error handling prevents stale protocol state.The try-catch around
protocol.connect()ensures:
- State cleanup on errors (openRequested, connected, connectionId reset)
- Event notification via disconnect event dispatch so waiters are unblocked
- Callback invocation with failure signal
- Graceful degradation with nested try-catch for cleanup operations
This prevents the protocol from getting stuck in an
openRequestedstate when connection fails, which would block future connection attempts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/protocols/WebSerial.js (1)
17-31: Use the shared logger inside the read loop helperEverything else in this module now routes through
logger, butstreamAsyncIterablestill writes straight toconsole.warn. Swapping those two calls tologger.warnkeeps logging consistent (and makes it easier to stub in tests).Apply this diff:
- console.warn(`${logHead} Read error in streamAsyncIterable:`, error); + logger.warn(`${logHead} Read error in streamAsyncIterable:`, error); @@ - console.warn(`${logHead} Error releasing reader lock:`, error); + logger.warn(`${logHead} Error releasing reader lock:`, error);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/js/logger.js(1 hunks)src/js/main.js(0 hunks)src/js/msp/MSPHelper.js(1 hunks)src/js/protocols/WebSerial.js(19 hunks)src/js/serial.js(4 hunks)src/js/tabs/firmware_flasher.js(5 hunks)src/js/utils/AutoDetect.js(3 hunks)test/js/MSPHelper.test.js(5 hunks)test/js/firmware_flasher.test.js(1 hunks)test/js/serial_connect.test.js(1 hunks)test/js/webserial.test.js(1 hunks)
💤 Files with no reviewable changes (1)
- src/js/main.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/js/logger.js
- test/js/webserial.test.js
- src/js/msp/MSPHelper.js
- test/js/firmware_flasher.test.js
- test/js/MSPHelper.test.js
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-02T07:45:48.606Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#4583
File: src/js/tabs/firmware_flasher.js:949-961
Timestamp: 2025-09-02T07:45:48.606Z
Learning: In src/js/tabs/firmware_flasher.js, the config file loading code path after firmware loading (in the load_file click handler) cannot be reached when UF2 firmware is loaded, according to the maintainer blckmn. This code is maintained for backward compatibility with unified target settings and will be refactored in a future PR.
Applied to files:
src/js/tabs/firmware_flasher.js
📚 Learning: 2025-06-19T22:13:09.136Z
Learnt from: blckmn
PR: betaflight/betaflight-configurator#4521
File: src/js/protocols/WebSerial.js:148-151
Timestamp: 2025-06-19T22:13:09.136Z
Learning: In WebSerial.js, there's a timing issue where the cached `this.ports` array doesn't immediately reflect newly permitted devices after `requestPermissionDevice()` completes. The `getDevices()` method needs to refresh the device list from the browser API to return accurate data immediately following a permission request and user acceptance.
Applied to files:
src/js/serial.jssrc/js/protocols/WebSerial.js
🧬 Code graph analysis (4)
src/js/tabs/firmware_flasher.js (3)
src/js/protocols/webstm32.js (1)
STM32(1007-1007)src/js/gui.js (1)
GUI(473-473)src/js/port_handler.js (2)
devicePath(110-110)PortHandler(16-50)
src/js/utils/AutoDetect.js (4)
src/js/port_handler.js (1)
PortHandler(16-50)src/js/gui.js (1)
TABS(8-8)src/js/gui_log.js (1)
gui_log(7-21)src/js/msp.js (1)
MSP(5-455)
test/js/serial_connect.test.js (2)
src/js/serial.js (2)
serial(440-440)serial(440-440)test/js/webserial.test.js (2)
result(83-83)result(149-149)
src/js/protocols/WebSerial.js (1)
src/js/logger.js (1)
logger(3-7)
|
OSD drag/drop is slow, but highly unlikely anything to do with this PR. |
nerdCopter
left a comment
nerdCopter
left a comment
There was a problem hiding this comment.
- approving after basic test. saw only 1 coderabbitai nitpick.
haslinghuis
force-pushed
the
ff-events
branch
2 times, most recently
from
c67ccf0 to
80bc37d
Compare
haslinghuis
force-pushed
the
ff-events
branch
from
80bc37d to
1d2bc31
Compare
|
|
Preview URL: https://pr4661.betaflight-app-preview.pages.dev |
|
No longer needed. |
3 participants
Purpose
This PR comprehensively fixes port event handling across the configurator, addressing several critical issues with connection management, event processing, and error recovery. The changes improve reliability when connecting to flight controllers, handling device removal, and managing concurrent connection attempts.
Key Issues Fixed
1. MSP Callback Handling with CRC Errors
Previously, callbacks were not notified when CRC errors occurred, leaving the application in an inconsistent state. Now callbacks receive the error flag along with preserved data, allowing proper error handling.
2. Port Event Debouncing and Race Conditions
The firmware flasher lacked proper debouncing for rapid port change events, leading to multiple simultaneous connection attempts. Implemented debouncing (100ms) and proper connection state guards.
3. WebSerial Connection Recovery
When a port was already open (e.g., from a previous failed connection), the configurator would fail to connect. Added recovery logic to attach to already-open ports by acquiring reader/writer streams.
4. Serial Connection Concurrency
Concurrent connection attempts to the same port could cause race conditions. Implemented proper queueing so the second caller waits for the first connection to complete.
5. Reconnection Logic
Improved handling of reconnection requests - if already connected to the requested port, the connection is preserved. If connected to a different port, proper disconnect/reconnect sequence is followed.
6. AutoDetect Preservation
Fixed AutoDetect to avoid disconnecting when a valid connection already exists, preventing unnecessary reconnection cycles.
Summary of Changes
Core Files Modified
src/js/msp/MSPHelper.js
process_datato always invoke callbacks, even with CRC errorscrcErrorflag for proper error handlingsrc/js/tabs/firmware_flasher.js
PORT_CHANGE_DEBOUNCE_MS(100ms),AUTO_DETECT_DELAY_MSdetectedUsbDevice,detectedSerialDevice,onPortChange,onDeviceRemovedsrc/js/protocols/WebSerial.js
src/js/serial.js
_handleReconnectionmethod for smart reconnection logicselectProtocolsignature (removedforceDisconnectparameter)src/js/utils/AutoDetect.js
verifyBoardto preserve existing connections instead of disconnectingsrc/js/logger.js (new)
info,warn,errormethodsTest Coverage
test/js/MSPHelper.test.js
crcErrorflag and preserved datatest/js/firmware_flasher.test.js (new)
test/js/serial_connect.test.js (new)
test/js/webserial.test.js (new)
Testing Instructions
Manual Testing
Basic Connection Flow
Rapid Port Changes
Device Removal During Operation
Firmware Flasher
Concurrent Connection Attempts (requires timing)
serial.connectcallsWebSerial Recovery (Chrome/Edge only)
Automated Testing
Run the test suite:
All new and existing tests should pass.
Related Issues
Potentially fixes #4595 (ports tab issues in latest Chrome/Edge)
Breaking Changes
Serial.selectProtocol()signature changed - removedforceDisconnectparameter (internal API)Notes
PORT_CHANGE_DEBOUNCE_MSconstant<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests