Closed meyergru closed 5 months ago
Hi! Thanks for pointing out the instability issue of the read operation.
I re-examined the UGOS driver and found that it only queries the status at initialization. I also found that the status query operation also randomly failed in my DX4600 Pro, no matter how long it sleeps.
For the write operation, I adjusted sleeping time. A write operation now requires 2500us to complete, and if it failed, extra 3ms sleep is required before retrying. In this case, repeatedly toggling an LED 1000 times (i.e., change the state 2000 times) results in ~2200 retries, but all of these operations success. Therefore, it seems that the problem is the MCU is not reliable. Besides, retrying when failed in the read operation also makes the status query robust.
Finally, as you imagined, at least in my device, writing to the non-existent LED address appears safe, and as you said, the availability check is unnecessary except for the all -status
command, so I removed it for other operations.
If you are interested, you can check whether the new version will be robust in DXP8800:
# there will be no errors (output nothing)
for i in $(seq 1 1000); do
./ugreen_leds_cli disk4 -on
./ugreen_leds_cli disk4 -off
done
BTW, I find your zpool-leds.sh
script very useful.
It seems to work fine now. No more errors.
I have modified the script for your binary, as well as added the LAN status by checking if the gateway is reachable in order to control the netdev LED:
#! /bin/bash
#set -x
devices=(p n x x x x x x x x)
map=(power netdev disk1 disk2 disk3 disk4 disk5 disk6 disk7 disk8)
gw=$(ip route | awk '/default/ { print $3 }')
if ping -q -c 1 -W 1 $gw >/dev/null; then
devices[1]=u
fi
while read line
do
DEV=($line)
#echo "${DEV[0]} ${DEV[1]}"
case "${DEV[0]}" in
sda*) index=2;;
sdb*) index=3;;
sdc*) index=4;;
sdd*) index=5;;
sde*) index=6;;
sdf*) index=7;;
sdg*) index=8;;
sdh*) index=9;;
esac
if [ ${DEV[1]} = "ONLINE" ]; then
#echo "$index on"
devices[$index]=o
else
#echo "$index fail"
devices[$index]=f
fi
done <<< "$(zpool status -L | egrep '^\s+sd[a-h][0-9]')"
for i in "${!devices[@]}"; do
# echo "$i: ${devices[$i]}"
case "${devices[$i]}" in
p)
ugreen_leds_cli ${map[$i]} -color 255 255 255 -on -brightness 128
;;
u)
ugreen_leds_cli ${map[$i]} -color 255 255 255 -on -brightness 128
;;
o)
ugreen_leds_cli ${map[$i]} -color 0 255 0 -on -brightness 128
;;
f)
ugreen_leds_cli ${map[$i]} -color 255 0 0 -blink 400 600 -brightness 128
;;
*)
ugreen_leds_cli ${map[$i]} -off
;;
esac
done
Feel free to include it here.
It can be used in /etc/zfs/zed.d/ to catch zfs events. Also, one might make systemd call it when a network change is detected.
I think Ugos even has the LEDs blink on activity - for that, you would need to intercept more granuarly. IMHO, the state changes are what matters most - I do not need hectic blinking.
Hi Yuhao!
First off: Well done to integrate the other Ugreen NASync devices into your code and making it more versatile. That way, I can drop my fork when it works. I only created it because I first thought that there was no "netdev" LED on the DXP8800, which turned out to be a false assumption.
That being said, I have tried your version and I am getting errors. Sometimes, I see:
ugreen_leds_cli disk6 -color 0 255 0 -on -brightness 128
disk6 is not available.
My own script to check the zpool status calls the tool 10 times in a row and I get these errors randomly for each disk, sometimes for more than one. Maybe this is a timing issue? FWIW, the is_led_available() function is not reliable.
I see what you are trying to do there, but with the exception of the "all -status" command, this is probably not neccessary: I imagine it would do no harm to set a non-existent LED ID for models with less disks.
Kind regards!