openwrt / openwrt

This repository is a mirror of https://git.openwrt.org/openwrt/openwrt.git It is for reference only and is not active for check-ins. We will continue to accept Pull Requests here. They will be merged via staging trees then into openwrt.git.
Other
19.76k stars 10.3k forks source link

Netgear WNDR3800: phy options missing from LED configuration in LuCI in 21.02.1 #9292

Open PinkFreud opened 2 years ago

PinkFreud commented 2 years ago

It appears to be impossible to tie a phy to an LED via the LED configuration in LuCI. The sysfs triggers are still listed in sysfs:

# cat /sys/class/leds/netgear\:blue\:wlan5g/trigger 
none switch0 timer default-on netdev usbport phy0rx phy0tx phy0assoc phy0radio phy0tpt phy1rx phy1tx phy1assoc phy1radio [phy1tpt] 

... but none of these are listed in the trigger drop-down in LuCI (I had to take a screenshot of the generated list in the inspector, since I can't seem to figure out how to get a screenshot with the drop-down list on-screen, but that's an annoyance for another time...). openwrt_led_config_21 02

jow- commented 2 years ago

@feckert

feckert commented 2 years ago

I took a look at it. This trigger behaves quite differently. This trigger has the name of the phy integrated in the trigger name so we have phy0assoc phy1assoc phy2assoc and so on if we have more then one phy. On my system I have the following triggers for the phy0 device phy0rx phy0tx phy0assoc phy0radio phy0tpt. If the trigger behaves like the netdev trigger then this would not be a problem. But this way I have a problem to map it correctly in the LuCI.

My suggestion would be, to create an application trigger phy that looks like this: Screenshot_20220221_130817

For this we need to add an application trigger file that will be executed when this trigger is selected and does the needed changes in the backend.

#!/bin/sh

main() {
        local led="$1"

        local device="$(uci_get system "$led" device)"
        local trigger="$(uci_get system "$led" phy_trigger)"
        local sysfs="$(uci_get system "$led" sysfs)"

        [ -f "/sys/class/leds/${sysfs}/trigger" ] && {
                echo "${device}${trigger}" > /sys/class/leds/${sysfs}/trigger
        }
}

main "$@"

Now you will ask yourselves why I would solve it like this. Let me try to explain.

The problem is that I have to store each triggerphy0assoc.js, phy0radio.js ... into the following /www/luci-static/resources/view/system/led-trigger/ to select it as a trigger in the LuCI. This is because of the change to support application led triggers in LuCI, that does not have a kernel trigger directly. If we now want to support more than one phy device, we have to clone these files for each device.

For this to work, the following line https://github.com/openwrt/openwrt/blob/cbfce9236754700a343632fff8e035acbc1b1384/package/base-files/files/etc/init.d/led#L33-L39 must be extended, so that the name of the uci section for the application trigger must be passed.


    # execute application led trigger
    [ -f "/usr/libexec/led-trigger/${trigger}" ] && {
        . "/usr/libexec/led-trigger/${trigger}" "$1"
        return 0
    }

@jow Sorry but I couldn't think of any other solution without completely changing the LED handling in the Luci.

If we do it this way, we can also change the switch trigger to this behaviour so we only have one file switch.js https://github.com/openwrt/luci/tree/master/applications/luci-app-ledtrig-switch/htdocs/luci-static/resources/view/system/led-trigger

Edit: If someone wants to continue working with the kernel trigger names phy0assoc, phy1assoc, they can do so. This change does not change the behavior and does not require an upgrade script.

feckert commented 2 years ago

@jow- Would this be a solution to the problem? Would like to continue here.

If this fits then I would create a pullrequest, with this changes in the related packages.