Add HDD park at shutdown for Odroid HC4 #9214
Conversation
Odroid HC4 is a NAS. Needs to park HDD heads prior to reboot or shutdown.
📝 WalkthroughWalkthroughA new shell function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Hey @ean365! 👋Thanks for submitting your first pull request to the Armbian project — we're excited to have you contributing! 🧡 If you'd like to stay informed about project updates or collaborate more closely with the team, Also, don’t forget to ⭐ star the repo if you haven’t already — and welcome aboard! 🚀 |
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: 0
🧹 Nitpick comments (1)
config/sources/families/meson-sm1.conf (1)
20-26: Excellent approach: reusing existing shutdown script for HC4.The implementation correctly gates HDD parking functionality to the odroidhc4 board and follows proper systemd shutdown hook conventions. The permissions (0755) and ownership (root:root) are appropriate. The
family_tweaks_bsp()function is automatically invoked during BSP package creation,$destinationis properly initialized in the build context, and theodroid.shutdownscript exists and is compatible with HC4's SATA hardware.Optional: Add defensive quoting for path variables
While the current implementation works correctly, quoting variables is a defensive coding best practice for shell scripts:
Suggested improvement
family_tweaks_bsp() { if [[ $BOARD == odroidhc4 ]]; then # park HDD heads on shutdown - mkdir -p $destination/lib/systemd/system-shutdown - install -o root -g root -m 0755 $SRC/packages/bsp/odroid/odroid.shutdown $destination/lib/systemd/system-shutdown/odroid.shutdown + mkdir -p "$destination/lib/systemd/system-shutdown" + install -o root -g root -m 0755 "$SRC/packages/bsp/odroid/odroid.shutdown" "$destination/lib/systemd/system-shutdown/odroid.shutdown" fi }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
config/sources/families/meson-sm1.conf
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.
Learnt from: igorpecovnik
Repo: armbian/build PR: 8720
File: lib/functions/rootfs/distro-specific.sh:38-47
Timestamp: 2025-11-09T22:30:27.163Z
Learning: In lib/functions/rootfs/distro-specific.sh, the systemd sleep.conf.d override that disables suspend/hibernation is intentionally applied system-wide to all Armbian images (desktop, CLI, and minimal), not gated to desktop-only builds, because suspend/resume is fragile on most boards.
📚 Learning: 2025-11-02T20:49:56.719Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8849
File: config/boards/radxa-e54c.csc:14-28
Timestamp: 2025-11-02T20:49:56.719Z
Learning: In Armbian board configuration files (config/boards/*.conf, *.csc, etc.), do not use kernel_config_set, kernel_config_set_m, kernel_config_set_y, or custom_kernel_config__* functions to modify kernel configuration. Kernel configuration is associated with LINUXFAMILY/BOARDFAMILY, not individual BOARD. Board-specific kernel modifications cause inconsistency in kernel packages published to the apt repository because boards within a family share the same kernel packages. Kernel configuration changes must be made in the appropriate kernel config file (e.g., config/kernel/linux-*-*.config) or in family configuration files (config/sources/families/*.conf, *.inc) instead.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-11-09T22:30:27.163Z
Learnt from: igorpecovnik
Repo: armbian/build PR: 8720
File: lib/functions/rootfs/distro-specific.sh:38-47
Timestamp: 2025-11-09T22:30:27.163Z
Learning: In lib/functions/rootfs/distro-specific.sh, the systemd sleep.conf.d override that disables suspend/hibernation is intentionally applied system-wide to all Armbian images (desktop, CLI, and minimal), not gated to desktop-only builds, because suspend/resume is fragile on most boards.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-09-14T06:32:29.806Z
Learnt from: amazingfate
Repo: armbian/build PR: 8619
File: config/sources/families/rockchip.conf:222-230
Timestamp: 2025-09-14T06:32:29.806Z
Learning: In the Armbian build system, the write_uboot_platform() function implementations follow different patterns across Rockchip family files. The newer standard (used in rockchip64_common.inc and rk3506) includes 'status=none' parameter in dd commands, while older implementations (rk3288, rk322x) use an older pattern without this parameter. The rk3506 implementation correctly follows the current Rockchip family standard.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-07-23T07:30:52.265Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 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/sources/families/meson-sm1.conf
📚 Learning: 2025-09-12T19:28:38.491Z
Learnt from: Grippy98
Repo: armbian/build PR: 8622
File: config/sources/families/k3.conf:66-66
Timestamp: 2025-09-12T19:28:38.491Z
Learning: In the Armbian k3 family build system (config/sources/families/k3.conf), builds do not fail when TIBOOT3_BOOTCONFIG is unset, even though tiboot3.bin is still listed in UBOOT_TARGET_MAP. The gating mechanism in pre_config_uboot_target__build_first_stage function works as intended to conditionally build/copy tiboot3.bin only when TIBOOT3_BOOTCONFIG is defined.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-10-14T05:08:11.785Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8754
File: config/boards/bestv-r3300-l.csc:14-16
Timestamp: 2025-10-14T05:08:11.785Z
Learning: In the Armbian build system, BOOTBRANCH_BOARD is a valid framework variable used as a fallback when BOOTBRANCH is unset. The framework checks BOOTBRANCH_BOARD before applying the default bootloader branch value (see config/sources/common.conf). Board configuration files can use BOOTBRANCH_BOARD to specify the bootloader branch.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-08-03T15:21:20.148Z
Learnt from: pyavitz
Repo: armbian/build PR: 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/sources/families/meson-sm1.conf
📚 Learning: 2025-12-16T12:22:20.156Z
Learnt from: tabrisnet
Repo: armbian/build PR: 9085
File: lib/functions/rootfs/rootfs-create.sh:303-306
Timestamp: 2025-12-16T12:22:20.156Z
Learning: The post_debootstrap_customize hook concept in lib/functions/rootfs/rootfs-create.sh has been tested in PR #9000. The hook placement after package installations (including desktop packages) and before cleanup operations (autoremove, qemu undeploy) is validated as appropriate for rootfs customization.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-09-07T17:39:32.272Z
Learnt from: EvilOlaf
Repo: armbian/build PR: 8586
File: config/boards/nanopi-r76s.conf:15-21
Timestamp: 2025-09-07T17:39:32.272Z
Learning: In the Armbian build system, the variables $BOARD and $SDCARD are always set by the build framework, so guard checks for these variables are unnecessary in board configuration files and hook functions.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-10-26T10:41:35.118Z
Learnt from: HackingGate
Repo: armbian/build PR: 8665
File: config/boards/photonicat2.csc:4-4
Timestamp: 2025-10-26T10:41:35.118Z
Learning: In the Armbian build system, rk3576 boards consistently use BOARDFAMILY="rk35xx" for both vendor and edge kernel targets. The rk35xx family configuration sources rockchip64_common.inc, which provides edge and current kernel branch definitions, making these branches available even though they're not defined directly in rk35xx.conf.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-12-13T11:39:08.046Z
Learnt from: pyavitz
Repo: armbian/build PR: 9058
File: patch/u-boot/legacy/u-boot-spacemit-k1/003-SpacemiT-K1X-Fixups.patch:28-67
Timestamp: 2025-12-13T11:39:08.046Z
Learning: In the Armbian build system for SpacemiT U-Boot patches (patch/u-boot/legacy/u-boot-spacemit-k1/), alignment with mainline U-Boot behavior is prioritized. For example, in boot mode handling, leaving devnum unchanged in the default case (when devtype is cleared) follows mainline conventions rather than explicitly clearing it to handle edge cases.
Applied to files:
config/sources/families/meson-sm1.conf
📚 Learning: 2025-12-12T23:09:56.813Z
Learnt from: tabrisnet
Repo: armbian/build PR: 9058
File: config/sources/families/spacemit.conf:39-45
Timestamp: 2025-12-12T23:09:56.813Z
Learning: In Armbian build configs for vendor kernel sources, prefer the following branch naming conventions: use 'vendor' or 'vendor-rt' for stable vendor releases, and 'vendor-edge' for bleeding-edge/pre-release vendor versions. The 'edge' naming without the 'vendor-' prefix is reserved for mainline kernel branches. Apply this pattern to family config files under config/sources/families (e.g., spacemit.conf) to ensure consistent vendor kernel sourcing naming across the repository.
Applied to files:
config/sources/families/meson-sm1.conf
rpardini
left a comment
rpardini
left a comment
There was a problem hiding this comment.
Nice, but please do board-specific stuff in the board file, not the family file.
You can use a hook called post_family_tweaks_bsp.
|
Also perhaps replacing the script with something like this (needs testing): |
|
Thanks. @rpardini, are you saying that it should be added to odroidhc4.csc instead? I put it in meson-sm1.conf because I'm not totally familiar with Armbian practice, thought it needed to be in a conf file, and because I already saw multiple device specific stuff in there. I can switch it into the .csc, if that is what you are recommending?? @igorpecovnik, I was trying to reuse the existing odroid.shutdown script, since it is used by the XU4 as well, and it is working. Do you see a big difference between them? If so, and you prefer the new one above, then I can put in a separate PR to update the script as you suggest. I have been chasing/fixing this issue all night into the wee hours. I'm going to bed, but will check your responses tomorrow (well later today). |
Correct. The board config file can also contain hooks to modify aspects. Hooks in the board family file (meson-sm1.conf in this case) would affect all boards depending on this family config (like odroid n2 for example). |
less dependencies. |
|
Hmmm. I hadn't realized it, but it looks like It's strange that the kernel driver for the HDD doesn't have this built-in -- total miss on that. Also, I'm not quite sure what you meant by moving to extensions, you have way more to say in how you want to structure the project, but it seems like it is part of bsp to me, where it has been for XU4. |
|
Hey, it's me again. As you noted, there's two parts to this: one script laying around somewhere, and the board-related code (legacy "tweaks", which is usually family code) that activates it. "Extensions" are a mechanism where we can define both in a single place (using heredocs, or Ref "minimal images don't have xxx": then don't do xxx for minimals. With an extension you can test for BUILD_MINIMAL (or whatever) and complain/warn/break/avoid-it etc. |
Yes. So the script looks for its existence and only then proceed. mdadm is thus not a dependency and nor require package if you don't intend to have raid setup.
This is just an idea as parking HDD is not something that is only tied to those boards. Extension is clean(er) way for sorting out those things. As with next board that might need this kind of functionality its only one line of code: Example: IMO if this part is getting attention, lets do it better? |
|
Such an improvement would benefit both official NAS — Helios4, Helios64, ODroidHC4, and other pets that use USB rotational drives. Will it work for USB drives? |
If by "USB drives" you mean USB connected HDDs, then yes I think it should work just the same to command them to park the heads (idle-immediate), but of course this implementation would need to be tested to be certain. |
There's only so much one can do as what SATA command is emitted by the USB controller is up to the USB controller. But yes, one can send |
|
As part of testing the other PR, I reworked the odroid.shutdown script into some #!/bin/bash
set -euo pipefail
declare LOG_PREFIX="sata.shutdown"
declare -i DRY_RUN=0
log_kmsg() {
declare msg="$1"
echo "${LOG_PREFIX}: ${msg}" > /dev/kmsg
}
sleep_and_log() {
declare -i wait="$1"
if ((wait > 0)); then
log_kmsg "Sleeping for ${wait} seconds."
fi
((DRY_RUN)) || sleep "${wait}"
return 0
}
[[ "${1:-}" == "--dry-run" ]] && DRY_RUN=1 && log_kmsg "Dry run mode enabled."
log_kmsg "Starting shutdown script (DRY_RUN==${DRY_RUN})."
export LANG=C LC_ALL=C
log_kmsg "Syncing filesystems."
((DRY_RUN)) && log_kmsg "[dry-run] Would sync filesystems." || sync || log_kmsg "Warning: sync failed."
if command -v mdadm &> /dev/null; then
log_kmsg "Waiting for mdadm arrays to clean."
((DRY_RUN)) && log_kmsg "[dry-run] Would run: mdadm --wait-clean --scan." || mdadm --wait-clean --scan || log_kmsg "Warning: mdadm --wait-clean failed."
else
log_kmsg "mdadm not found, skipping RAID clean."
fi
if ! command -v hdparm &> /dev/null; then
log_kmsg "hdparm not found, skipping disk parking."
exit 1
fi
log_kmsg "Parking SATA disks."
for dev in /sys/block/sd*; do
declare -i wait=0
if [[ ! -e "${dev}" ]]; then
log_kmsg "Could not find ${dev}, skipping."
continue
fi
declare dev_name="${dev##*/}"
declare dev_path="/dev/${dev_name}"
declare mounted=0 # Check if device or any partition is mounted using findmnt
if findmnt -rn -S "${dev_path}" > /dev/null; then
log_kmsg "Device ${dev_path} is mounted (findmnt). Will skip kernel delete."
mounted=1
else
for part in "/dev/${dev_name}"[0-9]*; do
[[ -b "${part}" ]] || continue
if findmnt -rn -S "${part}" > /dev/null; then
log_kmsg "Partition ${part} is mounted (findmnt). Will skip kernel delete for ${dev_path}."
mounted=1
break
fi
done
fi
if ((DRY_RUN)); then
log_kmsg "[dry-run] Would run: hdparm -y ${dev_path}"
log_kmsg "[dry-run] Would set wait=2."
wait=2
else
if hdparm -y "${dev_path}"; then
log_kmsg "Disk ${dev_path} parked."
wait=2
else
log_kmsg "Warning: hdparm failed for ${dev_path}"
fi
fi
if ((mounted)); then
log_kmsg "Mounted filesystems detected on ${dev_path} or its partitions. Skipping kernel delete."
elif ((DRY_RUN)); then
log_kmsg "[dry-run] Would run: echo 1 > /sys/class/block/${dev_name}/device/delete"
log_kmsg "[dry-run] Would log: Disk ${dev_path} deleted from kernel."
else
sleep_and_log "${wait}"
log_kmsg "Deleting disk ${dev_path} from kernel /sys/class/block/${dev_name}/device/delete ..."
if echo 1 > "/sys/class/block/${dev_name}/device/delete"; then
log_kmsg "Disk ${dev_path} deleted from kernel."
else
log_kmsg "Warning: failed to delete ${dev_path} from kernel."
fi
fi
sleep_and_log "${wait}"
done
log_kmsg "Shutdown script completed."
exit 0This produces serial console output similar to |
|
It's so strange to learn about the need to park disk heads, when the last time I saw this was before the Windows '95 era... Although back then it was common knowledge, and everyone always gave the command to park disks before turning off their PCs. |
|
I remember. Old HDDs in 80s-early-90s absolutely needed to be parked. Later, the automatic/emergency parking feature was added using stored energy. Even though, in modern times, it works fairly reliably and has been optimized, it is still nonetheless considered an emergency, somewhat "violent", and something to be avoided -- give it a warning and time to do it nicely, if possible... Linux ATA driver (and most modern OSs) still does send So, I think the issue in this case might just be that after sending the UPDATE: Actually, it is starting to look like Linux ATA driver only sends a |
5 participants
Odroid HC4 is a NAS. Needs to park HDD heads prior to reboot or shutdown. This simple PR adds the already existing odroid.shutdown bsp script (originally used for the XU4) to the meson-sm1.conf file for the HC4.
I have successfully tested the odroid.shutdown script on the HC4 with HDDs, and it properly parks the heads. Without this, the heads are forced to do an emergency retract, which is a violent action that degrades the HDD over time.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.