Multiple frequency channels supported in yans wifi channel and PHY

[Nicola Baldo, 2009-07-06 09:52:57]

I like this implementation in that it is effective in providing multi-channel
support with minimal code modifications. Some detailed comments below.

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

http://codereview.appspot.com/91057/diff/1/3#newcode313
Line 313: Simulator::Schedule (m_channelSwitchDelay,
&YansWifiPhy::DoSetChannelId, this, nch);
Good to see that you model the channel switching delay. However, with this
implementation the PHY will be still functional when switching channel, and also
a packet whose transmission will be started just prior to the execution of the
channel switch will still be performed correctly. In reality, a significant
portion of the channel switch time is due to the oscillator in the RF stage
moving to the new frequency. This will likely disrupt any ongoing TX/RX. I think
we should model this feature, otherwise it does not make sense to model the
channel switch time at all.

http://codereview.appspot.com/91057/diff/1/3#newcode495
Line 495: 
It would be good to add also Configure80211gParameters(), which will use 80211a
modes but in the 2.4 GHz band. 
We might also consider having Configure80211bgParameters(), which would work in
the 2.4 GHz band with both a and b modes.

http://codereview.appspot.com/91057/diff/1/4
File src/devices/wifi/yans-wifi-phy.h (right):

http://codereview.appspot.com/91057/diff/1/4#newcode75
Line 75: *
How do we handle devices which can switch between the 2.4 GHz and the 5 GHz
band? Do we need to call Configure80211a/b/g expicitly every time?

http://codereview.appspot.com/91057/diff/1/4#newcode81
Line 81: void SetFrequencyChannel (uint16_t id);
I think the name SetFrequencyChannel() is not very intuitive.
Why not call these "SetChannelNumber()", "GetChannelNumber()" and
"GetChannelFrequencyMHz()"?

Furthermore, I wonder if some of these methods can be moved to WifiPhy(), so
that we can have an interface for handling multiple channels which is uniform
across different WifiPhy impelmentations.

http://codereview.appspot.com/91057/diff/1/4#newcode82
Line 82: /// Return current channel ID, see SetChannelId()
reference to non-existent method SetChannelId()

[Pavel Boyko, 2009-07-06 11:27:35]

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

http://codereview.appspot.com/91057/diff/1/3#newcode313
Line 313: Simulator::Schedule (m_channelSwitchDelay,
&YansWifiPhy::DoSetChannelId, this, nch);
On 2009/07/06 09:52:57, Nicola Baldo wrote:
> Good to see that you model the channel switching delay. However, with this
> implementation the PHY will be still functional when switching channel, and
also
> a packet whose transmission will be started just prior to the execution of the
> channel switch will still be performed correctly. In reality, a significant
> portion of the channel switch time is due to the oscillator in the RF stage
> moving to the new frequency. This will likely disrupt any ongoing TX/RX. I
think
> we should model this feature, otherwise it does not make sense to model the
> channel switch time at all.

Agree. Detailed model should notify MAC to pause (and m.b. clean) queues, wait
for current tx/burst/rx+ack end, switch frequency, then notify MAC to reset NAV
and backoff and, probably, wait DIFS before standing ready and resuming queues. 
   

All this stuff is quite complicated and must be supported both in MAC and PHY,
so I propose to accept simple model now to proceed with mesh and open
enhancement request on bugzilla.

http://codereview.appspot.com/91057/diff/1/3#newcode495
Line 495: 
On 2009/07/06 09:52:57, Nicola Baldo wrote:
> It would be good to add also Configure80211gParameters(), which will use
80211a
> modes but in the 2.4 GHz band. 
> We might also consider having Configure80211bgParameters(), which would work
in
> the 2.4 GHz band with both a and b modes. 

Please post this as enhancement to bugzilla.

http://codereview.appspot.com/91057/diff/1/4
File src/devices/wifi/yans-wifi-phy.h (right):

http://codereview.appspot.com/91057/diff/1/4#newcode75
Line 75: *
On 2009/07/06 09:52:57, Nicola Baldo wrote:
> How do we handle devices which can switch between the 2.4 GHz and the 5 GHz
> band? Do we need to call Configure80211a/b/g expicitly every time? 

sure

http://codereview.appspot.com/91057/diff/1/4#newcode81
Line 81: void SetFrequencyChannel (uint16_t id);
On 2009/07/06 09:52:57, Nicola Baldo wrote:
> I think the name SetFrequencyChannel() is not very intuitive.
> Why not call these "SetChannelNumber()", "GetChannelNumber()" and
> "GetChannelFrequencyMHz()"?

 Done.

> Furthermore, I wonder if some of these methods can be moved to WifiPhy(), so
> that we can have an interface for handling multiple channels which is uniform
> across different WifiPhy impelmentations.

  I'd prefer to stabilize channel numbers interface and semantic in YansWifiPhy
and then promote it to the base class in the next release cycle.

http://codereview.appspot.com/91057/diff/1/4#newcode82
Line 82: /// Return current channel ID, see SetChannelId()
On 2009/07/06 09:52:57, Nicola Baldo wrote:
> reference to non-existent method SetChannelId()

Done.

[Mathieu Lacage, 2009-07-13 06:52:19]

I think that the biggest issue is something nicola pointed out already: I feel
that the switch time implementation is really weak and I would feel better if we
could at least make the PHY implementation a bit more robust to it and able to
keep track of this switching state.

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

http://codereview.appspot.com/91057/diff/1/3#newcode313
Line 313: Simulator::Schedule (m_channelSwitchDelay,
&YansWifiPhy::DoSetChannelId, this, nch);
On 2009/07/06 11:27:35, Pavel Boyko wrote:
> On 2009/07/06 09:52:57, Nicola Baldo wrote:
> > Good to see that you model the channel switching delay. However, with this
> > implementation the PHY will be still functional when switching channel, and
> also
> > a packet whose transmission will be started just prior to the execution of
the
> > channel switch will still be performed correctly. In reality, a significant
> > portion of the channel switch time is due to the oscillator in the RF stage
> > moving to the new frequency. This will likely disrupt any ongoing TX/RX. I
> think
> > we should model this feature, otherwise it does not make sense to model the
> > channel switch time at all.
> 
> Agree. Detailed model should notify MAC to pause (and m.b. clean) queues, wait
> for current tx/burst/rx+ack end, switch frequency, then notify MAC to reset
NAV
> and backoff and, probably, wait DIFS before standing ready and resuming
queues. 
>    
> 
> All this stuff is quite complicated and must be supported both in MAC and PHY,
> so I propose to accept simple model now to proceed with mesh and open
> enhancement request on bugzilla.
> 

I think that it would be nice if:
1) the PHY could keep track of its SWITCHING state
2) the PHY could assert that you don't switch _during_ a transmission
3) the PHY makes sure that if you SWITCH during a reception, the reception
fails.

http://codereview.appspot.com/91057/diff/1/4
File src/devices/wifi/yans-wifi-phy.h (right):

http://codereview.appspot.com/91057/diff/1/4#newcode81
Line 81: void SetFrequencyChannel (uint16_t id);
On 2009/07/06 11:27:35, Pavel Boyko wrote:

> > Furthermore, I wonder if some of these methods can be moved to WifiPhy(), so
> > that we can have an interface for handling multiple channels which is
uniform
> > across different WifiPhy impelmentations.
> 
>   I'd prefer to stabilize channel numbers interface and semantic in
YansWifiPhy
> and then promote it to the base class in the next release cycle.

I think that both SetChannelNumber, GetChannelNumber are fairly obvious and safe
to move to the base class. I am not sure what GetChannelFrequencyMhz would be
used for so, we can keep it in the subclass for now.

http://codereview.appspot.com/91057/diff/1/4#newcode153
Line 153: void DoSetChannelId(uint16_t id);
probably would make sense to rename 'channel id' to 'channel number'

[Ramon Bauza, 2009-07-13 13:41:05]

Just a comment regarding the path loss calculation depending on the
frequency/channel employed.

http://codereview.appspot.com/91057/diff/1/2
File src/devices/wifi/yans-wifi-channel.cc (right):

http://codereview.appspot.com/91057/diff/1/2#newcode90
Line 90: double rxPowerDbm = m_loss->CalcRxPower (txPowerDbm, senderMobility,
receiverMobility);
Probably, it would be also needed to pass the channel frequency to the path loss
model (Path loss models usually calculate the propagation losses based on the
frequency). This would thus require to modify the
PropagationLossModel::CalcRxPower function so that it can accept the frequency
as a parameter.

[Pavel Boyko, 2009-07-13 14:39:14]

On 2009/07/13 13:41:05, Ramon Bauza wrote:
> Just a comment regarding the path loss calculation depending on the
> frequency/channel employed.

  Indeed, in first approximation all path loss models depend on frequency as  k
* 10 * log_{10}(f) term with k from 0 (2-ray ground) to 2 (free space) to ~3
(indoor propagation). This gives us less than 1 dB change for frequency change
from 2.4 GHz to 2.5 GHz (complete ISM band). I think this effect can be
neglected since none of our propagation loss models really has 1 dB accuracy. 

> http://codereview.appspot.com/91057/diff/1/2
> File src/devices/wifi/yans-wifi-channel.cc (right):
> 
> http://codereview.appspot.com/91057/diff/1/2#newcode90
> Line 90: double rxPowerDbm = m_loss->CalcRxPower (txPowerDbm, senderMobility,
> receiverMobility);
> Probably, it would be also needed to pass the channel frequency to the path
loss
> model (Path loss models usually calculate the propagation losses based on the
> frequency). This would thus require to modify the
> PropagationLossModel::CalcRxPower function so that it can accept the frequency
> as a parameter.

[Pavel Boyko, 2009-07-14 05:42:24]

Cosmetics done (in https://forge.wenos.ru/hgprojects/ns3dev), correct channel
switching is pending.

http://codereview.appspot.com/91057/diff/1/4
File src/devices/wifi/yans-wifi-phy.h (right):

http://codereview.appspot.com/91057/diff/1/4#newcode81
Line 81: void SetFrequencyChannel (uint16_t id);
On 2009/07/13 06:52:19, Mathieu Lacage wrote:
> On 2009/07/06 11:27:35, Pavel Boyko wrote:
> 
> > > Furthermore, I wonder if some of these methods can be moved to WifiPhy(),
so
> > > that we can have an interface for handling multiple channels which is
> uniform
> > > across different WifiPhy impelmentations.
> > 
> >   I'd prefer to stabilize channel numbers interface and semantic in
> YansWifiPhy
> > and then promote it to the base class in the next release cycle.
> 
> I think that both SetChannelNumber, GetChannelNumber are fairly obvious and
safe
> to move to the base class. I am not sure what GetChannelFrequencyMhz would be
> used for so, we can keep it in the subclass for now.
> 

Done.

http://codereview.appspot.com/91057/diff/1/4#newcode153
Line 153: void DoSetChannelId(uint16_t id);
On 2009/07/13 06:52:19, Mathieu Lacage wrote:
> probably would make sense to rename 'channel id' to 'channel number'

Done.

[Pavel Boyko, 2009-07-16 07:10:09]

It seems that all but correct PHY switching process model issues are fixed. 

To be able to proceed with 802.11s model review & merge I propose to merge
channels in their current state and file me a bug with PHY switching
enhancements requirements.

[Mathieu Lacage, 2009-07-16 11:06:34]

http://codereview.appspot.com/91057/diff/2005/2008
File src/devices/wifi/yans-wifi-phy.cc (right):

http://codereview.appspot.com/91057/diff/2005/2008#newcode119
Line 119: .AddAttribute ("ChannelSwitchDelay",
you forgot to remove this attribute as well as the member variable declaration
for m_channelSwitchDelay in the header

[Pavel Boyko, 2009-07-16 12:07:05]

According to Mathieu Plan B I have pushed minimal  implementation to ns-3-dev.
Ramon, now it is your move (and thank you for that).

http://codereview.appspot.com/91057/diff/2005/2008
File src/devices/wifi/yans-wifi-phy.cc (right):

http://codereview.appspot.com/91057/diff/2005/2008#newcode119
Line 119: .AddAttribute ("ChannelSwitchDelay",
On 2009/07/16 11:06:34, Mathieu Lacage wrote:
> you forgot to remove this attribute as well as the member variable declaration
> for m_channelSwitchDelay in the header

Nope, I just reserved it for future switching model ;) Being serious -- it took
several days to correctly measure this time on atheros and I don't want to lose
it.