Fix AT32 Configuration can't be saved on MacOS #4455
Conversation
WalkthroughThe update introduces platform- and device-specific logic to the WebSerial class for handling data writes. It detects if the user is on macOS and, upon device connection, checks if the connected device is an AT32 flight controller. If both conditions are met, the class enables a new batch writing mode that splits outgoing data into 63-byte chunks and writes them sequentially to the serial port. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebSerial
participant SerialPort
User->>WebSerial: connect()
WebSerial->>WebSerial: checkIsNeedBatchWrite()
WebSerial->>SerialPort: open()
User->>WebSerial: send(data, callback)
alt Batch write needed
WebSerial->>WebSerial: batchWrite(data)
loop for each 63-byte chunk
WebSerial->>SerialPort: write(chunk)
end
else
WebSerial->>SerialPort: write(data)
end
WebSerial->>User: callback()
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
src/js/protocols/WebSerial.js (2)
58-59: Consider using a more reliable method for platform detectionUsing the userAgent string for platform detection is common but can be less reliable as it can be spoofed. For a more robust solution, consider using platform-specific APIs like
navigator.platformin combination with userAgent detection.- this.isMac = /macintosh|mac os x/i.test(navigator.userAgent); + this.isMac = /macintosh|mac os x/i.test(navigator.userAgent) && + (navigator.platform.toLowerCase().includes('mac') || navigator.platform.toLowerCase().includes('darwin'));
171-171: Consider adding logging for batch write mode detectionAdding a log statement when batch write mode is enabled would help with debugging and verifying that the condition is correctly triggered for AT32 devices on macOS.
this.isNeedBatchWrite = this.checkIsNeedBatchWrite(); +if (this.isNeedBatchWrite) { + console.log(`${this.logHead} Enabling batch write mode for AT32 on macOS`); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/protocols/WebSerial.js(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/js/protocols/WebSerial.js (1)
src/js/protocols/devices.js (2)
vendorIdNames(68-74)vendorIdNames(68-74)
🔇 Additional comments (2)
src/js/protocols/WebSerial.js (2)
336-338: Good targeted fix for the AT32 on macOS issueThis check correctly implements the condition based on both the platform and the specific device vendor ID.
361-365: The conditional batch writing approach looks goodThe implementation maintains backward compatibility for other devices/platforms while introducing the specialized handling for AT32 on macOS.
|
@kedeng two more changes please - see #4455 (review) |
|
Preview URL: https://dbf09742.betaflight-configurator.pages.dev |
navigator.platform seems deprecated. https://medium.com/@jortiz.dev/bye-navigator-platform-here-is-the-alternative-939b883bf050 |
This comment was marked as outdated.
This comment was marked as outdated.
Seems preview URL not right, I can see this log on the local machine but not see it in this URL: |
|
@blckmn , multiple preview URL comments are confusing. and the bot adding emojis is ridiculous 😹 . can you change these behaviors? if old comments cannot be removed, maybe they can be hidden as "outdated" |
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
Preview URL: https://ad58e4a4.betaflight-configurator.pages.dev |
Can fix #3514, but not good.
Summary by CodeRabbit