Add support for Arduino UNO Q (Qualcomm QRB2210)#9623
Add support for Arduino UNO Q (Qualcomm QRB2210)#9623SuperKali wants to merge 12 commits intoarmbian:mainfrom
Conversation
ABL overwrites kernel_addr_r/fdt_addr_r/ramdisk_addr_r at runtime, causing memory overlap that corrupts the kernel Image header. Switch from extlinux to boot.scr which sets correct addresses in the 0xC0000000 RAM bank before loading kernel, initrd and dtb. U-Boot env loads boot.scr from partition 0x43 (GPT 67 "efi") with sysboot as fallback.
- Use boot.scr instead of extlinux (BOOTSCRIPT/BOOTENV_FILE in family) - Add SERIALCON=ttyMSM0 for serial console - BOOTSIZE=512 to fit kernel + initrd - Remove BOARD_FIRMWARE_INSTALL="-full", copy only needed firmware - Add WiFi ath10k firmware copy - Add ADB daemon with Armbian branding - Add first-boot rootfs resize service - Update kernel to 6.19.0 from qcom-v6.19.0-unoq branch
Use dd with sector offsets instead of loop device partitions to extract boot and rootfs images. The previous approach failed because the build framework releases the loop device before the extension runs.
Add systemd service that expands the rootfs partition to fill the entire eMMC on first boot using sgdisk. Removes the empty userdata partition and recreates rootfs with all remaining space.
Update board-2.bin from linux-firmware (ath-20260204) with support for additional board IDs.
📝 WalkthroughWalkthroughThis pull request adds comprehensive support for the Arduino UNO Q board, a Qualcomm QRB2210-based platform. It introduces board configuration, kernel configuration, U-Boot bootloader patches, boot scripts, family configuration, image output extension, and BSP packages for automatic filesystem resizing and service initialization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
github-actions
bot
added
05
🚫 Missing required board assetsThis PR adds new board configuration(s). Required assets must already exist in github/armbian/armbian.github.io.
Missing items
Once the missing files are added (or a PR is opened in armbian/armbian.github.io), re-run this check. |
|
Armbianmonitor: https://paste.armbian.com/nejiwigeku |
![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 (2)
extensions/image-output-arduino.sh (1)
28-30: Derive the U-Boot package path from${BRANCH}.
lib/functions/compilation/uboot.shnames the packagelinux-u-boot-${BRANCH}-${BOARD}. Hardcodinglinux-u-boot-edge-arduino-uno-qmakes this hook branch-specific for no real benefit.💡 Suggested refactor
+ local uboot_pkg="linux-u-boot-${BRANCH}-${BOARD}" - cp rootfs_mount/usr/lib/linux-u-boot-edge-arduino-uno-q/boot.img arduino-images/flash + cp "rootfs_mount/usr/lib/${uboot_pkg}/boot.img" arduino-images/flash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/image-output-arduino.sh` around lines 28 - 30, The script hardcodes the U-Boot package directory; change it to derive the package name from BRANCH and BOARD (same convention as lib/functions/compilation/uboot.sh). After creating rootfs_loop from ROOTFS_IMAGE_FILE, build a variable like uboot_pkg="linux-u-boot-${BRANCH}-${BOARD}" and use quoted paths rootfs_mount/usr/lib/"${uboot_pkg}"/boot.img when copying to arduino-images/flash; update any references to the old hardcoded dirname (e.g., linux-u-boot-edge-arduino-uno-q) and ensure variables like rootfs_loop and ROOTFS_IMAGE_FILE remain quoted where used.config/boards/arduino-uno-q.csc (1)
36-38: Hardcoded path for ADB gadget binary may be fragile.The
sedcommands assume the adbd package installs the gadget binary at/usr/lib/android-sdk/platform-tools/adbd-usb-gadget. If the Debianadbdpackage changes this path in future versions, these commands will silently fail without error.Consider adding a guard to verify the file exists before modifying.
Suggested improvement
# ADB branding + if [[ -f "$SDCARD/usr/lib/android-sdk/platform-tools/adbd-usb-gadget" ]]; then chroot_sdcard sed -i 's/"Debian"/"Armbian"/' /usr/lib/android-sdk/platform-tools/adbd-usb-gadget chroot_sdcard sed -i 's/"ADB device"/"Arduino UNO Q"/' /usr/lib/android-sdk/platform-tools/adbd-usb-gadget + else + display_alert "adbd-usb-gadget not found" "skipping ADB branding" "warn" + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/boards/arduino-uno-q.csc` around lines 36 - 38, The sed commands that run via "chroot_sdcard sed -i 's/.../' ..." assume /usr/lib/android-sdk/platform-tools/adbd-usb-gadget exists; add a guard to check for the file's existence before attempting in-place edits (e.g., test -f or if [ -e ]) and only run the sed replacements when the file is present, otherwise log a warning or fail gracefully so the commands don't silently no-op; update the two occurrences that target adbd-usb-gadget to use this guarded check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/boards/arduino-uno-q.csc`:
- Around line 40-43: The enable for armbian-resize-filesystem-qcom.service is
happening in post_family_tweaks__* before the service file is installed in
post_family_tweaks_bsp__*; move the enable step so the service file exists when
enabling — either (A) install the service file earlier by adding its
installation into post_family_tweaks__* before the systemctl enable call, or (B)
append the systemctl enable command into the BSP postinst hook created by
post_family_tweaks_bsp__* (so the enable runs during package postinst),
targeting the armbian-resize-filesystem-qcom.service name to ensure enable
succeeds.
In `@config/bootscripts/boot-qcom.cmd`:
- Around line 19-20: The fallback rootdev currently uses "/dev/mmcblk0p1" which
doesn't match the layout where partition 67 is efi and partition 68 is the
rootfs; update the default setenv rootdev value in boot-qcom.cmd to the actual
rootfs partition (e.g., "/dev/mmcblk0p68") so the no-armbianEnv.txt boot path
remains functional and consistent with the resize helper and partition
numbering.
In `@config/sources/families/qrb2210.conf`:
- Around line 35-56: In uboot_custom_postprocess, remove the hardcoded directory
change (the run_host_command_logged cd ...) since the function already runs in
the u-boot build directory, and instead download mkbootimg to a local file,
verify its integrity against a configured checksum variable (e.g.,
MKBOOTIMG_SHA256) using sha256sum (or openssl dgst -sha256), aborting with a
logged error if the checksum does not match, then chmod +x and proceed to run
mkbootimg; reference the function name uboot_custom_postprocess and the
mkbootimg binary and use BOOT_FDT_FILE only for the u-boot artifact path.
In `@extensions/image-output-arduino.sh`:
- Around line 27-32: The current sequence that sets up a loop device and mounts
rootfs (creating rootfs_mount, setting rootfs_loop via losetup, then mount and
cp) can leave the mountpoint and loop device attached if a subsequent command
fails under set -e; add a cleanup handler using the repository's
add_cleanup_handler pattern: declare rootfs_loop and rootfs_mounted flags before
running losetup/mount, register a cleanup function (e.g., cleanup_rootfs) that
checks if rootfs_mount is mounted and runs umount rootfs_mount and checks if
rootfs_loop is non-empty and runs losetup -d "$rootfs_loop", then call
add_cleanup_handler cleanup_rootfs immediately after acquiring the loop device
(after setting rootfs_loop) so the handler runs on exit/failure; update the
cp/umount/losetup -d logic to rely on the cleanup handler (or remove duplicate
teardown) and ensure variables referenced are the same names (rootfs_loop,
rootfs_mount) used in the diff.
In `@packages/bsp/arduino/armbian-resize-filesystem-qcom`:
- Line 1: The script currently writes the completion flag `.rootfs-expanded`
before verifying success of critical commands and never validates the extracted
STARTLBA; change the flow to fail fast: enable safe error handling (e.g., set
-euo pipefail or check exit codes) and validate STARTLBA is non-empty and
numeric before using it in sgdisk/parted/resize operations, run
sgdisk/partprobe/resize2fs and check each command's exit status, and only
create/write `.rootfs-expanded` after all operations have completed
successfully; reference the extracted variable STARTLBA and the commands sgdisk,
partprobe, and resize2fs when adding checks and moving the flag-write to the
script’s successful end.
---
Nitpick comments:
In `@config/boards/arduino-uno-q.csc`:
- Around line 36-38: The sed commands that run via "chroot_sdcard sed -i
's/.../' ..." assume /usr/lib/android-sdk/platform-tools/adbd-usb-gadget exists;
add a guard to check for the file's existence before attempting in-place edits
(e.g., test -f or if [ -e ]) and only run the sed replacements when the file is
present, otherwise log a warning or fail gracefully so the commands don't
silently no-op; update the two occurrences that target adbd-usb-gadget to use
this guarded check.
In `@extensions/image-output-arduino.sh`:
- Around line 28-30: The script hardcodes the U-Boot package directory; change
it to derive the package name from BRANCH and BOARD (same convention as
lib/functions/compilation/uboot.sh). After creating rootfs_loop from
ROOTFS_IMAGE_FILE, build a variable like
uboot_pkg="linux-u-boot-${BRANCH}-${BOARD}" and use quoted paths
rootfs_mount/usr/lib/"${uboot_pkg}"/boot.img when copying to
arduino-images/flash; update any references to the old hardcoded dirname (e.g.,
linux-u-boot-edge-arduino-uno-q) and ensure variables like rootfs_loop and
ROOTFS_IMAGE_FILE remain quoted where used.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6589b63-064d-48ec-a053-77dd04c40f56
⛔ Files ignored due to path filters (16)
packages/blobs/arduino/firmware/ath10k/WCN3990/hw1.0/board-2.binis excluded by!**/*.binpackages/blobs/arduino/firmware/ath10k/WCN3990/hw1.0/firmware-5.binis excluded by!**/*.binpackages/blobs/arduino/firmware/ath10k/WCN3990/hw1.0/qcm2290/firmware-5.binis excluded by!**/*.binpackages/blobs/arduino/firmware/ath10k/WCN3990/hw1.0/qrb4210/firmware-5.binis excluded by!**/*.binpackages/blobs/arduino/firmware/qca/apnv11.binis excluded by!**/*.binpackages/blobs/arduino/flash/cdt.binis excluded by!**/*.binpackages/blobs/arduino/flash/gpt_backup0.binis excluded by!**/*.binpackages/blobs/arduino/flash/gpt_backup1.binis excluded by!**/*.binpackages/blobs/arduino/flash/gpt_backup2.binis excluded by!**/*.binpackages/blobs/arduino/flash/gpt_both0.binis excluded by!**/*.binpackages/blobs/arduino/flash/gpt_empty0.binis excluded by!**/*.binpackages/blobs/arduino/flash/gpt_main0.binis excluded by!**/*.binpackages/blobs/arduino/flash/gpt_main1.binis excluded by!**/*.binpackages/blobs/arduino/flash/gpt_main2.binis excluded by!**/*.binpackages/blobs/arduino/flash/zeros_1sector.binis excluded by!**/*.binpackages/blobs/arduino/flash/zeros_33sectors.binis excluded by!**/*.bin
📒 Files selected for processing (43)
config/boards/arduino-uno-q.cscconfig/bootenv/qcom.txtconfig/bootscripts/boot-qcom.cmdconfig/kernel/linux-qrb2210-edge.configconfig/sources/families/qrb2210.confextensions/image-output-arduino.shpackages/blobs/arduino/firmware/ath10k/WCN3990/hw1.0/qcm2290/wlanmdsp.mbnpackages/blobs/arduino/firmware/ath10k/WCN3990/hw1.0/qrb4210/wlanmdsp.mbnpackages/blobs/arduino/firmware/ath10k/WCN3990/hw1.0/wlanmdsp.mbnpackages/blobs/arduino/firmware/qca/apbtfw11.tlvpackages/blobs/arduino/firmware/qcom/a702_sqe.fwpackages/blobs/arduino/firmware/qcom/qcm2290/a702_zap.mbnpackages/blobs/arduino/firmware/qcom/qcm2290/adsp.mbnpackages/blobs/arduino/firmware/qcom/qcm2290/adspr.jsnpackages/blobs/arduino/firmware/qcom/qcm2290/adsps.jsnpackages/blobs/arduino/firmware/qcom/qcm2290/adspua.jsnpackages/blobs/arduino/firmware/qcom/qcm2290/modem.mbnpackages/blobs/arduino/firmware/qcom/qcm2290/modemr.jsnpackages/blobs/arduino/firmware/qcom/qcm2290/modemuw.jsnpackages/blobs/arduino/firmware/qcom/qcm2290/wlanmdsp.mbnpackages/blobs/arduino/firmware/qcom/venus-6.0/venus.mbnpackages/blobs/arduino/flash/abl.elfpackages/blobs/arduino/flash/devcfg.mbnpackages/blobs/arduino/flash/featenabler.mbnpackages/blobs/arduino/flash/hyp.mbnpackages/blobs/arduino/flash/imagefv.elfpackages/blobs/arduino/flash/km4.mbnpackages/blobs/arduino/flash/multi_image.mbnpackages/blobs/arduino/flash/patch0.xmlpackages/blobs/arduino/flash/prog_firehose_ddr.elfpackages/blobs/arduino/flash/qupv3fw.elfpackages/blobs/arduino/flash/rawprogram0.nouser.xmlpackages/blobs/arduino/flash/rawprogram0.xmlpackages/blobs/arduino/flash/rpm.mbnpackages/blobs/arduino/flash/storsec.mbnpackages/blobs/arduino/flash/tz.mbnpackages/blobs/arduino/flash/uefi_sec.mbnpackages/blobs/arduino/flash/xbl.elfpackages/blobs/arduino/flash/xbl_feature_config.elfpackages/bsp/arduino/armbian-resize-filesystem-qcompackages/bsp/arduino/armbian-resize-filesystem-qcom.servicepatch/u-boot/u-boot-qrb2210/0001-qcom-add-extlinux.patchpatch/u-boot/u-boot-qrb2210/0002-u-boot-use-extlinux.patch
|
Great work you two.
|
|
Also the board does have video output just via usb-c dp instead of hdmi |
Do you have any useful suggestions? Or a plan for how to do what you're proposing? Having another repository definitely doesn't make sense.
There's no other option; the GPT-style partitions are pre-configured, and I don't think the source code for them—which specifies the partition tables—is available online. Currently, the userdata partition is simply resized to make space for root partitions that are adjacent and don't cause any issues.
It's a minor issue; I'll fix it by editing it |
|
To add some more context with public references: The QCS6490 and QRB2210 have fundamentally different boot architectures. The QCS6490 uses UFS storage with a full UEFI/EDK2 firmware that can directly load GRUB from an EFI System Partition, which is why About the rootfs resize, the custom script is needed because the Qualcomm GPT includes an empty userdata partition (partition 69, size 0) that causes parted to fail with "Can't have the end before the start!", which breaks Armbian's standard armbian-resize-filesystem. The script simply removes that empty partition with sgdisk before expanding rootfs, it's a workaround for the GPT layout not a SoC-level intrinsic. If maybe you have a solution under the hood that I don’t know just say it, maybe giving more info about it |
|
lgtm |
|
I have started: https://github.com/armbian/qcombin |
Restore the ath10k board-2.bin firmware to the version provided in the original board bring-up, as the upstream version has not been validated on this hardware.
Move EDL flash binaries (ABL, XBL, TrustZone, GPT tables, etc.) from packages/blobs/arduino/flash/ to the external armbian/qcombin repository under the Agatti folder, following the same pattern as rkbin for Rockchip and mtkbin for MediaTek. Add qcombin.sh extension to fetch binaries during build. Update image-output-arduino.sh to source flash files from qcombin cache and derive U-Boot package path from BRANCH/BOARD variables.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
config/sources/families/qrb2210.conf (2)
47-50:⚠️ Potential issue | 🟠 MajorAdd checksum verification for downloaded
mkbootimgbinary.Downloading an executable from the internet without integrity verification creates a supply chain risk. If the Launchpad CDN or URL is compromised, a malicious binary could be executed during the build.
The previous review correctly identified this security concern. Add SHA256 verification:
🔒 Proposed fix with checksum
+ # SHA256 of mkbootimg from launchpadlibrarian.net/810765814 + declare -g MKBOOTIMG_SHA256="<INSERT_ACTUAL_SHA256_HERE>" + # https://bugs.launchpad.net/ubuntu/+source/android-platform-tools/+bug/2058228 # https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1050300 run_host_command_logged wget https://launchpadlibrarian.net/810765814/mkbootimg + echo "${MKBOOTIMG_SHA256} mkbootimg" | sha256sum -c - || exit_with_error "mkbootimg checksum verification failed" run_host_command_logged chmod +x mkbootimg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/sources/families/qrb2210.conf` around lines 47 - 50, Add SHA256 integrity checking for the downloaded mkbootimg: after run_host_command_logged wget ... mkbootimg, compute and verify its SHA256 against the expected hash (use run_host_command_logged to run sha256sum -c or echo "<expected-sha256> mkbootimg" | sha256sum -c -) before running run_host_command_logged chmod +x mkbootimg; if verification fails, log the error and exit non‑zero so the build stops. Ensure the verification step references the same mkbootimg filename and that chmod +x only runs upon successful verification.
38-38:⚠️ Potential issue | 🟡 MinorHardcoded U-Boot worktree path may break with different branch names.
The path
u-boot-worktree/u-boot/qcom-mainlineassumes the branch name will always beqcom-mainline. IfBOOTBRANCHis overridden (allowed by line 22's default syntax), this path will be incorrect.#!/bin/bash # Check how other family configs handle uboot_custom_postprocess working directory rg -n "function uboot_custom_postprocess" config/sources/families/ -A 5 | head -40🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/sources/families/qrb2210.conf` at line 38, The hardcoded path in the run_host_command_logged call inside uboot_custom_postprocess uses "u-boot-worktree/u-boot/qcom-mainline" which breaks if BOOTBRANCH is overridden; update that call to build the path dynamically using the BOOTBRANCH variable (or the same variable used earlier where BOOTBRANCH is set/defaulted) so the working directory becomes "$SRC/cache/sources/u-boot-worktree/u-boot/${BOOTBRANCH}" (or equivalent expansion), ensuring run_host_command_logged and any dependent commands use the variable-driven path rather than the literal "qcom-mainline".extensions/image-output-arduino.sh (1)
27-32:⚠️ Potential issue | 🟠 MajorAdd cleanup handler to prevent resource leaks on failure.
This segment acquires a loop device and mounts the rootfs without registering a cleanup handler. Under
set -e, ifmountorcpfails, the loop device remains attached.The previous review correctly identified this issue. Consider using a local trap or
add_cleanup_handlerpattern as shown inextensions/gxlimg.sh:♻️ Suggested approach using trap
+ local rootfs_loop="" + cleanup_arduino_rootfs() { + [[ -n "$rootfs_loop" ]] && { umount rootfs_mount 2>/dev/null || true; losetup -d "$rootfs_loop" 2>/dev/null || true; } + rm -rf rootfs_mount 2>/dev/null || true + } + trap cleanup_arduino_rootfs EXIT + mkdir -p rootfs_mount - local rootfs_loop=$(losetup -f --show "${ROOTFS_IMAGE_FILE}") + rootfs_loop=$(losetup -f --show "${ROOTFS_IMAGE_FILE}") mount ${rootfs_loop} rootfs_mount cp rootfs_mount/usr/lib/linux-u-boot-${BRANCH}-${BOARD}/boot.img arduino-images/flash/ umount rootfs_mount losetup -d ${rootfs_loop} + trap - EXIT + rm -rf rootfs_mount🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/image-output-arduino.sh` around lines 27 - 32, The code attaches a loop device to ROOTFS_IMAGE_FILE and mounts it without a cleanup handler, so failures (e.g., in mount or cp) leak the loop device; add a cleanup function (e.g., cleanup_rootfs) that checks and unmounts rootfs_mount if mounted and detaches the loop device stored in rootfs_loop, then register it with trap 'cleanup_rootfs EXIT' (or use the existing add_cleanup_handler pattern) immediately after creating rootfs_loop so any early exit runs the handler; ensure the handler safely no-ops if rootfs_loop is empty or rootfs_mount is not mounted and remove the trap/handler after normal successful cleanup if needed.
🧹 Nitpick comments (4)
config/sources/families/qrb2210.conf (2)
35-58: Inconsistent indentation inuboot_custom_postprocess.The function mixes tabs (lines 38-43, 52-57) and spaces (lines 45-50). This inconsistency can cause issues with some editors and reduces readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/sources/families/qrb2210.conf` around lines 35 - 58, The function uboot_custom_postprocess has mixed tabs and spaces causing inconsistent indentation; normalize the file by replacing tabs with spaces (or vice-versa per project style) inside uboot_custom_postprocess so all run_host_command_logged lines, the heredoc/cat continuation, the touch/wget/chmod/mkbootimg calls, and the continuation backslashes use the same indentation style; ensure lines referencing "${BOOT_FDT_FILE}", mkbootimg, and other symbols keep their current spacing semantics while aligning indentation consistently across the whole function.
73-75: Intentional no-op forwrite_uboot_platformis appropriate.Per the PR objectives, the QRB2210 uses U-Boot packaged for ABL chainloading rather than direct platform writing. The no-op implementation with a display alert correctly skips the standard U-Boot write process.
Consider adding a brief comment explaining why this is intentionally empty for future maintainers:
📝 Suggested documentation
+# QRB2210 uses U-Boot as an Android boot.img chainloaded by ABL; +# no direct platform write is needed during image build. write_uboot_platform() { display_alert "Skip." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/sources/families/qrb2210.conf` around lines 73 - 75, The no-op write_uboot_platform() implementation is correct but lacks explanatory documentation; add a short inline comment above or inside the write_uboot_platform function (referencing write_uboot_platform) stating that QRB2210 uses U-Boot packaged for ABL chainloading and therefore standard U-Boot platform write is intentionally skipped, so future maintainers understand this is deliberate and not an omission.extensions/image-output-arduino.sh (2)
12-15: Consolidatefdiskcalls and add robustness checks.Running
fdisk -lfour times on the same image is inefficient. Additionally, theawkparsing assumes specific column positions which may vary across fdisk versions or partition table types.♻️ Proposed consolidation
- local p1_start=$(fdisk -l "${img}" | grep "${img}1" | awk '{print $2}') - local p1_sectors=$(fdisk -l "${img}" | grep "${img}1" | awk '{print $4}') - local p2_start=$(fdisk -l "${img}" | grep "${img}2" | awk '{print $2}') - local p2_sectors=$(fdisk -l "${img}" | grep "${img}2" | awk '{print $4}') + local fdisk_output + fdisk_output=$(fdisk -l "${img}") + local p1_start=$(echo "$fdisk_output" | grep "${img}1" | awk '{print $2}') + local p1_sectors=$(echo "$fdisk_output" | grep "${img}1" | awk '{print $4}') + local p2_start=$(echo "$fdisk_output" | grep "${img}2" | awk '{print $2}') + local p2_sectors=$(echo "$fdisk_output" | grep "${img}2" | awk '{print $4}') + + # Validate extracted values + [[ -z "$p1_start" || -z "$p1_sectors" ]] && exit_with_error "Failed to parse partition 1 from fdisk output" + [[ -z "$p2_start" || -z "$p2_sectors" ]] && exit_with_error "Failed to parse partition 2 from fdisk output"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/image-output-arduino.sh` around lines 12 - 15, The current script calls fdisk -l on ${img} four times and relies on hardcoded column positions for p1_start, p1_sectors, p2_start and p2_sectors; replace this by running fdisk -l "${img}" once into a variable (e.g. fdout) and parse that output to extract the partition lines for "${img}1" and "${img}2" with more robust patterns (use grep -E for the exact device name and awk or cut that matches fields by whitespace or use sed to capture start/size columns), add checks that the partition lines exist and that extracted values are non-empty and numeric, and fail with a clear error if parsing fails. Ensure you update references to p1_start, p1_sectors, p2_start and p2_sectors accordingly.
23-40: Use absolute paths for staging directory to avoid working directory issues.The
arduino-imagesdirectory is created and used with a relative path. If the working directory changes unexpectedly (e.g., due to future code modifications or extension ordering), this could cause failures.♻️ Proposed fix using absolute paths
- rm -rf arduino-images - mkdir -p arduino-images/flash - cp -r "${QCOMBIN_DIR}/Agatti/"* arduino-images/flash/ + local staging_dir="${DESTIMG}/arduino-images" + rm -rf "${staging_dir}" + mkdir -p "${staging_dir}/flash" + cp -r "${QCOMBIN_DIR}/Agatti/"* "${staging_dir}/flash/" ... - cp rootfs_mount/usr/lib/linux-u-boot-${BRANCH}-${BOARD}/boot.img arduino-images/flash/ + cp rootfs_mount/usr/lib/linux-u-boot-${BRANCH}-${BOARD}/boot.img "${staging_dir}/flash/" ... - mv ${BOOTFS_IMAGE_FILE} arduino-images/disk-sdcard.img.esp - mv ${ROOTFS_IMAGE_FILE} arduino-images/disk-sdcard.img.root + mv "${BOOTFS_IMAGE_FILE}" "${staging_dir}/disk-sdcard.img.esp" + mv "${ROOTFS_IMAGE_FILE}" "${staging_dir}/disk-sdcard.img.root" - tar -cvf ${DESTIMG}/${version}.tar arduino-images - rm -rf arduino-images + tar -cvf "${DESTIMG}/${version}.tar" -C "${DESTIMG}" arduino-images + rm -rf "${staging_dir}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@extensions/image-output-arduino.sh` around lines 23 - 40, The script uses a relative staging folder "arduino-images" and "rootfs_mount" which can break if the working directory changes; change to an absolute staging path by introducing a single absolute STAGING_DIR (e.g., created with mktemp -d or derived from ${DESTIMG}) and replace all hardcoded "arduino-images" and "rootfs_mount" references with "${STAGING_DIR}/arduino-images" and "${STAGING_DIR}/rootfs_mount"; ensure subsequent operations (mkdir, cp from "${QCOMBIN_DIR}", mount/umount of ${ROOTFS_IMAGE_FILE}, moves of ${BOOTFS_IMAGE_FILE} and ${ROOTFS_IMAGE_FILE}, tar into ${DESTIMG}/${version}.tar and the final rm -rf cleanup) all use those absolute paths, and keep using the same ${rootfs_loop}, ${ROOTFS_IMAGE_FILE}, ${BOOTFS_IMAGE_FILE}, ${DESTIMG}, ${version} variables as identifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/sources/families/qrb2210.conf`:
- Around line 47-50: Add SHA256 integrity checking for the downloaded mkbootimg:
after run_host_command_logged wget ... mkbootimg, compute and verify its SHA256
against the expected hash (use run_host_command_logged to run sha256sum -c or
echo "<expected-sha256> mkbootimg" | sha256sum -c -) before running
run_host_command_logged chmod +x mkbootimg; if verification fails, log the error
and exit non‑zero so the build stops. Ensure the verification step references
the same mkbootimg filename and that chmod +x only runs upon successful
verification.
- Line 38: The hardcoded path in the run_host_command_logged call inside
uboot_custom_postprocess uses "u-boot-worktree/u-boot/qcom-mainline" which
breaks if BOOTBRANCH is overridden; update that call to build the path
dynamically using the BOOTBRANCH variable (or the same variable used earlier
where BOOTBRANCH is set/defaulted) so the working directory becomes
"$SRC/cache/sources/u-boot-worktree/u-boot/${BOOTBRANCH}" (or equivalent
expansion), ensuring run_host_command_logged and any dependent commands use the
variable-driven path rather than the literal "qcom-mainline".
In `@extensions/image-output-arduino.sh`:
- Around line 27-32: The code attaches a loop device to ROOTFS_IMAGE_FILE and
mounts it without a cleanup handler, so failures (e.g., in mount or cp) leak the
loop device; add a cleanup function (e.g., cleanup_rootfs) that checks and
unmounts rootfs_mount if mounted and detaches the loop device stored in
rootfs_loop, then register it with trap 'cleanup_rootfs EXIT' (or use the
existing add_cleanup_handler pattern) immediately after creating rootfs_loop so
any early exit runs the handler; ensure the handler safely no-ops if rootfs_loop
is empty or rootfs_mount is not mounted and remove the trap/handler after normal
successful cleanup if needed.
---
Nitpick comments:
In `@config/sources/families/qrb2210.conf`:
- Around line 35-58: The function uboot_custom_postprocess has mixed tabs and
spaces causing inconsistent indentation; normalize the file by replacing tabs
with spaces (or vice-versa per project style) inside uboot_custom_postprocess so
all run_host_command_logged lines, the heredoc/cat continuation, the
touch/wget/chmod/mkbootimg calls, and the continuation backslashes use the same
indentation style; ensure lines referencing "${BOOT_FDT_FILE}", mkbootimg, and
other symbols keep their current spacing semantics while aligning indentation
consistently across the whole function.
- Around line 73-75: The no-op write_uboot_platform() implementation is correct
but lacks explanatory documentation; add a short inline comment above or inside
the write_uboot_platform function (referencing write_uboot_platform) stating
that QRB2210 uses U-Boot packaged for ABL chainloading and therefore standard
U-Boot platform write is intentionally skipped, so future maintainers understand
this is deliberate and not an omission.
In `@extensions/image-output-arduino.sh`:
- Around line 12-15: The current script calls fdisk -l on ${img} four times and
relies on hardcoded column positions for p1_start, p1_sectors, p2_start and
p2_sectors; replace this by running fdisk -l "${img}" once into a variable (e.g.
fdout) and parse that output to extract the partition lines for "${img}1" and
"${img}2" with more robust patterns (use grep -E for the exact device name and
awk or cut that matches fields by whitespace or use sed to capture start/size
columns), add checks that the partition lines exist and that extracted values
are non-empty and numeric, and fail with a clear error if parsing fails. Ensure
you update references to p1_start, p1_sectors, p2_start and p2_sectors
accordingly.
- Around line 23-40: The script uses a relative staging folder "arduino-images"
and "rootfs_mount" which can break if the working directory changes; change to
an absolute staging path by introducing a single absolute STAGING_DIR (e.g.,
created with mktemp -d or derived from ${DESTIMG}) and replace all hardcoded
"arduino-images" and "rootfs_mount" references with
"${STAGING_DIR}/arduino-images" and "${STAGING_DIR}/rootfs_mount"; ensure
subsequent operations (mkdir, cp from "${QCOMBIN_DIR}", mount/umount of
${ROOTFS_IMAGE_FILE}, moves of ${BOOTFS_IMAGE_FILE} and ${ROOTFS_IMAGE_FILE},
tar into ${DESTIMG}/${version}.tar and the final rm -rf cleanup) all use those
absolute paths, and keep using the same ${rootfs_loop}, ${ROOTFS_IMAGE_FILE},
${BOOTFS_IMAGE_FILE}, ${DESTIMG}, ${version} variables as identifiers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0a69718-4358-431d-814c-d358920f8c6b
⛔ Files ignored due to path filters (1)
packages/blobs/arduino/firmware/ath10k/WCN3990/hw1.0/board-2.binis excluded by!**/*.bin
📒 Files selected for processing (4)
config/boards/arduino-uno-q.cscconfig/sources/families/qrb2210.confextensions/image-output-arduino.shextensions/qcombin.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- config/boards/arduino-uno-q.csc
|
I see no problem with the boot stack complexity here (we all wish the stack was simpler, but hey, sometimes you gotta do what you gotta do). I do agree that firmware blobs should be kept elsewhere. Maybe we should formalize this better: "Armbian should move all binary blobs out of armbian/build over time, starting by not adding any new ones, and later moving existing ones". |
I don't see any problem with this. To avoid delaying the merge of this PR, I'd prefer not to dwell on it too much; perhaps we could handle it via a new PR. |
|
qcombin side is done. Note |
Then for firmware only thing left is to move When that is done and the |
Move firmware blobs to armbian/firmware repository (armbian/firmware#123) and remove manual firmware copies from post_family_tweaks. Firmware is now installed via the armbian-firmware package. Rename boot-qcom to boot-qrb2210 to be family-specific.
|
Done. Firmware moved to armbian/firmware#123, flash binaries moved to armbian/qcombin#1, and boot-qcom.cmd renamed to boot-qrb2210.cmd. Anything else or can we finally press the green button? :) |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
config/bootscripts/boot-qrb2210.cmd (1)
20-20:⚠️ Potential issue | 🟠 MajorFix fallback
rootdevto the actual rootfs partition.Line 20 defaults to
/dev/mmcblk0p1, but this layout uses partition 67 for EFI and partition 68 for rootfs. The no-armbianEnv.txtpath can boot with the wrong root device.Suggested fix
-setenv rootdev "/dev/mmcblk0p1" +setenv rootdev "/dev/mmcblk0p68"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/bootscripts/boot-qrb2210.cmd` at line 20, The fallback root device currently set by the setenv rootdev assignment uses /dev/mmcblk0p1 which is incorrect for this layout; update the setenv rootdev value in boot-qrb2210.cmd (the setenv rootdev line) to point to the actual rootfs partition (/dev/mmcblk0p68) so the no-armbianEnv.txt boot path uses the correct root device.config/sources/families/qrb2210.conf (1)
38-50:⚠️ Potential issue | 🟠 MajorRemove hardcoded branch path and verify
mkbootimgbefore execution.Line 38 hardcodes
qcom-mainlinedespite configurableBOOTBRANCH, and Lines 49-50 execute a downloaded binary without checksum/signature verification.Suggested hardening/refactor
function uboot_custom_postprocess() { - run_host_command_logged cd "$SRC"/cache/sources/u-boot-worktree/u-boot/qcom-mainline run_host_command_logged rm -f u-boot-nodtb.bin.gz u-boot-nodtb.bin.gz-dtb empty-ramdisk boot.img mkbootimg run_host_command_logged gzip u-boot-nodtb.bin @@ - run_host_command_logged wget https://launchpadlibrarian.net/810765814/mkbootimg + local mkbootimg_url="https://launchpadlibrarian.net/810765814/mkbootimg" + local mkbootimg_sha256="<pin-known-sha256-here>" + run_host_command_logged wget "${mkbootimg_url}" -O mkbootimg + run_host_command_logged bash -c "echo '${mkbootimg_sha256} mkbootimg' | sha256sum -c -" run_host_command_logged chmod +x mkbootimg#!/bin/bash # Verify the two concerns in current tree: # 1) hardcoded qcom-mainline path # 2) missing checksum verification for mkbootimg rg -n 'qcom-mainline|mkbootimg|sha256sum|sha256' config/sources/families/qrb2210.conf -C 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/sources/families/qrb2210.conf` around lines 38 - 50, The script hardcodes the branch directory and runs an unverified binary: replace the literal "qcom-mainline" in the run_host_command_logged cd command with the configurable BOOTBRANCH variable (so the cd uses "$SRC/.../u-boot/${BOOTBRANCH}") and keep BOOT_FDT_FILE usage as-is; for mkbootimg, download and verify its integrity before making it executable by adding a verification step (e.g., fetch an expected sha256 or signature alongside mkbootimg, validate the downloaded file with sha256sum --check or signature verification, and only then run_host_command_logged chmod +x mkbootimg and subsequent execution), updating the run_host_command_logged wget/chmod sequence accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/bootscripts/boot-qrb2210.cmd`:
- Line 20: The fallback root device currently set by the setenv rootdev
assignment uses /dev/mmcblk0p1 which is incorrect for this layout; update the
setenv rootdev value in boot-qrb2210.cmd (the setenv rootdev line) to point to
the actual rootfs partition (/dev/mmcblk0p68) so the no-armbianEnv.txt boot path
uses the correct root device.
In `@config/sources/families/qrb2210.conf`:
- Around line 38-50: The script hardcodes the branch directory and runs an
unverified binary: replace the literal "qcom-mainline" in the
run_host_command_logged cd command with the configurable BOOTBRANCH variable (so
the cd uses "$SRC/.../u-boot/${BOOTBRANCH}") and keep BOOT_FDT_FILE usage as-is;
for mkbootimg, download and verify its integrity before making it executable by
adding a verification step (e.g., fetch an expected sha256 or signature
alongside mkbootimg, validate the downloaded file with sha256sum --check or
signature verification, and only then run_host_command_logged chmod +x mkbootimg
and subsequent execution), updating the run_host_command_logged wget/chmod
sequence accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b8cb3bf-fa07-44e1-a149-a1aa22f21d56
📒 Files selected for processing (4)
config/boards/arduino-uno-q.cscconfig/bootenv/qrb2210.txtconfig/bootscripts/boot-qrb2210.cmdconfig/sources/families/qrb2210.conf
✅ Files skipped from review due to trivial changes (1)
- config/bootenv/qrb2210.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- config/boards/arduino-uno-q.csc
|
Just curios. Why was btf disabled? |
|
Honestly not sure, it was already there in the original bring-up from chainsx. Probably to avoid build failures on hosts with limited RAM since BTF compilation is memory-hungry. I can remove it if you think it should be enabled. |
|
Perhaps for the sake of consistency I'd enable it but literally no other reason. |
5 participants
Description
Add support for the Arduino UNO Q board, based on the Qualcomm QRB2210 SoC (quad-core ARM64, 2/4 GB RAM, 16/32 GB eMMC).
The initial board bring-up was done by @chainsx who provided the full foundation: board config, family config, U-Boot patches, kernel config, firmware blobs, and the QDL flash infrastructure. This PR integrates his work into current upstream with additional fixes and improvements found during testing.
What's included:
arduino-uno-q.csc) and family config (qrb2210.conf)qcom_defconfig(sysboot support) and boot environment (boot.scr loading from eMMC GPT partition)boot-qcom.cmd) with explicit memory addresses to avoid ABL-reserved regions that cause memory overlapqcom-v6.19.0-unoqbranchimage-output-arduino.sh) that produces a QDL-flashable archive with Qualcomm partition layoutadbdpackage for USB shell access over USB-CTechnical notes:
kernel_addr_r, etc.) at runtime, so the boot script must explicitly set memory addresses before loading kernel/initrdHow to flash
Requirements
SUBSYSTEM=="usb", ATTR{idVendor}=="05c6", ATTR{idProduct}=="9008", MODE="0666"to/etc/udev/rules.d/51-arduino-uno-q.rulesBuild
Flash
cd output/images && tar xf *.tarcd arduino-images/flash qdl --allow-missing --storage emmc prog_firehose_ddr.elf rawprogram0.xml patch0.xmlAccess
adb shell(over USB-C, no network required)ttyMSM0(1.8V logic level — do not use 3.3V/5V adapters)On first boot the rootfs is automatically expanded to fill the entire eMMC.
How Has This Been Tested?
Checklist:
Summary by CodeRabbit