Issue 319050043: ns-3 Wi-Fi frame capture
Descriptionns-3 Wi-Fi frame capture
See: https://www.nsnam.org/bugzilla/show_bug.cgi?id=2368
Patch Set 1 #Patch Set 2 : Update to 2017-01-05 patch #
Total comments: 64
Patch Set 3 : frame-capture-patch-v3 #
Total comments: 17
Patch Set 4 : frame-capture-patch-v4 #
Total comments: 18
Patch Set 5 : frame-capture-patch v5 #
Total comments: 18
MessagesTotal messages: 16
Is there any test data to support the implementation? The test program 'test-interference-helper' is a program used to generate log files for specific scenarios, but what combinations of arguments to that program show the 'before' and 'after' effects of this model change? https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:165: Ptr<ErrorRateModel> error = CreateObject<NistErrorRateModel> (); why is this change needed? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */ Missing GPLv2 https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:43: double sinrDb = 10 * std::log10(sinr); We have WifiUtils::RatioToDb () for the above computation. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.h (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.h:1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */ Missing GPLv2 https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.h:10: class FrameCaptureModel : public Object Missing doxygen, including group label and class doxygen https://www.nsnam.org/docs/manual/html/documentation.html#id1 https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... File src/wifi/model/interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... src/wifi/model/interference-helper.cc:258: noiseInterference += i->GetDelta (); can you explain why a NiChange that occurs before the event start time contributes to the noiseInterference in this new model? Isn't m_firstPower supposed to account for these? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... File src/wifi/model/interference-helper.h (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... src/wifi/model/interference-helper.h:213: * \param sinr the sinr value at the current time \return the probability of successful capture https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... src/wifi/model/interference-helper.h:218: * \param sinr the sinr value at the current time \return the probability of successful capture https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:195: DoubleValue (0.0), this seems to be a separate change unrelated to frame capture? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:319: "Whether or not to enable cautpure at the payload(data) portion of the packet", capture https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2511: { NS_LOG_FUNCTION statement https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2575: { Need NS_LOG_FUNCTION statement https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2638: { Need NS_LOG_FUNCTION statement https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2656: { Need NS_LOG_FUNCTION statement at least https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h File src/wifi/model/wifi-phy.h (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1524: void SetPreambleCapture (bool enable); It seems that these setters and getters could be removed by just assigning the boolean directly in the attribute. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1758: * Due to newly arrived signal, the old reception cannot be continued and has to be abort missing parameter documentation https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1770: * \param mpdutype two extraneous parameters https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1777: * \param tag extraneous parameter
Sign in to reply to this message.
I had a first look, I'll go deeper this weekend. My main concern is that you do not provide any example. Also we need to make sure regression is still running fine, so no pcap to be rescanned, since it should behave exactly the same as now when frame capture is disabled for both preamble and payload. https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:56: #include "ns3/nist-error-rate-model.h" Why are you moving to Nist? https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:128: txModeB ("OfdmRate12Mbps"), Any reason for this change? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:66: Could you document how those calculations are done and how they were validated? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:195: DoubleValue (0.0), On 2017/01/05 15:32:22, Tom Henderson wrote: > this seems to be a separate change unrelated to frame capture? And this might explain why you need to rescan some pcap files. I suggest you remove this changes and give a try. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:319: "Whether or not to enable cautpure at the payload(data) portion of the packet", I suggest to remove "(data)" https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:390: m_doPayloadCapture (false) You do not need to initialize those in the constructor, since they are initialized in the attribute.
Sign in to reply to this message.
Looks already in good shape in general, please take care about coding style rules, we have a script you can run to correct you code without any effort. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... File src/wifi/model/interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... src/wifi/model/interference-helper.cc:254: for (NiChanges::const_iterator i = m_niChanges.begin (); i != m_niChanges.end (); ++i) why not + 1? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... src/wifi/model/interference-helper.cc:267: } Not sure why those changes are needed, can you clarify? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2339: Time currentRxStage = Seconds (0); Initialization not needed here, only used locally later in this fct https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2514: << ", old preamble type "<<m_rxPreambleType space missing https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2530: CalculateCcaBusyDuration (); do not use tab, please run style check before committing your code https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2539: { I would expect such if earlier in the process, so that if false you keep current implementation, otherwise you call this fct. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2561: { Indeed, from my previous comment, this could have come much earlier in the process, and we would avoid for sure any regression issues https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2648: m_state->SwitchFromRxAbort(packet, snr); missing space https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2693: CalculateCcaBusyDuration (); Why are those two lines needed? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2702: CalculateCcaBusyDuration (); Why are those two lines needed?
Sign in to reply to this message.
I will update the patches soon. https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:56: #include "ns3/nist-error-rate-model.h" On 2017/01/05 21:30:55, S. Deronne wrote: > Why are you moving to Nist? I am more familiar with nist model. When I test the code, it is easier for me to notice the difference. And the default error model in wifi-helper is nist mode, therefore, I changed the model to nist. If this has unexpected impacts, we can change it back. https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:128: txModeB ("OfdmRate12Mbps"), On 2017/01/05 21:30:55, S. Deronne wrote: > Any reason for this change? No particular reason. Any supported tx mode should work here. I just choose one with which I am more familiar. https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:165: Ptr<ErrorRateModel> error = CreateObject<NistErrorRateModel> (); On 2017/01/05 15:32:22, Tom Henderson wrote: > why is this change needed? The same answer at line 56 https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */ On 2017/01/05 15:32:22, Tom Henderson wrote: > Missing GPLv2 Done. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:43: double sinrDb = 10 * std::log10(sinr); On 2017/01/05 15:32:22, Tom Henderson wrote: > We have WifiUtils::RatioToDb () for the above computation. Done. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.h (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.h:10: class FrameCaptureModel : public Object On 2017/01/05 15:32:22, Tom Henderson wrote: > Missing doxygen, including group label and class doxygen > https://www.nsnam.org/docs/manual/html/documentation.html#id1 Done. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... File src/wifi/model/interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... src/wifi/model/interference-helper.cc:267: } On 2017/01/07 08:31:48, S. Deronne wrote: > Not sure why those changes are needed, can you clarify? Tom, Sebastien, The reason I changed the code is because I also used to this method to calculate the SINR of a signal which overlaps with another one. For example, while receiving signal A, signal B arrives. m_firstPower accounts for the niChanges before signal A but not signal B. To calculate the SINR of signal B, the receiver has to iterate from the beginning of the niChange list to the end, and then find the overlapped signals with signal B, especially in these scenarios, between signal A and signal B, there are many other signals arriving at the receiver. The calculated SINR of signal B is used to determine the probability of the successful reception switching. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... File src/wifi/model/interference-helper.h (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... src/wifi/model/interference-helper.h:213: * \param sinr the sinr value at the current time On 2017/01/05 15:32:22, Tom Henderson wrote: > \return the probability of successful capture Done. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/interferen... src/wifi/model/interference-helper.h:218: * \param sinr the sinr value at the current time On 2017/01/05 15:32:22, Tom Henderson wrote: > \return the probability of successful capture Done. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:195: DoubleValue (0.0), On 2017/01/05 15:32:22, Tom Henderson wrote: > this seems to be a separate change unrelated to frame capture? I agree this is a separate change from frame capture. However, I am not sure why the default value of tx/rx gain set to 1dB. I think the default value should be 0 dB. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:195: DoubleValue (0.0), On 2017/01/05 21:30:56, S. Deronne wrote: > On 2017/01/05 15:32:22, Tom Henderson wrote: > > this seems to be a separate change unrelated to frame capture? > > And this might explain why you need to rescan some pcap files. I suggest you > remove this changes and give a try. Thanks, I will change it back and run the test again. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:319: "Whether or not to enable cautpure at the payload(data) portion of the packet", On 2017/01/05 15:32:22, Tom Henderson wrote: > capture Done. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:319: "Whether or not to enable cautpure at the payload(data) portion of the packet", On 2017/01/05 21:30:56, S. Deronne wrote: > I suggest to remove "(data)" Done. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:390: m_doPayloadCapture (false) On 2017/01/05 21:30:56, S. Deronne wrote: > You do not need to initialize those in the constructor, since they are > initialized in the attribute. Has been removed, thanks. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2339: Time currentRxStage = Seconds (0); On 2017/01/07 08:31:48, S. Deronne wrote: > Initialization not needed here, only used locally later in this fct Has changed the declaration place https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2511: { On 2017/01/05 15:32:22, Tom Henderson wrote: > NS_LOG_FUNCTION statement Added the NS_LOG_FUNCTION statement https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2514: << ", old preamble type "<<m_rxPreambleType On 2017/01/07 08:31:48, S. Deronne wrote: > space missing Added the spacing https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2561: { On 2017/01/07 08:31:48, S. Deronne wrote: > Indeed, from my previous comment, this could have come much earlier in the > process, and we would avoid for sure any regression issues Thanks, the logic has changed and I will run the test again. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2575: { On 2017/01/05 15:32:22, Tom Henderson wrote: > Need NS_LOG_FUNCTION statement Has added https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2638: { On 2017/01/05 15:32:22, Tom Henderson wrote: > Need NS_LOG_FUNCTION statement Has added https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2648: m_state->SwitchFromRxAbort(packet, snr); On 2017/01/07 08:31:49, S. Deronne wrote: > missing space Added the space https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2656: { On 2017/01/05 15:32:22, Tom Henderson wrote: > Need NS_LOG_FUNCTION statement at least Has added https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2693: CalculateCcaBusyDuration (); On 2017/01/07 08:31:49, S. Deronne wrote: > Why are those two lines needed? The main reason I am doing this is because if a signal fails the PLCP check, I do not think the receiver is able to continue the reception of the signal since it cannot recognize whether the signal is a wi-fi signal. Then the signal becomes a portion of energy on the channel. Therefore, I think at this time the receiver should stop receiving this signal and switch from the RX state to another state. Otherwise, if we do not change the state of the receiver from the RX state to the IDLE/CCA_BUSY state the receiver will then try to run the frame capture function to lock a newly arrived signal which makes it much harder. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h File src/wifi/model/wifi-phy.h (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1524: void SetPreambleCapture (bool enable); On 2017/01/05 15:32:22, Tom Henderson wrote: > It seems that these setters and getters could be removed by just assigning the > boolean directly in the attribute. Has been removed, thanks https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1758: * Due to newly arrived signal, the old reception cannot be continued and has to be abort On 2017/01/05 15:32:22, Tom Henderson wrote: > missing parameter documentation has added the missing documentation https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1770: * \param mpdutype On 2017/01/05 15:32:22, Tom Henderson wrote: > two extraneous parameters Thanks, has been removed. https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1777: * \param tag On 2017/01/05 15:32:22, Tom Henderson wrote: > extraneous parameter Thanks, has removed the parameter
Sign in to reply to this message.
I will update the patches soon.
Sign in to reply to this message.
https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:56: #include "ns3/nist-error-rate-model.h" On 2017/01/11 16:36:03, cb3974 wrote: > On 2017/01/05 21:30:55, S. Deronne wrote: > > Why are you moving to Nist? > > I am more familiar with nist model. When I test the code, it is easier for me to > notice the difference. And the default error model in wifi-helper is nist mode, > therefore, I changed the model to nist. If this has unexpected impacts, we can > change it back. Ok I also think we should preferably use Nist https://codereview.appspot.com/319050043/diff/20001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:128: txModeB ("OfdmRate12Mbps"), On 2017/01/11 16:36:03, cb3974 wrote: > On 2017/01/05 21:30:55, S. Deronne wrote: > > Any reason for this change? > > No particular reason. Any supported tx mode should work here. I just choose one > with which I am more familiar. So we can keep the highest rate 54 Mbit/s here? https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/20001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:195: DoubleValue (0.0), I also do not know why 1 was selected as default. Tom, can we change this to 0 as default? If yes I suggest to do it in a separate commit and carefully adapt tests.
Sign in to reply to this message.
Thanks for the updated patch. Few general comments: - missing example to show frame capture effects; - changes done in InterferenceHelper are not fully clear and are causing regression failures; - error model is not explained. Could you provide a new patch set? I would like we finish this up for the upcoming release. https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/frame-c... File src/wifi/examples/frame-capture-model-validation.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/frame-c... src/wifi/examples/frame-capture-model-validation.cc:21: // Please add comments to explain the script and its outputs https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:225: cmd.AddValue ("enableMultipleFrameCaptureTests", "Enable test frame capture in difference scenarios", enableMultipleFrameCaptureTests); Note clear what the purpose of enableMultipleFrameCaptureTests is... I would instead expect to see two parameters to enable either frame or payload capture mode. Could you please provide a more meaningful explication? https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:291: enableMultipleFrameCaptureTests = true; Why do you force enableFrameCapture and enableMultipleFrameCaptureTests to true regardless of the passed configuration? https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:87: Still unclear where all those calculations come from and how this has been validated so far. https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/interferen... File src/wifi/model/interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/interferen... src/wifi/model/interference-helper.cc:264: else if (event->GetEndTime () == i->GetTime () && event->GetRxPowerW () == -i->GetDelta ()) I am doubting those changes, and they cause regression failures. Could you please double check this part of the code? https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/interferen... File src/wifi/model/interference-helper.h (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/interferen... src/wifi/model/interference-helper.h:340: // following variables used for frame capture capability please remove this comment https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2335: || preamble == WIFI_PREAMBLE_NONE) Why preamble == WIFI_PREAMBLE_NONE ??? https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2337: NS_LOG_DEBUG ("cannot perform frame capture capture"); I suggest to remove this: it sounds like something went nok while the user simply disabled frame capture mode https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2459: } The goto and a fct call can result in a different behavior. The goto will directly go to maybeCcaBusy and skip all other operations, whereas a call to CalculateCcaBusyDuration will perform the computation and will go back to the caller and will then continue operations. Please double check this does not introduce unexpected behavior.
Sign in to reply to this message.
I have updated the patch and now no regression tests fail. However, I have introduced some new changes to the InterferenceHelper class. The details can be in my reply. I also changed test-interference-helper.cc to show the impact of with/without frame capture capability by testing the code in different packet collision scenarios. The explanation can be found in my reply and also the comments in the code. https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/frame-c... File src/wifi/examples/frame-capture-model-validation.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/frame-c... src/wifi/examples/frame-capture-model-validation.cc:21: // On 2017/01/27 22:11:11, S. Deronne wrote: > Please add comments to explain the script and its outputs In the new patch, I have added some explanation. https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:225: cmd.AddValue ("enableMultipleFrameCaptureTests", "Enable test frame capture in difference scenarios", enableMultipleFrameCaptureTests); On 2017/01/27 22:11:11, S. Deronne wrote: > Note clear what the purpose of enableMultipleFrameCaptureTests is... > I would instead expect to see two parameters to enable either frame or payload > capture mode. > Could you please provide a more meaningful explication? There are four different packet collision scenarios. I updated this part in the new patch. The reason I set each test run multiple times, i.e., sending 1000 packets, is because I want to the user of this example can also recovery the model curve and valid the results. For each test and each SINR value, 1000 packets were sent and the user can count how many packets were captured successfully, how many packets were received successfully. Then the user can calculate the probability of capture/recovery for the given packet collision scenario and the given SINR value. However, how to identify which packet is captured/recovered successfully depends on the user's implementation, e.g., they can count the number of packets from the simulation logs. https://codereview.appspot.com/319050043/diff/40001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:291: enableMultipleFrameCaptureTests = true; On 2017/01/27 22:11:11, S. Deronne wrote: > Why do you force enableFrameCapture and enableMultipleFrameCaptureTests to true > regardless of the passed configuration? Have been removed in the new patch https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:87: On 2017/01/27 22:11:11, S. Deronne wrote: > Still unclear where all those calculations come from and how this has been > validated so far. Some explanations have been added in the new patch https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/interferen... File src/wifi/model/interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/interferen... src/wifi/model/interference-helper.cc:264: else if (event->GetEndTime () == i->GetTime () && event->GetRxPowerW () == -i->GetDelta ()) On 2017/01/27 22:11:11, S. Deronne wrote: > I am doubting those changes, and they cause regression failures. > Could you please double check this part of the code? I have updated the implementation of this part in the new patch. And now, all the tests are passed. The reason for the failed regression test was because of a special case that two packets arrive the receiver at exactly the same time. This is a rare case in the wireless communication context. I have fixed this issue in the new patch. The main reason I changed this function was that I want to use the same function to calculate interference power for a late arrived packet. Let us say, packet A arrives first, and then packet B arrives. The calculation before the modification was generally for packet A, not for packet B. Now we want to calculate the SINR for packet B in order to determine if it can be captured. For packet B, packet A is interference, and the rxPower of packet A has not been considered in the m_firstPower. Therefore, when calculating the interference for packet B, we want to iterate all the NI changes that happen before packet B. But there are other special cases, e.g., packets arrive at exactly the same time with the same rxPower. For this case, we can not identify which NI change is for the event and which NI change is for the interference. For example, packet A, B, C arrive the receiver at the same time with the same rxPower. Now, we want to calculate the SINR for packet B, it may be impossible to identify which NI change in m_niChanges is for packet B and others for packet A and packet C. If packet B is the first one in the list, then the m_firstPower is its interference. However, if packet B is the third on in the list, then both packet A and C become its interference. Therefore, I added the event which causes the NI change in the NiChange class to help identify different NI changes. Please let me know if you have any concern of this big change. https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2335: || preamble == WIFI_PREAMBLE_NONE) On 2017/01/27 22:11:11, S. Deronne wrote: > Why preamble == WIFI_PREAMBLE_NONE ??? The reason is that the newly arrived packet can be within a mpdu frame and it contains no header information. This packet can not be captured since it can not provide further information for decoding the packet. https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2337: NS_LOG_DEBUG ("cannot perform frame capture capture"); On 2017/01/27 22:11:11, S. Deronne wrote: > I suggest to remove this: it sounds like something went nok while the user > simply disabled frame capture mode Has been removed https://codereview.appspot.com/319050043/diff/40001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2459: } On 2017/01/27 22:11:11, S. Deronne wrote: > The goto and a fct call can result in a different behavior. > The goto will directly go to maybeCcaBusy and skip all other operations, whereas > a call to CalculateCcaBusyDuration will perform the computation and will go back > to the caller and will then continue operations. > Please double check this does not introduce unexpected behavior. > I agree they are different. However, the reason I moved the CcaBusy calculation into a new function is because it was needed in other function. I have checked the implementation. It seems no other unexpected behaviors were introduced.
Sign in to reply to this message.
https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.cc (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:31: // ns-3 NIST model to decode a OFDM 6 Mbps signal with 40 us transmission duration). If a different error model were to be used (e.g. Yans, or a link sim-based model that is more optimistic than NIST), would this model need to be refitted? For instance, Yans and NIST curves may differ by 3 to 4 dB. If so, is there a methodology that someone else could follow to obtain constants applicable to a different error model? https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/interferen... File src/wifi/model/interference-helper.h (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/interferen... src/wifi/model/interference-helper.h:216: double CalculatePreambleCaptureProbability (double sinr); should these be const methods? https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy-s... File src/wifi/model/wifi-phy-state-helper.cc (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy-s... src/wifi/model/wifi-phy-state-helper.cc:550: NotifyRxEndError (); I am skeptical of these calls and the need to have this new RxAbort state switch. m_rxErrorTrace is typically only called at the end of the frame, but here is being added to a drop case in the middle of the frame. However, this trace is not called upon late arriving frame in the current model; only the m_phyRxDropTrace is called. I might expect instead that the state stays in Rx even upon a SwitchReception event, and that the case of a earlier-arriving frame that loses out to a later arriving stronger frame is handled the same as in the current model when a late frame arrives to an already busy receiver. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2334: || m_doPayloadCapture == false it seems that the logic precludes setting only one of these capture effect flags to true? This probably leads to unexpected behavior. If this is intended, perhaps only a single attribute 'FrameCaptureEnabled' should be used. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2345: Time currentRxStage = Simulator::Now () - m_rxPacketEvent->GetStartTime (); s/currentRxStage/currentRxDuration https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2356: if (doReceptionSwitch) why are these steps not taken in CheckPreambleCapture and CheckPayloadCapture? It seems to me that those methods do most but not all of the steps required to switch (i.e. they call AbortCurrentReception() and SwitchReception()) but they don't do this last piece of scheduling the new events for some reason. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2489: if (m_random->GetValue () > snrPer.per) // the reception of the old packet can be continued does this model cause reception of packets to be evaluated multiple times against the error model (i.e. check each time this method is called to check for preamble capture, and, as before, at the end of reception)? If so, this is probably not desired. The tests consider only two packets A and B, but what if multiple (A, B, C, and more) exist? I might expect instead that the check compares snr of current packet against snr of newly arriving packet and abandons current packet if snr of newly arriving packet is some threshold dB above current packet, without consulting error model about reception at this point. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.h File src/wifi/model/wifi-phy.h (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1750: * Check whether preamble capture can be triggered This documentation is not as clear as it could be. This method checks whether 'packet' should take the place of the packet currently being received. If false, then 'packet' is dropped by this method (i.e. user should not subsequently drop the packet). If true, then the packet currently being received is dropped; however, the switching is not totally completed by this method and instead the client must schedule some new receive events (see my comment in the implementation about this).
Sign in to reply to this message.
I am sorry that I could not reply early. I am traveling these days. I will try to response the comments later day or tomorrow.
Sign in to reply to this message.
I have made a few changes to the patches according to Tom's comments. What I have done is the state transition part. I think we need a quick state transition between the old RX state and the new RX state when the capture happens. I think it would be more informative when someone reads the simulation logs. I agree that we probably need a new state, RxAbort, for this. But I am not sure if we want this new state implemented in this patch immediately. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/frame-capt... File src/wifi/model/frame-capture-model.cc (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/frame-capt... src/wifi/model/frame-capture-model.cc:31: // ns-3 NIST model to decode a OFDM 6 Mbps signal with 40 us transmission duration). On 2017/01/29 17:35:34, Tom Henderson wrote: > If a different error model were to be used (e.g. Yans, or a link sim-based model > that is more optimistic than NIST), would this model need to be refitted? For > instance, Yans and NIST curves may differ by 3 to 4 dB. > > If so, is there a methodology that someone else could follow to obtain constants > applicable to a different error model? The explanation in the code may be confusing. But, what I was trying to say is that we used the Nist model as the base model and then tried to calibrate the model according to the simulation output. In our model, the probability for preamble capture changes from 0 to 1 in an SINR range 8 to 10 dB. This is generally calibrated based on the simulation output. If a different base model is used, I think this probability transition range should keep the same. But for one particular SINR value, the corresponding probability value may be slightly different, given that the transition range is really small and the probability curve already changes sharply. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/interferen... File src/wifi/model/interference-helper.h (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/interferen... src/wifi/model/interference-helper.h:216: double CalculatePreambleCaptureProbability (double sinr); On 2017/01/29 17:35:34, Tom Henderson wrote: > should these be const methods? Yes, thanks. I have changed this. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy-s... File src/wifi/model/wifi-phy-state-helper.cc (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy-s... src/wifi/model/wifi-phy-state-helper.cc:550: NotifyRxEndError (); On 2017/01/29 17:35:34, Tom Henderson wrote: > I am skeptical of these calls and the need to have this new RxAbort state > switch. m_rxErrorTrace is typically only called at the end of the frame, but > here is being added to a drop case in the middle of the frame. However, this > trace is not called upon late arriving frame in the current model; only the > m_phyRxDropTrace is called. > > I might expect instead that the state stays in Rx even upon a SwitchReception > event, and that the case of a earlier-arriving frame that loses out to a later > arriving stronger frame is handled the same as in the current model when a late > frame arrives to an already busy receiver. I think adding a new state may be a better idea. But before adding the new state, let me try to remove these calls. But I would say it would be better if there is a quick state transition between the old RX and the new RX state when the capture happens. This state transition could be really useful for user identify the reception behavior. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2334: || m_doPayloadCapture == false On 2017/01/29 17:35:35, Tom Henderson wrote: > it seems that the logic precludes setting only one of these capture effect flags > to true? This probably leads to unexpected behavior. If this is intended, > perhaps only a single attribute 'FrameCaptureEnabled' should be used. This is a mistake. Thanks for pointing it out. I have updated it in the new version https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2345: Time currentRxStage = Simulator::Now () - m_rxPacketEvent->GetStartTime (); On 2017/01/29 17:35:35, Tom Henderson wrote: > s/currentRxStage/currentRxDuration has changed https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2356: if (doReceptionSwitch) On 2017/01/29 17:35:35, Tom Henderson wrote: > why are these steps not taken in CheckPreambleCapture and CheckPayloadCapture? > It seems to me that those methods do most but not all of the steps required to > switch (i.e. they call AbortCurrentReception() and SwitchReception()) but they > don't do this last piece of scheduling the new events for some reason. I have moved these steps to CheckPreambleCapture and CheckPayloadCapture method. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2489: if (m_random->GetValue () > snrPer.per) // the reception of the old packet can be continued On 2017/01/29 17:35:35, Tom Henderson wrote: > does this model cause reception of packets to be evaluated multiple times > against the error model (i.e. check each time this method is called to check for > preamble capture, and, as before, at the end of reception)? If so, this is > probably not desired. The tests consider only two packets A and B, but what if > multiple (A, B, C, and more) exist? > > I might expect instead that the check compares snr of current packet against snr > of newly arriving packet and abandons current packet if snr of newly arriving > packet is some threshold dB above current packet, without consulting error model > about reception at this point. Let us say packet A arrives first, and then the packet B. Before checking the whether packet B can be captured or not, we first used the error model to check whether the reception of packet A can be continued or not with the added interference power from packet B. If not, the reception of packet A is terminated and this is why we can it a reception ended with error. Without this step, I think we may need some new design, e.g., as you mentioned, adding a new state, to remove this step. I think the capture test is always about two packets. After checking the capture probability for packet B while receiving packet A, there are following possible outcomes: 1) The reception of packet A continues, but the packet B was dropped. Then the next arrived packet becomes the packet B again and then repeats the checking process. 2) Both packet A and B are dropped. The new packet arrives while the channel is at CCA_BUSY state. 3) The packet A is dropped, but the packet B was captured. Then the packet B is packet A now for the next arrived packet. The checking process repeats again. Therefore, even multiple packets arrive while the other reception is going on. Only two packets are checked each time. However, I agree these packets will in the interference list while calculating the per for the currently receiving packet. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.h File src/wifi/model/wifi-phy.h (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.h... src/wifi/model/wifi-phy.h:1750: * Check whether preamble capture can be triggered On 2017/01/29 17:35:35, Tom Henderson wrote: > This documentation is not as clear as it could be. This method checks whether > 'packet' should take the place of the packet currently being received. If > false, then 'packet' is dropped by this method (i.e. user should not > subsequently drop the packet). If true, then the packet currently being > received is dropped; however, the switching is not totally completed by this > method and instead the client must schedule some new receive events (see my > comment in the implementation about this). Has modified. Thanks for pointing this out.
Sign in to reply to this message.
https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy-s... File src/wifi/model/wifi-phy-state-helper.cc (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy-s... src/wifi/model/wifi-phy-state-helper.cc:550: NotifyRxEndError (); I am also wondering why is this state needed? Receiver stays in Rx state, it just receiving another packet. Of course, the rx end time can be recomputed, but I do not see a need to have this change. https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/60001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2489: if (m_random->GetValue () > snrPer.per) // the reception of the old packet can be continued I think Tom is right, this should compare SNR of all involved packets to make the decision.
Sign in to reply to this message.
Thanks for this update. My main concern unfortunately remains: frame capture model is not generic enough and should be adapted so that users can adapt it to their needs. I also miss an example showing performance comparison between the two frame capture cases and a scenario without frame capture enabled. Also, interference example needs some small rework. Other comments: - please rebase your patch with the latest ns-3-dev, I encountered some (minor) merge conflicts (mainly due to style changes); - please update Doxygen correctly. Since efforts from Robert Ammon, we want to keep no doxygen warnings. You can use "./doc/doxygen.warnings.report.sh -m wifi" command to give a check; - Please remove unwanted committed files.
Sign in to reply to this message.
https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/frame-c... File src/wifi/examples/frame-capture-model-validation.cc (right): https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/frame-c... src/wifi/examples/frame-capture-model-validation.cc:45: I suggest to draw both curves on the same plot https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (left): https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:197: cmd.Parse (argc, argv); I think you should rework how tests are selected, a single attribute would suffice to select the type of capture mode: none/preamble/payload. Parameters for power gap could then be added: min, max and range. https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:62: #define PAYLOAD_CAPTURE_TEST 4 not sure this is needed, a better approach should be envisioned (see comments below) https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:87: int numSentPackets; what is the need for this numSentPackets? https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:233: testDifferentPacketCollisionScenarios); not clear from description https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:299: { Below should be reworked, I think we can live with a single experiment, and based on parameters it will perform what we expect. https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:379: input.numSentPackets = 1000; Why 1000? You say you have 2 packets. Still unclear the purpose of this param. https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy-s... File src/wifi/model/wifi-phy-state-helper.cc (right): https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy-s... src/wifi/model/wifi-phy-state-helper.cc:546: { I do not see a real need for this, expect for tracing purpose... but I do not see any traces here. Please explain why this is there. https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:1963: || payloadMode.GetUniqueName () == "HtMcs31") Style cleanup should not be part of the review https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2335: || preamble == WIFI_PREAMBLE_NONE) Why a check on preamble == none? https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2346: if (m_rxPreambleType != WIFI_PREAMBLE_NONE all none preamble will be stopped earlier due to the previous check, my assumption is that previous check is not correct
Sign in to reply to this message.
I am indeed testing the example and it is totally not intuitive and we cannot see a lot (too much traces!). Also, I think the error model in not needed. It should simply switch to the packet with the highest power. I guess something like that is implemented in the field, and this avoid the need of a generic model.
Sign in to reply to this message.
https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/frame-c... File src/wifi/examples/frame-capture-model-validation.cc (right): https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/frame-c... src/wifi/examples/frame-capture-model-validation.cc:45: On 2017/02/20 21:42:25, S. Deronne wrote: > I suggest to draw both curves on the same plot Let me try to draw two curves on the same plot https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... File src/wifi/examples/test-interference-helper.cc (right): https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:62: #define PAYLOAD_CAPTURE_TEST 4 On 2017/02/20 21:42:25, S. Deronne wrote: > not sure this is needed, a better approach should be envisioned (see comments > below) The reason explained below https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:87: int numSentPackets; On 2017/02/20 21:42:25, S. Deronne wrote: > what is the need for this numSentPackets? The reason explained blow. https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:299: { On 2017/02/20 21:42:25, S. Deronne wrote: > Below should be reworked, I think we can live with a single experiment, and > based on parameters it will perform what we expect. Let us explain how I thought about this example. There are four packet collision situations. 1) The first packet and the second packet collide at the preamble portion of the first packet. In this case, firstly we want to know at which point the first packet can still be received even with the presence of the second packet. To show this, what I did is to in the following example (PREAMBLE RECOVERY case) is to send two packets each time. The second packet is delayed by 10 us. Such that two packets collide at the preamble portion of the packet. Then repeat this experiment for many times. The reason I did so is that the model we used was a probability model and from 1000 packets the user can count how many the first sent packets are successfully received, then calculate a probability for this configuration. Then I gradually increase the tx power of the first packet, therefore, the SINR of the first packet increases as well. Then the test is repeated for different SINR. The poweGap here is to gradually increase the SINR. Then at the end of the test, for each SINR value, the user may have a receiving probability for that SINR value. The user can compare the probability with the existing reference to see if the output matches. 2) A similar configuration as previous one, but this time we want to see when the second packet can be captured. So this time instead of increasing the tx power of the first packet, I increase the tx power of the second packet. Similarly each time two packets are used to create the collision situation but such experiment is repeated many times (1000 times) for a given SINR value. Therefore, the user can also reproduce the probability v.s. SINR curve for this test and compare the curve with our reference. 3) and 4) are for packet recovery and packet capture for payload part. Therefore, I designed to use four cases and for each case repeat the experiment 1000 times in order to obtain a statistical probability for that given situation. https://codereview.appspot.com/319050043/diff/80001/src/wifi/examples/test-in... src/wifi/examples/test-interference-helper.cc:379: input.numSentPackets = 1000; On 2017/02/20 21:42:25, S. Deronne wrote: > Why 1000? You say you have 2 packets. Still unclear the purpose of this param. Two packets are used to create the packet collision. The experiment is repeated 1000 times to obtain a statistical probability for that particular SINR. At the end of the time, one can generate a probability v.s. SINR curve. https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy-s... File src/wifi/model/wifi-phy-state-helper.cc (right): https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy-s... src/wifi/model/wifi-phy-state-helper.cc:546: { On 2017/02/20 21:42:26, S. Deronne wrote: > I do not see a real need for this, expect for tracing purpose... but I do not > see any traces here. Please explain why this is there. DoSwitchFromRx() will change the state from RX to IDLE or CCA_BUSY when terminating the previous transmission before starting the new one. https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy.cc File src/wifi/model/wifi-phy.cc (right): https://codereview.appspot.com/319050043/diff/80001/src/wifi/model/wifi-phy.c... src/wifi/model/wifi-phy.cc:2335: || preamble == WIFI_PREAMBLE_NONE) On 2017/02/20 21:42:26, S. Deronne wrote: > Why a check on preamble == none? If the preamble of the new packet is none, it contains no information of decoding the packet, so I do not think such packet can be captured.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
