Add board KickPi K2B and AIC8800 SDIO Wireless #8388
Closed
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
Firmware is hardcoded to: /lib/firmware/updates/aic8800 aic_powerlimit_8800d80.txt fmacfw_patch_tbl_8800dc_ipc_u02.bin aic_powerlimit_8800d80x2.txt fmacfw_patch_tbl_8800dc_u02.bin aic_userconfig_8800d80.txt fmacfw_rf.bin aic_userconfig_8800d80x2.txt fw_adid_8800d80_u02.bin aic_userconfig_8800dc.txt fw_adid_8800d80x2_u05.bin aic_userconfig_8800dw.txt fw_adid_8800dc_u02.bin aic_userconfig.txt fw_adid_8800dc_u02h.bin fmacfw_8800d80_h_u02.bin fw_adid.bin fmacfw_8800d80_h_u02_ipc.bin fw_adid_u03.bin fmacfw_8800d80_u02.bin fw_patch_8800d80_u02.bin fmacfw_8800d80_u02_ipc.bin fw_patch_8800d80_u02_ext0.bin fmacfw_8800d80x2.bin fw_patch_8800d80x2_u05.bin fmacfw_8800m_custmsg.bin fw_patch_8800dc_u02.bin fmacfw.bin fw_patch_8800dc_u02_ext0.bin fmacfwbt_8800d80_h_u02.bin fw_patch_8800dc_u02h.bin fmacfwbt_8800d80_u02.bin fw_patch.bin fmacfwbt.bin fw_patch_table_8800d80_u02.bin fmacfw_calib_8800dc_hbt_u02.bin fw_patch_table_8800d80x2_u05.bin fmacfw_calib_8800dc_h_u02.bin fw_patch_table_8800dc_u02.bin fmacfw_calib_8800dc_u02.bin fw_patch_table_8800dc_u02h.bin fmacfw_patch_8800dc_hbt_u02.bin fw_patch_table.bin fmacfw_patch_8800dc_h_u02.bin fw_patch_table_u03.bin fmacfw_patch_8800dc_ipc_u02.bin fw_patch_test.bin fmacfw_patch_8800dc_u02.bin fw_patch_u03.bin fmacfw_patch.bin lmacfw_rf_8800d80_u02.bin fmacfw_patch_tbl_8800dc_hbt_u02.bin lmacfw_rf_8800d80x2.bin fmacfw_patch_tbl_8800dc_h_u02.bin lmacfw_rf_8800dc.bin Current state of armbian/firmware: /lib/firmware/aic8800/SDIO aic8800 aic8800D80 aic8800DC <-- these are dir. Having each variant segregated in a dir is counter productive. There is no overlap in the firmware. The KickPi K2B can load the firmware from the same place as the Radxa Zero 3W. Diff FW; Same location. We should consider updating the firmware location. /lib/firmware/aic8800_sdio ? Signed-off-by: Patrick Yavitz <[email protected]>
Signed-off-by: Patrick Yavitz <[email protected]>
Signed-off-by: Patrick Yavitz <[email protected]>
Signed-off-by: Patrick Yavitz <[email protected]>
pyavitz
requested review from
ColorfulRhino,
efectn,
joekhoobyar,
paolosabatino,
rpardini,
AGM1968,
JohnTheCoolingFan,
PanderMusubi,
TheSnowfield,
Tonymac32,
chraac,
igorpecovnik,
janprunk,
krachlatte,
leggewie,
schwar3kat,
sgjava,
teknoid and
a team
as code owners
WalkthroughA new board configuration file for the KickPi K2B (Allwinner H618 SoC) has been introduced, detailing board-specific metadata and boot parameters. Two new functions are added: one to automate the installation and configuration of AIC8800 Bluetooth support, and another to enable a systemd service for the Bluetooth helper script. The changes also update the driver preparation harness to include the 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
08
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: 2
🧹 Nitpick comments (1)
config/boards/kickpik2b.conf (1)
37-53: Review Bluetooth helper script implementation.The script implementation has some concerns:
- No error handling for critical operations
- Hard-coded device path
/dev/ttyS1may not be universal- Fixed timing with sleep commands could be fragile
Consider improving robustness:
#!/bin/bash +set -e +# Check if UART device exists +if [[ ! -e /dev/ttyS1 ]]; then + echo "Error: UART device /dev/ttyS1 not found" + exit 1 +fi + # HW Reset hciattach -r -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
patch/kernel/archive/sunxi-6.12/0000.patching_config.yamlis excluded by!patch/**patch/kernel/archive/sunxi-6.12/dt/sun50i-h618-kickpi-k2b.dtsis excluded by!patch/**patch/kernel/archive/sunxi-6.15/0000.patching_config.yamlis excluded by!patch/**patch/kernel/archive/sunxi-6.15/dt/sun50i-h618-kickpi-k2b.dtsis excluded by!patch/**patch/misc/aic8800_sdio/001-Aicsemi-aic8800-fixups.patchis excluded by!patch/**patch/u-boot/v2025.01/board_kickpik2b/001-Add-board-KickPi-K2B.patchis excluded by!patch/**patch/u-boot/v2025.01/board_kickpik2b/002-sunxi-mmc-Improve-reset-procedure.patchis excluded by!patch/**patch/u-boot/v2025.01/board_kickpik2b/003-sunxi-h616-fix-DRAM-size-detection.patchis excluded by!patch/**patch/u-boot/v2025.01/board_kickpik2b/004-HACK-sunxi-h616-gpu-enable.patchis excluded by!patch/**patch/u-boot/v2025.01/board_kickpik2b/005-sunxi-h616-ths-workaround.patchis excluded by!patch/**patch/u-boot/v2025.01/board_kickpik2b/006-mach-sunxi-dram_helpers-add-delay-to-steady-dram-detection.patchis excluded by!patch/**patch/u-boot/v2025.01/board_kickpik2b/007-sunxi-pmic_bus-Move-SPL-I2C-addresses-into-Kconfig.patchis excluded by!patch/**
📒 Files selected for processing (3)
config/boards/kickpik2b.conf(1 hunks)lib/functions/compilation/patch/drivers-harness.sh(1 hunks)lib/functions/compilation/patch/drivers_network.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: EvilOlaf
PR: armbian/build#8328
File: lib/functions/compilation/patch/drivers_network.sh:542-545
Timestamp: 2025-06-24T10:08:40.313Z
Learning: In the Armbian build system, when a PR removes build support for a specific kernel version, version check issues for that removed version become practically irrelevant even if they appear incorrect in isolation. Context about which kernel versions are being deprecated/removed is important for understanding the impact of version-related code changes.
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: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
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: 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#8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
lib/functions/compilation/patch/drivers-harness.sh (2)
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: 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.
lib/functions/compilation/patch/drivers_network.sh (5)
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: 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: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
Learnt from: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
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.
config/boards/kickpik2b.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: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
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: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
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#8287
File: config/bootscripts/boot-sunxi.cmd:13-23
Timestamp: 2025-06-12T21:08:11.318Z
Learning: In config/bootscripts/boot-sunxi.cmd the variable name `align_overlap_oboe_avoidance` intentionally uses the acronym “OBOE” (Off-By-One Error); it is not a typo and should not be renamed.
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: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:148-161
Timestamp: 2025-06-12T21:03:39.686Z
Learning: In Armbian U-Boot bootscripts, the `${cpu}` environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.
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.
🧬 Code Graph Analysis (1)
lib/functions/compilation/patch/drivers-harness.sh (1)
lib/functions/compilation/patch/drivers_network.sh (1)
driver_aic8800_sdio(37-89)
🪛 GitHub Actions: Shellcheck - PR #8388 ("Add board KickPi K2B and AIC8800 SDIO Wireless")
config/boards/kickpik2b.conf
[error] 1-1: ShellCheck SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
🔇 Additional comments (9)
lib/functions/compilation/patch/drivers_network.sh (4)
37-44: LGTM: Function structure follows established patterns.The function properly uses version checking and family filtering, with an immutable commit reference as required by the codebase guidelines.
48-53: LGTM: Proper integration into kernel build system.The driver is correctly integrated by:
- Adding to the wireless drivers Makefile
- Disabling parallel build flags that could cause issues
- Enabling GPIO wakeup support
- Disabling SDIO power control
- Adding Kconfig source to the wireless drivers configuration
54-70: LGTM: Well-structured Kconfig definitions.The Kconfig files follow proper syntax and structure:
- Main AIC wireless support configuration
- Firmware path configuration with sensible default
- Proper dependency declarations
- Appropriate help text
71-89: LGTM: Complete Kconfig structure with optional patch support.The remaining Kconfig files for WLAN and Bluetooth support are properly structured, and the conditional patch application follows the established pattern.
lib/functions/compilation/patch/drivers-harness.sh (1)
106-106: LGTM: Proper driver integration into harness system.The new AIC8800 SDIO driver is correctly added to the drivers array, maintaining the established pattern for driver registration.
config/boards/kickpik2b.conf (4)
1-14: LGTM: Board metadata follows established conventions.The board configuration follows Armbian patterns with:
- Single-line hardware description comment (as per learning)
- Proper H618 SoC family assignment
- Correct overlay prefix and device tree file
- Appropriate kernel targets and testing configuration
55-72: LGTM: Well-structured systemd service configuration.The systemd service file follows proper conventions:
- Appropriate dependencies and ordering
- Correct service type for the use case
- Proper rfkill unblocking
- Suitable for multi-user target
74-77: LGTM: Proper systemd service enablement.The function correctly enables the Bluetooth helper service using the appropriate chroot_sdcard function provided by the Armbian framework.
1-1: Fix missing shebang to resolve pipeline failure.The ShellCheck error SC2148 indicates a missing shebang. Board configuration files in Armbian require the standard bash shebang.
+#!/usr/bin/env bash # Allwinner H618 quad core 1/2/4GB RAM SoC WiFi SPI USB-C⛔ Skipped due to learnings
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: leggewie PR: armbian/build#8133 File: extensions/apa.sh:1-2 Timestamp: 2025-04-28T08:27:26.890Z Learning: In the Armbian build system, extension scripts in the `extensions/` directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or `set -euo pipefail`.Learnt from: djurny PR: armbian/build#8287 File: config/bootscripts/boot-sunxi.cmd:38-44 Timestamp: 2025-06-12T21:14:36.024Z Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.Learnt from: djurny PR: armbian/build#8287 File: config/bootscripts/boot-sunxi.cmd:148-161 Timestamp: 2025-06-12T21:03:39.686Z Learning: In Armbian U-Boot bootscripts, the `${cpu}` environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.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: 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: 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.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: 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: djurny PR: armbian/build#8287 File: config/bootscripts/boot-sunxi.cmd:13-23 Timestamp: 2025-06-12T21:08:11.318Z Learning: In config/bootscripts/boot-sunxi.cmd the variable name `align_overlap_oboe_avoidance` intentionally uses the acronym “OBOE” (Off-By-One Error); it is not a typo and should not be renamed.Learnt from: pyavitz PR: armbian/build#8361 File: config/kernel/linux-sunxi64-edge.config:594-597 Timestamp: 2025-07-06T16:27:45.349Z Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
Signed-off-by: Patrick Yavitz <[email protected]>
github-actions
bot
removed
size/medium
coderabbitai
bot
added
Ready to merge
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
config/boards/kickpik2b.conf (1)
24-25: Security concern: Unvalidated git clone operation.This is the same security issue flagged in previous reviews. The function clones from a GitHub repository without verification of the repository integrity or commit hash.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/boards/kickpik2b.conf(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Shellcheck - PR #8388 ("Add board KickPi K2B and AIC8800 SDIO Wireless")
config/boards/kickpik2b.conf
[error] 1-1: ShellCheck SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
🔇 Additional comments (1)
config/boards/kickpik2b.conf (1)
75-78: LGTM!The function correctly enables the Bluetooth helper service using the appropriate chroot_sdcard utility during the build process.
Comment on lines
+56
to
+72
| cat <<EOF > "${destination}"/etc/systemd/system/bthelper.service | ||
| [Unit] | ||
| Description=Bluetooth Helper | ||
| After=bluetooth.service bluetooth.target getty.target | ||
| Requires=bluetooth.service bluetooth.target getty.target | ||
|
|
||
| [Service] | ||
| Type=idle | ||
| ExecStartPre=/usr/sbin/rfkill unblock all | ||
| ExecStart=/usr/bin/aic-bthelper | ||
| TimeoutStartSec=0 | ||
| RemainAfterExit=yes | ||
| SysVStartPriority=99 | ||
|
|
||
| [Install] | ||
| WantedBy=multi-user.target | ||
| EOF |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve systemd service configuration.
The service configuration has several issues that should be addressed:
- Deprecated directive:
SysVStartPriorityis deprecated in systemd - Inappropriate service type:
Type=idlemay cause unnecessary boot delays - Disabled timeout:
TimeoutStartSec=0could cause boot hangs if the service fails - Redundant dependencies:
getty.targetis not needed for Bluetooth functionality
[Unit]
Description=Bluetooth Helper
-After=bluetooth.service bluetooth.target getty.target
-Requires=bluetooth.service bluetooth.target getty.target
+After=bluetooth.service
+Wants=bluetooth.service
[Service]
-Type=idle
+Type=oneshot
ExecStartPre=/usr/sbin/rfkill unblock all
ExecStart=/usr/bin/aic-bthelper
-TimeoutStartSec=0
+TimeoutStartSec=30
RemainAfterExit=yes
-SysVStartPriority=99
[Install]
WantedBy=multi-user.target📝 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
| cat <<EOF > "${destination}"/etc/systemd/system/bthelper.service | |
| [Unit] | |
| Description=Bluetooth Helper | |
| After=bluetooth.service bluetooth.target getty.target | |
| Requires=bluetooth.service bluetooth.target getty.target | |
| [Service] | |
| Type=idle | |
| ExecStartPre=/usr/sbin/rfkill unblock all | |
| ExecStart=/usr/bin/aic-bthelper | |
| TimeoutStartSec=0 | |
| RemainAfterExit=yes | |
| SysVStartPriority=99 | |
| [Install] | |
| WantedBy=multi-user.target | |
| EOF | |
| cat <<EOF > "${destination}"/etc/systemd/system/bthelper.service | |
| [Unit] | |
| Description=Bluetooth Helper | |
| After=bluetooth.service | |
| Wants=bluetooth.service | |
| [Service] | |
| Type=oneshot | |
| ExecStartPre=/usr/sbin/rfkill unblock all | |
| ExecStart=/usr/bin/aic-bthelper | |
| TimeoutStartSec=30 | |
| RemainAfterExit=yes | |
| [Install] | |
| WantedBy=multi-user.target | |
| EOF |
🤖 Prompt for AI Agents
In config/boards/kickpik2b.conf around lines 56 to 72, the systemd service file
for bthelper uses deprecated and suboptimal directives. Remove the deprecated
SysVStartPriority directive, change the service Type from idle to simple to
avoid unnecessary boot delays, set a reasonable TimeoutStartSec value instead of
0 to prevent potential boot hangs, and remove getty.target from the After and
Requires dependencies since it is not relevant to Bluetooth functionality.
There was a problem hiding this comment.
The boot delays are intentional. There is no compatibility to load AIC8800 bluetooth in mainline.
There was a problem hiding this comment.
@pyavitz, understood! Thank you for clarifying the technical context. I acknowledge that the Type=idle and boot delays are intentionally designed to work around the AIC8800 Bluetooth compatibility issues in mainline. The current systemd service configuration is appropriate for this specific hardware requirement.
✏️ Learnings added
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:56-72
Timestamp: 2025-07-17T04:16:29.520Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:1-14
Timestamp: 2025-07-17T04:12:33.085Z
Learning: In the Armbian build system, board configuration files (*.conf) follow a project standard of not including shebang lines, even though they contain bash code and are sourced by the build system. This is an established Armbian convention that individual contributors are expected to follow, and changes to this standard would require broader project maintainer approval.
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: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
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: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
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: 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: 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: 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#8287
File: config/bootscripts/boot-sunxi.cmd:13-23
Timestamp: 2025-06-12T21:08:11.318Z
Learning: In config/bootscripts/boot-sunxi.cmd the variable name `align_overlap_oboe_avoidance` intentionally uses the acronym “OBOE” (Off-By-One Error); it is not a typo and should not be renamed.
Learnt from: EvilOlaf
PR: armbian/build#8328
File: lib/functions/compilation/patch/drivers_network.sh:542-545
Timestamp: 2025-06-24T10:08:40.313Z
Learning: In the Armbian build system, when a PR removes build support for a specific kernel version, version check issues for that removed version become practically irrelevant even if they appear incorrect in isolation. Context about which kernel versions are being deprecated/removed is important for understanding the impact of version-related code changes.
Comment on lines
+1
to
+14
| # Allwinner H618 quad core 1/2/4GB RAM SoC WiFi SPI USB-C | ||
| BOARD_NAME="KickPi K2B" | ||
| BOARDFAMILY="sun50iw9-bpi" | ||
| BOARD_MAINTAINER="pyavitz" | ||
| BOOTCONFIG="kickpi_k2b_defconfig" | ||
| OVERLAY_PREFIX="sun50i-h616" | ||
| BOOT_FDT_FILE="sun50i-h618-kickpi-k2b.dtb" | ||
| BOOT_LOGO="desktop" | ||
| KERNEL_TARGET="current,edge" | ||
| KERNEL_TEST_TARGET="current" | ||
| FORCE_BOOTSCRIPT_UPDATE="yes" | ||
| BOOTBRANCH_BOARD="tag:v2025.01" | ||
| BOOTPATCHDIR="v2025.01" | ||
| PACKAGE_LIST_BOARD="rfkill bluetooth bluez bluez-tools" |
There was a problem hiding this comment.
Add shebang to fix pipeline failure.
The pipeline failure indicates that ShellCheck requires a shebang directive. Since this is a configuration file that will be sourced by the build system, add a bash shebang at the top.
+#!/bin/bash
+
# Allwinner H618 quad core 1/2/4GB RAM SoC WiFi SPI USB-C
BOARD_NAME="KickPi K2B"📝 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
| # Allwinner H618 quad core 1/2/4GB RAM SoC WiFi SPI USB-C | |
| BOARD_NAME="KickPi K2B" | |
| BOARDFAMILY="sun50iw9-bpi" | |
| BOARD_MAINTAINER="pyavitz" | |
| BOOTCONFIG="kickpi_k2b_defconfig" | |
| OVERLAY_PREFIX="sun50i-h616" | |
| BOOT_FDT_FILE="sun50i-h618-kickpi-k2b.dtb" | |
| BOOT_LOGO="desktop" | |
| KERNEL_TARGET="current,edge" | |
| KERNEL_TEST_TARGET="current" | |
| FORCE_BOOTSCRIPT_UPDATE="yes" | |
| BOOTBRANCH_BOARD="tag:v2025.01" | |
| BOOTPATCHDIR="v2025.01" | |
| PACKAGE_LIST_BOARD="rfkill bluetooth bluez bluez-tools" | |
| #!/bin/bash | |
| # Allwinner H618 quad core 1/2/4GB RAM SoC WiFi SPI USB-C | |
| BOARD_NAME="KickPi K2B" | |
| BOARDFAMILY="sun50iw9-bpi" | |
| BOARD_MAINTAINER="pyavitz" | |
| BOOTCONFIG="kickpi_k2b_defconfig" | |
| OVERLAY_PREFIX="sun50i-h616" | |
| BOOT_FDT_FILE="sun50i-h618-kickpi-k2b.dtb" | |
| BOOT_LOGO="desktop" | |
| KERNEL_TARGET="current,edge" | |
| KERNEL_TEST_TARGET="current" | |
| FORCE_BOOTSCRIPT_UPDATE="yes" | |
| BOOTBRANCH_BOARD="tag:v2025.01" | |
| BOOTPATCHDIR="v2025.01" | |
| PACKAGE_LIST_BOARD="rfkill bluetooth bluez bluez-tools" |
🧰 Tools
🪛 GitHub Actions: Shellcheck - PR #8388 ("Add board KickPi K2B and AIC8800 SDIO Wireless")
[error] 1-1: ShellCheck SC2148: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
🤖 Prompt for AI Agents
In config/boards/kickpik2b.conf at lines 1 to 14, the file lacks a shebang line
which causes the pipeline to fail ShellCheck validation. Add the line
"#!/bin/bash" at the very top of the file to specify the script interpreter and
fix the pipeline failure.
There was a problem hiding this comment.
I agree. All these *.conf files should have a shebang. This isn't up to me to change and it is currently an Armbian standard not to have them.
There was a problem hiding this comment.
🧩 Analysis chain
@pyavitz Thank you for clarifying the Armbian project standards. Let me investigate the current state of .conf files in the Armbian codebase to better understand the established patterns.
🏁 Script executed:
#!/bin/bash
# Check if other board configuration files have shebangs
echo "=== Checking existing board .conf files for shebangs ==="
fd -e conf . config/boards/ | head -10 | xargs -I {} sh -c 'echo "File: {}"; head -1 "{}" | grep -E "^#!" || echo "No shebang found"; echo ""'
echo "=== Checking family configuration files for shebangs ==="
fd -e conf . config/sources/families/ | head -5 | xargs -I {} sh -c 'echo "File: {}"; head -1 "{}" | grep -E "^#!" || echo "No shebang found"; echo ""'
echo "=== Checking ShellCheck configuration or exceptions ==="
find . -name ".shellcheckrc" -o -name "shellcheck.yml" -o -name "*shellcheck*" | head -5 | xargs -I {} sh -c 'echo "File: {}"; cat "{}" 2>/dev/null || echo "File not readable"'
Length of output: 12280
@pyavitz You're absolutely correct, and the evidence confirms your statement. I checked the existing codebase and found that all board configuration files (.conf) and family configuration files consistently lack shebang lines - this is indeed an established Armbian project standard.
The issue here is that the CI pipeline's ShellCheck configuration is enforcing rule SC2148 (missing shebang), which conflicts with the project's established convention. This creates a systematic problem that affects not just your PR but potentially any new .conf files added to the project.
Potential solutions to resolve this CI conflict:
-
Add SC2148 to ShellCheck exclusions - The most appropriate solution would be to add
SC2148to the excluded rules in the ShellCheck configuration (inlib/tools/shellcheck.sh) for config files, since they are sourced rather than executed directly. -
Configure CI to handle this specific case - The CI pipeline could be configured to treat
.conffiles differently or skip the shebang check for configuration files. -
Project-wide decision - The project maintainers could decide whether to adopt shebangs consistently across all
.conffiles, but this would be a breaking change from established patterns.
Since this appears to be a project-wide infrastructure issue rather than a problem with your specific contribution, I'd recommend escalating this to the project maintainers or opening a separate issue about the CI configuration conflict with established coding standards.
Your PR correctly follows the existing Armbian conventions, and the CI failure is due to a configuration mismatch rather than a code issue.
✏️ Learnings added
Learnt from: pyavitz
PR: armbian/build#8388
File: config/boards/kickpik2b.conf:1-14
Timestamp: 2025-07-17T04:12:33.085Z
Learning: In the Armbian build system, board configuration files (*.conf) follow a project standard of not including shebang lines, even though they contain bash code and are sourced by the build system. This is an established Armbian convention that individual contributors are expected to follow, and changes to this standard would require broader project maintainer approval.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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: pyavitz
PR: armbian/build#8361
File: config/kernel/linux-sunxi64-edge.config:594-597
Timestamp: 2025-07-06T16:27:45.349Z
Learning: In Armbian sunxi64-edge kernel configurations, PCI/PCIe support may be enabled for future-proofing purposes to support upcoming Allwinner SoCs like the T536 that include PCIe Gen2 interfaces, even if current primary targets like H616 lack PCIe controllers. This forward-looking approach prepares the configuration for next-generation hardware in the same SoC family.
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: 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: 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: 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: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:13-23
Timestamp: 2025-06-12T21:08:11.318Z
Learning: In config/bootscripts/boot-sunxi.cmd the variable name `align_overlap_oboe_avoidance` intentionally uses the acronym “OBOE” (Off-By-One Error); it is not a typo and should not be renamed.
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: 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#8287
File: config/bootscripts/boot-sunxi.cmd:38-44
Timestamp: 2025-06-12T21:14:36.024Z
Learning: In config/bootscripts/boot-sunxi.cmd the unconditional "+1" increment in func_align_addr_next (when align_overlap_oboe_avoidance="on") is intentional. It compensates for a known off-by-one error in U-Boot where the end address is calculated as start+size instead of start+size-1, so the extra page prevents overlap. This behavior should not be “optimized away”.
Learnt from: leggewie
PR: armbian/build#8133
File: extensions/apa.sh:1-2
Timestamp: 2025-04-28T08:27:26.890Z
Learning: In the Armbian build system, extension scripts in the `extensions/` directory contain hook functions and are meant to be sourced, not executed directly. These scripts don't require a shebang or `set -euo pipefail`.
Learnt from: djurny
PR: armbian/build#8287
File: config/bootscripts/boot-sunxi.cmd:148-161
Timestamp: 2025-06-12T21:03:39.686Z
Learning: In Armbian U-Boot bootscripts, the `${cpu}` environment variable is always pre-set by U-Boot (or defaults to a valid value), so additional guards against it being unset are unnecessary.
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: djurny
PR: armbian/build#8315
File: lib/functions/rootfs/distro-agnostic.sh:54-68
Timestamp: 2025-06-22T00:45:26.182Z
Learning: In Armbian build system, external command dependencies like envsubst are handled by adding the required packages (e.g., gettext for envsubst) to host_dependencies in adaptative_prepare_host_dependencies function in lib/functions/host/prepare-host.sh, rather than checking command availability at runtime.
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.
Learnt from: EvilOlaf
PR: armbian/build#8328
File: lib/functions/compilation/patch/drivers_network.sh:542-545
Timestamp: 2025-06-24T10:08:40.313Z
Learning: In the Armbian build system, when a PR removes build support for a specific kernel version, version check issues for that removed version become practically irrelevant even if they appear incorrect in isolation. Context about which kernel versions are being deprecated/removed is important for understanding the impact of version-related code changes.
Comment on lines
+38
to
+54
| cat <<EOF > "${destination}"/usr/bin/aic-bthelper | ||
| #!/bin/bash | ||
|
|
||
| # HW Reset | ||
| hciattach -r -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1 | ||
|
|
||
| # Kill hciattach | ||
| sleep .50 | ||
| killall hciattach | ||
|
|
||
| # Attach Bluetooth HCI UART | ||
| sleep .50 | ||
| hciattach -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1 | ||
| rfkill unblock all | ||
|
|
||
| exit 0 | ||
| EOF |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and reduce timing dependencies in Bluetooth helper script.
The script has several issues that could affect reliability:
- Hardcoded device path:
/dev/ttyS1may not exist on all systems - Timing dependencies: Multiple
sleepcommands create race conditions - No error handling: Commands may fail silently
- Potential process conflicts:
killall hciattachaffects all hciattach processes
#!/bin/bash
+
+# Check if device exists
+if [ ! -e /dev/ttyS1 ]; then
+ echo "Error: /dev/ttyS1 not found" >&2
+ exit 1
+fi
# HW Reset
-hciattach -r -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1
+if ! hciattach -r -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1; then
+ echo "Error: Failed to reset Bluetooth device" >&2
+ exit 1
+fi
# Kill hciattach
-sleep .50
-killall hciattach
+sleep 0.5
+# Kill only the specific hciattach process for ttyS1
+pkill -f "hciattach.*ttyS1" || true
# Attach Bluetooth HCI UART
-sleep .50
-hciattach -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1
-rfkill unblock all
+sleep 0.5
+if ! hciattach -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1; then
+ echo "Error: Failed to attach Bluetooth HCI UART" >&2
+ exit 1
+fi
+
+if ! rfkill unblock all; then
+ echo "Warning: Failed to unblock rfkill" >&2
+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
| cat <<EOF > "${destination}"/usr/bin/aic-bthelper | |
| #!/bin/bash | |
| # HW Reset | |
| hciattach -r -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1 | |
| # Kill hciattach | |
| sleep .50 | |
| killall hciattach | |
| # Attach Bluetooth HCI UART | |
| sleep .50 | |
| hciattach -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1 | |
| rfkill unblock all | |
| exit 0 | |
| EOF | |
| cat <<EOF > "${destination}"/usr/bin/aic-bthelper | |
| #!/bin/bash | |
| # Check if device exists | |
| if [ ! -e /dev/ttyS1 ]; then | |
| echo "Error: /dev/ttyS1 not found" >&2 | |
| exit 1 | |
| fi | |
| # HW Reset | |
| if ! hciattach -r -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1; then | |
| echo "Error: Failed to reset Bluetooth device" >&2 | |
| exit 1 | |
| fi | |
| # Kill hciattach | |
| sleep 0.5 | |
| # Kill only the specific hciattach process for ttyS1 | |
| pkill -f "hciattach.*ttyS1" || true | |
| # Attach Bluetooth HCI UART | |
| sleep 0.5 | |
| if ! hciattach -s 1500000 /dev/ttyS1 any 1500000 flow nosleep > /dev/null 2>&1; then | |
| echo "Error: Failed to attach Bluetooth HCI UART" >&2 | |
| exit 1 | |
| fi | |
| if ! rfkill unblock all; then | |
| echo "Warning: Failed to unblock rfkill" >&2 | |
| fi | |
| exit 0 | |
| EOF |
🤖 Prompt for AI Agents
In config/boards/kickpik2b.conf around lines 38 to 54, the Bluetooth helper
script uses a hardcoded device path, relies on fixed sleep durations causing
race conditions, lacks error handling, and uses killall which may terminate
unrelated processes. To fix this, replace the hardcoded device path with a
configurable variable or detect the correct device dynamically, remove or
minimize sleep commands by using process status checks or wait loops, add error
checking after each command to handle failures explicitly, and replace killall
with a more targeted approach to terminate only the specific hciattach process
started by the script.
This was referenced Jul 21, 2025
This was referenced Jul 29, 2025
Merged
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.
The KickPi K2B is another cheap H618 unit.
https://www.kickpi.com/product/k2b/
https://a.co/d/aqIJ3gA
AIC8800 SDIO
https://paste.armbian.com/iqodabitet.yaml