Bugzilla – Bug 2488
Error in UanPdp::SumTapsFromMaxNc for single tap
Last modified: 2016-09-10 15:03:58 UTC
If there is a single tap, this function will currently return the value of that tap, regardless of the values of "delay" and "duration" passed in. This does not seem correct to me, as if I pass in a delay > 0, I would suspect that I should get a 0 back from this function (there is no value at any delay other than 0 defined for a pdp of length 1). Returning the value of the tap produces incorrect results in the UanPhyCalcSinrFhFsk::CalcSinrDb function when it calculates isiUpa. For the case of a single tap, isiUpa (inter-symbol interference) should be 0, but instead, it ends up being equal to the received power (i.e., all the received power is treated as interference). I suggest replacing the following code in UanPdp::SumTapsFromMaxNc: if (m_resolution <= Seconds (0)) { NS_ASSERT_MSG (GetNTaps () == 1, "Attempted to sum taps over time interval in " "UanPdp with resolution 0 and multiple taps"); return m_taps[0].GetAmp (); } with: if (m_resolution <= Seconds (0)) { NS_ASSERT_MSG (GetNTaps () == 1, "Attempted to sum taps over time interval in " "UanPdp with resolution 0 and multiple taps"); if(delay == 0) return m_taps[0].GetAmp (); else return 0; }
Hi Randall, thanks. I agree with the fix, which should be ported to WOSS Integration Framework by they way.
Hi Randall, the proposed patch should be modified to (two functions to be modified, check abs since Nc is non coherent, check NS3 syntax): double UanPdp::SumTapsFromMaxNc (Time delay, Time duration) const { if (m_resolution <= Seconds (0)) { NS_ASSERT_MSG (GetNTaps () == 1, "Attempted to sum taps over time interval in " "UanPdp with resolution 0 and multiple taps"); if (delay == 0) { return std::abs (m_taps[0].GetAmp ()); } return 0; } std::complex<double> UanPdp::SumTapsFromMaxC (Time delay, Time duration) const { if (m_resolution <= Seconds (0)) { NS_ASSERT_MSG (GetNTaps () == 1, "Attempted to sum taps over time interval in " "UanPdp with resolution 0 and multiple taps"); if (delay == 0) { return m_taps[0].GetAmp (); } return 0; } furthermore please check how are these functions being called around UAN code and check for possible side effects in case of the new behavior. thanks Federico
from max NC is actually std::complex<double> UanPdp::SumTapsFromMaxC (Time delay, Time duration) const { if (m_resolution <= Seconds (0)) { NS_ASSERT_MSG (GetNTaps () == 1, "Attempted to sum taps over time interval in " "UanPdp with resolution 0 and multiple taps"); if (delay == 0) { return m_taps[0].GetAmp (); } return ::std::complex<double> (0.0, 0.0); }
Created attachment 2566 [details] uan-prop-model.cc patch
Comment on attachment 2566 [details] uan-prop-model.cc patch I searched the entire NS3 code base and it appears that the SumTapsFromMax functions are only used in this one function (UanPhyCalcSinrFhFsk::CalcSinrDb) and thus there should be no negative side effects of this mod.
Hi Randall, thanks for the patch. unfortunately the indentation is not ns3 compliant, please check my example at comment 2 which is compliant. if you have any doubt please check https://www.nsnam.org/developers/contributing-code/coding-style/ thanks again
aside for this small issue, the patch looks good :)
I'm not sure what is not compliant. The NS3 coding style states that "single-statement blocks may be placed on the same line of an if () statement". I have also used 2 spaces for the indentation. Is there something else you are referring to?
My fault then, sorry
(In reply to Federico Guerra from comment #9) > My fault then, sorry Was this mistakenly marked RESOLVED FIXED? Nothing was pushed to ns-3-dev. Should I push? It is OK to put single statement blocks on one line, although most of the codebase still uses braces for that. One small change I would make is to change 'delay == 0' to 'delay.IsZero ()'. I think it will work the same, but just wonder about making it immune to future compiler warnings about type mismatch.
Thanks Tom for your comment. Yep my fault on the state change, I'm not that familiar with Bugzilla states
pushed in changeset 12313:69ac866f029f