openwrt / luci

LuCI - OpenWrt Configuration Interface
Apache License 2.0
6.28k stars 2.51k forks source link

LEDs: usbport trigger port attribute not configurable through luci #971

Closed howl closed 7 years ago

howl commented 7 years ago

The title says it all. If you select the new usbport trigger it's not possible to set the port attribute. Could be interesting to present it like the netdev trigger, a combo with the possible usb ports that could be setup.

howl commented 7 years ago

Better than a combo a checkbox list could be better, as some routers need to set multiple usb ports for a led like described here https://bugs.lede-project.org/index.php?do=details&task_id=423.

danielfdickinson commented 7 years ago

Does usbport trigger actually support specific ports? I know before usbport the usbdev trigger did, but I'm not sure about usbport.

hnyman commented 7 years ago

Likely fixed by https://github.com/openwrt/luci/commit/1640d141913367827ab442276a943b0a3bba2e20

hnyman commented 7 years ago

Not quite sure if the feature addition by @jow- works quite as expected.

I tested with my R7800, and the results were unexpected.

on GUI it actually offers" hub X, port 0" type of values:

USB Ports
Hub 1, Port 0
Hub 2, Port 0 

And it added a new value "ports" is addition to the already existing "port" list option. Additionally, the values offered were 1-0, 2-0, 3-0, 4-0, while the existing ones (from board.d) were 3-1 and 4-1 for USB2:

config led 'led_usb2'
        option name 'USB 2'
        option sysfs 'r7800:white:usb2'
        option trigger 'usbport'
        list port 'usb3-port1'
        list port 'usb4-port1'
NEW       option default '0'
NEW       option ports 'usb4-port0'

At least for me the correct values are in directories /sys/bus/usb/devices/usb[0-9]/ instead of dir /sys/bus/usb/devices/[0-9]-[0-9]:[0-9]-[0-9]

Apparently the ones found in /sys/bus/usb/devices/1-0:1.0 are the USB hubs, not the ports themselves. Port are named as /sys/bus/usb/devices/usb1/ etc.

root@lede:/etc/config# ls /sys/bus/usb/devices/
1-0:1.0  2-0:1.0  3-0:1.0  4-0:1.0  usb1     usb2     usb3     usb4

root@lede:/etc/config# ls /sys/bus/usb/devices/usb1/
1-0:1.0                       bmAttributes                  manufacturer
authorized                    busnum                        maxchild
...

root@lede:/etc/config# ls /sys/bus/usb/devices/usb2/
2-0:1.0                       bmAttributes                  manufacturer
authorized                    busnum                        maxchild
...
root@lede:/etc/config# cat /sys/bus/usb/devices/1-0\:1.0/uevent
DEVTYPE=usb_interface
DRIVER=hub
PRODUCT=1d6b/2/404
TYPE=9/0/1
INTERFACE=9/0/0
MODALIAS=usb:v1D6Bp0002d0404dc09dsc00dp01ic09isc00ip00in00

root@lede:/etc/config# cat /sys/bus/usb/devices/usb1/uevent
MAJOR=189
MINOR=0
DEVNAME=bus/usb/001/001
DEVTYPE=usb_device
DRIVER=usb
PRODUCT=1d6b/2/404
TYPE=9/0/1
BUSNUM=001
DEVNUM=001

I am not sure if this feature can be automated to such extent, that it can offer correct options for each device. Likely the platform's USB implementation has impact on where the correct values can be found.

howl commented 7 years ago

When you set up the usbport trigger for a led you get the directory ports inside the led directory with all the possible options, if there is no other way to get the correct ports perhaps adding a message saying that you have to apply changes for usbport to be configured an then use the ports dir in luci to show the ports.

Myself doesn't like the option I'm purposing.

howl commented 7 years ago

I'm looking at the Archer C7 and WRT1900ACS sysfs and there is the ports there.

ls /sys/bus/usb/devices/ brings the following: C7: 1-0:1.0 2-0:1.0 usb1 usb2

WRT1900ACS: 1-0:1.0 2-0:1.0 3-0:1.0 usb1 usb2 usb3

The C7 possible values are 1-0:1.0 that is usb1-port1 and 2-0:1.0 that is usb2-port1, just the two values that can be selected: ls /sys/class/leds/tp-link\:green\:usb1/ports/ usb1-port1 usb2-port1

The WRT1900ACS possible values are 1-0:1.0 (usb1-port1) and 2-0:1.0 (usb2-port1) and 3-0:1.0 (usb3-port1), just the values that can be selected: ls /sys/class/leds/pca963x\:shelby\:white\:usb2/ports/ usb1-port1 usb2-port1 usb3-port1

In your R7800: ls /sys/bus/usb/devices/ 1-0:1.0 2-0:1.0 3-0:1.0 4-0:1.0 usb1 usb2 usb3 usb4

So the possible values for your router should be usb1-port1, usb2-port1, usb3-port1 and usb4-port1.

Don't know if it could be a better way to poll for it.

jow- commented 7 years ago

Please provide the output of cat /sys/bus/usb/devices/usb*/maxchild from your devices.

The "port" vs. "ports" thing is due to a typo I made in the CBI file.

hnyman commented 7 years ago

R7800 ipq806x:

Reboot (SNAPSHOT, r3189-12db207e9b)

root@lede:~# cat /sys/bus/usb/devices/usb*/maxchild
1
1
1
1

WNDR3700 ar71xx:

Reboot (SNAPSHOT, r3137-f67b0276b6)

root@OpenWrt2:~# cat /sys/bus/usb/devices/usb*/maxchild
2
2
howl commented 7 years ago

Right now I only have the Archer C7 v2, if necessary I can check also the WRT1900ACS.

Archer C7 v2 ar71xx:

Reboot (SNAPSHOT, r3189-12db207)

cat /sys/bus/usb/devices/usb*/maxchild
1
1
howl commented 7 years ago

What I suggested about 1-0:1.0 being usb1-port1 and so on... now I see that is incorrect. I have took two tl-wr1043nd I have, one with OpenWrt chaos calmer 15.05.1 and the other one with LEDE snapshot r3198. The openwrt one have the usbdev trigger system and lede the usbport system to check how it worked before, as I mess up with the usb leds until the wrt1900acs where I had usbport system.

With usbdev there is no way yo set up the led in luci without connecting a usb device in the usb port. The list that appears there is empty because there is no /sys/bus/usb/devices/#-# at all until a device is connected.

With usbport we are trying to make it work without the need to attach any usb device. I don't know if it could be helpful but inside the hubs there are folders with the name pattern usb#-port#. For example in the Archer C7 you can see: /sys/bus/usb/devices/1-0:1.0/usb1-port1 /sys/bus/usb/devices/2-0:1.0/usb2-port1

Due to symlinks also accessible from the usb# folders: /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port1 /sys/bus/usb/devices/usb2/2-0:1.0/usb2-port1

Jow, I suppose that you have asked about maxchild because it could be possible to also guest the values with it. If the WNDR3700 has values like usb1-port1, usb1-port2, usb2-port1 and usb2-port2 with maxchild one can guess the ports quantity.

jow- commented 7 years ago

@howl - your suggestion is even better than what I had in mind (building entries from maxchild value). It appears that an echo /sys/bus/usb/devices/*/usb[0-9]*-port[0-9]* will yield a list of usable hub/port combinations.

jow- commented 7 years ago

@howl @hnyman - please try the changes in https://github.com/openwrt/luci/commit/926935f83170e38452a5191d13aa8b04021606ff on your devices.

hnyman commented 7 years ago

Works ok in R7800/ipq806x LEDE17.01 build that uses the usbport trigger. There the values shown are the expected 1-1, 2-1, 3-1 and 4-1 and the previously selected values are shown ok.

But in my wndr3700/ar71xx the LEDs are still defined via old usbdev syntax in board.d (and thus in /etc/config/system), so the selected port value are not shown. LuCI offers 1-1, 1-2, 2-1 and 2-2. After manually editing /etc/config/system to the new usbport syntax, the selected port 1-1 is shown ok. (I have no idea, if the other values would work, as there is just one USB port...)

(That is actually a rather large missing step in the transition to usbport that ar71xx board.d/01_leds is still in the old style.)

build000 commented 7 years ago

@jow- @hnyman Identical situation to WNDR4300 (17.01-snapshot). Working only old style

17.01-snapshot:

#line 113 
usbport = s:option(MultiValue, "ports", translate("USB Ports"))

to https://github.com/openwrt/luci/blob/lede-17.01/modules/luci-mod-admin-full/luasrc/model/cbi/admin_system/leds.lua .... and 17.01.0-rc1 - its the same...

howl commented 7 years ago

I manually modified from usbdev to usbport the configuration time ago, so I started with a valid configuration. Even more, I had 'option port usb1-port1' instead of 'list port usb1-port1' and the config parsed the default values right, and when editing they get converted as it should have been 'list port usb1-port1'.

I have tested with the Archer C7 but will try the tl-wr1043nd. Tested that the values in configuration gets changed as they should, also that the default checked values always corresponds to the actual configuration and also that the values are applied checking with cat /sys/class/leds/tp-link\:green\:usb1/ports/usb*

I think this one should be closed as fixed and only if someone found a router that doesn't work as expected the reopen.

I will file a bug in lede about converting the generated configuration for all the families from usbdev to usbport.

howl commented 7 years ago

Tested the tl-wr1043nd, expose two usb but there is only one physical, checking /sys/class/leds/tp-link:green:usb/ports also appears both, so I don't think there is an easy way to only show the physical ones without perhaps making a per device configuration where the usbs are hidden some way. And if someone mods it's router board soldering the second usb port, the actual behavior will be needed.

So indeed now works as expected, nice job jow.

hnyman commented 7 years ago

@jow- @hnyman Identical situation to WNDR4300 (17.01-snapshot). ..not working. 17.01-snapshot:

@build000 Jow has not yet backported the usbport fix into 17.01. Likely it will get included into rc2