Issue 14632043: Introduce additional time units (Closed)
|
Created:
10 years, 6 months ago by Vedran Miletić Modified:
10 years, 4 months ago CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
DescriptionIntroduce additional time units
Patch Set 1 #
Total comments: 6
Patch Set 2 : Updated patch set per Peter and Tom's comments #
Total comments: 5
MessagesTotal messages: 12
Hello Vedran, When you tossed this into the bug954 discussion I thought it would be easy to add later, and I had it on my to do list RSN. However, I think there are some subtleties with the way we do conversions using integer arithmetic. See my comments on time.cc lines 201 and 235 for a start. https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h File src/core/model/nstime.h (right): https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... src/core/model/nstime.h:111: A = 0, //!< year Please document that you've chosen 365.25 days per year, a Julian year. https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... src/core/model/nstime.h:216: * - `a` (years) Should we support alternative symbols when converting from strings? 'mn', 'hr', 'day', 'week', 'y', 'yr', 'year' https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc File src/core/model/time.cc (right): https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201 src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576, 6048, 864, 36, 6, 1, 1, 1, 1, 1, 1 }; I think the choice of int32_t here (and int for quotient, below) is a mistake. It leads to premature truncation in computing factor. To see the problem, run Time::SetResolution (Time::D); std::cout << (Days (4 * 365.25).GetYears ()) << std::endl; I get '4.0027397', but it should be 4. I don't have a good suggestion yet on how to fix this... https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode219 src/core/model/time.cc:219: double realFactor = std::pow (10, shift) I need 'std::pow (10, (double)shift)' to compile. std::pow(int, int) is ambiguous) https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode225 src/core/model/time.cc:225: // to avoid checking equality of doubles You could avoid this completely by if (realFactor > 1) { ... } else if (realFactor < 1) { ... } else { ... } https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode235 src/core/model/time.cc:235: info->timeFrom = int64x64_t (factor); We should rethink the use of int64x64_t here. Because we treat factor > 1 and factor < 1 separately, only one of the 64-bit fields is ever being used. We could just as well store factor in info, and rely on to/fromMul to select multiply or divide in the conversions.
Sign in to reply to this message.
On 2013/10/18 18:25:56, Peter Barnes wrote: > Hello Vedran, > > When you tossed this into the bug954 discussion I thought it would be easy to > add later, and I had it on my to do list RSN. > > However, I think there are some subtleties with the way we do conversions using > integer arithmetic. See my comments on time.cc lines 201 and 235 for a start. Hi Peter, thanks for your review. My comments are inline. > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h > File src/core/model/nstime.h (right): > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... > src/core/model/nstime.h:111: A = 0, //!< year > Please document that you've chosen 365.25 days per year, a Julian year. Sure. By the way, do you believe that 365 days would have been a better choice? > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... > src/core/model/nstime.h:216: * - `a` (years) > Should we support alternative symbols when converting from strings? 'mn', 'hr', > 'day', 'week', 'y', 'yr', 'year' Sure, why not? > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc > File src/core/model/time.cc (right): > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201 > src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576, 6048, > 864, 36, 6, 1, 1, 1, 1, 1, 1 }; > I think the choice of int32_t here (and int for quotient, below) is a mistake. > It leads to premature truncation in computing factor. To see the problem, run > > Time::SetResolution (Time::D); > std::cout << (Days (4 * 365.25).GetYears ()) << std::endl; > > I get '4.0027397', but it should be 4. > > I don't have a good suggestion yet on how to fix this... Damn, I have to nail this. > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode219 > src/core/model/time.cc:219: double realFactor = std::pow (10, shift) > I need 'std::pow (10, (double)shift)' to compile. std::pow(int, int) is > ambiguous) Fixed. > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode225 > src/core/model/time.cc:225: // to avoid checking equality of doubles > You could avoid this completely by > > if (realFactor > 1) { ... } > else if (realFactor < 1) { ... } > else { ... } Can you guarantee that our 'intuitive' realFactor == 1 will not end in realFactor > 1 or realFactor < 1 branch unless we handle it first? > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode235 > src/core/model/time.cc:235: info->timeFrom = int64x64_t (factor); > We should rethink the use of int64x64_t here. Because we treat factor > 1 and > factor < 1 separately, only one of the 64-bit fields is ever being used. We > could just as well store factor in info, and rely on to/fromMul to select > multiply or divide in the conversions. Neat. Let me try to nail the imprecision first, and then we can discuss this further.
Sign in to reply to this message.
On 2013/10/21 07:51:17, Vedran Miletić wrote: > On 2013/10/18 18:25:56, Peter Barnes wrote: > > Hello Vedran, > > > > When you tossed this into the bug954 discussion I thought it would be easy to > > add later, and I had it on my to do list RSN. > > > > However, I think there are some subtleties with the way we do conversions > using > > integer arithmetic. See my comments on time.cc lines 201 and 235 for a start. > > Hi Peter, > > thanks for your review. My comments are inline. > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h > > File src/core/model/nstime.h (right): > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... > > src/core/model/nstime.h:111: A = 0, //!< year > > Please document that you've chosen 365.25 days per year, a Julian year. > > Sure. By the way, do you believe that 365 days would have been a better choice? > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... > > src/core/model/nstime.h:216: * - `a` (years) > > Should we support alternative symbols when converting from strings? 'mn', > 'hr', > > 'day', 'week', 'y', 'yr', 'year' > > Sure, why not? > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc > > File src/core/model/time.cc (right): > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201 > > src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576, 6048, > > 864, 36, 6, 1, 1, 1, 1, 1, 1 }; > > I think the choice of int32_t here (and int for quotient, below) is a mistake. > > > It leads to premature truncation in computing factor. To see the problem, run > > > > Time::SetResolution (Time::D); > > std::cout << (Days (4 * 365.25).GetYears ()) << std::endl; > > > > I get '4.0027397', but it should be 4. > > > > I don't have a good suggestion yet on how to fix this... > > Damn, I have to nail this. Can't reproduce actually (this doesn't change anything however). For me (Debian testing amd64, GCC 4.8.1), this results in: assert failed. cond="quotient * coefficient[(int) unit] == coefficient[i]", file=../src/core/model/time.cc, line=209 terminate called without an active exception But I _will_ dig deeper and test on some machines running Debian stable amd64. Regards, V.
Sign in to reply to this message.
On 2013/10/21 09:03:22, Vedran Miletić wrote: > On 2013/10/21 07:51:17, Vedran Miletić wrote: > > On 2013/10/18 18:25:56, Peter Barnes wrote: > > > Hello Vedran, > > > > > > When you tossed this into the bug954 discussion I thought it would be easy > to > > > add later, and I had it on my to do list RSN. > > > > > > However, I think there are some subtleties with the way we do conversions > > using > > > integer arithmetic. See my comments on time.cc lines 201 and 235 for a > start. > > > > Hi Peter, > > > > thanks for your review. My comments are inline. > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h > > > File src/core/model/nstime.h (right): > > > > > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... > > > src/core/model/nstime.h:111: A = 0, //!< year > > > Please document that you've chosen 365.25 days per year, a Julian year. > > > > Sure. By the way, do you believe that 365 days would have been a better > choice? > > > > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... > > > src/core/model/nstime.h:216: * - `a` (years) > > > Should we support alternative symbols when converting from strings? 'mn', > > 'hr', > > > 'day', 'week', 'y', 'yr', 'year' > > > > Sure, why not? > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc > > > File src/core/model/time.cc (right): > > > > > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201 > > > src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576, > 6048, > > > 864, 36, 6, 1, 1, 1, 1, 1, 1 }; > > > I think the choice of int32_t here (and int for quotient, below) is a > mistake. > > > > > It leads to premature truncation in computing factor. To see the problem, > run > > > > > > Time::SetResolution (Time::D); > > > std::cout << (Days (4 * 365.25).GetYears ()) << std::endl; > > > > > > I get '4.0027397', but it should be 4. > > > > > > I don't have a good suggestion yet on how to fix this... > > > > Damn, I have to nail this. > > Can't reproduce actually (this doesn't change anything however). For me (Debian > testing amd64, GCC 4.8.1), this results in: > > assert failed. cond="quotient * coefficient[(int) unit] == coefficient[i]", > file=../src/core/model/time.cc, line=209 > terminate called without an active exception > > But I _will_ dig deeper and test on some machines running Debian stable amd64. > > Regards, > V. I understand the issue. What we could do is drop Week (I highly doubt it would be used much if at all anyway), and put Year to be 365 days. Then we would have const int32_t coefficient [LAST] = { 315360, 864, 36, 6, 1, 1, 1, 1, 1, 1 }; Now each unit is an integer multiple of a lower unit, and problem is solved. Comments, opinions? Regards, Vedran
Sign in to reply to this message.
On 2013/10/21 13:23:06, Vedran Miletić wrote: > On 2013/10/21 09:03:22, Vedran Miletić wrote: > > On 2013/10/21 07:51:17, Vedran Miletić wrote: > > > On 2013/10/18 18:25:56, Peter Barnes wrote: > > > > Hello Vedran, > > > > > > > > When you tossed this into the bug954 discussion I thought it would be easy > > to > > > > add later, and I had it on my to do list RSN. > > > > > > > > However, I think there are some subtleties with the way we do conversions > > > using > > > > integer arithmetic. See my comments on time.cc lines 201 and 235 for a > > start. > > > > > > Hi Peter, > > > > > > thanks for your review. My comments are inline. > > > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h > > > > File src/core/model/nstime.h (right): > > > > > > > > > > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... > > > > src/core/model/nstime.h:111: A = 0, //!< year > > > > Please document that you've chosen 365.25 days per year, a Julian year. > > > > > > Sure. By the way, do you believe that 365 days would have been a better > > choice? > > > > > > > > > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcod... > > > > src/core/model/nstime.h:216: * - `a` (years) > > > > Should we support alternative symbols when converting from strings? 'mn', > > > 'hr', > > > > 'day', 'week', 'y', 'yr', 'year' > > > > > > Sure, why not? > > > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc > > > > File src/core/model/time.cc (right): > > > > > > > > > > > > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201 > > > > src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576, > > 6048, > > > > 864, 36, 6, 1, 1, 1, 1, 1, 1 }; > > > > I think the choice of int32_t here (and int for quotient, below) is a > > mistake. > > > > > > > It leads to premature truncation in computing factor. To see the problem, > > run > > > > > > > > Time::SetResolution (Time::D); > > > > std::cout << (Days (4 * 365.25).GetYears ()) << std::endl; > > > > > > > > I get '4.0027397', but it should be 4. > > > > > > > > I don't have a good suggestion yet on how to fix this... > > > > > > Damn, I have to nail this. > > > > Can't reproduce actually (this doesn't change anything however). For me > (Debian > > testing amd64, GCC 4.8.1), this results in: > > > > assert failed. cond="quotient * coefficient[(int) unit] == coefficient[i]", > > file=../src/core/model/time.cc, line=209 > > terminate called without an active exception > > > > But I _will_ dig deeper and test on some machines running Debian stable amd64. > > > > Regards, > > V. > > I understand the issue. What we could do is drop Week (I highly doubt it would > be used much if at all anyway), and put Year to be 365 days. Then we would have > > const int32_t coefficient [LAST] = { 315360, 864, 36, 6, 1, 1, 1, 1, 1, 1 }; > > Now each unit is an integer multiple of a lower unit, and problem is solved. > > Comments, opinions? > > Regards, > Vedran I support this solution. Please add ns-3-reviews@googlegroups.com to the cc list. Any other comments, or should we proceed with this for ns-3.19?
Sign in to reply to this message.
Updated, rebased on ns-3-dev (no changes needed though). Year is now 365 days, and week is gone. In-line doxygen documentation has been updated to provide information about year being 365 days, days being 24 hours etc. However, we have four failing tests (-regression ones with pcap traces). I did some inspection into devices-mesh-dot11s-regression, and it turns out that src/mesh/test/dot11s/hwmp-simplest-regression-test-0-1.pcap and generated pacp differ "only" in: < 17 1.942149 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command < 18 1.942192 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP 0x1e Response < 19 1.942399 00:00:00_00:00:03 -> Broadcast 802.11 69 Action, SN=3, FN=0, Flags=o......., SSID=% [truncated] < 20 1.942657 00:00:00_00:00:04 -> 00:00:00_00:00:03 802.11 63 Action, SN=3, FN=0, Flags=o......., SSID= [truncated] < 21 1.942673 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o....... < 22 1.942805 00:00:00_00:00:03 -> 00:00:00_00:00:04 LLC 76 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command < 23 1.942993 -> 00:00:00_00:00:03 (RA) 802.11 14 Acknowledgement, Flags=o....... < 24 1.943296 00:00:00_00:00:04 -> 00:00:00_00:00:03 LLC 176 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command < 25 1.943312 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o....... < 26 1.952296 00:00:00_00:00:03 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command < 27 1.952595 00:00:00_00:00:04 -> 00:00:00_00:00:03 LLC 76 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command < 28 1.952611 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o....... < 29 1.952815 00:00:00_00:00:03 -> 00:00:00_00:00:04 LLC 176 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command < 30 1.953135 -> 00:00:00_00:00:03 (RA) 802.11 14 Acknowledgement, Flags=o....... < 31 1.953324 00:00:00_00:00:03 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP 0x1e Response --- > 17 1.942369 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command > 18 1.942412 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP 0x1e Response > 19 1.942619 00:00:00_00:00:03 -> Broadcast 802.11 69 Action, SN=3, FN=0, Flags=o......., SSID=% [truncated] > 20 1.942877 00:00:00_00:00:04 -> 00:00:00_00:00:03 802.11 63 Action, SN=3, FN=0, Flags=o......., SSID= [truncated] > 21 1.942893 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o....... > 22 1.943025 00:00:00_00:00:03 -> 00:00:00_00:00:04 LLC 76 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command > 23 1.943213 -> 00:00:00_00:00:03 (RA) 802.11 14 Acknowledgement, Flags=o....... > 24 1.943516 00:00:00_00:00:04 -> 00:00:00_00:00:03 LLC 176 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command > 25 1.943532 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o....... > 26 1.953441 00:00:00_00:00:03 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command > 27 1.953740 00:00:00_00:00:04 -> 00:00:00_00:00:03 LLC 76 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command > 28 1.953756 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o....... > 29 1.953960 00:00:00_00:00:03 -> 00:00:00_00:00:04 LLC 176 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command > 30 1.954280 -> 00:00:00_00:00:03 (RA) 802.11 14 Acknowledgement, Flags=o....... > 31 1.954469 00:00:00_00:00:03 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP 0x1e Response I would attribute this to new set of constructors for each time unit accepting double/float/uint/int, but I haven't yet found time to dig deeper to find out where the issues is triggered. I could probably do it if needed, but I bet someone experienced with wifi would find it much quicker than me if we rasied the issue on ns-developers. Regards, Vedran
Sign in to reply to this message.
I've played with alternative implementations of the unit conversion code and seen the same thing. In many cases I can trace it to a single bit difference in the underlying 64-bit time, representing one ns difference. In others it seems to get amplified dramatically; I've seen differences even higher than this, over 1 ms. I haven't been able to track down how this happens in the protocol implementation. I hypothesize that the single bit error occasionally causes an event to cross a boundary in a windowed protocol, such as the 1024 mus window in (is it 802.11?). The other possibility, which I haven't looked at, is the bug in printing/reading of int64x64_t [bug 1786]. Is this used in generating test output? (Hmm, probably not in pcap, which is binary.) If we are convinced this isn't a bug, we could regenerate the test references and be done. I don't see how your new units should affect old tests, however, suggesting there is a bug, or at least an interaction we don't understand. Peter On 2013/11/24 13:20:36, Vedran Miletić wrote: > Updated, rebased on ns-3-dev (no changes needed though). Year is now 365 days, > and week is gone. In-line doxygen documentation has been updated to provide > information about year being 365 days, days being 24 hours etc. > > However, we have four failing tests (-regression ones with pcap traces). I did > some inspection into devices-mesh-dot11s-regression, and it turns out that > src/mesh/test/dot11s/hwmp-simplest-regression-test-0-1.pcap and generated pacp > differ "only" in: > > < 17 1.942149 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; > DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command > … > > I would attribute this to new set of constructors for each time unit accepting > double/float/uint/int, but I haven't yet found time to dig deeper to find out > where the issues is triggered. I could probably do it if needed, but I bet > someone experienced with wifi would find it much quicker than me if we rasied > the issue on ns-developers.
Sign in to reply to this message.
https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h File src/core/model/nstime.h (right): https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... src/core/model/nstime.h:89: Y = 0, //!< year //!< Julian calendar year, 365.25 days. If you want to use common years, 365 days, we should discuss the reasons. On the one hand, it keeps "years" starting at midnight, on the other, it drifts wrt the civil calendar. I presume someone using years wants a lot of them, so the drift could be significant. But the main thing is to document and explain our choice. https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... src/core/model/nstime.h:90: D = 1, //!< day //!< Day, 24 hours, or 86,400 seconds. https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... src/core/model/nstime.h:91: H = 2, //!< hour //!< Hour, 60 minutes. https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... src/core/model/nstime.h:92: MIN = 3, //!< minute //!< Minute, 60 seconds. https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... src/core/model/nstime.h:93: S = 4, //!< second //!< SI second.
Sign in to reply to this message.
On 2013/11/25 20:23:31, Peter Barnes wrote: > https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h > File src/core/model/nstime.h (right): > > https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... > src/core/model/nstime.h:89: Y = 0, //!< year > //!< Julian calendar year, 365.25 days. > > If you want to use common years, 365 days, we should discuss the reasons. On > the one hand, it keeps "years" starting at midnight, on the other, it drifts wrt > the civil calendar. I presume someone using years wants a lot of them, so the > drift could be significant. But the main thing is to document and explain our > choice. > Main reason is to get "every larger unit is an integer multiple of smaller". Regarding lots of years, I expect most people will use years as a shorthand for 365 days, and my line of thought here (reliability engineering, specifically availability estimation) would be that you don't care too much if it's 100 years of 365 days or 365.25 days, as long as you know which one is used. > https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... > src/core/model/nstime.h:90: D = 1, //!< day > //!< Day, 24 hours, or 86,400 seconds. > > https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... > src/core/model/nstime.h:91: H = 2, //!< hour > //!< Hour, 60 minutes. > > https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... > src/core/model/nstime.h:92: MIN = 3, //!< minute > //!< Minute, 60 seconds. > > https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#ne... > src/core/model/nstime.h:93: S = 4, //!< second > //!< SI second. Agree with all of these, will fix them before pushing.
Sign in to reply to this message.
On 2013/11/25 20:23:20, Peter Barnes wrote: > I've played with alternative implementations of the unit conversion code and > seen the same thing. In many cases I can trace it to a single bit difference in > the underlying 64-bit time, representing one ns difference. In others it seems > to get amplified dramatically; I've seen differences even higher than this, over > 1 ms. I haven't been able to track down how this happens in the protocol > implementation. I hypothesize that the single bit error occasionally causes an > event to cross a boundary in a windowed protocol, such as the 1024 mus window in > (is it 802.11?). > > The other possibility, which I haven't looked at, is the bug in printing/reading > of int64x64_t [bug 1786]. Is this used in generating test output? (Hmm, > probably not in pcap, which is binary.) > > If we are convinced this isn't a bug, we could regenerate the test references > and be done. I don't see how your new units should affect old tests, however, > suggesting there is a bug, or at least an interaction we don't understand. > I will check the "new set of constructors causes this hypothesis", and we can proceed from there. Regards, Vedran
Sign in to reply to this message.
On 2013/11/25 20:23:20, Peter Barnes wrote: > I've played with alternative implementations of the unit conversion code and > seen the same thing. In many cases I can trace it to a single bit difference in > the underlying 64-bit time, representing one ns difference. In others it seems > to get amplified dramatically; I've seen differences even higher than this, over > 1 ms. I haven't been able to track down how this happens in the protocol > implementation. I hypothesize that the single bit error occasionally causes an > event to cross a boundary in a windowed protocol, such as the 1024 mus window in > (is it 802.11?). > > The other possibility, which I haven't looked at, is the bug in printing/reading > of int64x64_t [bug 1786]. Is this used in generating test output? (Hmm, > probably not in pcap, which is binary.) > > If we are convinced this isn't a bug, we could regenerate the test references > and be done. I don't see how your new units should affect old tests, however, > suggesting there is a bug, or at least an interaction we don't understand. I tested this a bit to make sure that the test differences were not due to random variable issues (static initialization order), and this seems to be something different. Just FYI... > > Peter > > On 2013/11/24 13:20:36, Vedran Miletić wrote: > > Updated, rebased on ns-3-dev (no changes needed though). Year is now 365 days, > > and week is gone. In-line doxygen documentation has been updated to provide > > information about year being 365 days, days being 24 hours etc. > > > > However, we have four failing tests (-regression ones with pcap traces). I did > > some inspection into devices-mesh-dot11s-regression, and it turns out that > > src/mesh/test/dot11s/hwmp-simplest-regression-test-0-1.pcap and generated pacp > > differ "only" in: > > > > < 17 1.942149 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; > > DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command > > … > > > > I would attribute this to new set of constructors for each time unit accepting > > double/float/uint/int, but I haven't yet found time to dig deeper to find out > > where the issues is triggered. I could probably do it if needed, but I bet > > someone experienced with wifi would find it much quicker than me if we rasied > > the issue on ns-developers.
Sign in to reply to this message.
Overloaded constructors are the problem. Removed them; no fails. Apparently we have some code that relies on some automatic int->float or float->int conversion. I removed overloads, added the documentation suggestions made by Peter and pushed, since I consider this now to be no worse than we had in 3.18. We can look into fixing problematic code later, and then add overloads if they are needed.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
