Serial refactor part 2 #4404
Serial refactor part 2 #4404
Conversation
✅ Deploy Preview for origin-betaflight-app ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the device handling and protocol management logic across several modules while enhancing error handling and consistency. Key changes include:
- Consolidating connection reinitialization and reboot handling in serial_backend.js.
- Introducing a unified protocol map and helper methods in serial.js.
- Adjusting the reader lock release in WebSerial.js to fix a DFU mode bug.
- Streamlining device permission and management flows in port_handler.js.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/js/serial_backend.js | Reworked connection reinitialization and reboot dialog handling. |
| src/js/serial.js | Added a unified protocol map and helper functions for protocol access. |
| src/js/protocols/WebSerial.js | Modified reader lock handling to address DFU mode issues. |
| src/js/port_handler.js | Updated device permission and management flows with unified methods. |
Files not reviewed (1)
- locales/en/messages.json: Language not supported
Comments suppressed due to low confidence (3)
src/js/serial_backend.js:835
- The callback is invoked both here and again inside the setTimeout in showRebootDialog, which may lead to unexpected double invocation. Consider consolidating the callback so it is only called once after the reboot process completes.
if (callback && typeof callback === "function") { callback(); }
src/js/serial.js:27
- For consistency with the _getProtocolType method, update the protocol map key to 'webserial' (and similarly update 'bluetooth' to 'webbluetooth') so that the naming aligns across the code.
serial: this._webSerial, // TODO: should be 'webserial'
src/js/protocols/WebSerial.js:277
- Ensure that releasing the reader lock without calling cancel fully cleans up the reader, as this change is critical for DFU mode functionality.
await this.reader.releaseLock();
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the device handling logic and improves protocol management and error handling across serial communication modules. Key changes include:
- Consolidation and simplification of device addition/removal logic in port_handler.js.
- Enhanced protocol mapping and safer device access methods in serial.js.
- UI improvements for connection reinitialization and reboot progress in serial_backend.js, along with a bug fix in WebSerial.js to ensure proper reader lock release.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/js/serial_backend.js | Added reboot progress modal and refactored connection reinit. |
| src/js/serial.js | Updated protocol mapping/handling and error logging. |
| src/js/protocols/WebSerial.js | Fixed bug by releasing reader lock correctly. |
| src/js/port_handler.js | Unified device addition/removal and updated device list logic. |
Files not reviewed (1)
- locales/en/messages.json: Language not supported
Comments suppressed due to low confidence (2)
src/js/serial.js:27
- [nitpick] Consider updating the key from 'serial' to 'webserial' as noted in the TODO comment to maintain consistent protocol naming.
serial: this._webSerial, // TODO: should be 'webserial'
src/js/serial_backend.js:840
- [nitpick] It might be beneficial to clear the progress update interval once progress reaches 100% rather than waiting for the overall timeout, to avoid redundant style updates.
progress += 5;
| // Get device path safely | ||
| const devicePath = device?.path || (typeof device === "string" ? device : null); | ||
|
|
||
| if (!devicePath) { |
There was a problem hiding this comment.
After logging a warning for a missing devicePath, exit the function immediately to prevent calling 'devicePath.startsWith' on a null value.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors and simplifies device handling while enhancing protocol management and adding UI improvements for device reboot feedback. Key updates include consolidating device handling methods, enhancing the Serial class with a unified protocol map and error-safe getters, and introducing a progress dialog for reboot operations.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/js/serial_backend.js | Added reboot progress dialog UI and improved connection reinitialization flow. |
| src/js/serial.js | Updated protocol mapping and event forwarding to support consistent protocol types. |
| src/js/protocols/WebSerial.js | Fixed asynchronous release of the reader lock by awaiting the releaseLock call. |
| src/js/port_handler.js | Unified device handling methods with a common updateDeviceList approach. |
Files not reviewed (1)
- locales/en/messages.json: Language not supported
Comments suppressed due to low confidence (2)
src/js/serial.js:27
- [nitpick] Consider renaming the 'serial' key in _protocolMap to 'webserial' for consistency with the protocol naming conventions suggested in the comment.
serial: this._webSerial, // TODO: should be 'webserial'
src/js/serial.js:28
- [nitpick] Consider renaming the 'bluetooth' key in _protocolMap to 'webbluetooth' to align with the suggested naming style and improve clarity.
bluetooth: this._bluetooth, // TODO: should be 'webbluetooth'
|
it tries to reboot on disconnect while the FC in gpspassthrough mode. |
|
Should a new reboot status be shown when the preset is applied? |
|
|
|
@nerdCopter will be fixed in #4405 |
|
|
|
This PR specifically updates bluetooth connection to use new serial facade. Tested with SPBE F405 MINI.
|
|
Rebased on master |
|
github-project-automation
bot
moved this from App
to Done
in Finalizing Firmware 4.6 Release
This pull request introduces significant refactoring and enhancements to the port handling logic, aiming to simplify the codebase and improve device management. The key changes include consolidating device handling methods, adding new messages, and enhancing the Serial class for better protocol management.
Refactoring and Simplification:
addedSerialDevice,addedBluetoothDevice, andaddedUsbDevicewithhandleDeviceAdded, and similarly for removal methods. This reduces redundancy and centralizes device management logic (src/js/port_handler.js). [1] [2] [3]updateDeviceListmethod to handle device list updates for different types (serial, bluetooth, usb) in a unified manner (src/js/port_handler.js).Protocol Management Enhancements:
_getProtocolByTypeand_getProtocolTypemethods in theSerialclass to safely access and identify protocols, improving the flexibility and maintainability of protocol management (src/js/serial.js). [1] [2]getDevicesandrequestPermissionDevicemethods in theSerialclass to support specific protocol types, allowing for more granular control over device interactions (src/js/serial.js).UI and Messaging Improvements:
locales/en/messages.jsonfile, enhancing user feedback during device operations (locales/en/messages.json).Code Cleanup:
port_handler.js, streamlining the code and improving readability (src/js/port_handler.js). [1] [2]These changes collectively enhance the robustness and maintainability of the port handling and protocol management logic, providing a more streamlined and user-friendly experience.