Radxa Zero 3: fix bluetooth, for edge & current, switch to u-boot master#8511
Radxa Zero 3: fix bluetooth, for edge & current, switch to u-boot master#8511leggewie merged 3 commits intoarmbian:mainfrom
Conversation
github-actions
bot
added
size/medium
WalkthroughAdds AIC8800 wireless declarations and post-family tweak hooks to multiple board configs (radxa-zero3, kickpik2b, radxa-cubie-a5e): introduces PACKAGE_LIST_BOARD entries, sets AIC8800_TYPE and enables the radxa-aic8800 extension. Adds post_family_tweaks_bsp__aic8800_wireless() to create modprobe/modules-load configs, write aic8800-wireless.conf and aic8800-btlpm.conf, and install the aic-bluetooth binary and a systemd unit when BSP assets exist (warns when absent). Adds post_family_tweaks__enable_aic8800_bluetooth_service() to enable the unit if present. Replaces cp/mkdir with install(1) and moves unit files to /usr/lib/systemd/system. Updates mainline U-Boot version/handling and adds an inner write_uboot_platform() helper. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
requested review from
RadxaYuntian,
amazingfate,
chainsx and
pyavitz
coderabbitai
bot
added
BSP
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
config/boards/radxa-zero3.csc (3)
20-44: Install service into /lib/systemd/system and use install(1) for robustnessThe flow mirrors other boards, but two tweaks improve consistency and resilience:
- Place the unit in /lib/systemd/system (systemd’s canonical unit dir used elsewhere in this repo, e.g., bluetooth-hciattach), not /etc/systemd/system.
- Use install instead of mkdir/cp/chmod to ensure directories, permissions, and atomic writes.
Apply this diff to adjust the service/script installation:
@@ - mkdir -p "${destination}"/etc/systemd/system - mkdir -p "${destination}"/usr/bin - cp -f "$SRC/packages/bsp/aic8800/aic-bluetooth" "${destination}"/usr/bin - chmod +x "${destination}"/usr/bin/aic-bluetooth - cp -f "$SRC/packages/bsp/aic8800/aic-bluetooth.service" "${destination}"/etc/systemd/system + install -d -m 0755 "${destination}/usr/bin" + install -m 0755 "$SRC/packages/bsp/aic8800/aic-bluetooth" "${destination}/usr/bin/aic-bluetooth" + install -d -m 0755 "${destination}/lib/systemd/system" + install -m 0644 "$SRC/packages/bsp/aic8800/aic-bluetooth.service" "${destination}/lib/systemd/system/aic-bluetooth.service"Optional follow-up:
- If AIC8800 BSP assets are unexpectedly absent, consider logging a warning to ease troubleshooting:
else display_alert "$BOARD" "Skipping AIC8800 BT assets (packages/bsp/aic8800 not found)" "warn" fiNotes:
- Keeping the modules-load list (hidp, rfcomm, bnep, aic8800_btlpm_sdio) mirrors prior boards; be aware some kernels may lack hidp/rfcomm as modules and will log harmless “module not found” at boot.
46-50: Guard enabling the service to avoid build-time failures if the unit is missingIn rare environments where packages/bsp/aic8800 is unavailable, enabling unconditionally will fail in chroot. Guarding on the unit file’s presence matches the robustness seen in other boards.
Apply this diff:
- chroot_sdcard systemctl --no-reload enable aic-bluetooth.service + if chroot_sdcard test -f /lib/systemd/system/aic-bluetooth.service || chroot_sdcard test -f /etc/systemd/system/aic-bluetooth.service; then + chroot_sdcard systemctl --no-reload enable aic-bluetooth.service + else + display_alert "$BOARD" "aic-bluetooth.service not found in image; skipping enable" "warn" + fiHeads-up:
- Per prior learnings for AIC8800 BT, a Type=idle and delayed start are intentional for mainline kernels with this chipset. Assuming the BSP unit carries those, this enable is correct.
65-69: Comment clarity nitMinor grammar/clarity improvements in the explanatory comments.
Apply this diff:
- ## for binman-atf-mainline: this requires setting it for BOOT_SCENARIO at the top, which breaks branch=vendor - # cannot set BOOT_SOC=rk3566 as has side effects in Armbian scripts, but ATF_TARGET_MAP works - # ATF does not separate rk3566 from rk3568 + ## For binman-atf-mainline: setting BOOT_SCENARIO at the top would break branch=vendor, so we don't enable it globally. + # We cannot set BOOT_SOC=rk3566 as it has side effects in Armbian scripts; ATF_TARGET_MAP is the safer override. + # ATF does not separate rk3566 from rk3568.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
patch/u-boot/v2025.10/cmd-fileenv-read-string-from-file-into-env.patchis excluded by!patch/**
📒 Files selected for processing (1)
config/boards/radxa-zero3.csc(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: EvilOlaf
PR: armbian/build#8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
📚 Learning: 2025-07-17T04:16:29.551Z
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-07-23T07:30:52.265Z
Learnt from: EvilOlaf
PR: armbian/build#8417
File: config/boards/orangepi5pro.csc:57-58
Timestamp: 2025-07-23T07:30:52.265Z
Learning: In the Armbian build system, BOOTPATCHDIR can contain board-specific subdirectories (e.g., board_orangepi5pro) for applying patches to specific boards only. The framework automatically checks if such board-specific subdirectories exist for the board being built and applies those patches accordingly.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-03-31T22:20:48.475Z
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.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-03-31T22:20:41.849Z
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.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-08-02T05:46:10.664Z
Learnt from: EvilOlaf
PR: armbian/build#0
File: :0-0
Timestamp: 2025-08-02T05:46:10.664Z
Learning: In the Armbian build system, the modern recommended approach for kernel configuration is to use the kernel-config command via "./compile.sh BOARD=boardname BRANCH=branchname kernel-config" instead of the deprecated KERNEL_CONFIGURE=yes flag. This provides a two-step workflow: configure using menuconfig, then build, with better transparency and control over configuration changes.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-06-25T03:42:09.086Z
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.
Applied to files:
config/boards/radxa-zero3.csc
🧬 Code Graph Analysis (1)
config/boards/radxa-zero3.csc (3)
extensions/radxa-aic8800.sh (1)
post_install_kernel_debs__install_aic8800_dkms_package(11-63)extensions/bluetooth-hciattach.sh (3)
post_family_tweaks_bsp__bluetooth_hciattach_add_systemd_service(31-60)post_family_config__bluetooth_hciattach_add_bluetooth_packages(25-28)post_family_tweaks__bluetooth_hciattach_enable_bt_service_in_image(63-69)config/sources/vendors/mekotronics/mekotronics-rk3588.hooks.sh (2)
post_family_config__vendor_uboot_mekotronics(8-16)post_family_config__meko_use_mainline_uboot(22-42)
🔇 Additional comments (2)
config/boards/radxa-zero3.csc (2)
16-16: Board-level Bluetooth packages: good additionExplicitly pulling in rfkill/bluez packages at board level makes the BT helper/service predictable across images.
61-63: Manual Verification Required: Validate v2025.10 Patch Directory and Defconfig NamingThe sandbox clone didn’t include
patch/u-boot/v2025.10, so please manually verify:
- The PR adds
patch/u-boot/v2025.10/and that all patches (including any board-specific subdirs for rk3566) apply cleanly.- The defconfig mapping—using
radxa-zero-3-rk3566_defconfigfor mainline and retainingradxa-zero3-rk3566_defconfigfor vendor—is intentional and aligns with upstream naming to prevent mismatches on future rebases.
github-actions
bot
removed
the
Ready to merge
|
@twwn thank you for your work
would those suggestions be applicable to |
|
attaching uboot to master is a great way for disaster (breakage at some point to say). Could you at least use the most recent rc or something? Not sure if there are .10 rcs already. Because I know for a fact that it will be forgotten later on and then when things break the search begins... |
|
@leggewie Yes, the suggestions were mere cleanups (eg., @EvilOlaf Done (to rc2). Currently there's not much to break, |
github-actions
bot
removed
the
Ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
config/boards/radxa-zero3.csc (4)
16-18: Packages and extension enablement look right; consider OBEX if neededThe selected packages are sensible for BT bring-up. If you want OBEX file transfer on desktop images, consider adding bluez-obexd (optional).
-PACKAGE_LIST_BOARD="rfkill bluetooth bluez bluez-tools" +PACKAGE_LIST_BOARD="rfkill bluetooth bluez bluez-tools" +# Optional: add bluez-obexd if OBEX file transfer is desired on desktop images +# PACKAGE_LIST_BOARD+=" bluez-obexd"
25-28: Align firmware path (even in comments) with firmware best practicePer build-system guidance, /lib/firmware/updates takes precedence and avoids conflicts. Update the commented example path to reduce confusion.
-#options aic8800_bsp_sdio aic_fw_path=/lib/firmware/aic8800_fw/SDIO/aic8800 +#options aic8800_bsp_sdio aic_fw_path=/lib/firmware/updates/aic8800_fw/SDIO/aic8800Context: retrieved learning recommends /lib/firmware/updates for user-installed firmware.
37-44: Optionally install unit under /usr/lib/systemd/system to align with usrmergeBoth /lib/systemd/system and /usr/lib/systemd/system work on usr-merged systems; consider using /usr/lib for consistency with other assets (e.g., the extension writes into /usr/lib/systemd/network).
- install -d -m 0755 "${destination}/lib/systemd/system" - install -m 0644 "$SRC/packages/bsp/aic8800/aic-bluetooth.service" "${destination}/lib/systemd/system/aic-bluetooth.service" + install -d -m 0755 "${destination}/usr/lib/systemd/system" + install -m 0644 "$SRC/packages/bsp/aic8800/aic-bluetooth.service" "${destination}/usr/lib/systemd/system/aic-bluetooth.service"If you take this change, also broaden the enable-time existence check (see next comment).
70-75: Tidy comment and keep ATF guidance crispMinor typo and trailing hash; also suggest a slightly clearer note for future maintainers.
- # We cannot set BOOT_SOC=rk3566 as it has side effects in Armbian scripts; ATF_TARGET_MAP is the safer override. - # ATF does not separate rk3566 from rk3568.# + # We cannot set BOOT_SOC=rk3566 due to side effects in Armbian scripts; ATF_TARGET_MAP is the safer override. + # ATF does not separate rk3566 from rk3568.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
patch/u-boot/v2025.10/cmd-fileenv-read-string-from-file-into-env.patchis excluded by!patch/**
📒 Files selected for processing (1)
config/boards/radxa-zero3.csc(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
📚 Learning: 2025-07-17T04:16:29.551Z
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-08-03T15:21:20.148Z
Learnt from: pyavitz
PR: armbian/build#8455
File: config/sources/families/sun50iw1.conf:19-24
Timestamp: 2025-08-03T15:21:20.148Z
Learning: In the Armbian build system, when copying firmware files during family_tweaks_s(), use /lib/firmware/updates/ instead of /lib/firmware/ to avoid conflicts with the Armbian firmware package. The /lib/firmware/updates directory takes precedence in Linux firmware loading hierarchy and is the proper location for user-installed firmware files.
Applied to files:
config/boards/radxa-zero3.csc
🧬 Code Graph Analysis (1)
config/boards/radxa-zero3.csc (2)
extensions/radxa-aic8800.sh (2)
post_install_kernel_debs__install_aic8800_dkms_package(11-63)extension_finish_config__install_kernel_headers_for_aic8800_dkms(1-9)config/sources/vendors/mekotronics/mekotronics-rk3588.hooks.sh (1)
post_family_config__meko_use_mainline_uboot(22-42)
🔇 Additional comments (5)
config/boards/radxa-zero3.csc (5)
30-35: Verify kernel module availability (hidp often disabled; aic8800_btlpm_sdio may not exist)Loading non-existent modules via modules-load.d will spam boot logs. Confirm these are available across vendor/current/edge.
- Ensure CONFIG_BT_HIDP, CONFIG_BT_BNEP, and RFCOMM are enabled as modules for your kernels.
- Confirm the exact name of the AIC8800 BT LPM module (aic8800_btlpm_sdio vs an alternative), or consider loading it within the aic-bluetooth script (modprobe -b) instead of via modules-load.d.
60-60: No action neededEarly return for non-target branches is fine.
96-96: No action neededFile closure/nesting looks correct.
47-55: AIC8800 Bluetooth service unit validated – no changes neededThe
packages/bsp/aic8800/aic-bluetooth.servicefile already declaresType=idleas required, and our enablement logic will pick up the unit under/lib/systemd/system. There are no explicit “sleep” delays, but relying onType=idleis sufficient to defer startup until after the system is ready.• No edits required in
config/boards/radxa-zero3.csc.
• Only add/usr/lib/systemd/systemto the lookup if the install path ever changes.
20-45: No change needed
Armbian’s extension framework defines a hook point namedpost_family_tweaks_bsp, and all functions prefixedpost_family_tweaks_bsp__*(includingpost_family_tweaks_bsp__aic8800_wireless) are correctly discovered and executed.Likely an incorrect or invalid review comment.
noted. would it be OK to ask you to push a separate commit to change those as well at the same time? |
|
@leggewie Of course, I'll do so tomorrow. I don't have these devices, so I wasn't sure whether that's appropriate. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
config/boards/radxa-zero3.csc (1)
68-69: Pin to v2025.10-rc2 and align BOOTPATCHDIR: LGTMThis addresses the earlier concern about tracking master and matches the added patch directory.
🧹 Nitpick comments (3)
config/boards/radxa-zero3.csc (3)
22-47: AIC8800 tweaks and asset install: LGTM; minor nitThe overall approach mirrors other boards and looks correct. Optional nit: for consistency you could use install -d instead of mkdir -p for the /etc/* directories as well.
Here’s the optional consistency tweak:
- mkdir -p "${destination}"/etc/modprobe.d - mkdir -p "${destination}"/etc/modules-load.d + install -d -m 0755 "${destination}/etc/modprobe.d" + install -d -m 0755 "${destination}/etc/modules-load.d"Also, if you later enable the commented aic_fw_path, prefer /lib/firmware/updates/ over /lib/firmware/ to avoid conflicts with the Armbian firmware package.
72-77: Clarify BOOT_SCENARIO comment vs. current behaviorComment says not enabled globally; however, BOOT_SCENARIO remains “spl-blobs” set at the top, which will still be used under current/edge. If that’s intentional, note that explicitly; otherwise consider unsetting or overriding BOOT_SCENARIO here for mainline.
Possible tweak:
# use mainline u-boot for _current_ and _edge_ if [[ "$BRANCH" != "current" && "$BRANCH" != "edge" ]]; then return 0 fi + # Keep spl-blobs for mainline on purpose; binman-atf-mainline is opt-in due to vendor breakage. + # To experiment, set BOOT_SCENARIO="binman-atf-mainline" manually.
39-44: Avoid repetition across boards; consider centralizing AIC8800 install/enableThe install/enable logic is duplicated in three board files. Moving it into the radxa-aic8800 extension (e.g., a post_family_tweaks hook guarded by AIC8800_TYPE) would reduce drift and simplify maintenance.
I can propose a small helper in extensions/radxa-aic8800.sh that installs and enables the service (with proper /usr/lib check), called from boards that set AIC8800_TYPE.
Also applies to: 50-57
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
patch/u-boot/v2025.10/cmd-fileenv-read-string-from-file-into-env.patchis excluded by!patch/**
📒 Files selected for processing (3)
config/boards/kickpik2b.csc(1 hunks)config/boards/radxa-cubie-a5e.csc(1 hunks)config/boards/radxa-zero3.csc(2 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
📚 Learning: 2025-07-17T04:16:29.551Z
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
Applied to files:
config/boards/radxa-cubie-a5e.cscconfig/boards/kickpik2b.cscconfig/boards/radxa-zero3.csc
📚 Learning: 2025-03-31T22:20:41.849Z
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.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-03-31T22:20:48.475Z
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.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-07-25T03:51:50.830Z
Learnt from: EvilOlaf
PR: armbian/build#8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-07-25T03:51:50.830Z
Learnt from: EvilOlaf
PR: armbian/build#8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub API (https://api.github.com/repos/armbian/build/pulls/{pr_number}/files) to get the complete picture of what files are being added or modified, especially for U-Boot patches that will be applied during the build process.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-07-23T07:30:52.265Z
Learnt from: EvilOlaf
PR: armbian/build#8417
File: config/boards/orangepi5pro.csc:57-58
Timestamp: 2025-07-23T07:30:52.265Z
Learning: In the Armbian build system, BOOTPATCHDIR can contain board-specific subdirectories (e.g., board_orangepi5pro) for applying patches to specific boards only. The framework automatically checks if such board-specific subdirectories exist for the board being built and applies those patches accordingly.
Applied to files:
config/boards/radxa-zero3.csc
📚 Learning: 2025-08-03T15:21:20.148Z
Learnt from: pyavitz
PR: armbian/build#8455
File: config/sources/families/sun50iw1.conf:19-24
Timestamp: 2025-08-03T15:21:20.148Z
Learning: In the Armbian build system, when copying firmware files during family_tweaks_s(), use /lib/firmware/updates/ instead of /lib/firmware/ to avoid conflicts with the Armbian firmware package. The /lib/firmware/updates directory takes precedence in Linux firmware loading hierarchy and is the proper location for user-installed firmware files.
Applied to files:
config/boards/radxa-zero3.csc
🧬 Code Graph Analysis (3)
config/boards/radxa-cubie-a5e.csc (2)
extensions/bluetooth-hciattach.sh (2)
post_family_tweaks_bsp__bluetooth_hciattach_add_systemd_service(31-60)post_family_tweaks__bluetooth_hciattach_enable_bt_service_in_image(63-69)extensions/radxa-aic8800.sh (1)
post_install_kernel_debs__install_aic8800_dkms_package(11-63)
config/boards/kickpik2b.csc (2)
extensions/bluetooth-hciattach.sh (2)
post_family_tweaks_bsp__bluetooth_hciattach_add_systemd_service(31-60)post_family_tweaks__bluetooth_hciattach_enable_bt_service_in_image(63-69)extensions/radxa-aic8800.sh (1)
post_install_kernel_debs__install_aic8800_dkms_package(11-63)
config/boards/radxa-zero3.csc (2)
extensions/radxa-aic8800.sh (2)
post_install_kernel_debs__install_aic8800_dkms_package(11-63)extension_finish_config__install_kernel_headers_for_aic8800_dkms(1-9)extensions/bluetooth-hciattach.sh (3)
post_family_tweaks_bsp__bluetooth_hciattach_add_systemd_service(31-60)post_family_config__bluetooth_hciattach_add_bluetooth_packages(25-28)post_family_tweaks__bluetooth_hciattach_enable_bt_service_in_image(63-69)
🔇 Additional comments (5)
config/boards/kickpik2b.csc (1)
39-44: Switch to install(1) with correct perms/locations: LGTMUsing install(1) with 0755/0644 and placing the unit under
/usr/lib/systemd/systemis correct and aligned with usr-merge. ExecStart inpackages/bsp/aic8800/aic-bluetooth.servicecorrectly points to/usr/bin/aic-bluetooth. Nice cleanup.config/boards/radxa-cubie-a5e.csc (1)
42-47: install(1)-based deployment and usr-lib unit location: LGTMClean switch to install(1) with explicit modes; placing the unit in /usr/lib/systemd/system is consistent with usr-merge.
config/boards/radxa-zero3.csc (3)
16-18: Adding core Bluetooth packages: LGTMBaseline rfkill/bluez/bluez-tools are appropriate. Keeping bluez-obexd as opt-in is sensible.
62-62: Early return for non-current/edge: LGTMExplicitly bailing out for branches other than current/edge is clear and avoids unintended overrides.
66-66: No Change Needed: Upstream Uses “radxa-zero-3-rk3566_defconfig”Verified via upstream U-Boot patch and Armbian build scripts that the canonical defconfig name includes the dash (“zero-3”), not the no-dash variant. The existing line in config/boards/radxa-zero3.csc is already correct:
BOOTCONFIG="radxa-zero-3-rk3566_defconfig"No update required.
Likely an incorrect or invalid review comment.
|
You guys should have just ignored AI and left it as is from the get go. I get adding the Plus AI is just searching the web, which means it is always going to argue something it doesn't really know anything about. Making it a 50/50 at best. What I'm saying is; this could have been merged a long time ago :) |
I tend to agree with you somewhat. I wouldn't say "ignore AI" and I don't. I treat it as a good starting point to think again and an opportunity to do even better. No more, no less. |
leggewie
left a comment
leggewie
left a comment
There was a problem hiding this comment.
"impact on 3E is unknown."
3E lacks any kind of wireless
I didn't mean Ignore it, in its entirety. Just that it will lead you down a rabbit hole from time to time. |
leggewie
changed the title
Bluetooth on the 3W was non-functional. Required functions are copied from radxa-cubie-a5e.csc (also used in kickpik2b.csc). These should ideally be extracted into a shared location. The antenna switch (internal vs external) is still missing. Radxa implements this via DT overlays and their rsetup tool: https://docs.radxa.com/en/zero/zero3/os-config/rsetup\#overlays https://github.com/radxa-pkg/radxa-overlays/blob/main/arch/arm64/boot/dts/rockchip/overlays/radxa-zero3-external-antenna.dts Only tested on 3W; impact on 3E is unknown. The functions and packages should ideally be limited to 3W builds, but this seems impossible without splitting the CSC file.
Same as for Radxa Zero 3
Switch to newer U-Boot since it works reliably and provides upstream improvements. Add patch/u-boot/v2025.10 and the generic patch, as BOOTPATCHDIR cannot be unset (fallback to legacy fails). BOOT_SCENARIO="binman-atf-mainline" cannot be set here since it applies globally to the CSC file and would break branch=vendor. Users who want it can enable it manually.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config/boards/radxa-cubie-a5e.csc (1)
54-58: Enable guard omits /usr/lib/systemd/system; unit may not be enabledService is installed under /usr/lib/systemd/system above, but the check only looks in /lib and /etc. This will skip enabling on images where the unit lives in /usr/lib.
- if chroot_sdcard test -f /lib/systemd/system/aic-bluetooth.service || chroot_sdcard test -f /etc/systemd/system/aic-bluetooth.service; then + if chroot_sdcard test -f /lib/systemd/system/aic-bluetooth.service || chroot_sdcard test -f /usr/lib/systemd/system/aic-bluetooth.service || chroot_sdcard test -f /etc/systemd/system/aic-bluetooth.service; then chroot_sdcard systemctl --no-reload enable aic-bluetooth.service else display_alert "$BOARD" "aic-bluetooth.service not found in image; skipping enable" "warn" fi
🧹 Nitpick comments (1)
config/boards/radxa-cubie-a5e.csc (1)
42-47: Use install -D to simplify and make directory creation implicitCurrent approach is fine, but you can reduce two commands and make the block a bit more robust/idempotent by letting install(1) create parent dirs.
- install -d -m 0755 "${destination}/usr/bin" - install -m 0755 "$SRC/packages/bsp/aic8800/aic-bluetooth" "${destination}/usr/bin/aic-bluetooth" - install -d -m 0755 "${destination}/usr/lib/systemd/system" - install -m 0644 "$SRC/packages/bsp/aic8800/aic-bluetooth.service" "${destination}/usr/lib/systemd/system/aic-bluetooth.service" + install -D -m 0755 "$SRC/packages/bsp/aic8800/aic-bluetooth" "${destination}/usr/bin/aic-bluetooth" + install -D -m 0644 "$SRC/packages/bsp/aic8800/aic-bluetooth.service" "${destination}/usr/lib/systemd/system/aic-bluetooth.service"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
patch/u-boot/v2025.10/cmd-fileenv-read-string-from-file-into-env.patchis excluded by!patch/**
📒 Files selected for processing (3)
config/boards/kickpik2b.csc(1 hunks)config/boards/radxa-cubie-a5e.csc(1 hunks)config/boards/radxa-zero3.csc(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- config/boards/radxa-zero3.csc
- config/boards/kickpik2b.csc
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
📚 Learning: 2025-07-17T04:16:29.551Z
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
Applied to files:
config/boards/radxa-cubie-a5e.csc
🧬 Code graph analysis (1)
config/boards/radxa-cubie-a5e.csc (2)
extensions/bluetooth-hciattach.sh (2)
post_family_tweaks_bsp__bluetooth_hciattach_add_systemd_service(31-60)post_family_tweaks__bluetooth_hciattach_enable_bt_service_in_image(63-69)extensions/radxa-aic8800.sh (1)
post_install_kernel_debs__install_aic8800_dkms_package(11-63)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
config/boards/kickpik2b.csc(1 hunks)config/boards/radxa-cubie-a5e.csc(1 hunks)config/boards/radxa-zero3.csc(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- config/boards/radxa-cubie-a5e.csc
- config/boards/radxa-zero3.csc
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: EvilOlaf
PR: armbian/build#8428
File: config/boards/lckfb-taishanpi.csc:5-9
Timestamp: 2025-07-25T03:51:50.830Z
Learning: When reviewing PRs in the Armbian build system, U-Boot defconfig files and patches may be added as part of the PR changes but might not be visible in the current repository clone state during review. It's important to check the actual PR file changes directly via GitHub or the PR API to get the complete picture of what files are being added or modified.
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
📚 Learning: 2025-07-17T04:16:29.551Z
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.551Z
Learning: For AIC8800 Bluetooth devices in Armbian board configurations, systemd service configurations intentionally use Type=idle and boot delays because there is no compatibility to load AIC8800 bluetooth in mainline kernel. These delays and service timing are necessary workarounds for hardware limitations rather than performance issues.
Applied to files:
config/boards/kickpik2b.csc
🧬 Code graph analysis (1)
config/boards/kickpik2b.csc (2)
extensions/bluetooth-hciattach.sh (2)
post_family_tweaks_bsp__bluetooth_hciattach_add_systemd_service(31-60)post_family_tweaks__bluetooth_hciattach_enable_bt_service_in_image(63-69)extensions/radxa-aic8800.sh (1)
post_install_kernel_debs__install_aic8800_dkms_package(11-63)
🔇 Additional comments (1)
config/boards/kickpik2b.csc (1)
39-44: Approved: use install -D and ExecStart already correctThe install lines can be consolidated with
install -Das suggested. The service’sExecStartis already pointing to/usr/bin/aic-bluetooth, so no change is needed there. Optionally, you can add a fail-fast guard fordestination.• In config/boards/kickpik2b.csc, replace lines 39–44 with:
- install -d -m 0755 "${destination}/usr/bin" - install -m 0755 "$SRC/packages/bsp/aic8800/aic-bluetooth" "${destination}/usr/bin/aic-bluetooth" - install -d -m 0755 "${destination}/usr/lib/systemd/system" - install -m 0644 "$SRC/packages/bsp/aic8800/aic-bluetooth.service" "${destination}/usr/lib/systemd/system/aic-bluetooth.service" + install -Dm0755 "$SRC/packages/bsp/aic8800/aic-bluetooth" \ + "${destination}/usr/bin/aic-bluetooth" + install -Dm0644 "$SRC/packages/bsp/aic8800/aic-bluetooth.service" \ + "${destination}/usr/lib/systemd/system/aic-bluetooth.service"• (Optional) After line 22, add a guard to ensure
destinationis set:: "${destination:?destination is not set}"
4 participants
Description
The required functions are copied from
radxa-cubie-a5e.csc(also used inkickpik2b.csc), but altered to match coderabbitai's suggestions.The software switch for internal vs. external antenna remains unimplemented. Radxa provides this via a DT overlay and their
rsetuptool:This patch was tested only on the 3W. Functionality on the 3E is unverified.
Ideally, the additional functions and packages should be limited to 3W builds, but this does not appear possible without splitting the CSC file?
Adds
patch/u-boot/v2025.10(the next release) as folder and to it the generic patch (fromv2025.04), then setsBOOTPATCHDIRaccordingly — as it cannot be unset (falling back to legacy causes failures).BOOT_SCENARIO="binman-atf-mainline"is possible for thecurrentandedgebranches. But it breaks the build unless it is set for the entire CSC file, and that in turn would breakvendor. Hence it is only prepared and users who want it must edit the file manually.How Has This Been Tested?
Compiled & running on my own 3W as edge.
Checklist:
Please delete options that are not relevant.