[RFC wpan-tools] info: add frequency output to channel listing

[RFC wpan-tools] info: add frequency output to channel listing

[Christoffer Holmstedt]

Add the frequency to the channel numbers output when
running "iwpan list". The output now looks like this:

	supported channels:
		page 0:
			[11] 2405 MHz
			[12] 2410 MHz
			[13] 2415 MHz
			[14] 2420 MHz
			[15] 2425 MHz
			[16] 2430 MHz
			[17] 2435 MHz
			[18] 2440 MHz
			[19] 2445 MHz
			[20] 2450 MHz
			[21] 2455 MHz
			[22] 2460 MHz
			[23] 2465 MHz
			[24] 2470 MHz
			[25] 2475 MHz
			[26] 2480 MHz
	current_page: 0
	current_channel: 13 (2415 MHz)

Signed-off-by: Christoffer Holmstedt <christoffer@christofferholmstedt.se>
---
Just added implementation for channel page 0 but wanted to know if I'm on the
right track here. Any feedback concerning visual output and implementation
is appreciated.

 src/info.c |   43 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/src/info.c b/src/info.c
index e8f5dda8da94..86e4dc4e4a14 100644
--- a/src/info.c
+++ b/src/info.c
@@ -23,6 +23,33 @@ static void print_minmax_handler(int min, int max)
 	printf("\b \n");
 }
 
+static void print_freq(int page, int channel)
+{
+	float freq = 0;
+
+	switch (page) {
+	case 0:
+		if (channel == 0) {
+			freq = 868.3;
+			printf("%.1f", freq);
+			break;
+		}
+		else if (channel > 0 && channel < 11) {
+			freq = 906 + 2 * (channel - 1);
+		}
+		else {
+			freq = 2405 + 5 * (channel - 11);
+		}
+		printf("%.0f", freq);
+		break;
+	default:
+		/* Unknown page */
+		break;
+	}
+
+
+}
+
 static int print_phy_handler(struct nl_msg *msg, void *arg)
 {
 	struct nlattr *tb_msg[NL802154_ATTR_MAX + 1];
@@ -54,10 +81,14 @@ static int print_phy_handler(struct nl_msg *msg, void *arg)
 				    rem_page) {
 			channel = nla_get_u32(nl_page);
 			if (channel) {
-				printf("\tpage %d: ", page, channel);
+				printf("\tpage %d:", page, channel);
 				for (i = 0; i <= 31; i++) {
 					if (channel & 0x1)
-						printf("%d,", i);
+					{
+						printf("\n\t\t[%d] ", i);
+						print_freq(page, i);
+						printf(" MHz ");
+					}
 					channel >>= 1;
 				}
 				/* TODO hack use sprintf here */
@@ -71,7 +102,13 @@ static int print_phy_handler(struct nl_msg *msg, void *arg)
 		printf("current_page: %d\n", nla_get_u8(tb_msg[NL802154_ATTR_PAGE]));
 
 	if (tb_msg[NL802154_ATTR_CHANNEL])
-		printf("current_channel: %d\n", nla_get_u8(tb_msg[NL802154_ATTR_CHANNEL]));
+	{
+		unsigned char curr_channel;
+		curr_channel = nla_get_u8(tb_msg[NL802154_ATTR_CHANNEL]);
+		printf("current_channel: %d (", curr_channel);
+		print_freq(nla_get_u8(tb_msg[NL802154_ATTR_PAGE]), curr_channel);
+		printf(" MHz)\n");
+	}
 
 	if (tb_msg[NL802154_ATTR_CCA_MODE]) {
 		cca_mode = nla_get_u32(tb_msg[NL802154_ATTR_CCA_MODE]);
-- 
1.7.10.4


-- 
Christoffer Holmstedt

Re: [RFC wpan-tools] info: add frequency output to channel listing

[Alexander Aring]

Hi,

On Fri, May 29, 2015 at 12:16:03PM +0000, Christoffer Holmstedt wrote:
> Add the frequency to the channel numbers output when
> running "iwpan list". The output now looks like this:
> 
> 	supported channels:
> 		page 0:
> 			[11] 2405 MHz
> 			[12] 2410 MHz
> 			[13] 2415 MHz
> 			[14] 2420 MHz
> 			[15] 2425 MHz
> 			[16] 2430 MHz
> 			[17] 2435 MHz
> 			[18] 2440 MHz
> 			[19] 2445 MHz
> 			[20] 2450 MHz
> 			[21] 2455 MHz
> 			[22] 2460 MHz
> 			[23] 2465 MHz
> 			[24] 2470 MHz
> 			[25] 2475 MHz
> 			[26] 2480 MHz
> 	current_page: 0
> 

I think to have the information which frequency it is - is a very nice
feature for the userspace software. So I of course like to have a
feature like this.

I tested it with the fakelb driver which have a lot of supported
channels according to the page. The channel listing will be 117 lines,
so fakelb is here a special case, but maybe we can do ~3 frequencies per
line sperarated with comma and a tab. After ~3 frequencies then starts a
newline again.

Example:

		page 0:
			[11] 2405 MHz,	[12] 2410 MHz,	[13] 2415 MHz,
			[14] 2420 MHz,	[15] 2425 MHz,	[16] 2430 MHz,
			[17] 2435 MHz,	[18] 2440 MHz,	[19] 2445 MHz,
			....

(normally, it should something which fits in 80 chars terminal width)

don't know if this is the best idea. Or we introduce some verbose output
option. 

> 	current_channel: 13 (2415 MHz)

This is very nice. :-)

Also I detected that I get a lot of:


	page 1:
                [0]  MHz 
                [1]  MHz 
                [2]  MHz 
                [3]  MHz 
                [4]  MHz 
                [5]  MHz 
                [6]  MHz 
                [7]  MHz 
                [8]  MHz 
                [9]  MHz 
                [10]  MHz

with the fakelb driver. I suppose this is only because you implemented
page 0 only. Maybe do a "[0] unknown MHz", here? Just added the wort
"unknown".


Another note is that this should be implemented to the new channel/page
attr style which is part of the phy capabilities [0]. Currently there exists
two implementations to get the supported channel/page attributes. The
one which you implemented is obsolete and exists to deal with old kernel
versions only. We will remove it in the next releases. So please use the
capability output at [0] for channel dump.
(There I did no format string fail. :-))

> Signed-off-by: Christoffer Holmstedt <christoffer@christofferholmstedt.se>
> ---
> Just added implementation for channel page 0 but wanted to know if I'm on the
> right track here. Any feedback concerning visual output and implementation
> is appreciated.
> 
>  src/info.c |   43 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 3 deletions(-)
> 
> diff --git a/src/info.c b/src/info.c
> index e8f5dda8da94..86e4dc4e4a14 100644
> --- a/src/info.c
> +++ b/src/info.c
> @@ -23,6 +23,33 @@ static void print_minmax_handler(int min, int max)
>  	printf("\b \n");
>  }
>  
> +static void print_freq(int page, int channel)
> +{
> +	float freq = 0;
> +
> +	switch (page) {
> +	case 0:
> +		if (channel == 0) {
> +			freq = 868.3;
> +			printf("%.1f", freq);
> +			break;
> +		}
> +		else if (channel > 0 && channel < 11) {
> +			freq = 906 + 2 * (channel - 1);
> +		}
> +		else {
> +			freq = 2405 + 5 * (channel - 11);
> +		}
> +		printf("%.0f", freq);
> +		break;
> +	default:
> +		/* Unknown page */
> +		break;
> +	}
> +
> +
> +}
> +
>  static int print_phy_handler(struct nl_msg *msg, void *arg)
>  {
>  	struct nlattr *tb_msg[NL802154_ATTR_MAX + 1];
> @@ -54,10 +81,14 @@ static int print_phy_handler(struct nl_msg *msg, void *arg)
>  				    rem_page) {
>  			channel = nla_get_u32(nl_page);
>  			if (channel) {
> -				printf("\tpage %d: ", page, channel);

mhh, this looks wrong right now. The format string describes only one
argument, but we have "page, channel" there. There must be page only.
Can you send a fix for this?

- Alex

[0] https://github.com/linux-wpan/wpan-tools/blob/master/src/info.c#L147

Re: [RFC wpan-tools] info: add frequency output to channel listing

[Alexander Aring]

On Fri, May 29, 2015 at 12:16:03PM +0000, Christoffer Holmstedt wrote:
>  
>  	if (tb_msg[NL802154_ATTR_CHANNEL])
> -		printf("current_channel: %d\n", nla_get_u8(tb_msg[NL802154_ATTR_CHANNEL]));
> +	{
> +		unsigned char curr_channel;
> +		curr_channel = nla_get_u8(tb_msg[NL802154_ATTR_CHANNEL]);
> +		printf("current_channel: %d (", curr_channel);
> +		print_freq(nla_get_u8(tb_msg[NL802154_ATTR_PAGE]), curr_channel);

Also this is problematic, because we don't check before if
tb_msg[NL802154_ATTR_PAGE] isn't null.

When you like to use this value there, the easiest way would be:

if (tb_msg[NL802154_ATTR_CHANNEL] && tb_msg[NL802154_ATTR_PAGE])

instead of

if (tb_msg[NL802154_ATTR_CHANNEL]) above.

- Alex

Re: [RFC wpan-tools] info: add frequency output to channel listing

[Christoffer Holmstedt]

On Sun, May 31, 2015 at 02:24:49PM +0200, Alexander Aring wrote:
> On Fri, May 29, 2015 at 12:16:03PM +0000, Christoffer Holmstedt wrote:
> >  
> >  	if (tb_msg[NL802154_ATTR_CHANNEL])
> > -		printf("current_channel: %d\n", nla_get_u8(tb_msg[NL802154_ATTR_CHANNEL]));
> > +	{
> > +		unsigned char curr_channel;
> > +		curr_channel = nla_get_u8(tb_msg[NL802154_ATTR_CHANNEL]);
> > +		printf("current_channel: %d (", curr_channel);
> > +		print_freq(nla_get_u8(tb_msg[NL802154_ATTR_PAGE]), curr_channel);
> 
> Also this is problematic, because we don't check before if
> tb_msg[NL802154_ATTR_PAGE] isn't null.
> 
> When you like to use this value there, the easiest way would be:
> 
> if (tb_msg[NL802154_ATTR_CHANNEL] && tb_msg[NL802154_ATTR_PAGE])
> 
> instead of
> 
> if (tb_msg[NL802154_ATTR_CHANNEL]) above.
> 
> - Alex

Of course, will fix that.

-- 
Christoffer Holmstedt

Re: [RFC wpan-tools] info: add frequency output to channel listing

[Christoffer Holmstedt]

On Sun, May 31, 2015 at 01:37:35PM +0200, Alexander Aring wrote:
> Hi,
> 
> On Fri, May 29, 2015 at 12:16:03PM +0000, Christoffer Holmstedt wrote:
> > Add the frequency to the channel numbers output when
> > running "iwpan list". The output now looks like this:
> > 
> > 	supported channels:
> > 		page 0:
> > 			[11] 2405 MHz
> > 			[12] 2410 MHz
> > 			[13] 2415 MHz
> > 			[14] 2420 MHz
> > 			[15] 2425 MHz
> > 			[16] 2430 MHz
> > 			[17] 2435 MHz
> > 			[18] 2440 MHz
> > 			[19] 2445 MHz
> > 			[20] 2450 MHz
> > 			[21] 2455 MHz
> > 			[22] 2460 MHz
> > 			[23] 2465 MHz
> > 			[24] 2470 MHz
> > 			[25] 2475 MHz
> > 			[26] 2480 MHz
> > 	current_page: 0
> > 
> 
> I think to have the information which frequency it is - is a very nice
> feature for the userspace software. So I of course like to have a
> feature like this.

Good ;)

> 
> I tested it with the fakelb driver which have a lot of supported
> channels according to the page. The channel listing will be 117 lines,
> so fakelb is here a special case, but maybe we can do ~3 frequencies per
> line sperarated with comma and a tab. After ~3 frequencies then starts a
> newline again.
> 
> Example:
> 
> 		page 0:
> 			[11] 2405 MHz,	[12] 2410 MHz,	[13] 2415 MHz,
> 			[14] 2420 MHz,	[15] 2425 MHz,	[16] 2430 MHz,
> 			[17] 2435 MHz,	[18] 2440 MHz,	[19] 2445 MHz,
> 			....
> 
> (normally, it should something which fits in 80 chars terminal width)
> 
> don't know if this is the best idea. Or we introduce some verbose output
> option. 

Right. I started printing them out one by one without linebreaks but the
alignment didn't work out very well (hard to read) so I skipped that solution.
I will try your idea to do 3 or perhaps even 4 or 5 channels per line with
alignment.

> 
> > 	current_channel: 13 (2415 MHz)
> 
> This is very nice. :-)
> 
> Also I detected that I get a lot of:
> 
> 
> 	page 1:
>                 [0]  MHz 
>                 [1]  MHz 
>                 [2]  MHz 
>                 [3]  MHz 
>                 [4]  MHz 
>                 [5]  MHz 
>                 [6]  MHz 
>                 [7]  MHz 
>                 [8]  MHz 
>                 [9]  MHz 
>                 [10]  MHz
> 
> with the fakelb driver. I suppose this is only because you implemented
> page 0 only. Maybe do a "[0] unknown MHz", here? Just added the wort
> "unknown".

Correct, only page 0 so far, "unknown" looks like a good addition. I will use
"fakelb" to test future improvements (frequencies that I otherwise can't test).

> 
> 
> Another note is that this should be implemented to the new channel/page
> attr style which is part of the phy capabilities [0]. Currently there exists
> two implementations to get the supported channel/page attributes. The
> one which you implemented is obsolete and exists to deal with old kernel
> versions only. We will remove it in the next releases. So please use the
> capability output at [0] for channel dump.
> (There I did no format string fail. :-))

aha, that's why channels were listed twice in iwpan output.

> 
> > Signed-off-by: Christoffer Holmstedt <christoffer@christofferholmstedt.se>
> > ---
> > Just added implementation for channel page 0 but wanted to know if I'm on the
> > right track here. Any feedback concerning visual output and implementation
> > is appreciated.
> > 
> >  src/info.c |   43 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 40 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/info.c b/src/info.c
> > index e8f5dda8da94..86e4dc4e4a14 100644
> > --- a/src/info.c
> > +++ b/src/info.c
> > @@ -23,6 +23,33 @@ static void print_minmax_handler(int min, int max)
> >  	printf("\b \n");
> >  }
> >  
> > +static void print_freq(int page, int channel)
> > +{
> > +	float freq = 0;
> > +
> > +	switch (page) {
> > +	case 0:
> > +		if (channel == 0) {
> > +			freq = 868.3;
> > +			printf("%.1f", freq);
> > +			break;
> > +		}
> > +		else if (channel > 0 && channel < 11) {
> > +			freq = 906 + 2 * (channel - 1);
> > +		}
> > +		else {
> > +			freq = 2405 + 5 * (channel - 11);
> > +		}
> > +		printf("%.0f", freq);
> > +		break;
> > +	default:
> > +		/* Unknown page */
> > +		break;
> > +	}
> > +
> > +
> > +}
> > +
> >  static int print_phy_handler(struct nl_msg *msg, void *arg)
> >  {
> >  	struct nlattr *tb_msg[NL802154_ATTR_MAX + 1];
> > @@ -54,10 +81,14 @@ static int print_phy_handler(struct nl_msg *msg, void *arg)
> >  				    rem_page) {
> >  			channel = nla_get_u32(nl_page);
> >  			if (channel) {
> > -				printf("\tpage %d: ", page, channel);
> 
> mhh, this looks wrong right now. The format string describes only one
> argument, but we have "page, channel" there. There must be page only.
> Can you send a fix for this?

Yea, I was thinking about that it looked like a leftover from some refactoring
but didn't dig into the git history. I will send a patch during the day.

> 
> - Alex
> 
> [0] https://github.com/linux-wpan/wpan-tools/blob/master/src/info.c#L147

-- 
Christoffer Holmstedt