| Message ID | 20200131142254.24953-1-pepe2k@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Piotr Dymacz |
| Headers | show |
| Series | [OpenWrt-Devel] base-files: diag: restore default trigger for 'boot' LED | expand |
Hi, > -----Original Message----- > From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] On > Behalf Of Piotr Dymacz > Sent: Freitag, 31. Januar 2020 15:23 > To: openwrt-devel@lists.openwrt.org > Subject: [OpenWrt-Devel] [PATCH] base-files: diag: restore default trigger for > 'boot' LED > > For devices without a dedicated 'diag' LED, we use sometimes one of > other LEDs for indicating at least 'boot', 'failsafe' and 'upgrade' > stages. In some cases, at the same time these LEDs have defined default > triggers in DTS using 'linux,default-trigger' property. Current 'diag' > setup removes the trigger and turns off 'boot' LED after bootup. > > One of the examples of such device is TP-Link TL-WR841N v14 (ramips) > which uses 'wlan' LED with defined 'linux,default-trigger' for 'diag': > > aliases { > led-boot = &led_wlan; > led-failsafe = &led_wlan; > led-upgrade = &led_wlan; > }; > > [...] > > led_wlan: wlan { > label = "tl-wr841n-v14:green:wlan"; > gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; > linux,default-trigger = "phy0tpt"; > }; > > This patch extends 'diag.sh' and 'leds.sh' scripts to make sure default > trigger defined in DTS is restored for 'diag' LED which isn't used for > indicating 'running' stage. I'm not really a fan of using LEDs for diag in that case at all, and I'd actually prefer to have the aliases removed there (unless vendor also used multiple purpose LEDs the same way). But since we have these case in our code right now, I think this workaround is necessary ATM. Acked-by: Adrian Schmutzler <freifunk@adrianschmutzler.de> Best Adrian > > Signed-off-by: Piotr Dymacz <pepe2k@gmail.com> > --- > package/base-files/Makefile | 2 +- > package/base-files/files/etc/diag.sh | 2 ++ > .../base-files/files/lib/functions/leds.sh | 27 ++++++++++++++++--- > 3 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/package/base-files/Makefile b/package/base-files/Makefile > index e389148d47..18325564dc 100644 > --- a/package/base-files/Makefile > +++ b/package/base-files/Makefile > @@ -12,7 +12,7 @@ include $(INCLUDE_DIR)/version.mk > include $(INCLUDE_DIR)/feeds.mk > > PKG_NAME:=base-files > -PKG_RELEASE:=213 > +PKG_RELEASE:=214 > PKG_FLAGS:=nonshared > > PKG_FILE_DEPENDS:=$(PLATFORM_DIR)/ $(GENERIC_PLATFORM_DIR)/base- > files/ > diff --git a/package/base-files/files/etc/diag.sh b/package/base- > files/files/etc/diag.sh > index 8eb36c6feb..37a8ec758e 100644 > --- a/package/base-files/files/etc/diag.sh > +++ b/package/base-files/files/etc/diag.sh > @@ -37,6 +37,8 @@ set_led_state() { > ;; > done) > status_led_off > + [ "$status_led" != "$running" ] && \ > + status_led_restore_trigger "boot" > [ -n "$running" ] && { > status_led="$running" > status_led_on > diff --git a/package/base-files/files/lib/functions/leds.sh b/package/base- > files/files/lib/functions/leds.sh > index 8a1d21caef..43b2fe02ed 100644 > --- a/package/base-files/files/lib/functions/leds.sh > +++ b/package/base-files/files/lib/functions/leds.sh > @@ -1,16 +1,24 @@ > #!/bin/sh > # Copyright (C) 2013 OpenWrt.org > > -get_dt_led() { > - local label > +get_dt_led_path() { > local ledpath > local basepath="/proc/device-tree" > local nodepath="$basepath/aliases/led-$1" > > [ -f "$nodepath" ] && ledpath=$(cat "$nodepath") > + [ -n "$ledpath" ] && ledpath="$basepath$ledpath" > + > + echo "$ledpath" > +} > + > +get_dt_led() { > + local label > + local ledpath=$(get_dt_led_path $1) > + > [ -n "$ledpath" ] && \ > - label=$(cat "$basepath$ledpath/label" 2>/dev/null) || \ > - label=$(cat "$basepath$ledpath/chan-name" 2>/dev/null) > + label=$(cat "$ledpath/label" 2>/dev/null) || \ > + label=$(cat "$ledpath/chan-name" 2>/dev/null) > > echo "$label" > } > @@ -35,6 +43,17 @@ led_off() { > led_set_attr $1 "brightness" 0 > } > > +status_led_restore_trigger() { > + local trigger > + local ledpath=$(get_dt_led_path $1) > + > + [ -n "$ledpath" ] && \ > + trigger=$(cat "$ledpath/linux,default-trigger" 2>/dev/null) > + > + [ -n "$trigger" ] && \ > + led_set_attr "$(get_dt_led $1)" "trigger" "$trigger" > +} > + > status_led_set_timer() { > led_timer $status_led "$1" "$2" > [ -n "$status_led2" ] && led_timer $status_led2 "$1" "$2" > -- > 2.20.1 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Hi Adrian, On 04.02.2020 13:32, Adrian Schmutzler wrote: > Hi, > >> -----Original Message----- >> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] On >> Behalf Of Piotr Dymacz >> Sent: Freitag, 31. Januar 2020 15:23 >> To: openwrt-devel@lists.openwrt.org >> Subject: [OpenWrt-Devel] [PATCH] base-files: diag: restore default trigger for >> 'boot' LED >> >> For devices without a dedicated 'diag' LED, we use sometimes one of >> other LEDs for indicating at least 'boot', 'failsafe' and 'upgrade' >> stages. In some cases, at the same time these LEDs have defined default >> triggers in DTS using 'linux,default-trigger' property. Current 'diag' >> setup removes the trigger and turns off 'boot' LED after bootup. >> >> One of the examples of such device is TP-Link TL-WR841N v14 (ramips) >> which uses 'wlan' LED with defined 'linux,default-trigger' for 'diag': >> >> aliases { >> led-boot = &led_wlan; >> led-failsafe = &led_wlan; >> led-upgrade = &led_wlan; >> }; >> >> [...] >> >> led_wlan: wlan { >> label = "tl-wr841n-v14:green:wlan"; >> gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; >> linux,default-trigger = "phy0tpt"; >> }; >> >> This patch extends 'diag.sh' and 'leds.sh' scripts to make sure default >> trigger defined in DTS is restored for 'diag' LED which isn't used for >> indicating 'running' stage. > > I'm not really a fan of using LEDs for diag in that case at all, and I'd > actually prefer to have the aliases removed there (unless vendor also used > multiple purpose LEDs the same way). Without any LED defined at least for 'boot' and 'failsafe', users might have problems entering failsafe and knowing whether device boots at all.
On 04/02/20 13:32, Adrian Schmutzler wrote: > Hi, > >> -----Original Message----- >> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org] On >> Behalf Of Piotr Dymacz >> Sent: Freitag, 31. Januar 2020 15:23 >> To: openwrt-devel@lists.openwrt.org >> Subject: [OpenWrt-Devel] [PATCH] base-files: diag: restore default trigger for >> 'boot' LED >> >> For devices without a dedicated 'diag' LED, we use sometimes one of >> other LEDs for indicating at least 'boot', 'failsafe' and 'upgrade' >> stages. In some cases, at the same time these LEDs have defined default >> triggers in DTS using 'linux,default-trigger' property. Current 'diag' >> setup removes the trigger and turns off 'boot' LED after bootup. >> >> One of the examples of such device is TP-Link TL-WR841N v14 (ramips) >> which uses 'wlan' LED with defined 'linux,default-trigger' for 'diag': >> >> aliases { >> led-boot = &led_wlan; >> led-failsafe = &led_wlan; >> led-upgrade = &led_wlan; >> }; >> >> [...] >> >> led_wlan: wlan { >> label = "tl-wr841n-v14:green:wlan"; >> gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; >> linux,default-trigger = "phy0tpt"; >> }; >> >> This patch extends 'diag.sh' and 'leds.sh' scripts to make sure default >> trigger defined in DTS is restored for 'diag' LED which isn't used for >> indicating 'running' stage. > I'm not really a fan of using LEDs for diag in that case at all, and I'd > actually prefer to have the aliases removed there (unless vendor also used > multiple purpose LEDs the same way). > I don't like this either, but I think functionality always wins over esthetics. Showing boot status and the moment to press the reset button to enter failsafe is more important than the "uglyness" of hijacking a LED. Besides, in many cases (wifi led for example) the device won't be using that led during boot anyway. -Alberto
diff --git a/package/base-files/Makefile b/package/base-files/Makefile index e389148d47..18325564dc 100644 --- a/package/base-files/Makefile +++ b/package/base-files/Makefile @@ -12,7 +12,7 @@ include $(INCLUDE_DIR)/version.mk include $(INCLUDE_DIR)/feeds.mk PKG_NAME:=base-files -PKG_RELEASE:=213 +PKG_RELEASE:=214 PKG_FLAGS:=nonshared PKG_FILE_DEPENDS:=$(PLATFORM_DIR)/ $(GENERIC_PLATFORM_DIR)/base-files/ diff --git a/package/base-files/files/etc/diag.sh b/package/base-files/files/etc/diag.sh index 8eb36c6feb..37a8ec758e 100644 --- a/package/base-files/files/etc/diag.sh +++ b/package/base-files/files/etc/diag.sh @@ -37,6 +37,8 @@ set_led_state() { ;; done) status_led_off + [ "$status_led" != "$running" ] && \ + status_led_restore_trigger "boot" [ -n "$running" ] && { status_led="$running" status_led_on diff --git a/package/base-files/files/lib/functions/leds.sh b/package/base-files/files/lib/functions/leds.sh index 8a1d21caef..43b2fe02ed 100644 --- a/package/base-files/files/lib/functions/leds.sh +++ b/package/base-files/files/lib/functions/leds.sh @@ -1,16 +1,24 @@ #!/bin/sh # Copyright (C) 2013 OpenWrt.org -get_dt_led() { - local label +get_dt_led_path() { local ledpath local basepath="/proc/device-tree" local nodepath="$basepath/aliases/led-$1" [ -f "$nodepath" ] && ledpath=$(cat "$nodepath") + [ -n "$ledpath" ] && ledpath="$basepath$ledpath" + + echo "$ledpath" +} + +get_dt_led() { + local label + local ledpath=$(get_dt_led_path $1) + [ -n "$ledpath" ] && \ - label=$(cat "$basepath$ledpath/label" 2>/dev/null) || \ - label=$(cat "$basepath$ledpath/chan-name" 2>/dev/null) + label=$(cat "$ledpath/label" 2>/dev/null) || \ + label=$(cat "$ledpath/chan-name" 2>/dev/null) echo "$label" } @@ -35,6 +43,17 @@ led_off() { led_set_attr $1 "brightness" 0 } +status_led_restore_trigger() { + local trigger + local ledpath=$(get_dt_led_path $1) + + [ -n "$ledpath" ] && \ + trigger=$(cat "$ledpath/linux,default-trigger" 2>/dev/null) + + [ -n "$trigger" ] && \ + led_set_attr "$(get_dt_led $1)" "trigger" "$trigger" +} + status_led_set_timer() { led_timer $status_led "$1" "$2" [ -n "$status_led2" ] && led_timer $status_led2 "$1" "$2"
For devices without a dedicated 'diag' LED, we use sometimes one of other LEDs for indicating at least 'boot', 'failsafe' and 'upgrade' stages. In some cases, at the same time these LEDs have defined default triggers in DTS using 'linux,default-trigger' property. Current 'diag' setup removes the trigger and turns off 'boot' LED after bootup. One of the examples of such device is TP-Link TL-WR841N v14 (ramips) which uses 'wlan' LED with defined 'linux,default-trigger' for 'diag': aliases { led-boot = &led_wlan; led-failsafe = &led_wlan; led-upgrade = &led_wlan; }; [...] led_wlan: wlan { label = "tl-wr841n-v14:green:wlan"; gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; linux,default-trigger = "phy0tpt"; }; This patch extends 'diag.sh' and 'leds.sh' scripts to make sure default trigger defined in DTS is restored for 'diag' LED which isn't used for indicating 'running' stage. Signed-off-by: Piotr Dymacz <pepe2k@gmail.com> --- package/base-files/Makefile | 2 +- package/base-files/files/etc/diag.sh | 2 ++ .../base-files/files/lib/functions/leds.sh | 27 ++++++++++++++++--- 3 files changed, 26 insertions(+), 5 deletions(-)