|
Bugzilla – Full Text Bug Listing |
| Summary: | code review: RRPAA for WiFi | ||
|---|---|---|---|
| Product: | ns-3 | Reporter: | Tom Henderson <tomh> |
| Component: | wifi | Assignee: | Matías Richart <matis18> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | ns-bugs, sebastien.deronne |
| Priority: | P5 | ||
| Version: | pre-release | ||
| Hardware: | All | ||
| OS: | All | ||
|
Description
Tom Henderson
2016-03-31 15:23:31 UTC
Matias, any updated patch for this? Matias, do you have updates on this? We are close to the next release and I think it would be nice to have this one included. (In reply to sebastien.deronne from comment #2) > Matias, do you have updates on this? We are close to the next release and I > think it would be nice to have this one included. Sorry for not answering sooner. I tried to extend the algorithm for HT but it is more difficult than expected (I mentioned ). A new and different strategy is needed to do it correctly. The algorithm is not directly applicable to HT. So, I can correct the last comments on code review and uploaded as it is or we can dismiss the contribution (do you see it relevant if HT is not supported?). (In reply to Matías Richart from comment #3) > (In reply to sebastien.deronne from comment #2) > > Matias, do you have updates on this? We are close to the next release and I > > think it would be nice to have this one included. > > Sorry for not answering sooner. > > I tried to extend the algorithm for HT but it is more difficult than > expected (I mentioned ). A new and different strategy is needed to do it > correctly. The algorithm is not directly applicable to HT. > > So, I can correct the last comments on code review and uploaded as it is or > we can dismiss the contribution (do you see it relevant if HT is not > supported?). Tom, what is your opinion? I think we could deliver this for the next release, and eventually open a new thread to have support for HT/VHT, unless you decide this will not support HT/VHT. Tom, Mathias, Can we include this patch in the upcoming release with a restriction for HT/VHT/HE? If once we plan to add support for HT/VHT/HE, then a new thread could be opened (a lot of other managers do not support HT yet anyway). (In reply to sebastien.deronne from comment #5) > Tom, Mathias, > Can we include this patch in the upcoming release with a restriction for > HT/VHT/HE? Yes. > If once we plan to add support for HT/VHT/HE, then a new thread could be > opened (a lot of other managers do not support HT yet anyway). Agree. Matias, can you update the patch and close this bug? (In reply to sebastien.deronne from comment #7) > Matias, can you update the patch and close this bug? I updated the patch and commented the changes in rietveld. (In reply to Matías Richart from comment #8) > (In reply to sebastien.deronne from comment #7) > > Matias, can you update the patch and close this bug? > > I updated the patch and commented the changes in rietveld. OK for me Tom, are you also fine with the latest patch? I merged the code with the latest ns-3-dev, all tests pass fine. Matias, could you just please address those 4 Doxygen warnings so that I can drop the code changes: src/wifi/model/rrpaa-wifi-manager.h:56: warning: Member m_ori (variable) of class ns3::Thresholds is not documented. src/wifi/model/rrpaa-wifi-manager.h:57: warning: Member m_mtl (variable) of class ns3::Thresholds is not documented. src/wifi/model/rrpaa-wifi-manager.h:58: warning: Member m_ewnd (variable) of class ns3::Thresholds is not documented. src/wifi/model/rrpaa-wifi-manager.h:235: warning: Member m_uniformRandomVariable (variable) of class ns3::RrpaaWifiManager is not documented. (In reply to sebastien.deronne from comment #11) > I merged the code with the latest ns-3-dev, all tests pass fine. > > Matias, could you just please address those 4 Doxygen warnings so that I can > drop the code changes: > > src/wifi/model/rrpaa-wifi-manager.h:56: warning: Member m_ori (variable) of > class ns3::Thresholds is not documented. > src/wifi/model/rrpaa-wifi-manager.h:57: warning: Member m_mtl (variable) of > class ns3::Thresholds is not documented. > src/wifi/model/rrpaa-wifi-manager.h:58: warning: Member m_ewnd (variable) of > class ns3::Thresholds is not documented. > src/wifi/model/rrpaa-wifi-manager.h:235: warning: Member > m_uniformRandomVariable (variable) of class ns3::RrpaaWifiManager is not > documented. Done. I uploaded a new patch set, but if you just need a patch for those warning let me know. In the patch set I included some corrections for merge conflicts with master. (In reply to Matías Richart from comment #12) > (In reply to sebastien.deronne from comment #11) > > I merged the code with the latest ns-3-dev, all tests pass fine. > > > > Matias, could you just please address those 4 Doxygen warnings so that I can > > drop the code changes: > > > > src/wifi/model/rrpaa-wifi-manager.h:56: warning: Member m_ori (variable) of > > class ns3::Thresholds is not documented. > > src/wifi/model/rrpaa-wifi-manager.h:57: warning: Member m_mtl (variable) of > > class ns3::Thresholds is not documented. > > src/wifi/model/rrpaa-wifi-manager.h:58: warning: Member m_ewnd (variable) of > > class ns3::Thresholds is not documented. > > src/wifi/model/rrpaa-wifi-manager.h:235: warning: Member > > m_uniformRandomVariable (variable) of class ns3::RrpaaWifiManager is not > > documented. > > Done. I uploaded a new patch set, but if you just need a patch for those > warning let me know. > In the patch set I included some corrections for merge conflicts with master. Yes just the warnings, the rest is merged already. Did you change something else? Otherwise I just take the Doxygen changes. (In reply to sebastien.deronne from comment #13) > (In reply to Matías Richart from comment #12) > > (In reply to sebastien.deronne from comment #11) > > > I merged the code with the latest ns-3-dev, all tests pass fine. > > > > > > Matias, could you just please address those 4 Doxygen warnings so that I can > > > drop the code changes: > > > > > > src/wifi/model/rrpaa-wifi-manager.h:56: warning: Member m_ori (variable) of > > > class ns3::Thresholds is not documented. > > > src/wifi/model/rrpaa-wifi-manager.h:57: warning: Member m_mtl (variable) of > > > class ns3::Thresholds is not documented. > > > src/wifi/model/rrpaa-wifi-manager.h:58: warning: Member m_ewnd (variable) of > > > class ns3::Thresholds is not documented. > > > src/wifi/model/rrpaa-wifi-manager.h:235: warning: Member > > > m_uniformRandomVariable (variable) of class ns3::RrpaaWifiManager is not > > > documented. > > > > Done. I uploaded a new patch set, but if you just need a patch for those > > warning let me know. > > In the patch set I included some corrections for merge conflicts with master. > > Yes just the warnings, the rest is merged already. > Did you change something else? Otherwise I just take the Doxygen changes. I can see from diff the updated Doxygen, thanks. I will deliver the final patch. pushed in changeset 12694:5209b094838e (In reply to sebastien.deronne from comment #15) > pushed in changeset 12694:5209b094838e great! thanks to you and Tom for the revision. |