Fix firmware flasher event cleanup #4667
Conversation
WalkthroughAdds debounce-based port-change handling, public device handlers and state, wires those handlers to EventBus, exposes start/clear functions on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI/External
participant FF as firmware_flasher
participant EB as EventBus
participant AD as AutoDetect
participant FL as Flashing
rect rgb(240,245,255)
note over FF: Initialization
UI->>FF: initialize()
FF->>EB: subscribe detectedUsbDevice / detectedSerialDevice / onPortChange / onDeviceRemoved
FF->>FF: expose startFlashing / clearBufferedFirmware
EB-->>FF: publish existing ports?
FF->>AD: (optional) verifyBoard()
end
rect rgb(245,255,245)
note over EB,FF: Debounced port change
EB-->>FF: portChanged(port)
FF->>FF: start/reset PORT_CHANGE_DEBOUNCE_MS timer
alt connect_lock or reboot active
FF-->>AD: skip verifyBoard()
else flash_on_connect enabled
FF->>AD: verifyBoard(port)
end
end
rect rgb(255,248,240)
note over EB,FF: Device removed
EB-->>FF: deviceRemoved(path)
alt no active operation and not rebooting
FF->>FF: clear board selection
FF->>FF: clearBufferedFirmware()
else
FF-->>FF: defer cleanup
end
end
rect rgb(240,240,240)
note over UI,FF: Cleanup
UI->>FF: cleanup()
FF->>EB: unsubscribe handlers
FF->>FF: clear pending portChangeTimer
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
54-58: Fix logging placeholder usageTemplate string includes “%s” which won’t be interpolated. Simplify to a single interpolated message.
- console.log( - `${firmware_flasher.logHead} Reboot mode: %s, flash on connect`, - STM32.rebootMode, - isFlashOnConnect, - ); + console.log( + `${firmware_flasher.logHead} Reboot mode: ${STM32.rebootMode}, flash on connect: ${isFlashOnConnect}`, + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 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)
🔇 Additional comments (7)
src/js/tabs/firmware_flasher.js (7)
46-77: USB detection flow: good guards and reboot resumeHonors connect_lock, only clears it when resuming from reboot, and invokes startFlashing safely. Looks solid.
78-93: Serial detection: safe early-exit + guarded auto-detectEarly return under connect_lock and try/catch around AutoDetect is appropriate.
94-133: Debounced port-change handling is correctClears existing timer, re-checks connect_lock at timeout, and avoids auto-detect during reboot/flash-on-connect. Good defensive logic.
134-144: Removal handler correctly avoids interfering with active flowsSkips during connect_lock/reboot and clears board + buffered firmware otherwise. Good.
845-850: Exposing startFlashing/clearBufferedFirmware is appropriateMakes public handlers callable by module-scoped listeners and tests. Good placement before event registration.
851-854: EventBus registrations are clean and symmetric with cleanupGood move to centralize through public handlers for easy unregistration.
1594-1611: EventBus cleanup + timer clear: correct and completeHandler references are correctly used for $off; debounce timer is cleared to prevent late execution. This addresses the event cleanup bug.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/tabs/firmware_flasher.js (1)
1571-1577: Address inconsistent auto-detect conditions and fix indentation.This issue was previously flagged: The init-time auto-detect (line 1573) only checks
!isFlashOnConnect, but the port-change path (line 109) also checks!STM32.rebootMode. This inconsistency can cause unexpected auto-detection when resuming from a reboot.Additionally, ESLint reports indentation errors on lines 1572-1576 (expected 8 spaces but found 12-16).
Apply this diff to align the behavior with the port-change path and fix indentation:
- - const isFlashOnConnect = $("input.flash_on_connect").is(":checked"); - if (PortHandler.portAvailable && !isFlashOnConnect) { - console.log(`${self.logHead} 💥 Auto-detecting board for already connected device`); - AutoDetect.verifyBoard(); - } - + const isFlashOnConnect = $("input.flash_on_connect").is(":checked"); + if (PortHandler.portAvailable && !isFlashOnConnect && !STM32.rebootMode) { + console.log(`${self.logHead} Auto-detecting board for already connected device`); + AutoDetect.verifyBoard(); + } +
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
50-144: Consider timing guarantees for handler initialization.The handlers use optional chaining (
?.()) when callingstartFlashing(line 75) andclearBufferedFirmware(line 143), suggesting these methods might not be available when the handlers execute. Since these methods are assigned duringinitialize()(lines 848-849) but the handlers are wired to EventBus at the same time, there's a potential race condition if events fire before initialization completes.Consider either:
- Moving the EventBus subscriptions to after the method assignments (lines 851-854 should come after 848-849), or
- Adding explicit checks with meaningful error messages if the methods are unavailable
Otherwise, the debouncing logic and state management look solid.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js(5 hunks)
🧰 Additional context used
🧬 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)
🪛 ESLint
src/js/tabs/firmware_flasher.js
[error] 1572-1572: Expected indentation of 8 spaces but found 12.
(indent)
[error] 1573-1573: Expected indentation of 8 spaces but found 12.
(indent)
[error] 1574-1574: Expected indentation of 12 spaces but found 16.
(indent)
[error] 1575-1575: Expected indentation of 12 spaces but found 16.
(indent)
[error] 1576-1576: Expected indentation of 8 spaces but found 12.
(indent)
🔇 Additional comments (3)
src/js/tabs/firmware_flasher.js (3)
845-854: LGTM! Method exposure and event wiring looks correct.The code properly exposes internal functions to the module scope and wires up EventBus subscriptions. The ordering is correct: method assignments (lines 848-849) happen before EventBus subscriptions (lines 851-854), which prevents the race condition mentioned in the previous comment.
1596-1612: Excellent cleanup implementation!The cleanup logic properly:
- Unsubscribes all EventBus handlers that were registered during initialization
- Clears any pending
portChangeTimerto prevent late callbacks after cleanup- Uses a helper function to avoid code duplication
This thoroughly addresses the event cleanup concerns mentioned in the PR title.
50-77: LGTM! Subtle reboot and connect-lock handling is correct.The logic carefully distinguishes between resuming from a reboot and flash-on-connect scenarios:
- Captures the reboot state before clearing it (line 68)
- Only clears
GUI.connect_lockwhen resuming from a reboot (lines 72-74), preventing interference with other active operations- Properly handles both the reboot resumption path and the flash-on-connect path
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 (1)
src/js/tabs/firmware_flasher.js(5 hunks)
🧰 Additional context used
🧬 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 (2)
devicePath(110-110)PortHandler(16-50)
🪛 ESLint
src/js/tabs/firmware_flasher.js
[error] 1571-1571: Expected indentation of 8 spaces but found 12.
(indent)
[error] 1572-1572: Expected indentation of 8 spaces but found 12.
(indent)
[error] 1573-1573: Expected indentation of 12 spaces but found 16.
(indent)
[error] 1574-1574: Expected indentation of 12 spaces but found 16.
(indent)
[error] 1575-1575: Expected indentation of 8 spaces but found 12.
(indent)
🔇 Additional comments (4)
src/js/tabs/firmware_flasher.js (4)
24-25: LGTM! Well-chosen debounce duration.The 500ms debounce constant is appropriate for port-change events, balancing responsiveness with noise reduction from rapid port state transitions.
45-143: Excellent implementation of debounced event handlers.The new public handlers provide robust port and device management:
- Debouncing (lines 95-98, 112-123): Properly clears pending timers and re-validates
connect_lockstate on timeout to prevent stale operations.- State management: Correctly handles
GUI.connect_lockandSTM32.rebootModeto avoid interfering with active operations.- Defensive programming (lines 74, 142): Appropriate use of optional chaining (
?.) sincestartFlashingandclearBufferedFirmwareare exposed after handlers register.- Logging: Consistent use of
logHeadprefix aids debugging.
844-853: LGTM! Proper function exposure and event wiring.Exposing
startFlashingandclearBufferedFirmwareon thefirmware_flasherobject before subscribing handlers ensures external callers and event handlers can safely reference these functions, even if called beforeinitialize()completes.
1590-1614: LGTM! Thorough cleanup implementation.The cleanup function properly manages lifecycle:
- Event unsubscription (lines 1595-1605): DRY helper function unsubscribes all registered handlers.
- Timer cleanup (lines 1607-1611): Clears pending debounce timer to prevent late execution after tab switch.
- Defensive checks: Validates handler existence before unsubscribing.
This prevents memory leaks and ensures clean tab transitions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
118-118: Consider extracting complex condition for readability.The condition on line 118 checks four separate criteria. Extracting it to a named variable would improve clarity:
+ const shouldAutoDetect = port && port !== "0" && + !$("input.flash_on_connect").is(":checked") && + !STM32.rebootMode; - if (port && port !== "0" && $("input.flash_on_connect").is(":checked") === false && !STM32.rebootMode) { + if (shouldAutoDetect) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js(5 hunks)
🧰 Additional context used
🧬 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)
🔇 Additional comments (3)
src/js/tabs/firmware_flasher.js (3)
49-153: Well-implemented event handlers with proper guards and debouncing.The new event handlers demonstrate solid defensive programming:
- Concurrency control: All handlers check
GUI.connect_lockto prevent interference with active operations- Reboot state management:
detectedUsbDevicecorrectly clearsconnect_lockonly when resuming from a reboot (lines 66-73), avoiding accidental interruption of flash-on-connect flows- Debouncing:
onPortChangeimplements proper debouncing by clearing stale timers and re-checkingconnect_lockinside the timeout callback (lines 105-108, 122-127)- Error resilience: Try-catch blocks and optional chaining (
?.()) prevent crashes when functions aren't yet initializedThe logic correctly handles the interaction between reboot mode, flash-on-connect, and port changes.
854-863: Good initialization pattern for handler-function coupling.The code correctly exposes locally-scoped functions before subscribing event handlers. This ordering ensures handlers can immediately call
firmware_flasher.startFlashing?.()andfirmware_flasher.clearBufferedFirmware?.()even if events fire synchronously during subscription.The comment clearly explains the rationale, and the optional chaining provides safety if handlers somehow run before initialization completes.
1604-1620: Comprehensive cleanup prevents memory leaks and late callbacks.The cleanup implementation is thorough:
- Event unsubscription: All four EventBus handlers are properly unsubscribed via the helper function
- Timer cancellation: Clearing
portChangeTimer(lines 1617-1620) ensures the debounced callback cannot fire after the tab closes, preventing potential errors or stale state updates- Clean pattern: The
cleanupHandlerhelper makes the unsubscription logic readable and maintainableThis properly handles the lifecycle of tab-scoped event handlers.
nerdCopter
left a comment
nerdCopter
left a comment
There was a problem hiding this comment.
- approving based on #4661 testing
haslinghuis
force-pushed
the
fix-ff-events
branch
from
a75eb96 to
dd87ca1
Compare
|
|
Preview URL: https://pr4667.betaflight-app-preview.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/tabs/firmware_flasher.js (1)
49-76: Consider adding defensive error handling for consistency.The handler correctly manages
connect_lockand reboot-mode resumption. However, unlikedetectedSerialDevice(which wrapsAutoDetect.verifyBoardin try-catch), this path doesn't have explicit error handling if exceptions occur during the flashing initiation.While
startFlashinguses optional chaining and is unlikely to throw, consider wrapping the entire conditional block in try-catch for consistency with the serial handler pattern:if (STM32.rebootMode || isFlashOnConnect) { const wasReboot = !!STM32.rebootMode; STM32.rebootMode = 0; // Only clear the global connect lock when we are resuming from a reboot // so we don't accidentally interrupt another active operation. if (wasReboot) { GUI.connect_lock = false; } - firmware_flasher.startFlashing?.(); + try { + firmware_flasher.startFlashing?.(); + } catch (e) { + console.warn(`${firmware_flasher.logHead} startFlashing threw:`, e); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/tabs/firmware_flasher.js(5 hunks)
🧰 Additional context used
🧬 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 (2)
devicePath(110-110)PortHandler(16-50)
🔇 Additional comments (7)
src/js/tabs/firmware_flasher.js (7)
24-25: LGTM! Good use of named constant for debounce duration.The 500ms debounce window is appropriate for hardware port-change events and prevents rapid re-entry during device state transitions.
77-102: LGTM! Proper error handling and control flow.The handler correctly respects
connect_lock, distinguishes between flash-on-connect and auto-detect scenarios, and includes defensive try-catch aroundAutoDetect.verifyBoard().
103-142: LGTM! Excellent defensive programming with debounce re-check.The debounce implementation is solid:
- Clears pending timers to prevent re-entry
- Re-validates
connect_lockwhen the timeout fires (lines 124-127), preventing race conditions if an operation starts during the debounce window- Includes try-catch for error resilience
143-153: LGTM! Proper state-aware cleanup.The handler correctly skips cleanup when removal is expected (during flashing or reboot) and only acts when the removal is unexpected.
854-863: LGTM! Clean exposure and EventBus wiring.Exposing
startFlashingandclearBufferedFirmwareon the module surface allows the handlers (defined at lines 49-153) to safely invoke them. The EventBus subscriptions properly wire the public handlers to their respective events, and the corresponding cleanup (lines 1604-1620) ensures proper unsubscription.
1580-1584: LGTM! Initialization auto-detect with proper guards.The logic correctly triggers auto-detect only when a device is already connected AND flash-on-connect is disabled, preventing surprise auto-detection when users expect immediate flashing.
1604-1620: LGTM! Thorough cleanup prevents stale callbacks.The cleanup properly:
- Unsubscribes all EventBus handlers using a defensive helper
- Clears the pending debounce timer (lines 1617-1620), which is critical to prevent the debounced
AutoDetect.verifyBoard()call from firing after the tab is closedThis prevents both memory leaks and unexpected behavior from stale event handlers.
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Improvements