Bug 644

Summary: Radiotap header is specific to 802.11g
Product: ns-3 Reporter: Tom Henderson <tomh>
Component: wifiAssignee: Nicola Baldo <nicola>
Status: RESOLVED FIXED    
Severity: normal CC: andreev, kirillano, mathieu.lacage, ns-bugs
Priority: P5    
Version: ns-3-dev   
Hardware: All   
OS: All   
Attachments: patch fixing channel field in radiotap and prism headers

Description Tom Henderson 2009-07-24 10:13:26 UTC
Reported by Kirill:

It writes statically 802.11g mode and channel frequency. The former is a bug - because we have not only 802.11g, the latter is problem that caused regression failure. In attachment there is a patch, which makes regression successfull (where chanel frequncy was replaced with 2437 Mhz, as it was before this commit) 

====

There are two possible bugs that I see here:

1) 802.11g flag is always on
2) channelFreqMhz parameter is presently not being used (backed out by changeset ca8cbdd42786)
Comment 1 Nicola Baldo 2009-07-24 10:42:41 UTC
(In reply to comment #0)
> Reported by Kirill:
> 
> It writes statically 802.11g mode and channel frequency. The former is a bug -
> because we have not only 802.11g, the latter is problem that caused regression
> failure. In attachment there is a patch, which makes regression successfull
> (where chanel frequncy was replaced with 2437 Mhz, as it was before this
> commit) 


For the fact that 802.11g mode is statically written, you're right, that's because when radiotap support was implemented there was only 802.11g. Patches welcome.

For the second issue, changeset ca8cbdd42786 fixes regression by breaking the ability of radiotap to correctly report the channel on which the device is tuned. I guess the regression failure is just due to a change in the default channel being used. If this is confirmed, I would suggest to undo changeset ca8cbdd42786, and update the regression traces instead. 

Nicola
Comment 2 Tom Henderson 2009-07-24 10:51:04 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > Reported by Kirill:
> > 
> > It writes statically 802.11g mode and channel frequency. The former is a bug -
> > because we have not only 802.11g, the latter is problem that caused regression
> > failure. In attachment there is a patch, which makes regression successfull
> > (where chanel frequncy was replaced with 2437 Mhz, as it was before this
> > commit) 
> 
> 
> For the fact that 802.11g mode is statically written, you're right, that's
> because when radiotap support was implemented there was only 802.11g. Patches
> welcome.
> 
> For the second issue, changeset ca8cbdd42786 fixes regression by breaking the
> ability of radiotap to correctly report the channel on which the device is
> tuned. I guess the regression failure is just due to a change in the default
> channel being used. If this is confirmed, I would suggest to undo changeset
> ca8cbdd42786, and update the regression traces instead. 
> 
> Nicola
> 

If you can confirm that the traces generated by undoing ca8cbdd42786 are OK, please go ahead and revert that and update the reference traces, or just let me know in this tracker and I will do it.  Then, we can leave this bug open for the 802.11g mode issue.

Comment 3 Kirill Andreev 2009-07-24 11:02:28 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > Reported by Kirill:
> > 
> > It writes statically 802.11g mode and channel frequency. The former is a bug -
> > because we have not only 802.11g, the latter is problem that caused regression
> > failure. In attachment there is a patch, which makes regression successfull
> > (where chanel frequncy was replaced with 2437 Mhz, as it was before this
> > commit) 
> 
> 
> For the fact that 802.11g mode is statically written, you're right, that's
> because when radiotap support was implemented there was only 802.11g. Patches
> welcome.
> 
> For the second issue, changeset ca8cbdd42786 fixes regression by breaking the
> ability of radiotap to correctly report the channel on which the device is
> tuned. I guess the regression failure is just due to a change in the default
> channel being used. If this is confirmed, I would suggest to undo changeset
> ca8cbdd42786, and update the regression traces instead. 
> 
> Nicola
> 
Hi!
As I think, regression traces should be updated by the following reasons:
1. 802.11g mode is not implemented in NS now.
2. Channel in radiotap header should be idicated correctly.
Of course, this patch shoul be reverted as soon as radiotap shall be fixed.
And what about making config for each scenario to prevent regression from failure when we change default settings?

Comment 4 Nicola Baldo 2009-07-24 13:03:49 UTC
> If you can confirm that the traces generated by undoing ca8cbdd42786 are OK,
> please go ahead and revert that and update the reference traces, or just let me
> know in this tracker and I will do it.  Then, we can leave this bug open for
> the 802.11g mode issue.
> 

I checked it, and I confirm that the only change in the traces is the channel number. 

> As I think, regression traces should be updated by the following reasons:
> 1. 802.11g mode is not implemented in NS now.

You're right, when working on radiotap+prism support I have always mistaken 802.11a for 802.11g, but strictly speaking only 802.11a is in ns3. Sorry for the confusion.

> 2. Channel in radiotap header should be idicated correctly.

please find below a patch which should address this issue. If you all agree with that, next monday I will push it and upgrade the reference traces.

Nicola

Comment 5 Nicola Baldo 2009-07-24 13:05:36 UTC
Created attachment 539 [details]
patch fixing channel field in radiotap and prism headers
Comment 6 Tom Henderson 2009-07-27 10:18:02 UTC
> 
> > As I think, regression traces should be updated by the following reasons:
> > 1. 802.11g mode is not implemented in NS now.
> 
> You're right, when working on radiotap+prism support I have always mistaken
> 802.11a for 802.11g, but strictly speaking only 802.11a is in ns3. Sorry for
> the confusion.

we do have 802.11b phy now

> 
> > 2. Channel in radiotap header should be idicated correctly.
> 
> please find below a patch which should address this issue. If you all agree
> with that, next monday I will push it and upgrade the reference traces.

My only comment is to check whether you have support for 802.11b (I don't know offhand what flags are enabled for 802.11b, but should it have DBPSK/DQPSK?), 
Comment 7 Nicola Baldo 2009-07-27 11:21:33 UTC
(In reply to comment #6)
> My only comment is to check whether you have support for 802.11b (I don't know
> offhand what flags are enabled for 802.11b, but should it have DBPSK/DQPSK?), 
> 

Yes the patch supports 802.11b

The reference for the channel flags in radiotap is here:
http://www.radiotap.org/defined-fields/Channel

as you can see there is no flag specific for each modulation (e.g., DBPSK/DQPSK), so the only flag which needs to be set is the CCK flag. Note that this flag is also set for 1Mbps and 2Mbps modulation. As a consequence, the flag is a little bit of a misnomer (1 and 2Mbps are not CCK, strictly speaking). However, this is consistent with the behavior in madwifi -- you can check this by looking for the IEEE80211_CHAN_B macro in the madwifi code. Furthermore, madwifi got this behavior from ethereal, so with the proposed patch an 802.11b packet should correctly show up as 802.11b in wireshark.
Comment 8 Mathieu Lacage 2009-08-13 07:17:07 UTC
It would be nice if this patch used a WifiMode instead of the raw 500kbps argument. This might allow you to handle easily the 5/10/10mhz modes, no ?
Comment 9 Mathieu Lacage 2009-08-13 07:41:24 UTC
changeset d0041768ff60