diff mbox series

[OpenWrt-Devel] lantiq: add dsl line_state mapping

Message ID 20200616082613.892-1-fe@dev.tdt.de
State RFC
Delegated to: John Crispin
Headers show
Series [OpenWrt-Devel] lantiq: add dsl line_state mapping | expand

Commit Message

Florian Eckert June 16, 2020, 8:26 a.m. UTC
The line_state of the DSL connection is described in the system via a
hexadecimal variable. With this change the hexadecimal is mapped to a decimal
value. With this change it is now possible to store this value in a database,
so that it can be easily evaluated.

This is especially relevant for the collectd and gravana backend.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 .../base-files/lib/functions/lantiq_dsl.sh    | 82 +++++++++++++------
 1 file changed, 55 insertions(+), 27 deletions(-)

Comments

Adrian Schmutzler June 16, 2020, 11:27 a.m. UTC | #1
Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Florian Eckert
> Sent: Dienstag, 16. Juni 2020 10:26
> To: john@phrozen.org; dev@kresin.me; Eckert.Florian@googlemail.com
> Cc: openwrt-devel@lists.openwrt.org; Florian Eckert <fe@dev.tdt.de>
> Subject: [OpenWrt-Devel] [PATCH] lantiq: add dsl line_state mapping
> 
> The line_state of the DSL connection is described in the system via a
> hexadecimal variable. With this change the hexadecimal is mapped to a
> decimal value. With this change it is now possible to store this value in a
> database, so that it can be easily evaluated.
> 
> This is especially relevant for the collectd and gravana backend.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  .../base-files/lib/functions/lantiq_dsl.sh    | 82 +++++++++++++------
>  1 file changed, 55 insertions(+), 27 deletions(-)
> 
> diff --git a/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
> b/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
> index 11b02fc4aa..4827d10bc5 100755
> --- a/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
> +++ b/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
> @@ -650,40 +650,68 @@ line_data() {
>  line_state() {
>  	local lsg=$(dsl_cmd lsg)
>  	local ls=$(dsl_val "$lsg" nLineState);
> -	local s;
> +	local s n;
> 
>  	case "$ls" in
> -		"0x0")		s="not initialized" ;;
> -		"0x1")		s="exception" ;;
> -		"0x10")		s="not updated" ;;
> -		"0xff")		s="idle request" ;;
> -		"0x100")	s="idle" ;;
> -		"0x1ff")	s="silent request" ;;
> -		"0x200")	s="silent" ;;
> -		"0x300")	s="handshake" ;;
> -		"0x380")	s="full_init" ;;
> -		"0x400")	s="discovery" ;;
> -		"0x500")	s="training" ;;
> -		"0x600")	s="analysis" ;;
> -		"0x700")	s="exchange" ;;
> -		"0x800")	s="showtime_no_sync" ;;
> -		"0x801")	s="showtime_tc_sync" ;;
> -		"0x900")	s="fastretrain" ;;
> -		"0xa00")	s="lowpower_l2" ;;
> -		"0xb00")	s="loopdiagnostic active" ;;
> -		"0xb10")	s="loopdiagnostic data exchange" ;;
> -		"0xb20")	s="loopdiagnostic data request" ;;
> -		"0xc00")	s="loopdiagnostic complete" ;;
> -		"0x1000000")	s="test" ;;
> -		"0xd00")	s="resync" ;;
> -		"0x3c0")	s="short init entry" ;;
> -		"")		s="not running daemon"; ls="0xfff" ;;
> -		*)		s="unknown" ;;
> +		"0x0")		s="not initialized"
> +				n=1 ;;

Wouldn't it be more user-friendly to just use the decimal number equivalent of the hex code?
Empty and error could be modelled with negative numbers then.

Just meant as a suggestion, I won't block this if you keep the current scheme ...

Best

Adrian

> +		"0x1")		s="exception"
> +				n=2 ;;
> +		"0x10")		s="not updated"
> +				n=3 ;;
> +		"0xff")		s="idle request"
> +				n=4 ;;
> +		"0x100")	s="idle"
> +				n=5 ;;
> +		"0x1ff")	s="silent request"
> +				n=6 ;;
> +		"0x200")	s="silent"
> +				n=7 ;;
> +		"0x300")	s="handshake"
> +				n=8 ;;
> +		"0x380")	s="full_init"
> +				n=9 ;;
> +		"0x400")	s="discovery"
> +				n=10 ;;
> +		"0x500")	s="training"
> +				n=11 ;;
> +		"0x600")	s="analysis"
> +				n=12 ;;
> +		"0x700")	s="exchange"
> +				n=13 ;;
> +		"0x800")	s="showtime_no_sync"
> +				n=14 ;;
> +		"0x801")	s="showtime_tc_sync"
> +				n=15 ;;
> +		"0x900")	s="fastretrain"
> +				n=16 ;;
> +		"0xa00")	s="lowpower_l2"
> +				n=17 ;;
> +		"0xb00")	s="loopdiagnostic active"
> +				n=18 ;;
> +		"0xb10")	s="loopdiagnostic data exchange"
> +				n=19 ;;
> +		"0xb20")	s="loopdiagnostic data request"
> +				n=20 ;;
> +		"0xc00")	s="loopdiagnostic complete"
> +				n=21 ;;
> +		"0x1000000")	s="test"
> +				n=22 ;;
> +		"0xd00")	s="resync"
> +				n=23 ;;
> +		"0x3c0")	s="short init entry"
> +				n=24 ;;
> +		"")		s="not running daemon"
> +				ls="0xfff"
> +				n=25 ;;
> +		*)		s="unknown"
> +				n=26 ;;
>  	esac
> 
>  	if [ "$action" = "lucistat" ]; then
>  		echo "dsl.line_state_num=$ls"
>  		echo "dsl.line_state_detail=\"$s\""
> +		echo "dsl.line_state_mapping=$n"
>  		if [ "$ls" = "0x801" ]; then
>  			echo "dsl.line_state=\"UP\""
>  		else
> --
> 2.20.1
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Florian Eckert June 16, 2020, 12:18 p.m. UTC | #2
Hi Adrian,

thanks for your comment

>> -		"0xd00")	s="resync" ;;
>> -		"0x3c0")	s="short init entry" ;;
>> -		"")		s="not running daemon"; ls="0xfff" ;;
>> -		*)		s="unknown" ;;
>> +		"0x0")		s="not initialized"
>> +				n=1 ;;
> 
> Wouldn't it be more user-friendly to just use the decimal number
> equivalent of the hex code?

I've been thinking the same thing.
But then I have a problem when I want to display the values over time 
(collectd/gravana).
 From my point of view the distances are then too small or too large it 
does not give equal gradations.

>> -		"0x1000000")	s="test" ;;

Especially the test value jumps out of line there. I think this is 
unlikely and could be neglected,
but I would like to have a linear gradation.


>> -		"")		s="not running daemon"; ls="0xfff" ;;
>> -		*)		s="unknown" ;;
> Empty and error could be modelled with negative numbers then.

We can still do that. So we have for "" and * the value -1 and -2.

Best regards

Florian
Adrian Schmutzler June 16, 2020, 3:42 p.m. UTC | #3
> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Florian Eckert
> Sent: Dienstag, 16. Juni 2020 10:26
> To: john@phrozen.org; dev@kresin.me; Eckert.Florian@googlemail.com
> Cc: openwrt-devel@lists.openwrt.org; Florian Eckert <fe@dev.tdt.de>
> Subject: [OpenWrt-Devel] [PATCH] lantiq: add dsl line_state mapping
> 
> The line_state of the DSL connection is described in the system via a
> hexadecimal variable. With this change the hexadecimal is mapped to a
> decimal value. With this change it is now possible to store this value in a
> database, so that it can be easily evaluated.

Interesting file this lantiq_dsl.sh ...

I'm wondering whether all of this really need to be in this file, or whether stuff can be moved to the package actually dealing with it?
This might also make it easier to change it when necessary.

Adrian

> 
> This is especially relevant for the collectd and gravana backend.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  .../base-files/lib/functions/lantiq_dsl.sh    | 82 +++++++++++++------
>  1 file changed, 55 insertions(+), 27 deletions(-)
> 
> diff --git a/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
> b/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
> index 11b02fc4aa..4827d10bc5 100755
> --- a/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
> +++ b/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
> @@ -650,40 +650,68 @@ line_data() {
>  line_state() {
>  	local lsg=$(dsl_cmd lsg)
>  	local ls=$(dsl_val "$lsg" nLineState);
> -	local s;
> +	local s n;
> 
>  	case "$ls" in
> -		"0x0")		s="not initialized" ;;
> -		"0x1")		s="exception" ;;
> -		"0x10")		s="not updated" ;;
> -		"0xff")		s="idle request" ;;
> -		"0x100")	s="idle" ;;
> -		"0x1ff")	s="silent request" ;;
> -		"0x200")	s="silent" ;;
> -		"0x300")	s="handshake" ;;
> -		"0x380")	s="full_init" ;;
> -		"0x400")	s="discovery" ;;
> -		"0x500")	s="training" ;;
> -		"0x600")	s="analysis" ;;
> -		"0x700")	s="exchange" ;;
> -		"0x800")	s="showtime_no_sync" ;;
> -		"0x801")	s="showtime_tc_sync" ;;
> -		"0x900")	s="fastretrain" ;;
> -		"0xa00")	s="lowpower_l2" ;;
> -		"0xb00")	s="loopdiagnostic active" ;;
> -		"0xb10")	s="loopdiagnostic data exchange" ;;
> -		"0xb20")	s="loopdiagnostic data request" ;;
> -		"0xc00")	s="loopdiagnostic complete" ;;
> -		"0x1000000")	s="test" ;;
> -		"0xd00")	s="resync" ;;
> -		"0x3c0")	s="short init entry" ;;
> -		"")		s="not running daemon"; ls="0xfff" ;;
> -		*)		s="unknown" ;;
> +		"0x0")		s="not initialized"
> +				n=1 ;;
> +		"0x1")		s="exception"
> +				n=2 ;;
> +		"0x10")		s="not updated"
> +				n=3 ;;
> +		"0xff")		s="idle request"
> +				n=4 ;;
> +		"0x100")	s="idle"
> +				n=5 ;;
> +		"0x1ff")	s="silent request"
> +				n=6 ;;
> +		"0x200")	s="silent"
> +				n=7 ;;
> +		"0x300")	s="handshake"
> +				n=8 ;;
> +		"0x380")	s="full_init"
> +				n=9 ;;
> +		"0x400")	s="discovery"
> +				n=10 ;;
> +		"0x500")	s="training"
> +				n=11 ;;
> +		"0x600")	s="analysis"
> +				n=12 ;;
> +		"0x700")	s="exchange"
> +				n=13 ;;
> +		"0x800")	s="showtime_no_sync"
> +				n=14 ;;
> +		"0x801")	s="showtime_tc_sync"
> +				n=15 ;;
> +		"0x900")	s="fastretrain"
> +				n=16 ;;
> +		"0xa00")	s="lowpower_l2"
> +				n=17 ;;
> +		"0xb00")	s="loopdiagnostic active"
> +				n=18 ;;
> +		"0xb10")	s="loopdiagnostic data exchange"
> +				n=19 ;;
> +		"0xb20")	s="loopdiagnostic data request"
> +				n=20 ;;
> +		"0xc00")	s="loopdiagnostic complete"
> +				n=21 ;;
> +		"0x1000000")	s="test"
> +				n=22 ;;
> +		"0xd00")	s="resync"
> +				n=23 ;;
> +		"0x3c0")	s="short init entry"
> +				n=24 ;;
> +		"")		s="not running daemon"
> +				ls="0xfff"
> +				n=25 ;;
> +		*)		s="unknown"
> +				n=26 ;;
>  	esac
> 
>  	if [ "$action" = "lucistat" ]; then
>  		echo "dsl.line_state_num=$ls"
>  		echo "dsl.line_state_detail=\"$s\""
> +		echo "dsl.line_state_mapping=$n"
>  		if [ "$ls" = "0x801" ]; then
>  			echo "dsl.line_state=\"UP\""
>  		else
> --
> 2.20.1
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Florian Eckert June 17, 2020, 10:21 a.m. UTC | #4
Hi Adrian,

>> The line_state of the DSL connection is described in the system via a
>> hexadecimal variable. With this change the hexadecimal is mapped to a
>> decimal value. With this change it is now possible to store this value 
>> in a
>> database, so that it can be easily evaluated.
> 
> Interesting file this lantiq_dsl.sh ...
> 

That´s probably right!

> I'm wondering whether all of this really need to be in this file, or
> whether stuff can be moved to the package actually dealing with it?
> This might also make it easier to change it when necessary.
> 

This file is sourced twice:
- dsl_control of package ltq-adsl-app [1]
- dsl_control of package ltq-vdsl-app [2]

If we take this from the target folder then we have to make our own 
packet ltq-dsl-common for example.
And the packages ltq-adsl-app and ltq-vdsl-app could depend on this.

When we create a new package, we may also want to move other files from 
the target directory to the new package?

- lantiq.sh [3] This is sourced in 02_network files on the lantiq 
targets.
- led_dsl.sh [4]
- pppoa.sh [5]
- uci-defaults [6]
- dsl_notify.sh [7]

These are candidates that could also moved to the new package

Best regards

Florian

[1] 
https://github.com/openwrt/openwrt/blob/master/package/network/config/ltq-adsl-app/files/dsl_control#L11
[2] 
https://github.com/openwrt/openwrt/blob/master/package/network/config/ltq-vdsl-app/files/dsl_control#L11
[3] 
https://github.com/openwrt/openwrt/blob/master/target/linux/lantiq/base-files/lib/functions/lantiq.sh
[4] 
https://github.com/openwrt/openwrt/blob/master/target/linux/lantiq/base-files/etc/hotplug.d/dsl/led_dsl.sh
[5] 
https://github.com/openwrt/openwrt/blob/master/target/linux/lantiq/base-files/etc/hotplug.d/dsl/pppoa.sh
[6] 
https://github.com/openwrt/openwrt/tree/master/target/linux/lantiq/base-files/etc/uci-defaults
[7] 
https://github.com/openwrt/openwrt/blob/master/target/linux/lantiq/base-files/sbin/dsl_notify.sh
Sebastian Moeller June 17, 2020, 10:36 a.m. UTC | #5
Hi Florian,


> On Jun 16, 2020, at 14:18, Florian Eckert <fe@dev.tdt.de> wrote:
> 
> Hi Adrian,
> 
> thanks for your comment
> 
>>> -		"0xd00")	s="resync" ;;
>>> -		"0x3c0")	s="short init entry" ;;
>>> -		"")		s="not running daemon"; ls="0xfff" ;;
>>> -		*)		s="unknown" ;;
>>> +		"0x0")		s="not initialized"
>>> +				n=1 ;;
>> Wouldn't it be more user-friendly to just use the decimal number
>> equivalent of the hex code?
> 
> I've been thinking the same thing.
> But then I have a problem when I want to display the values over time (collectd/gravana).
> From my point of view the distances are then too small or too large it does not give equal gradations.

	I guess lantiq (or rather maxlinear nowadays) might introduce new hex codes into existing gaps which will cause a distruption of the nice monotonic order oc the enumerating decimals you went for. Is there no other data type in graphana that is ordered yet not scaled?

Best Regards
	Sebastian


> 
>>> -		"0x1000000")	s="test" ;;
> 
> Especially the test value jumps out of line there. I think this is unlikely and could be neglected,
> but I would like to have a linear gradation.
> 
> 
>>> -		"")		s="not running daemon"; ls="0xfff" ;;
>>> -		*)		s="unknown" ;;
>> Empty and error could be modelled with negative numbers then.
> 
> We can still do that. So we have for "" and * the value -1 and -2.
> 
> Best regards
> 
> Florian
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Martin Schiller June 18, 2020, 5:03 a.m. UTC | #6
On 2020-06-17 12:21, Florian Eckert wrote:
> Hi Adrian,
> 
>>> The line_state of the DSL connection is described in the system via a
>>> hexadecimal variable. With this change the hexadecimal is mapped to a
>>> decimal value. With this change it is now possible to store this 
>>> value in a
>>> database, so that it can be easily evaluated.
>> 
>> Interesting file this lantiq_dsl.sh ...
>> 
> 
> That´s probably right!
> 
>> I'm wondering whether all of this really need to be in this file, or
>> whether stuff can be moved to the package actually dealing with it?
>> This might also make it easier to change it when necessary.
>> 
> 
> This file is sourced twice:
> - dsl_control of package ltq-adsl-app [1]
> - dsl_control of package ltq-vdsl-app [2]
> 
> If we take this from the target folder then we have to make our own
> packet ltq-dsl-common for example.
> And the packages ltq-adsl-app and ltq-vdsl-app could depend on this.
> 
> When we create a new package, we may also want to move other files
> from the target directory to the new package?
> 
> - lantiq.sh [3] This is sourced in 02_network files on the lantiq 
> targets.
> - led_dsl.sh [4]
> - pppoa.sh [5]
> - uci-defaults [6]
> - dsl_notify.sh [7]
> 
> These are candidates that could also moved to the new package

That's what I've already done in my working tree for VRX518 support on
FB7530, to get this scripts also available on other targets than lantiq:

https://github.com/3headeddevs/openwrt/commit/9f45c750f91eea230dc638c0936fb6e761214abb

> 
> Best regards
> 
> Florian
> 
> [1]
> https://github.com/openwrt/openwrt/blob/master/package/network/config/ltq-adsl-app/files/dsl_control#L11
> [2]
> https://github.com/openwrt/openwrt/blob/master/package/network/config/ltq-vdsl-app/files/dsl_control#L11
> [3]
> https://github.com/openwrt/openwrt/blob/master/target/linux/lantiq/base-files/lib/functions/lantiq.sh
> [4]
> https://github.com/openwrt/openwrt/blob/master/target/linux/lantiq/base-files/etc/hotplug.d/dsl/led_dsl.sh
> [5]
> https://github.com/openwrt/openwrt/blob/master/target/linux/lantiq/base-files/etc/hotplug.d/dsl/pppoa.sh
> [6]
> https://github.com/openwrt/openwrt/tree/master/target/linux/lantiq/base-files/etc/uci-defaults
> [7]
> https://github.com/openwrt/openwrt/blob/master/target/linux/lantiq/base-files/sbin/dsl_notify.sh
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff mbox series

Patch

diff --git a/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh b/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
index 11b02fc4aa..4827d10bc5 100755
--- a/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
+++ b/target/linux/lantiq/base-files/lib/functions/lantiq_dsl.sh
@@ -650,40 +650,68 @@  line_data() {
 line_state() {
 	local lsg=$(dsl_cmd lsg)
 	local ls=$(dsl_val "$lsg" nLineState);
-	local s;
+	local s n;
 
 	case "$ls" in
-		"0x0")		s="not initialized" ;;
-		"0x1")		s="exception" ;;
-		"0x10")		s="not updated" ;;
-		"0xff")		s="idle request" ;;
-		"0x100")	s="idle" ;;
-		"0x1ff")	s="silent request" ;;
-		"0x200")	s="silent" ;;
-		"0x300")	s="handshake" ;;
-		"0x380")	s="full_init" ;;
-		"0x400")	s="discovery" ;;
-		"0x500")	s="training" ;;
-		"0x600")	s="analysis" ;;
-		"0x700")	s="exchange" ;;
-		"0x800")	s="showtime_no_sync" ;;
-		"0x801")	s="showtime_tc_sync" ;;
-		"0x900")	s="fastretrain" ;;
-		"0xa00")	s="lowpower_l2" ;;
-		"0xb00")	s="loopdiagnostic active" ;;
-		"0xb10")	s="loopdiagnostic data exchange" ;;
-		"0xb20")	s="loopdiagnostic data request" ;;
-		"0xc00")	s="loopdiagnostic complete" ;;
-		"0x1000000")	s="test" ;;
-		"0xd00")	s="resync" ;;
-		"0x3c0")	s="short init entry" ;;
-		"")		s="not running daemon"; ls="0xfff" ;;
-		*)		s="unknown" ;;
+		"0x0")		s="not initialized"
+				n=1 ;;
+		"0x1")		s="exception"
+				n=2 ;;
+		"0x10")		s="not updated"
+				n=3 ;;
+		"0xff")		s="idle request"
+				n=4 ;;
+		"0x100")	s="idle"
+				n=5 ;;
+		"0x1ff")	s="silent request"
+				n=6 ;;
+		"0x200")	s="silent"
+				n=7 ;;
+		"0x300")	s="handshake"
+				n=8 ;;
+		"0x380")	s="full_init"
+				n=9 ;;
+		"0x400")	s="discovery"
+				n=10 ;;
+		"0x500")	s="training"
+				n=11 ;;
+		"0x600")	s="analysis"
+				n=12 ;;
+		"0x700")	s="exchange"
+				n=13 ;;
+		"0x800")	s="showtime_no_sync"
+				n=14 ;;
+		"0x801")	s="showtime_tc_sync"
+				n=15 ;;
+		"0x900")	s="fastretrain"
+				n=16 ;;
+		"0xa00")	s="lowpower_l2"
+				n=17 ;;
+		"0xb00")	s="loopdiagnostic active"
+				n=18 ;;
+		"0xb10")	s="loopdiagnostic data exchange"
+				n=19 ;;
+		"0xb20")	s="loopdiagnostic data request"
+				n=20 ;;
+		"0xc00")	s="loopdiagnostic complete"
+				n=21 ;;
+		"0x1000000")	s="test"
+				n=22 ;;
+		"0xd00")	s="resync"
+				n=23 ;;
+		"0x3c0")	s="short init entry"
+				n=24 ;;
+		"")		s="not running daemon"
+				ls="0xfff"
+				n=25 ;;
+		*)		s="unknown"
+				n=26 ;;
 	esac
 
 	if [ "$action" = "lucistat" ]; then
 		echo "dsl.line_state_num=$ls"
 		echo "dsl.line_state_detail=\"$s\""
+		echo "dsl.line_state_mapping=$n"
 		if [ "$ls" = "0x801" ]; then
 			echo "dsl.line_state=\"UP\""
 		else