prism/radiotap header support

[Mathieu Lacage]

looks good to me. The biggest issue remaining is the change to the helper API
conventions in 
http://codereview.appspot.com/67056/diff/1/12#newcode243

Craig, comments ?

http://codereview.appspot.com/67056/diff/1/5
File examples/wifi-wired-bridging.cc (right):

http://codereview.appspot.com/67056/diff/1/5#newcode93
Line 93: YansWifiPhyHelper wifiPhy = YansWifiPhyHelper::Default ();
what is the point of moving that code here from where it was ?

http://codereview.appspot.com/67056/diff/1/6
File src/common/pcap-writer.cc (right):

http://codereview.appspot.com/67056/diff/1/6#newcode387
Line 387: //   if (packet.IsFragmented ())
kill commented code ?

http://codereview.appspot.com/67056/diff/1/9
File src/devices/wifi/wifi-phy.h (right):

http://codereview.appspot.com/67056/diff/1/9#newcode312
Line 312: void NotifyPromiscSniff (Ptr<const Packet> packet, uint16_t
channelFreqMhz, WifiMode mode, WifiPreamble preamble, bool isTx, double
signalDbm = 0, double noiseDbm = 0);
gaaaah. No default values for method arguments. ever. Use overloading if you
must.

http://codereview.appspot.com/67056/diff/1/10
File src/devices/wifi/yans-wifi-phy.cc (right):

http://codereview.appspot.com/67056/diff/1/10#newcode407
Line 407: NotifyPromiscSniff (packet, 2437, txMode, preamble, true);
what is that magic 2437 value ? Would you mind define a constant either with a
macro or a const variable to make its intend clearer through its name ?

http://codereview.appspot.com/67056/diff/1/10#newcode564
Line 564: 
uneeded space change ?

http://codereview.appspot.com/67056/diff/1/10#newcode569
Line 569: NotifyPromiscSniff (packet, 2437, event->GetPayloadMode (),
event->GetPreambleType (), false, signalDbm, noiseDbm);
same here: 2437 ?

Are you aware you changed the order of the trace events ?

http://codereview.appspot.com/67056/diff/1/10#newcode580
Line 580: 
unneeded space

http://codereview.appspot.com/67056/diff/1/11
File src/helper/yans-wifi-helper.cc (right):

http://codereview.appspot.com/67056/diff/1/11#newcode139
Line 139: YansWifiPhyHelper::YansWifiPhyHelper ()
must update the doxygen to state what is the default pcap format

http://codereview.appspot.com/67056/diff/1/12
File src/helper/yans-wifi-helper.h (right):

http://codereview.appspot.com/67056/diff/1/12#newcode243
Line 243: void EnablePcap (std::string filename, Ptr<NetDevice> nd);
removing the static keyword here changes the helper API convention so, it needs
to be run through craig.

[Nicola Baldo]

Hi Mathieu and Craig,

here some replies to the previous comments, to explain how I would deal with the
raised issues. Once we clarify this YansWifiPhyHelper issue, and if you agree on
the other proposed changes, I will prepare a new patch for a (hopefully) final
revision.

Regards,

Nicola

http://codereview.appspot.com/67056/diff/1/5
File examples/wifi-wired-bridging.cc (right):

http://codereview.appspot.com/67056/diff/1/5#newcode93
Line 93: YansWifiPhyHelper wifiPhy = YansWifiPhyHelper::Default ();
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> what is the point of moving that code here from where it was ?

the point is that now it is needed to access wifiPhy outside the loop to call
wifiPhy.EnablePcap()

http://codereview.appspot.com/67056/diff/1/6
File src/common/pcap-writer.cc (right):

http://codereview.appspot.com/67056/diff/1/6#newcode387
Line 387: //   if (packet.IsFragmented ())
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> kill commented code ?

OK

http://codereview.appspot.com/67056/diff/1/9
File src/devices/wifi/wifi-phy.h (right):

http://codereview.appspot.com/67056/diff/1/9#newcode312
Line 312: void NotifyPromiscSniff (Ptr<const Packet> packet, uint16_t
channelFreqMhz, WifiMode mode, WifiPreamble preamble, bool isTx, double
signalDbm = 0, double noiseDbm = 0);
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> gaaaah. No default values for method arguments. ever. Use overloading if you
> must.

Ok, I'll split it in the following two methods:

void NotifyPromiscSniffRx (Ptr<const Packet> packet, uint16_t channelFreqMhz,
WifiMode mode, WifiPreamble preamble, double signalDbm, double noiseDbm);

and

void NotifyPromiscSniffTx (Ptr<const Packet> packet, uint16_t channelFreqMhz,
WifiMode mode, WifiPreamble preamble);

http://codereview.appspot.com/67056/diff/1/10
File src/devices/wifi/yans-wifi-phy.cc (right):

http://codereview.appspot.com/67056/diff/1/10#newcode407
Line 407: NotifyPromiscSniff (packet, 2437, txMode, preamble, true);
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> what is that magic 2437 value ? Would you mind define a constant either with a
> macro or a const variable to make its intend clearer through its name ?

What about having a new variable YansWifiPhy::m_channelFreqMhz ?

http://codereview.appspot.com/67056/diff/1/10#newcode564
Line 564: 
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> uneeded space change ?

I'll remove it.

http://codereview.appspot.com/67056/diff/1/10#newcode569
Line 569: NotifyPromiscSniff (packet, 2437, event->GetPayloadMode (),
event->GetPreambleType (), false, signalDbm, noiseDbm);
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> same here: 2437 ?

same answer as above

> 
> Are you aware you changed the order of the trace events ?

I'll revert the original order.

http://codereview.appspot.com/67056/diff/1/10#newcode580
Line 580: 
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> unneeded space
> 

I'll remove it.

http://codereview.appspot.com/67056/diff/1/11
File src/helper/yans-wifi-helper.cc (right):

http://codereview.appspot.com/67056/diff/1/11#newcode139
Line 139: YansWifiPhyHelper::YansWifiPhyHelper ()
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> must update the doxygen to state what is the default pcap format

I'll add this.

http://codereview.appspot.com/67056/diff/1/12
File src/helper/yans-wifi-helper.h (right):

http://codereview.appspot.com/67056/diff/1/12#newcode243
Line 243: void EnablePcap (std::string filename, Ptr<NetDevice> nd);
On 2009/05/19 14:16:48, Mathieu Lacage wrote:
> removing the static keyword here changes the helper API convention so, it
needs
> to be run through craig.

The fact is that the PcapWriter needs to know which format will be used for the
traces (radiotap, prism or 802.11 only). 

If keeping the API unmodified is a must, we could keep the static keyword, have
the current version of EnablePcap()  use the default pcap format (say 802.11
headers only), and add new overladed versions of EnablePcap() which will accept
a PcapFormat parameter.

The reason why I don't like this approach is that there are already 5 overloaded
versions of EnablePcap, and with this solution we would need to double this
number.