config: boards: pocketbeagle2: Enable better USB Gadget #8382
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Use configfs API to setup Multifunction Composite Gadget - This is already done on all BeagleBoard debian images. - Allows ssh and serial over USB without any user setup. Signed-off-by: Ayush Singh <[email protected]>
WalkthroughThe changes introduce USB gadget networking support for the PocketBeagle 2 board. The configuration file for the board is updated to remove the secondary serial console and add two new functions: one to install USB gadget network scripts and services during firmware preparation, and another to enable and configure the necessary systemd service and DHCP server within the target environment. A new shell script is added to set up the USB gadget network interface using configfs, configure device descriptors, activate the network and serial functions, and assign IP addresses with a DHCP server. Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 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 (
|
github-actions
bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
config/boards/pocketbeagle2.conf (2)
13-16: Temporary removal of ttyGS0 - Track the underlying issueThe removal of
ttyGS0from bothSERIALCONandSRC_CMDLINEis consistent with the PR objectives. Since this is a temporary workaround for the armbian-first-run failure, consider adding a TODO comment to track this issue.Would you like me to create a tracking issue for investigating and resolving the ttyGS0/armbian-first-run conflict?
37-50: Improve service configuration reliabilityThe function correctly handles the Armbian repository manipulation and service configuration. Consider these improvements:
Use a heredoc for better readability and maintainability:
- echo -e "[Service]\nEnvironment=\"unudhcpd_host_ip=192.168.7.2\"\nEnvironment=\"unudhcpd_client_ip=192.168.7.3\"" > "${SDCARD}"/etc/systemd/system/usbgadget-rndis.service.d/override.conf + cat > "${SDCARD}"/etc/systemd/system/usbgadget-rndis.service.d/override.conf <<-EOF + [Service] + Environment="unudhcpd_host_ip=192.168.7.2" + Environment="unudhcpd_client_ip=192.168.7.3" + EOFAlso consider adding error handling for the systemctl enable command.
packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh (1)
58-58: Fix typo in error message- echo " Couldn't write configration name" + echo " Couldn't write configuration name"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/boards/pocketbeagle2.conf(1 hunks)packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh (2)
Learnt from: djurny
PR: armbian/build#8235
File: packages/bsp/mvebu/helios4/helios4-wol.service:0-0
Timestamp: 2025-05-29T01:52:21.381Z
Learning: When implementing systemd services that need to iterate over network interfaces, the pattern `/bin/ls -1 /sys/class/net/ | /bin/grep -E "^(eth|en[do])[0-9]+" | /bin/xargs -n1 -I{} /sbin/ethtool -s "{}" wol g` is more robust than using shell variables, as it avoids systemd variable resolution issues that can cause service failures.
Learnt from: EvilOlaf
PR: armbian/build#8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: Armbian build framework extensions are NOT designed to run standalone but are called by the supervisor. The framework provides environment variables like USERPATCHES_PATH and SDCARD, and commands are executed with protection via run_host_command_logged and chroot_sdcard functions that use bash -e -o pipefail.
config/boards/pocketbeagle2.conf (10)
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-06-16T03:24:00.458Z
Learning: In Armbian board configuration files (.csc), the standard pattern is to have only one line as a comment describing the board hardware specifications. This single-line description typically includes the SoC model, core count, RAM options, and key features like connectivity options, storage interfaces, and special features.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:40:52.109Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, when KERNELSOURCE is not explicitly declared in a case branch, the system falls back to a default KERNELSOURCE value, so missing KERNELSOURCE declarations are not critical issues.
Learnt from: EvilOlaf
PR: armbian/build#8330
File: config/sources/families/sun55iw3.conf:32-36
Timestamp: 2025-06-25T03:42:09.086Z
Learning: In Armbian build system configuration files like config/sources/families/*.conf, KERNELSOURCE is explicitly declared when using unofficial or 3rd party kernel repositories (like the "dev" branch using https://github.com/apritzel/linux), but can be omitted when using the standard mainline kernel (like the "edge" branch) since it will fall back to the default mainline source.
Learnt from: Grippy98
PR: armbian/build#8152
File: lib/functions/configuration/interactive.sh:209-266
Timestamp: 2025-05-05T12:35:07.143Z
Learning: For the interactive kernel selection in Armbian, KERNEL_MAJOR_MINOR and KERNEL_DESCRIPTION are parsed from family.conf but deliberately not set as environment variables to avoid potential interference with other parts of the build system.
Learnt from: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:76-86
Timestamp: 2025-03-31T22:20:48.475Z
Learning: For the Armbian build project, maintaining consistency with existing patches across U-Boot versions (such as between 2025.01 and 2025.04) is prioritized over refactoring individual patches for code improvements.
Learnt from: rpardini
PR: armbian/build#8044
File: patch/u-boot/v2025.04/cmd-fileenv-read-string-from-file-into-env.patch:73-75
Timestamp: 2025-03-31T22:20:41.849Z
Learning: When porting patches between U-Boot versions (like from 2025.01 to 2025.04), rpardini prefers to maintain patches as-is rather than introducing refactoring changes, even when potential improvements are identified. This approach prioritizes consistency and reduces the risk of introducing new issues.
Learnt from: EvilOlaf
PR: armbian/build#8298
File: extensions/https_proxy.sh:1-9
Timestamp: 2025-06-14T05:53:10.627Z
Learning: Armbian build framework extensions are NOT designed to run standalone but are called by the supervisor. The framework provides environment variables like USERPATCHES_PATH and SDCARD, and commands are executed with protection via run_host_command_logged and chroot_sdcard functions that use bash -e -o pipefail.
Learnt from: libiunc
PR: armbian/build#8033
File: config/kernel/linux-starfive2-vendor.config:43-43
Timestamp: 2025-03-31T12:57:13.880Z
Learning: For StarFive2 platform kernel configurations, maintain alignment with vendor-provided configurations rather than modifying security settings like SECCOMP. This ensures hardware compatibility as intended by the manufacturer.
Learnt from: djurny
PR: armbian/build#8272
File: config/bootscripts/boot-mvebu.cmd:182-186
Timestamp: 2025-06-04T23:45:38.860Z
Learning: In config/bootscripts/boot-mvebu.cmd, the `fdtfile` variable is mandatory for booting and is pre-set by U-Boot, but can be overridden via armbianEnv.txt. If `fdtfile` is empty, the subsequent device tree file search logic will eventually fail and trigger the critical error "Cannot find DT!" with proper error handling.
Learnt from: Grippy98
PR: armbian/build#8202
File: config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh:2-3
Timestamp: 2025-05-16T15:34:34.672Z
Learning: The shell scripts in the Armbian build system's desktop configuration files, such as config/desktop/trixie/environments/gnome/armbian/create_desktop_package.sh, rely on environment variables like $SRC and $destination that are expected to be set externally before the script is executed.
Comment on lines
+21
to
+35
| function post_family_tweaks_bsp__pocketbeagle2_firmware() { | ||
| display_alert "Setup USB Gadget for ${BOARD}" "${RELEASE}" "warn" | ||
|
|
||
| # USB Gadget Network service | ||
| mkdir -p $destination/usr/local/bin/ | ||
| mkdir -p $destination/usr/lib/systemd/system/ | ||
| mkdir -p $destination/etc/initramfs-tools/scripts/init-bottom/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh $destination/usr/local/bin/setup-usbgadget-network.sh | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/remove-usbgadget-network.sh $destination/usr/local/bin/ | ||
| install -Dm644 $SRC/packages/bsp/usb-gadget-network/usbgadget-rndis.service $destination/usr/lib/systemd/system/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-hook $destination/etc/initramfs-tools/hooks/usb-gadget | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/usb-gadget-initramfs-premount $destination/etc/initramfs-tools/scripts/init-premount/usb-gadget | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/dropbear $destination/etc/initramfs-tools/scripts/init-premount/ | ||
| install -Dm655 $SRC/packages/bsp/usb-gadget-network/kill-dropbear $destination/etc/initramfs-tools/scripts/init-bottom/ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and clarify file renaming
The function implementation looks good but could benefit from:
- Error handling for the install commands
- Clarification on why
setup-usbgadget-network-multi.shis renamed tosetup-usbgadget-network.shduring installation
Consider adding error checking:
function post_family_tweaks_bsp__pocketbeagle2_firmware() {
display_alert "Setup USB Gadget for ${BOARD}" "${RELEASE}" "warn"
# USB Gadget Network service
mkdir -p $destination/usr/local/bin/
mkdir -p $destination/usr/lib/systemd/system/
mkdir -p $destination/etc/initramfs-tools/scripts/init-bottom/
- install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh $destination/usr/local/bin/setup-usbgadget-network.sh
+ install -Dm655 $SRC/packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh $destination/usr/local/bin/setup-usbgadget-network.sh || exit_with_error "Failed to install USB gadget setup script"Also, consider keeping the original filename to avoid confusion, or add a comment explaining the renaming rationale.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In config/boards/pocketbeagle2.conf around lines 21 to 35, the install commands
lack error handling and the renaming of setup-usbgadget-network-multi.sh to
setup-usbgadget-network.sh is unclear. Add error checking after each install
command to verify success and handle failures appropriately. Either keep the
original filename during installation to avoid confusion or add a comment
explaining why the file is renamed.
Comment on lines
+1
to
+8
| #!/bin/bash | ||
|
|
||
| deviceinfo_name="USB Gadget Network" | ||
| deviceinfo_manufacturer="Armbian" | ||
| #deviceinfo_usb_idVendor= | ||
| #deviceinfo_usb_idProduct= | ||
| #deviceinfo_usb_serialnumber= | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and safety flags
For a system configuration script that modifies kernel interfaces, consider adding safety flags to catch errors early.
#!/bin/bash
+set -euo pipefail
deviceinfo_name="USB Gadget Network"📝 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.
Suggested change
| #!/bin/bash | |
| deviceinfo_name="USB Gadget Network" | |
| deviceinfo_manufacturer="Armbian" | |
| #deviceinfo_usb_idVendor= | |
| #deviceinfo_usb_idProduct= | |
| #deviceinfo_usb_serialnumber= | |
| #!/bin/bash | |
| set -euo pipefail | |
| deviceinfo_name="USB Gadget Network" | |
| deviceinfo_manufacturer="Armbian" | |
| #deviceinfo_usb_idVendor= | |
| #deviceinfo_usb_idProduct= | |
| #deviceinfo_usb_serialnumber= |
🤖 Prompt for AI Agents
In packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh at the
beginning (lines 1 to 8), add safety flags such as 'set -euo pipefail' to ensure
the script exits immediately on errors, treats unset variables as errors, and
fails on pipeline errors. This will improve error handling and make the script
safer when modifying kernel interfaces.
Comment on lines
+113
to
+114
| setup_usb_network_configfs | ||
| set_usbgadget_ipaddress |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for function execution
The script should handle failures properly:
-setup_usb_network_configfs
-set_usbgadget_ipaddress
+if setup_usb_network_configfs; then
+ set_usbgadget_ipaddress
+else
+ echo "Failed to setup USB network configfs"
+ exit 1
+fi📝 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.
Suggested change
| setup_usb_network_configfs | |
| set_usbgadget_ipaddress | |
| # Replace the final calls in setup-usbgadget-network-multi.sh: | |
| if setup_usb_network_configfs; then | |
| set_usbgadget_ipaddress | |
| else | |
| echo "Failed to setup USB network configfs" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh at lines 113
to 114, the calls to setup_usb_network_configfs and set_usbgadget_ipaddress lack
error handling. Add checks after each function call to verify if it succeeded,
and if not, output an error message and exit the script with a non-zero status
to properly handle failures.
Comment on lines
+95
to
+98
| ip a add "${host_ip}/255.255.0.0" dev usb0 2> /dev/null && ip link set usb0 up && INTERFACE=usb0 | ||
| if [ -z $INTERFACE ]; then | ||
| ip a add "${host_ip}/255.255.0.0" dev eth0 2> /dev/null && eth0 && INTERFACE=eth0 | ||
| fi |
There was a problem hiding this comment.
Fix syntax error and reconsider eth0 fallback
Critical issues in the interface selection logic:
Line 97 has a syntax error - && eth0 && should be && ip link set eth0 up &&:
if [ -z $INTERFACE ]; then
- ip a add "${host_ip}/255.255.0.0" dev eth0 2> /dev/null && eth0 && INTERFACE=eth0
+ ip a add "${host_ip}/16" dev eth0 2> /dev/null && ip link set eth0 up && INTERFACE=eth0
fiAlso:
- Use CIDR notation
/16instead of/255.255.0.0 - Consider if falling back to eth0 is appropriate for a USB gadget network setup - this could interfere with the main network interface
🤖 Prompt for AI Agents
In packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh around lines
95 to 98, fix the syntax error by replacing '&& eth0 &&' with '&& ip link set
eth0 up &&' to correctly bring up the eth0 interface. Change the netmask from
the dotted format '/255.255.0.0' to CIDR notation '/16' for both interfaces.
Additionally, review whether falling back to eth0 is appropriate in this USB
gadget network context and remove or adjust this fallback if it risks
interfering with the main network interface.
Comment on lines
+30
to
+82
| echo " Setting up an USB gadget through configfs" | ||
| # Create an usb gadet configuration | ||
| mkdir $CONFIGFS/g1 || echo " Couldn't create $CONFIGFS/g1" | ||
| echo "$usb_idVendor" > "$CONFIGFS/g1/idVendor" | ||
| echo "$usb_idProduct" > "$CONFIGFS/g1/idProduct" | ||
| echo 0x0404 > "$CONFIGFS/g1/bcdDevice" | ||
| echo 0x0200 > "$CONFIGFS/g1/bcdUSB" | ||
|
|
||
| # Create english (0x409) strings | ||
| mkdir $CONFIGFS/g1/strings/0x409 || echo " Couldn't create $CONFIGFS/g1/strings/0x409" | ||
|
|
||
| # shellcheck disable=SC2154 | ||
| echo "$deviceinfo_manufacturer" > "$CONFIGFS/g1/strings/0x409/manufacturer" | ||
| echo "$usb_serialnumber" > "$CONFIGFS/g1/strings/0x409/serialnumber" | ||
| # shellcheck disable=SC2154 | ||
| echo "$deviceinfo_name" > "$CONFIGFS/g1/strings/0x409/product" | ||
|
|
||
| # Create network function. | ||
| mkdir $CONFIGFS/g1/functions/"$usb_network_function" || | ||
| echo " Couldn't create $CONFIGFS/g1/functions/$usb_network_function" | ||
|
|
||
| # Create configuration instance for the gadget | ||
| mkdir $CONFIGFS/g1/configs/c.1 || | ||
| echo " Couldn't create $CONFIGFS/g1/configs/c.1" | ||
| echo 250 > $CONFIGFS/g1/configs/c.1/MaxPower | ||
| mkdir $CONFIGFS/g1/configs/c.1/strings/0x409 || | ||
| echo " Couldn't create $CONFIGFS/g1/configs/c.1/strings/0x409" | ||
| echo "NCM Configuration" > $CONFIGFS/g1/configs/c.1/strings/0x409/configuration || | ||
| echo " Couldn't write configration name" | ||
|
|
||
| # Link the network instance to the configuration | ||
| ln -s $CONFIGFS/g1/functions/"$usb_network_function" $CONFIGFS/g1/configs/c.1 || | ||
| echo " Couldn't symlink $usb_network_function" | ||
|
|
||
| # Enable ACM if first-run is done. | ||
| # Enabling it when armbian first-run executes seems to cause some kind of reset which kind of | ||
| # concludes the config without any actual configuration. | ||
| mkdir -p $CONFIGFS/g1/functions/"$usb_serial_function" || | ||
| echo " Couldn't create $CONFIGFS/g1/functions/$usb_serial_function" | ||
| ln -s $CONFIGFS/g1/functions/"$usb_serial_function" $CONFIGFS/g1/configs/c.1 || | ||
| echo " Couldn't symlink $usb_serial_function" | ||
|
|
||
| # Check if there's an USB Device Controller | ||
| if [ -z "$(ls /sys/class/udc)" ]; then | ||
| echo " No USB Device Controller available" | ||
| return | ||
| fi | ||
|
|
||
| # Link the gadget instance to an USB Device Controller. This activates the gadget. | ||
| # See also: https://github.com/postmarketOS/pmbootstrap/issues/338 | ||
| # shellcheck disable=SC2005 | ||
| echo "$(ls /sys/class/udc)" > $CONFIGFS/g1/UDC || echo " Couldn't write UDC" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and simplify UDC assignment
Several improvements needed for reliability:
- Add proper error handling instead of just echoing errors:
- mkdir $CONFIGFS/g1 || echo " Couldn't create $CONFIGFS/g1"
+ mkdir $CONFIGFS/g1 || { echo " Couldn't create $CONFIGFS/g1"; return 1; }- Simplify the UDC assignment on line 81:
- echo "$(ls /sys/class/udc)" > $CONFIGFS/g1/UDC || echo " Couldn't write UDC"
+ ls /sys/class/udc | head -1 > $CONFIGFS/g1/UDC || echo " Couldn't write UDC"- Add safety check for UDC directory:
# Check if there's an USB Device Controller
- if [ -z "$(ls /sys/class/udc)" ]; then
+ if [ ! -d /sys/class/udc ] || [ -z "$(ls /sys/class/udc 2>/dev/null)" ]; then
echo " No USB Device Controller available"
return
fi🤖 Prompt for AI Agents
In packages/bsp/usb-gadget-network/setup-usbgadget-network-multi.sh from lines
30 to 82, improve error handling by replacing simple echo error messages with
proper checks that exit or handle failures gracefully after mkdir or echo
commands fail. Simplify the UDC assignment by directly redirecting the output of
ls /sys/class/udc to the UDC file without using echo and command substitution.
Add a safety check before assigning UDC to ensure the directory /sys/class/udc
exists and contains at least one device, preventing errors if no USB Device
Controller is present.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Enable Multifunction Composite Gadget for PocketBeagle 2. This allows ssh over USB with zero configuration by the user.
Currently, enabling
ttyGS0causes armbian-first-run to not work, so had to disable it. Will work on finding a solution for it.setup-usbgadget-network-multi.shusessetup-usbgadget-network.shas a base but exposes a Multifunction Composite Gadget instead of just NCM.How Has This Been Tested?
Checklist:
Please delete options that are not relevant.