fix(armbian-leds): strip brightness only for :link / phy*tpt triggers

[iav]

Summary

armbian-led-state-save.sh saves LED state on shutdown for restore at boot.
For two narrow trigger families, brightness from sysfs is not config
but the trigger's instantaneous output (link-up/down boolean, tpt blink
state). Capturing it on shutdown and replaying it at boot produces:

• Ghost-LED bugs on restore — :link triggers showed cable-up while
  unplugged after restoring a saved brightness=1 (PR armbian-led-state: avoid binary parameters, other minor cleanups #7337's case)
• Constant churn in /etc/armbian-leds.conf on every shutdown — visible
  to anyone with etckeeper / etc-diff alerts (rtw88 phy0tpt flapped
  0/1; the forum-thread case)

Fix: strip brightness only for the noisy set:

case "$TRIGGER_VALUE" in
    *:link | phy*tpt) ... ;;
esac

Everything else keeps brightness as legitimate config — including the
kernel LED ABI "ceiling" semantics (writing non-zero brightness while a
trigger is active sets the top brightness for the trigger's output),
which config/boards/radxa-e52c.conf and radxa-e54c.conf rely on with
brightness=1 under trigger=netdev for a dimmed link blink.

The token-safe whole-word filter (instead of ${PARAMS//brightness/}
substring substitution) is required either way to avoid corrupting
sibling files like max_brightness → max_.

History

Pattern grew incrementally from existing point-fixes for the same root cause:

• :link-only strip in PR armbian-led-state: avoid binary parameters, other minor cleanups #7337 (commit 2960ffaff) — original ghost-LED case
• phy*tpt extension here — covers the rtw88 forum-thread case
• Restore-side [[ $VALUE -eq 0 ]] && continue guard in
  armbian-led-state-restore.sh — independent guard against writing 0
  (which would kick trigger back to none); kept untouched

A previous version of this PR (commit 56a978bd4) blanket-stripped
brightness for any non-none trigger, but that regressed legitimate dim
ceilings (radxa-e52c case above). Narrowed to the explicit blacklist on
review feedback.

Forum reference

Documented in detail (with diff samples and root-cause analysis by
@tparys) on the Armbian forum:
Regular changes in file /etc/armbian-leds.conf on ODroid-N2

Test plan

Standalone scaffold (.tmp/test-led-state-save.sh) mocks
/sys/class/leds/ and runs the save script:

• trigger=eth0:link, saved brightness=1 → brightness stripped
• trigger=phy0tpt, saved brightness=0 → brightness stripped
• trigger=netdev, saved brightness=1 (radxa dim case) → preserved
• trigger=heartbeat, saved brightness=42 → preserved
• trigger=disk-activity, saved brightness=255 → preserved
• trigger=none, saved brightness=128 → preserved
• Other writable knobs (delay_on=100, link=1, tx=0, rx=1)
  preserved on every LED
• max_brightness not corrupted by token-safe filter (was
  ${PARAMS//brightness/} → max_ substring bug)

[coderabbitai]

📝 Walkthrough

Walkthrough

Normalize input-redirection whitespace when reading trigger and parameter files; implement trigger-aware filtering that removes brightness only for noisy triggers matching *:link or phy*tpt using a token-safe PARAMS filter.

Changes

LED Brightness Parameter Filtering

Layer / File(s)	Summary
Trigger file read packages/bsp/common/usr/lib/armbian/armbian-led-state-save.sh	Reads trigger content from sysfs with normalized whitespace around the < redirection operator.
Brightness filtering for noisy triggers packages/bsp/common/usr/lib/armbian/armbian-led-state-save.sh	Replaces prior *:link-substring workaround with a case that strips brightness only for noisy triggers matching *:link or phy*tpt; uses a token-safe filter of PARAMS and normalizes per-parameter value reads.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A rabbit peeks at sysfs light,
Filters brightness only when noisy at night,
Whitespace tidied, tokens spared with care,
Saved states hum softly, clean and fair. 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title directly summarizes the main change: fixing brightness stripping behavior for specific trigger types (:link and phy*tpt), which is the core focus of the changeset.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/armbian-leds-active-trigger-brightness

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/bsp/common/usr/lib/armbian/armbian-led-state-save.sh (1)

53-58: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use token-safe filtering for brightness (current replacement corrupts other param names).

Line 53 does a substring replacement, so max_brightness becomes max_. That yields invalid paths in Line 57/58 and can abort persistence when reads fail.

Suggested fix

-	[[ "$TRIGGER_VALUE" != "none" ]] && PARAMS=${PARAMS//"brightness"/}
+	if [[ "$TRIGGER_VALUE" != "none" ]]; then
+		# Remove only the exact filename token, keep e.g. max_brightness intact
+		PARAMS="$(printf '%s\n' "$PARAMS" | awk '$0 != "brightness"')"
+	fi

Based on learnings: In the Armbian build framework, scripts run with set -e, so non-zero failures should be treated as abort paths unless explicitly handled.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/bsp/common/usr/lib/armbian/armbian-led-state-save.sh` around lines
53 - 58, The current substring replacement on PARAMS corrupts tokens like
max_brightness (so PARAM_PATH="$PATH/$PARAM" fails); instead filter tokens
safely before the loop by removing only the exact token "brightness" (e.g.,
rebuild PARAMS to exclude any word equal to "brightness") so PARAM remains
intact; change the logic around TRIGGER_VALUE/PARAMS (the lines manipulating
TRIGGER_VALUE, PARAMS and the for PARAM in $PARAMS loop and uses of PATH and
PARAM_PATH) to skip the brightness token rather than using
${PARAMS//"brightness"/}, ensuring no invalid paths are produced and the script
won't abort under set -e when reading PARAM_PATH.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@packages/bsp/common/usr/lib/armbian/armbian-led-state-save.sh`:
- Around line 53-58: The current substring replacement on PARAMS corrupts tokens
like max_brightness (so PARAM_PATH="$PATH/$PARAM" fails); instead filter tokens
safely before the loop by removing only the exact token "brightness" (e.g.,
rebuild PARAMS to exclude any word equal to "brightness") so PARAM remains
intact; change the logic around TRIGGER_VALUE/PARAMS (the lines manipulating
TRIGGER_VALUE, PARAMS and the for PARAM in $PARAMS loop and uses of PATH and
PARAM_PATH) to skip the brightness token rather than using
${PARAMS//"brightness"/}, ensuring no invalid paths are produced and the script
won't abort under set -e when reading PARAM_PATH.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d16e0681-e0b1-4eb5-8822-bbda7d2dd0e4

📥 Commits

Reviewing files that changed from the base of the PR and between 4aa01fe and 22b938f.

📒 Files selected for processing (1)

• packages/bsp/common/usr/lib/armbian/armbian-led-state-save.sh

[iav]

Adopted, see commit 11f05650.

[tparys]

I suppose I didn't notice that LED was PHY related. That's an icky side effect. I'm certainly in favor of this fix.

[EvilOlaf]

This PR contains unrelated changes to interactive.sh?

[iav]

This PR contains unrelated changes to interactive.sh?

You're right, I dragged it out by mistake. I've already removed it.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/bsp/common/usr/lib/armbian/armbian-led-state-save.sh`:
- Around line 70-79: The current case branch filters out "brightness" only for
TRIGGER_VALUE matching "*:link" or "phy*tpt", but we need to drop brightness for
any active trigger; update the logic in the case block that inspects
TRIGGER_VALUE so that whenever TRIGGER_VALUE denotes an active trigger (e.g. not
"none" or empty), you iterate over PARAMS and remove any "brightness" entries
(same filtering loop using _filtered and _p). Keep using the same variable names
(TRIGGER_VALUE, PARAMS, _filtered, _p) and preserve existing behavior for
specific link/phy patterns while broadening the condition to cover all non-none
triggers.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0ad49e99-8ef2-42ee-881c-39c9b941655f

📥 Commits

Reviewing files that changed from the base of the PR and between 56a978b and 37d3e12.

📒 Files selected for processing (1)

• packages/bsp/common/usr/lib/armbian/armbian-led-state-save.sh