luci-app-ledtrig-switch: add trigger mode option

[ptpt52]

Also fix issue of options being overwritten: switch0.js switch1.js use the same option name, and one would overwrite the other

[feckert]

Unfortunately, I cannot test this. But so far it looks quite good. But I would like to change this trigger. So that it works exactly like this https://github.com/openwrt/luci/pull/5727

This would simplify the whole handling and we would not have to create a file for each switch[0-9] trigger. It would be generic. The interface could then be configured via dropdown. Can you tell me which driver creates these switch[0-9] triggers in the led subsystem? I can't find anything about it.

[ptpt52]

@feckert my custom hardware contain 2 switch chips (mt7530 + gsw150) so I have switch0 and switch1

anyway could you rework on the js, I am not very familiar with js code I could help test.

[feckert]

@ptpt52 I have now rewritten the luci-app-ledtrig-switch so that it does not use the trigger for the LED directly as a kernel trigger but via the script under /usr/libexec/led-trigger/switch. Thus we do not need several files switch[0-9]* to configure them via the LuCI. https://github.com/TDT-AG/luci/tree/pr/20220329-luci-app-ledtrig-switch

There is now a new option device that selects which switch and therefore which trigger should be used.

The following line still has to be adapted so that the drop down can be filled with the switches that are available on this target with LED support. https://github.com/TDT-AG/luci/commit/f70248dae248af006c7455cce35f730260348e6c#diff-8f48e2ecb85ec7a9cf3e7e97a5efa28295e4a614c92c98da08a3a0820476e598R19

Unfortunately, I cannot test this. Maybe you can find out where we can read the information in /sys about which switch devices are available.

Another problem could be the mode option, which is saved as a list. I don't know how the trigger expects this or how it should be written into the mode file by the new /usr/libexec/led-trigger/switch file. You would have to take a look at what this trigger expects.

config led 'led_Power'
        option name 'Power'
        option sysfs 'apu:green:1'
        option trigger 'switch'
        option device 'switch0'
        list mode 'link'
        list mode 'tx'
        list mode 'rx'

[ptpt52]

@feckert

I don't know how the trigger expects this or how it should be written into the mode file by the new /usr/libexec/led-trigger/switch file. You would have to take a look at what this trigger expects.

any input order could be accepted here. like:

echo "link" >/sys/class/leds/${sysfs}/mode
or 
echo "tx link rx" >/sys/class/leds/${sysfs}/mode

BTW, I notice that the led config in /etc/config/system is now handled by /etc/init.d/led

and to resolve an switch led list:

#maybe somewhat like:
cat /sys/class/leds/*/trigger  | grep -o switch[0-9]* | sort | uniq
switch0

sample output of command cat /sys/class/leds/*/trigger is:
none switch0 90000.mdio-1:04:link 90000.mdio-1:04:1Gbps 90000.mdio-1:04:100Mbps 90000.mdio-1:04:10Mbps timer heartbeat default-on netdev usbport phy0rx phy0tx phy0assoc phy0radio [phy0tpt] phy1rx phy1tx phy1assoc phy1radio phy1tpt
[none] switch0 90000.mdio-1:04:link 90000.mdio-1:04:1Gbps 90000.mdio-1:04:100Mbps 90000.mdio-1:04:10Mbps timer heartbeat default-on netdev usbport phy0rx phy0tx phy0assoc phy0radio phy0tpt phy1rx phy1tx phy1assoc phy1radio phy1tpt
[none] switch0 90000.mdio-1:04:link 90000.mdio-1:04:1Gbps 90000.mdio-1:04:100Mbps 90000.mdio-1:04:10Mbps timer heartbeat default-on netdev usbport phy0rx phy0tx phy0assoc phy0radio phy0tpt phy1rx phy1tx phy1assoc phy1radio phy1tpt

it seems there is not path like /sys/class/ieee80211/ to get the list device

maybe another way the get the list is get the switch list:

swconfig list
Found: switch0 - 90000.mdio-1

[ptpt52]

we may not need a new /usr/libexec/led-trigger/switch but just make sure config save to /etc/config/system and it should works file: /etc/config/system

config led 'led_lan4'
	option name 'LAN4'
	option sysfs 'blue:lan-4'
	option trigger 'switch0'
	option port_mask '0x01'
	option speed_mask '0x0e'
	list mode 'link'
	list mode 'tx'
	list mode 'rx'

[feckert]

we may not need a new /usr/libexec/led-trigger/switch but just make sure config save to /etc/config/system and it should works file: /etc/config/system

This led trigger has already worked before. Only the mode option was missing, that you add with this pull request. The problem with this solution is, that you have to copy the same file for each switch in order to use switch0, switch1, ... and so on. I would rather have a generic solution here, so that we don't always have to copy everything. Therefore, I would prefer the new application trigger switch. The old trigger still works.

I just need a simple solution to fill the drop down for the device selction. Your suggestion with swconfig is deprecated because this API is superseded by the DSA (Distributed Switch Architecture) and was only available on openwrt. So I would prefer your first solution. cat /sys/class/leds/*/trigger | grep -o switch[0-9]* | sort | uniq I'll have to take a closer look tomorrow. Can you tell me what drivers in Openwrt master you are using for this switches to use this led triggers?

[ptpt52]

@feckert for mt7620 mt7628 it still using the swconfig and driver target/linux/ramips/files/drivers/net/ethernet/ralink/ and for my custom build for some mt7621 I am still using this ralink driver and also I am using the new mt753x driver (port from target/linux/mediatek/files-5.10/drivers/net/phy/mtk/mt753x/)

see: https://github.com/x-wrt/x-wrt/tree/master/target/linux/ramips/files/drivers/net/phy

it looks like upstream openwrt drop most swconfig but now mt7620/mt7628 still on it.

[ptpt52]

and as known to all, the switch led trigger is service by target/linux/generic/files/drivers/net/phy/swconfig_leds.c

[ptpt52]

anyway, why I prefer to use the swconfig driver? because it provide full switch support and full vlan handle but dsa is not, and maybe dsa still buggy for now.

[feckert]

@ptpt52 thanks for the links. I don't have a target that uses this hardware with swconfig! So I agree with you that this led trigger is only valid with the package swconfig and if the kernel packend swconfig is compiled with SWCONFIG_LEDS enabled https://github.com/openwrt/openwrt/blob/f7f12495bc9725e221b595680c47c4240d437abb/target/linux/generic/hack-5.10/700-swconfig_switch_drivers.patch#L27. So I would add the new application led trigger switch to the swconfig package and add a dependency from luci-app-ledtrig-switch to swconfig.

What is the output of swonfig? Then I would use that to fill dropdown for device selection. Or is there another elegenat way to fill the dropdown to select the device? For example, under /sys/* Because using the information for /sys/* I don't need to create an ACL for the luci-app-ledtrig-switch to execute the swconfig command.

[ptpt52]

@feckert I think swconfig command is the only/best way to get switch list, sure that not /sys/* to do that.

swconfig list command output:

Found: switch0 - mt7530
Found: switch1 - gsw150

[feckert]

@ptpt52 I have now pushed my changes for this issue https://github.com/TDT-AG/luci/commit/f0dddf37bebfd43732a940130af38b202edcbfcf. That's how I would think it should work. Can you try it out?

[ptpt52]

@feckert ok I am building and testing on your commit.

[ptpt52]

@feckert test almost good. but when I enter edit, it shows up device always be switch0, even I change it to switch1 and save.

[ptpt52]

@feckert and also I have to change the option mode name, it looks like conflict with somewhat..

diff --git a/applications/luci-app-ledtrig-switch/htdocs/luci-static/resources/view/system/led-trigger/switch.js b/applications/luci-app-ledtrig-switch/htdocs/luci-static/resources/view/system/l
index 21c08afafe..f4517fea14 100644
--- a/applications/luci-app-ledtrig-switch/htdocs/luci-static/resources/view/system/led-trigger/switch.js
+++ b/applications/luci-app-ledtrig-switch/htdocs/luci-static/resources/view/system/led-trigger/switch.js
@@ -35,7 +35,8 @@ return baseclass.extend({
                o.modalonly = true;
                o.depends('trigger', 'switch');
 
-               o = s.option(form.MultiValue, 'mode', _('Trigger Mode'));
+               o = s.option(form.MultiValue, '_switch_mode', _('Trigger Mode'));
+               o.ucioption = 'mode';
                o.rmempty = true;
                o.modalonly = true;
                o.depends('trigger', 'switch');

[feckert]

@ptpt52 It works on my system as expected. Is it possible that you have a different branch then master? I suspect that you need o.ucioption option it is because of this commit. https://github.com/openwrt/luci/commit/a75ae38b6b36fec76c259c8313857003f5b23841 This commit is also available at 22.03 and 21.02

I have also not mention (sorry for that) that you need this commit https://patchwork.ozlabs.org/project/openwrt/patch/[email protected]/ This is pending upstream and should be get backported to openwrt-22.03 and openwrt-21.02 branch if this was expected. If this commit is not applied the switch callback script could not get the uci section information.

[ptpt52]

@feckert I am using the master code based on upstream master

[feckert]

@ptpt52 for something like this, i would suggest that we communicate by email [email protected]?

[ptpt52]

It looks like we have to handle multi changes

1. update the ucidef_set_led_switch() in package/base-files/files/lib/functions/uci-defaults.sh
2. migrate the led config

[ptpt52]

to generate new style of led config

diff --git a/package/base-files/files/bin/config_generate b/package/base-files/files/bin/config_generate
index 8d03aac066..03771dd315 100755
--- a/package/base-files/files/bin/config_generate
+++ b/package/base-files/files/bin/config_generate
@@ -440,9 +440,10 @@ generate_led() {
                ;;
 
                switch)
-                       local port_mask speed_mask mode
-                       json_get_vars port_mask speed_mask mode
+                       local device port_mask speed_mask mode
+                       json_get_vars device port_mask speed_mask mode
                        uci -q batch <<-EOF
+                               set system.$cfg.device='$device'
                                set system.$cfg.port_mask='$port_mask'
                                set system.$cfg.speed_mask='$speed_mask'
                                set system.$cfg.mode='$mode'
diff --git a/package/base-files/files/lib/functions/uci-defaults.sh b/package/base-files/files/lib/functions/uci-defaults.sh
index f96e645e73..8578270369 100644
--- a/package/base-files/files/lib/functions/uci-defaults.sh
+++ b/package/base-files/files/lib/functions/uci-defaults.sh
@@ -506,7 +506,8 @@ ucidef_set_led_switch() {
 
        _ucidef_set_led_common "$1" "$2" "$3"
 
-       json_add_string trigger "$trigger_name"
+       json_add_string trigger switch
+       json_add_string device "$trigger_name"
        json_add_string type switch
        json_add_string mode "$mode"
        json_add_string port_mask "$port_mask"

[ptpt52]

Also, if all done, we have to remove the code in switch[0-9] case in /etc/init.d/led

[ptpt52]

@feckert great test good with your latest changes

but there is something else to do and now we have to look at here above

[feckert]

Also, if all done, we have to remove the code in switch[0-9] case in /etc/init.d/led

Of course, that would not be bad, but I think that the upstream is not accepted, because there is configuration in the field where the old one exists. The led-trigger would have to be installed with the kernel modules in the openwrt repository where the swich led-trigger is compiled.

@jow- would have to say something about whether this is possible. The same applies to the luci-app-ledtrig-ath10k https://github.com/openwrt/luci/pull/5727