Bug 2831 - runtime channel width switch has no effect
runtime channel width switch has no effect
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: wifi
ns-3.27
All All
: P3 normal
Assigned To: sebastien.deronne
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-30 14:19 EST by Tom Henderson
Modified: 2018-02-23 17:20 EST (History)
2 users (show)

See Also:


Attachments
test case to reproduce (12.62 KB, text/plain)
2017-11-30 14:20 EST, Tom Henderson
Details
proposed solution (31.48 KB, patch)
2018-01-23 17:18 EST, sebastien.deronne
Details | Diff
updated patch (43.68 KB, patch)
2018-02-20 15:57 EST, sebastien.deronne
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Henderson 2017-11-30 14:19:32 EST
Reported on the ns-3-users list by Michael Rademacher:

https://groups.google.com/forum/#!topic/ns-3-users/0W6fhQYj2Rk
Comment 1 Tom Henderson 2017-11-30 14:20:14 EST
Created attachment 2966 [details]
test case to reproduce
Comment 2 sebastien.deronne 2017-12-23 10:32:07 EST
I do not think this can work, we need a re-association if we change what is supported by a station, otherwise the access point has no way to see that capabilities have changed.
Comment 3 sebastien.deronne 2018-01-15 04:53:22 EST
I will work on a patch later this week to re-associate when changing the channel width at runtime.
Comment 4 sebastien.deronne 2018-01-16 14:50:53 EST
I did not manage to do the same in Linux, such operation seems not permitted at runtime. In some drivers code, I saw that if the NSS and/or the MCS mask is changed, a reassociation is done. I will assume for now that we also do a reassociation if channel width is changed if it is fine for all (and I will extend to a change of the NSS).
Comment 5 sebastien.deronne 2018-01-23 17:18:52 EST
Created attachment 3010 [details]
proposed solution

Here is a proposal, which triggers a reassociation if PHY capabilities did change.
Comment 6 sebastien.deronne 2018-01-31 14:07:57 EST
Any feedback on the proposal?
Comment 7 Michael Rademacher 2018-02-01 09:14:51 EST
(In reply to sebastien.deronne from comment #4)
> I did not manage to do the same in Linux, such operation seems not permitted
> at runtime. In some drivers code, I saw that if the NSS and/or the MCS mask
> is changed, a reassociation is done. I will assume for now that we also do a
> reassociation if channel width is changed if it is fine for all (and I will
> extend to a change of the NSS).

Thank you for this work. Out of curiosity, which drivers did you looked at? ath9k, ath10k?

Which implications does a re-association has for higher layers? Do we get a problem with routing for example?
Comment 8 sebastien.deronne 2018-02-03 08:06:53 EST
(In reply to Michael Rademacher from comment #7)
> (In reply to sebastien.deronne from comment #4)
> > I did not manage to do the same in Linux, such operation seems not permitted
> > at runtime. In some drivers code, I saw that if the NSS and/or the MCS mask
> > is changed, a reassociation is done. I will assume for now that we also do a
> > reassociation if channel width is changed if it is fine for all (and I will
> > extend to a change of the NSS).
> 
> Thank you for this work. Out of curiosity, which drivers did you looked at?
> ath9k, ath10k?

If I remember well, I was looking into ath10k at that moment.

> 
> Which implications does a re-association has for higher layers? Do we get a
> problem with routing for example?

This should be quite transparent for higher layers in my opinion.
Comment 9 sebastien.deronne 2018-02-06 16:38:41 EST
I am planning to push this one if no further comments.
Comment 10 Tom Henderson 2018-02-07 00:49:45 EST
(In reply to sebastien.deronne from comment #9)
> I am planning to push this one if no further comments.

I agree with the approach; two possible suggestions for improvement:

1) the new capability has no test or example code; at a minimum, can a run-time channel switch example be added and tested for regression (ideally, a new test case)
2) there is a lot of new logic in ap-wifi-mac.cc that lacks any NS_LOGging
Comment 11 sebastien.deronne 2018-02-07 07:47:36 EST
(In reply to Tom Henderson from comment #10)
> (In reply to sebastien.deronne from comment #9)
> > I am planning to push this one if no further comments.
> 
> I agree with the approach; two possible suggestions for improvement:
> 
> 1) the new capability has no test or example code; at a minimum, can a
> run-time channel switch example be added and tested for regression (ideally,
> a new test case)

Agreed, I will add a test case.

> 2) there is a lot of new logic in ap-wifi-mac.cc that lacks any NS_LOGging

I realize a lot of logging are missing already (no indication that we receive an assoc req, a beacon, ...). Is this what you mean here (indication that a re-assoc req/response was received)?
Comment 12 Tom Henderson 2018-02-07 14:52:28 EST
(In reply to sebastien.deronne from comment #11)
> (In reply to Tom Henderson from comment #10)
> > (In reply to sebastien.deronne from comment #9)
> > > I am planning to push this one if no further comments.
> > 
> > I agree with the approach; two possible suggestions for improvement:
> > 
> > 1) the new capability has no test or example code; at a minimum, can a
> > run-time channel switch example be added and tested for regression (ideally,
> > a new test case)
> 
> Agreed, I will add a test case.
> 
> > 2) there is a lot of new logic in ap-wifi-mac.cc that lacks any NS_LOGging
> 
> I realize a lot of logging are missing already (no indication that we
> receive an assoc req, a beacon, ...). Is this what you mean here (indication
> that a re-assoc req/response was received)?


I suggest that at least req/response received or sent, and in the code, whenever you set 'problem' to true, you put a NS_LOG_DEBUG ("") to briefly summarize the conditions that make it true (i.e. which branch of code caused it to be set).
Comment 13 sebastien.deronne 2018-02-20 07:51:53 EST
I am working on an updated patch.
Comment 14 sebastien.deronne 2018-02-20 15:57:39 EST
Created attachment 3054 [details]
updated patch

I updated the patch with additional logging + a test case
Comment 15 sebastien.deronne 2018-02-23 15:59:01 EST
I am planning to push this.
Comment 16 Tom Henderson 2018-02-23 16:49:41 EST
(In reply to sebastien.deronne from comment #15)
> I am planning to push this.

OK with me.
Comment 17 sebastien.deronne 2018-02-23 17:20:21 EST
Pushed in changeset 13312:7865825507eb