Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger

[Rafał Miłecki]

On 13 October 2016 at 09:44, Rafał Miłecki  wrote:
> From: Rafał Miłecki 
>
> This makes init.d script handle existing UCI entries using the new
> trigger. It also switches all targets to use its package.
>
> Signed-off-by: Rafał Miłecki 

Pushed with code moved out of switch for cleaner code sharing with
usbport trigger. Also added comments to make it clear what sed
commands are for.

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger

[Mathias Kresin]

2016-10-17 12:10 GMT+02:00 Rafal Milecki :
> On 10/16/2016 03:12 PM, Mathias Kresin wrote:
>>
>> 13.10.2016 09:44, Rafał Miłecki:
>>>
>>> diff --git a/package/base-files/files/etc/init.d/led
>>> b/package/base-files/files/etc/init.d/led
>>> index 79f2904..507dcbf 100755
>>> --- a/package/base-files/files/etc/init.d/led
>>> +++ b/package/base-files/files/etc/init.d/led
>>> @@ -47,6 +47,8 @@ load_led() {
>>>  echo 0 >/sys/class/leds/${sysfs}/brightness
>>>
>>>  echo $trigger > /sys/class/leds/${sysfs}/trigger 2> /dev/null
>>> +# Backward compatibility
>>> +[ $trigger = "usbdev" ] && echo usbport >
>>> /sys/class/leds/${sysfs}/trigger 2> /dev/null
>>>  ret="$?"
>>>
>>>  [ $default = 1 ] &&
>>> @@ -72,9 +74,14 @@ load_led() {
>>>  ;;
>>>
>>>  "usbdev")
>>> -[ -n "$dev" ] && {
>>> -echo $dev > /sys/class/leds/${sysfs}/device_name
>>> -echo $interval >
>>> /sys/class/leds/${sysfs}/activity_interval
>>> +local usbport
>>> +
>>> +# Translate USB dev/port format of the old usbdev trigger
>>> +usbport=$(echo "$dev" | sed -n
>>> 's/^\([0-9]*\)-\([0-9]*\)$/usb\1-port\2/p')
>>> +[ -z "$usbport" ] && usbport=$(echo "$dev" | sed -n
>>> 's/\./-port/p')
>>
>>
>> I'm not sure if I got the purpose of this sed call correctly. As far as I
>> can see, it should fixup usb ports defined as "usb1.1". Via a quick grep I
>> couldn't find anything like that used.
>
>
> This second sed call is needed for ports of (internal) hubs. If you grep
> targets
> for ucidef_set_led_usbdev you can find e.g.
> ucidef_set_led_usbdev "usb" "USB" "arduino:white:usb" "1-1.1"
> ucidef_set_led_usbdev "usb1" "USB1" "tp-link:green:usb1" "1-1.1"
> ucidef_set_led_usbdev "usb2" "USB2" "tp-link:green:usb2" "1-1.2"
>
> So e.g. 2-1 has to be translated into usb2-port1
> While 1-1.1 has to be translated into 1-1-port1

Thanks for the explanation.

>
> This is what I meant when I wrote:
>> Now after coming back home I also tried it with extra hub and:
>> ucidef_set_led_usbdev "foo" "FOO" "bcm53xx:red:wan" "2-2.4"
>
>
>> I rather would suggest to add a line which allows to use the new syntax
>> instead:
>>
>> [ -z "$usbport" ] && usbport="${dev}"
>
>
> For using new trigger and its syntax directly, I mean to add a new call like
> ucidef_set_led_usbport. It should also support specifying more than 1 USB
> port.
> You can find my initial work on it in:
> https://patchwork.ozlabs.org/patch/678014/

Ah okay. I was fooled by the fact that you old patch is marked as
superseded in patchwork. I was the opinion that this patch is the one
that supersedes the old one.

Personally, I prefer to keep ucidef_set_led_usbdev() and to add the
suggested changes. This way we don't need to touch all the
board.d/leds files, possibly existing documentation remains valid and
boards added in custom trees doesn't need an update as well. It would
be possible to add a ucidef_set_led_usbport() function without
removing ucidef_set_led_usbdev() to workaround this. But I'm not that
addicted of having two function doing basically the same.

But I guess it's just a matter of taste.

Mathias

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger

[Rafal Milecki]

On 10/16/2016 03:12 PM, Mathias Kresin wrote:

13.10.2016 09:44, Rafał Miłecki:

diff --git a/package/base-files/files/etc/init.d/led 
b/package/base-files/files/etc/init.d/led
index 79f2904..507dcbf 100755
--- a/package/base-files/files/etc/init.d/led
+++ b/package/base-files/files/etc/init.d/led
@@ -47,6 +47,8 @@ load_led() {
 echo 0 >/sys/class/leds/${sysfs}/brightness

 echo $trigger > /sys/class/leds/${sysfs}/trigger 2> /dev/null
+# Backward compatibility
+[ $trigger = "usbdev" ] && echo usbport > 
/sys/class/leds/${sysfs}/trigger 2> /dev/null
 ret="$?"

 [ $default = 1 ] &&
@@ -72,9 +74,14 @@ load_led() {
 ;;

 "usbdev")
-[ -n "$dev" ] && {
-echo $dev > /sys/class/leds/${sysfs}/device_name
-echo $interval > /sys/class/leds/${sysfs}/activity_interval
+local usbport
+
+# Translate USB dev/port format of the old usbdev trigger
+usbport=$(echo "$dev" | sed -n 
's/^\([0-9]*\)-\([0-9]*\)$/usb\1-port\2/p')
+[ -z "$usbport" ] && usbport=$(echo "$dev" | sed -n 's/\./-port/p')


I'm not sure if I got the purpose of this sed call correctly. As far as I can see, it 
should fixup usb ports defined as "usb1.1". Via a quick grep I couldn't find 
anything like that used.


This second sed call is needed for ports of (internal) hubs. If you grep targets
for ucidef_set_led_usbdev you can find e.g.
ucidef_set_led_usbdev "usb" "USB" "arduino:white:usb" "1-1.1"
ucidef_set_led_usbdev "usb1" "USB1" "tp-link:green:usb1" "1-1.1"
ucidef_set_led_usbdev "usb2" "USB2" "tp-link:green:usb2" "1-1.2"

So e.g. 2-1 has to be translated into usb2-port1
While 1-1.1 has to be translated into 1-1-port1

This is what I meant when I wrote:
> Now after coming back home I also tried it with extra hub and:
> ucidef_set_led_usbdev "foo" "FOO" "bcm53xx:red:wan" "2-2.4"



I rather would suggest to add a line which allows to use the new syntax instead:

[ -z "$usbport" ] && usbport="${dev}"


For using new trigger and its syntax directly, I mean to add a new call like
ucidef_set_led_usbport. It should also support specifying more than 1 USB port.
You can find my initial work on it in:
https://patchwork.ozlabs.org/patch/678014/



+
+[ -n "$usbport" ] && {
+echo 1 > /sys/class/leds/${sysfs}/ports/$usbport
 }


What about adding something that allows to add all or multiple usb ports (using 
the new syntax of course) to a single LED (which is thanks to your usbport 
trigger now possible):

[ "$usbport" = "*" ] && usbport=$(ls /sys/class/leds/${sysfs}/ports/)

for port in ${usbport}; do
echo 1 > "/sys/class/leds/${sysfs}/ports/${port}"
done


I'm OK with that, it sounds like a good idea! I'd just rather wait for a new
call implementation (ucidef_set_led_usbport?) instead adding more features to
this old one.

If there won't be any complains about this patchset, I'll push it soon and then
I'll try to add support for specifying multiple ports. I'd like to see your
patch then!

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

Re: [LEDE-DEV] [PATCH V2 2/3] switch to the new usbport LED trigger

[Rafał Miłecki]

On 13 October 2016 at 09:44, Rafał Miłecki  wrote:
> From: Rafał Miłecki 
>
> This makes init.d script handle existing UCI entries using the new
> trigger. It also switches all targets to use its package.
>
> Signed-off-by: Rafał Miłecki 

Before sending this patchset I tested it with some simple:
ucidef_set_led_usbdev "foo" "FOO" "bcm53xx:red:wan" "2-1"

Now after coming back home I also tried it with extra hub and:
ucidef_set_led_usbdev "foo" "FOO" "bcm53xx:red:wan" "2-2.4"

Both ports were translated correctly and worked with usbdev as well as
with the usbport. This lets me hope there won't be any regressions
caused by this.

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev