manage dtoverlays: Fixed the bug of adding overlay names #609
Conversation
|
""" WalkthroughThe device tree overlay management script was updated to set the Raspberry Pi overlay configuration file path to Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
size/small
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/modules/system/manage_dtoverlays.sh (2)
23-23: Add fallback for Raspberry Pi config path
Right now we unconditionally setoverlayconf="/boot/firmware/config.txt". On some Pi variants or boot setups (e.g. older firmware), the config file may instead reside at/boot/config.txt. Consider testing for both locations and failing gracefully if neither exists:- overlayconf="/boot/firmware/config.txt" + if [[ -f /boot/firmware/config.txt ]]; then + overlayconf="/boot/firmware/config.txt" + elif [[ -f /boot/config.txt ]]; then + overlayconf="/boot/config.txt" + else + echo "Error: Raspberry Pi config file not found" >&2 + return 1 + fi
70-70: Ensure dialog prompt newlines render correctly
In a double-quoted shell string,\nis literal, sodialogmay display\ninstead of a line break. Switch to$'…'quoting or embed real newlines for proper formatting:- --checklist "\nUse <space> to toggle functions and save them.\nExit when you are done.\n\n overlay_prefix=$overlay_prefix\n " \ + --checklist $'\nUse <space> to toggle functions and save them.\nExit when you are done.\n\n overlay_prefix='"$overlay_prefix"$'\n' \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/modules/system/manage_dtoverlays.sh(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Duplicati install (noble)
- GitHub Check: OpenMediaVault install (bookworm)
github-actions
bot
removed
the
Ready to merge
|
I'm sorry. I must have lost my mind. |
|
"If the stars are lighting up in the sky, then someone needs it." What was the reason to display all the overlays and give the user the opportunity to select even those that do not match the overlay prefix? |
Naming inconsistency - it was not displaying those that should. |
Naming overlays: |
Yes, this is not consistent across families. We also have mainline naming and vendor (board vendor) specifics. It's a mess.
This can only be done by different maintainers. That's the main problem. |
We have only two options.
Is that true? Let's resolve this issue in this pull request. |
|
I will be able to check the code for sunxi, sunxi64, rockcip64. |
|
Without those changes, things works ... Example of problem: This board comes with LCD overlay and when we want to add overclocking overlay, nothing is changed ... |
I'm still working on it. |
|
Analyze which scripts can load the armbian/build> awk -F"/" '/if load.*overlay\/\${overlay_prefix}-\${overlay_file}.dtbo/ {print FILENAME":", FNR}' config/bootscripts/*
config/bootscripts/boot-jethub.cmd: 75
config/bootscripts/boot-meson64.cmd: 152
config/bootscripts/boot-meson-gx.cmd: 42
config/bootscripts/boot-meson-s4t7.cmd: 62
config/bootscripts/boot-mvebu.cmd: 63
config/bootscripts/boot-rk322x.cmd: 54
config/bootscripts/boot-rk3576.cmd: 51
config/bootscripts/boot-rk35xx.cmd: 51
config/bootscripts/boot-rockchip64.cmd: 51
config/bootscripts/boot-rockchip64-ttyS0.cmd: 51
config/bootscripts/boot-rockchip.cmd: 54
config/bootscripts/boot-sun50i-next.cmd: 90
config/bootscripts/boot-sunxi.cmd: 10613 files. Analyze which scripts can load the armbian/build> awk -F"/" '/if load.*overlay\/\${overlay_file}.dtbo/ {print FILENAME":", FNR}' config/bootscripts/*
config/bootscripts/boot-rk3576.cmd: 54
config/bootscripts/boot-rk35xx.cmd: 54Just two files. Analyze which scripts can load a custom overlay file: armbian/build> awk -F"/" '/if load.*overlay-user\/\${overlay_file}.dtbo/ {print FILENAME":", FNR}' config/bootscripts/*
config/bootscripts/boot-jethub.cmd: 82
config/bootscripts/boot-meson64.cmd: 159
config/bootscripts/boot-meson-gx.cmd: 48
config/bootscripts/boot-meson-s4t7.cmd: 69
config/bootscripts/boot-mvebu.cmd: 73
config/bootscripts/boot-rk322x.cmd: 60
config/bootscripts/boot-rk3576.cmd: 60
config/bootscripts/boot-rk35xx.cmd: 60
config/bootscripts/boot-rockchip64.cmd: 57
config/bootscripts/boot-rockchip64-ttyS0.cmd: 57
config/bootscripts/boot-rockchip.cmd: 60
config/bootscripts/boot-sun50i-next.cmd: 96
config/bootscripts/boot-sun50iw9.cmd: 58
config/bootscripts/boot-sunxi.cmd: 11214 files. Only two scripts will be able to load both spellings of the overlay file name. The script can be changed by the user for some of their tasks directly on the device. Based on these assumptions, we need to analyze the existing script and write the names |
Maybe this is a mistake: |
|
An amateur's question. |
I don't think that this is possible. Perhaps @Tearran knows more? |
The current syntax in these two files works correctly at least in u-boot version 2025.01. Due to backward compatibility with older versions of u-boot, the following syntax was implemented for sunxi: |
github-actions
bot
added
size/medium
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tools/modules/system/manage_dtoverlays.sh (2)
75-83: Mirror improvements forbuiltin_overlayspipeline
Same issues here:
- Quote
${overlaydir}infind.- Use
sub(/\.dtbo$/, "", $NF)instead ofgsub(".dtbo", "", $0).- Simplify with
find ... ! -name "${overlay_prefix}-*.dtbo"and a leanerawk.
53-53: Remove unused variablej
The variablejis assigned but never used (SC2034). Consider deleting this line to clean up dead code.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 53-53: j appears unused. Verify use (or export if used externally).
(SC2034)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/modules/system/manage_dtoverlays.sh(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: The-going
PR: armbian/configng#609
File: tools/modules/system/manage_dtoverlays.sh:41-51
Timestamp: 2025-06-01T20:51:38.861Z
Learning: In tools/modules/system/manage_dtoverlays.sh, the original code behavior was to list ALL .dtbo files in the overlay directory regardless of overlay_prefix, but the refactored code filters to only show files starting with overlay_prefix. This behavioral change can break systems where overlay_prefix is empty/misconfigured or where users expect to see all available overlays.
Learnt from: The-going
PR: armbian/configng#609
File: tools/modules/system/manage_dtoverlays.sh:41-51
Timestamp: 2025-06-01T20:51:38.861Z
Learning: In tools/modules/system/manage_dtoverlays.sh, the original code behavior was to list ALL .dtbo files in the overlay directory regardless of overlay_prefix, but the refactored code filters to only show files starting with overlay_prefix. This behavioral change can break systems where overlay_prefix is empty/misconfigured or where users expect to see all available overlays.
tools/modules/system/manage_dtoverlays.sh (2)
Learnt from: The-going
PR: armbian/configng#609
File: tools/modules/system/manage_dtoverlays.sh:41-51
Timestamp: 2025-06-01T20:51:38.861Z
Learning: In tools/modules/system/manage_dtoverlays.sh, the original code behavior was to list ALL .dtbo files in the overlay directory regardless of overlay_prefix, but the refactored code filters to only show files starting with overlay_prefix. This behavioral change can break systems where overlay_prefix is empty/misconfigured or where users expect to see all available overlays.
Learnt from: The-going
PR: armbian/configng#609
File: tools/modules/system/manage_dtoverlays.sh:41-51
Timestamp: 2025-06-01T20:51:38.861Z
Learning: In tools/modules/system/manage_dtoverlays.sh, the original code behavior was to list ALL .dtbo files in the overlay directory regardless of overlay_prefix, but the refactored code filters to only show files starting with overlay_prefix. This behavioral change can break systems where overlay_prefix is empty/misconfigured or where users expect to see all available overlays.
🧬 Code Graph Analysis (1)
tools/modules/system/manage_dtoverlays.sh (1)
tools/modules/functions/set_runtime_variables.sh (1)
update_kernel_env(92-110)
🪛 Shellcheck (0.10.0)
tools/modules/system/manage_dtoverlays.sh
[warning] 53-53: j appears unused. Verify use (or export if used externally).
(SC2034)
🪛 GitHub Actions: Coding style check
tools/modules/system/manage_dtoverlays.sh
[error] 45-48: EditorConfig indentation error: wrong indentation type (spaces instead of tabs) on lines 45-48.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Uptimekuma install (noble)
- GitHub Check: Stable repository (bookworm)
- GitHub Check: samba install (bookworm)
- GitHub Check: OpenHab install (bookworm)
- GitHub Check: Rolling repository (bookworm)
- GitHub Check: Portainer install (noble)
- GitHub Check: Home Assistant install (bookworm)
- GitHub Check: Navidrome install (noble)
- GitHub Check: Duplicati install (noble)
- GitHub Check: Cockpit install (noble)
🔇 Additional comments (1)
tools/modules/system/manage_dtoverlays.sh (1)
23-23: Moved Raspberry Pi config path earlier in function
Great: now/boot/firmware/config.txtis set before determining overlays for bcm2711. Ensures prefix parsing reads from the correct file on RPi4.
github-actions
bot
removed
the
Ready to merge
Colors: Yes? No? Maybe!?While The so-called entry point However, since the TUI is already improperly integrated in this module, one might as well lean into it and use The escape sequence { dialog --colors \
--title "Colored Text" \
--msgbox "\Z1First line in red\n\Z3Second line in yellow\n\Z2Third line in green" 10 50; }
For systems limited to {
text=$(awk 'NR==1{ print toupper($0) } NR>1{ print }' <<EOF
first line to emphasize
Second line normal
EOF
)
whiptail --title "Notice" --msgbox "$text" 10 50
}
If we follow a color pattern — say, green, yellow, and red to suggest Go, Caution, and Stop — we can align visual emphasis accordingly:
Or mix them in any combination the module might require. |
Yes, it works well for text messages, but not for a selection list. I made changes directly on the device and then transferred them to the github. |
|
Assemble the modules into a production library From the repo folder ./configng/tools/config-assemble.sh" -p Launch alternatively can bypass menu and directly run |
|
manage_dtoverlays module is missing a help message and trigger |
|
Tip For best results, initiate chat on the files or code changes. @Tearran Thanks for spotting that! I’ll prepare a small patch that:
_about_manage_dtoverlays() {
cat <<-EOF
Device-Tree Overlay Manager (DTO): enable/disable builtin or user-provided
*.dtbo* overlays and persist the choice in /boot/*.
Handles prefix variations and displays the effective boot-script scenario.
EOF
}
Let me know if you’d like different wording or extra details and I’ll include that in the plan. |
|
@igorpecovnik I think I believed the possible options. They're working. Working FDT set to 12000000
441 bytes read in 14 ms (30.3 KiB/s)
Applying kernel provided DT overlay rockchip-rk3588-panthor-gpu.dtbo
Failed to load '/boot/dtb/rockchip/overlay/rockchip-rk3588-orangepi-5-plus-ov13850.dtbo'
1547 bytes read in 16 ms (93.8 KiB/s)
Applying kernel provided DT overlay orangepi-5-plus-ov13850.dtboAs we can see, both overlays with the short and full spelling of the name were loaded. After the changes in this pull request, the overlays corresponding to the prefix will be placed at the top I tested it on an OPI-5-plus with a gnome image that I put together myself. With the new changes, everything happens correctly and the overlays load. |
|
OK, here I think we need to test on:
I assume Allwinner and Rockchip (vendor) both works as expected? |
I think this will work for all the options available in Armbian today. |
|
By the way, this condition may be superfluous. |
There was a problem hiding this comment.
On the contrary, if people change the names of variables, for example armbian/build#8287, we will get a mess and constant fixes in our overlay management module.
OK, I assume he will pay attention.
Merge at will.
github-actions
bot
removed
the
Ready to merge
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tools/modules/system/manage_dtoverlays.sh (4)
61-66: Consider hardening the overlay-listing pipeline.
- Quote the
-namepattern as-name "${overlay_prefix}*.dtbo".- Use
subwith anchored regex to strip suffix only (sub(/\.dtbo$/, "", $NF)).- Optionally anchor prefix removal (
sub("^" overlay_prefix "-", "", $NF)).Example diff:
-find "${overlaydir}"/ -name "$overlay_prefix"'*.dtbo' 2>/dev/null | \ -awk -F'/' -v p="${overlay_prefix}-" '{ - gsub(p, "", $NF) - gsub(".dtbo", "", $NF) - print $NF -}' | sort +find "${overlaydir}/" -type f -name "${overlay_prefix}*.dtbo" 2>/dev/null | \ +awk -F'/' -v p="${overlay_prefix}-" '{ + sub("^" p, "", $NF) + sub(/\.dtbo$/, "", $NF) + print $NF +}' | sort
77-85: Harden builtin overlay extraction similarly.
- Quote wildcard patterns uniformly.
- Replace
gsub(".dtbo", "", $NF)withsub(/\.dtbo$/, "", $NF).- Drop the redundant
if ($0 !~ p)sincefind ! -namealready excludes prefix matches.Suggested snippet:
-find "${overlaydir}/" -name '*.dtbo' ! -name "$overlay_prefix"'*.dtbo' 2>/dev/null | \ -awk -F'/' -v p="${overlay_prefix}" '{ - if ($0 !~ p) { - gsub(".dtbo", "", $NF) - print $NF - } -}' | sort +find "${overlaydir}/" -type f -name '*.dtbo' ! -name "${overlay_prefix}*.dtbo" 2>/dev/null | \ +awk -F'/' '{ + sub(/\.dtbo$/, "", $NF) + print $NF +}' | sort
102-102: Quote variables in the loop and grep to prevent word-splitting.
Even if overlay names lack spaces, consistent quoting avoids future pitfalls.Example:
-for overlay in ${available_overlays} ${builtin_overlays}; do +IFS=$'\n' +for overlay in ${available_overlays} ${builtin_overlays}; do local status="OFF" - grep '^overlays' ${overlayconf} | grep -qw ${overlay} && status=ON - grep '^dtoverlay' ${overlayconf} | grep -qw ${overlay} && status=ON + grep -qE "^overlays.*\b${overlay}\b" "${overlayconf}" && status=ON + grep -qE "^dtoverlay=.*\b${overlay}\b" "${overlayconf}" && status=ON options+=( "$overlay" "" "$status") done +unset IFS
110-111: Refine the dialog prompt string for consistency.
Consider movingoverlay_prefix=$overlay_prefixabove the instructions and removing the trailing space/newline for a cleaner UI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tools/modules/system/manage_dtoverlays.sh(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: The-going
PR: armbian/configng#609
File: tools/modules/system/manage_dtoverlays.sh:41-51
Timestamp: 2025-06-01T20:51:38.861Z
Learning: In tools/modules/system/manage_dtoverlays.sh, the original code behavior was to list ALL .dtbo files in the overlay directory regardless of overlay_prefix, but the refactored code filters to only show files starting with overlay_prefix. This behavioral change can break systems where overlay_prefix is empty/misconfigured or where users expect to see all available overlays.
Learnt from: The-going
PR: armbian/configng#609
File: tools/modules/system/manage_dtoverlays.sh:41-51
Timestamp: 2025-06-01T20:51:38.861Z
Learning: In tools/modules/system/manage_dtoverlays.sh, the original code behavior was to list ALL .dtbo files in the overlay directory regardless of overlay_prefix, but the refactored code filters to only show files starting with overlay_prefix. This behavioral change can break systems where overlay_prefix is empty/misconfigured or where users expect to see all available overlays.
tools/modules/system/manage_dtoverlays.sh (2)
Learnt from: The-going
PR: armbian/configng#609
File: tools/modules/system/manage_dtoverlays.sh:41-51
Timestamp: 2025-06-01T20:51:38.861Z
Learning: In tools/modules/system/manage_dtoverlays.sh, the original code behavior was to list ALL .dtbo files in the overlay directory regardless of overlay_prefix, but the refactored code filters to only show files starting with overlay_prefix. This behavioral change can break systems where overlay_prefix is empty/misconfigured or where users expect to see all available overlays.
Learnt from: The-going
PR: armbian/configng#609
File: tools/modules/system/manage_dtoverlays.sh:41-51
Timestamp: 2025-06-01T20:51:38.861Z
Learning: In tools/modules/system/manage_dtoverlays.sh, the original code behavior was to list ALL .dtbo files in the overlay directory regardless of overlay_prefix, but the refactored code filters to only show files starting with overlay_prefix. This behavioral change can break systems where overlay_prefix is empty/misconfigured or where users expect to see all available overlays.
🪛 Shellcheck (0.10.0)
tools/modules/system/manage_dtoverlays.sh
[warning] 53-53: j appears unused. Verify use (or export if used externally).
(SC2034)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: OpenHab install (bookworm)
- GitHub Check: Duplicati install (noble)
- GitHub Check: Home Assistant install (bookworm)
🔇 Additional comments (5)
tools/modules/system/manage_dtoverlays.sh (5)
4-4: Maintainer metadata update looks correct.
The addition of@igorpecovnik,@The-goingas maintainers aligns with the PR collaboration.
23-23: Early override ofoverlayconffor Raspberry Pi is valid.
Switching to/boot/firmware/config.txtbefore scanning overlays ensures downstream code points to the correct file.
36-48: Scenario detection comment and AWK patterns are clear and correct.
Theoverlays?\/quantifier matches bothoverlay/andoverlays/, and escaping\${…}ensures literal matching in/boot/boot.scr.
55-55: Conditional branch for prefix-only scenario is implemented correctly.
The[[ "${scenario}" == "10" || "${scenario}" == "11" ]]check cleanly gates the listing of prefix-based overlays.
75-75: Vendor-kernel branch overlay support gating is correct.
The scenario check for"01"or"11", combined with the rockchip-rk3588/vender guard, ensures only relevant built-in overlays are added.
Signed-off-by: The-going <[email protected]>
…lly load. We will not provide a selection of options that cannot be loaded by this script.
github-actions
bot
removed
the
Ready to merge
3 participants
Description
Issue reference: #592
Testing Procedure
Check the fixes on the working board.