rockchip64: add multiple SPI images support to armbian-install #8123
Conversation
WalkthroughThe function Suggested labels
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:
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
05
![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 (1)
config/sources/families/include/rockchip64_common.inc (1)
334-339: Consider adding error handling for dd commandWhile the implementation correctly handles dialog cancellation, it would be beneficial to check for errors from the
ddcommand to ensure the write operation succeeded.if [ $? -eq 0 ]; then - dd if=$1/${MENU_ITEMS[($CHOICE*3)-2]} of=$2 conv=notrunc status=none > /dev/null 2>&1 + if ! dd if=$1/${MENU_ITEMS[($CHOICE*3)-2]} of=$2 conv=notrunc status=none > /dev/null 2>&1; then + echo "Error writing SPI image to device." + exit 1 + fi else echo "No SPI image chosen." exit 1 fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/sources/families/include/rockchip64_common.inc(1 hunks)
🔇 Additional comments (5)
config/sources/families/include/rockchip64_common.inc (5)
302-306: LGTM: Effective file search pattern for SPI images.The implementation correctly searches for SPI images using a wildcard pattern, allowing it to find multiple SPI loader files. This is essential for supporting different boot scenarios on boards like the Orange Pi 5.
308-317: LGTM: Clean array implementation for menu items.Good implementation using a while loop to populate the menu items array with file indices and basenames. The array structure (index, filename, empty description) aligns well with dialog's radiolist format requirements.
319-322: LGTM: Smart optimization for single image case.Good optimization to skip the dialog when only one SPI image is found, improving user experience. The implementation correctly uses the index value to determine when only one image is present.
324-333: LGTM: Nice user-friendly dialog implementation.The dialog implementation is well-structured with a descriptive title and proper Armbian branding in the backtitle. Good practice to source the board information from
/etc/armbian-releaseto customize the interface.
302-340: Great implementation of multiple SPI images support.The implementation successfully addresses the requirements in PR objectives for supporting multiple SPI images on rockchip boards. The code provides a user-friendly selection interface while maintaining backward compatibility for single image scenarios. The dialog UI is well-structured and the calculations for menu item selection are correct.
rpardini
left a comment
rpardini
left a comment
There was a problem hiding this comment.
Good stuff, the SATA variants have long been hidden despite available.
The only downside I can see here is that the function becomes intrinsically tied to a TUI using dialog, which I think would be best handled by the caller utility (armbian-install or some "configng").
If this affected the mmc code path I'd be against it, as I know many different scripts consume write_uboot_platform directly, I'd say the TUI would be blocker -- many callers do not expect to be interactive -- since that TUI is in decrepit state, and this only affects the mtd code path, I think it's fine.
| ((i++)) | ||
| done <<< "$FILES" | ||
|
|
||
| # If there is only one image, we can skip the dialog |
There was a problem hiding this comment.
Also skip and log if
- terminal not interactive (eg
[[ -t 0 ]]for stdin if I am not mistaken) dialognot available
Yes agree, i was also hesitant to make this global for all rockchip boards. It might be better to add this to armbian-install and keep platform codes minimal. I just didn't want to mess with legacy installer code, but will investigate it. |
I feel you. I've been there, and it's not looking pretty. A full rewrite (using a proper TUI framework eg Python's On armbian/build side I've started writing out some metadata, but it still relies on bash functions to do the actual writing (burden of the Also the way the image-builder ends up consuming all this is just bizarre (through the chroot's installed .deb!), not to mention the whole UBOOT_TARGET_MAP-hell.
Sincerely, I'd just merge this change as-is and we can coordinate the long-needed structural changes separately. |
OK then, lets keep this as-is until armbian/configng#384 is merged.
Agree, it bothers me every time i play with uboot configurations :D I will apply your requests and then let's merge it. As far as i've seen write_uboot_platform_mtd is only used by |
Description
This PR adds multiple SPI image selection option to armbian-install for rockchip boards when there are
rkspi_loader*.imgfiles more than one. This is useful for boards like Orange Pi 5 as there should be different SPI image for mSata boot scenario.GitHub issue reference:
Jira reference number [AR-9999]
Documentation summary for feature / change
Please delete this section if entry to main documentation is not needed.
If documentation entry is predicted, please provide key elements for further implementation into main documentation and set label to "Needs Documentation". You are welcome to open a PR to documentation or you can leave following information for technical writer:
How Has This Been Tested?
Checklist:
Please delete options that are not relevant.