Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LEDs sometimes "not available" #1

Closed
meyergru opened this issue Jun 2, 2024 · 3 comments
Closed

LEDs sometimes "not available" #1

meyergru opened this issue Jun 2, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@meyergru
Copy link

meyergru commented Jun 2, 2024

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!

@miskcoo
Copy link
Owner

miskcoo commented Jun 2, 2024

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

miskcoo added a commit that referenced this issue Jun 2, 2024
miskcoo added a commit that referenced this issue Jun 2, 2024
@miskcoo
Copy link
Owner

miskcoo commented Jun 2, 2024

BTW, I find your zpool-leds.sh script very useful.

@miskcoo miskcoo added the bug Something isn't working label Jun 2, 2024
@meyergru
Copy link
Author

meyergru commented Jun 2, 2024

  1. It seems to work fine now. No more errors.

  2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants