openthread_border_router: Fix migration script for TI CC2652 devices #4249
Conversation
📝 WalkthroughWalkthroughThis release (2.15.3) adds support for deasserting RTS/DTR signals on USB TI CC2652-based devices during migration. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
openthread_border_router/CHANGELOG.md(1 hunks)openthread_border_router/config.yaml(1 hunks)openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run(2 hunks)openthread_border_router/rootfs/usr/local/bin/migrate_otbr_settings.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
*/**(html|markdown|md)
⚙️ CodeRabbit configuration file
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
*/**(html|markdown|md): - Use bold to mark UI strings.
If "" are used to mark UI strings, replace them by bold.
Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Use sentence-style capitalization also in headings.
do not comment on HTML used for icons
Avoid flagging inline HTML for embedding videos in future reviews for this repository.
Files:
openthread_border_router/CHANGELOG.mdopenthread_border_router/config.yaml
🪛 LanguageTool
openthread_border_router/CHANGELOG.md
[grammar] ~4-~4: Use a hyphen to join words.
Context: ...R in migration scripts for USB TI CC2652 based devices ## 2.15.2 - Add baudrate ...
(QB_NEW_EN_HYPHEN)
⏰ 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). (2)
- GitHub Check: Build aarch64 openthread_border_router add-on
- GitHub Check: Build amd64 openthread_border_router add-on
🔇 Additional comments (9)
openthread_border_router/config.yaml (1)
2-2: LGTM!Version bump aligns with the 2.15.3 release addressing the RTS/DTR deassertion fix.
openthread_border_router/rootfs/usr/local/bin/migrate_otbr_settings.py (6)
4-4: LGTM!The
urllibimport is needed for URL parsing to determine whether the port is a socket/TCP connection or a serial port, enabling conditional deassert logic.
72-74: LGTM!The function signature change adds the
deassertparameter with a sensible default ofFalse, maintaining backward compatibility.
76-108: LGTM!The deassert implementation correctly:
- Parses the port URL to distinguish serial ports from network sockets
- Applies manual RTS/DTR deassertion only to serial ports (not socket/tcp schemes)
- Properly manages resources with exception handling that closes the serial instance on failure
- Preserves the original connection flow when deassert is not requested
The 200ms settling time after deasserting RTS/DTR is appropriate for the auto-flash circuit behavior described in the PR.
117-119: Verify that exit(0) is the intended behavior for all timeout scenarios.The script exits with code 0 (success) when it fails to connect to the device during migration. While the PR description indicates this is intentional to allow devices that "would otherwise work once otbr-agent runs" to continue startup, consider whether:
- This gracefully handles all expected timeout scenarios (e.g., temporary device unavailability during boot)
- Genuine connectivity issues that would also affect otbr-agent might be masked
- Additional logging or diagnostics would help distinguish transient timeouts from persistent failures
154-156: LGTM!The
--deassertCLI argument is properly defined with clear help text describing its purpose.
161-161: LGTM!The function invocation correctly passes the
deassertargument from the CLI toget_adapter_hardware_addr.openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run (2)
34-41: LGTM!The conditional logic correctly sets
deassert_argbased on theflow_controlconfiguration:
- When flow control is enabled, RTS/DTR are used for flow control, so deassertion is not needed
- When flow control is disabled, the script sets
deassert_arg="--deassert"to ensure RTS/DTR are deasserted before the device bootsThis aligns with the PR's goal of supporting CC2652 dongles with auto-flash circuits tied to RTS/DTR.
150-150: LGTM!The command correctly passes
${deassert_arg}unquoted, which ensures:
- When empty, no argument is passed to the migration script
- When set to
--deassert, the flag is properly passedThis is the correct shell pattern for optional command-line flags.
| ## 2.15.3 | ||
| - Fix deassert of RTS/DTR in migration scripts for USB TI CC2652 based devices | ||
|
|
There was a problem hiding this comment.
Fix hyphenation in compound adjective.
The phrase "USB TI CC2652 based devices" should use a hyphen: "USB TI CC2652-based devices". Compound adjectives that modify a noun should be hyphenated.
Apply this diff:
## 2.15.3
-- Fix deassert of RTS/DTR in migration scripts for USB TI CC2652 based devices
+- Fix deassert of RTS/DTR in migration scripts for USB TI CC2652-based devicesBased on coding guidelines, documentation should follow the Microsoft Style Guide for clarity and consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2.15.3 | |
| - Fix deassert of RTS/DTR in migration scripts for USB TI CC2652 based devices | |
| ## 2.15.3 | |
| - Fix deassert of RTS/DTR in migration scripts for USB TI CC2652-based devices |
🧰 Tools
🪛 LanguageTool
[grammar] ~4-~4: Use a hyphen to join words.
Context: ...R in migration scripts for USB TI CC2652 based devices ## 2.15.2 - Add baudrate ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In openthread_border_router/CHANGELOG.md around lines 3 to 5, the phrase "USB TI
CC2652 based devices" is missing a hyphen in the compound adjective; change it
to "USB TI CC2652-based devices" to correctly hyphenate the compound modifier
and match the Microsoft Style Guide.
1 participant
Recent changes with the migration script cause otbr-agent to fail to start on CC2652 dongles.
Many USB CC2652 dongles (such as SMLIGHT SLZB-07p7 ) have an auto-flash circuit connected to RTS/DTR and these devices need these lines deasserted before they will boot.
I have add a deassert flow to the migration script so it can work with these devices. This is inline with the logic already in place when starting otbr-agent with flow control disabled.
Also catch the error, that was breaking startup on devices that would otherwise work once the otbr-agent runs.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.