OpenThread Border Router: Allow network_device without USB device#4620
OpenThread Border Router: Allow network_device without USB device#4620jmarcelomb wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hi @jmarcelomb
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSchema now allows omitting ChangesOptional Device Configuration with Fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run`:
- Around line 51-53: The current check uses if [ -z "${device}" ] which can be
bypassed because bashio::config returns the literal "null"; replace this logic
by using bashio::config.has_value for both keys (device and network_device) and
fail only when neither has_value returns true; update the run script's
validation block to call bashio::config.has_value "device" and
bashio::config.has_value "network_device" and invoke bashio::exit.nok with the
same message when both are absent to ensure proper detection of unset configs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08380915-989d-438b-95ae-13f03a6da18f
📒 Files selected for processing (2)
openthread_border_router/config.yamlopenthread_border_router/rootfs/etc/s6-overlay/s6-rc.d/otbr-agent/run
8a9e83e to
beaefbf
Compare
Make the 'device' option optional in the schema so users with network-connected RCP radios (via network_device) don't need a USB serial device configured. Use bashio::config.has_value for both device and network_device validation instead of -z string test, which fails to catch bashio's literal 'null' string for unset options.
beaefbf to
965016d
Compare
I don't really like it. It means that users who use a USB device get a strange error if they forget to select it. And these users should be our primary concern. Tunneling Thread radio messages through network is generally a bad idea, it is a timing sensitive protocol and the higher level protocol itself is a network protocol, so why not just install a BR firmware on the device if its network capable!? 🤷 What I actually would prefer is have a separate app, which we'd then also could clearly mark as experimental. It would also have the advantage that you can run a proper locally connected radio still. |
devices are starting to offer this, but it's often beta support at best. Users may have an acceptable network and be willing to accept the risks of tunneling thread radio message between the radio and the OTBR server (a common use case being HA physically running in a suboptimal location for radio placement -- aka a corner in the basement) . It feels like there are 2 things worth keeping distinct in discussion: The oddity of the UI behavior here and the general complications and nuances of the configuration it currently blocks: network only radio devices. |
|
Blocking useful network functionality because a USB user might misconfigure their settings feels like the wrong trade-off. Instead of restricting choices for users who intentionally deploy network-attached coordinators, shouldn't we just improve the UI validation or error messaging to guide the USB users better? Network-attached RCPs (like the SLZB-06 or TubeZB coordinators) are incredibly common for centralising radios away from server racks. Giving users the choice of how they connect their hardware, while keeping the configuration unified in one place, is a massive UX win. |
|
I agree that using a network_device instead of a USB device isn't ideal, but as OTBR suppots this, this app should support the configuration. |
|
@jmarcelomb @agners any chance we could support this in some mode, possibly with some checkbox/tunable? |
I'm trying to use an SLZB-MR4 (network-connected RCP) on my Raspberry Pi 4B. The add-on requires
deviceto be set even though I'm usingnetwork_device— there's no USB serial device to provide, and setting a dummy value feels wrong.This makes
deviceoptional in the schema and adds a check that fails clearly if neitherdevicenornetwork_deviceis configured.Summary by CodeRabbit
New Features
Bug Fixes