https://redmine.named-data.net/https://redmine.named-data.net/favicon.ico?14759811232018-04-16T12:04:59ZNDN project issue tracking systemndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230242018-04-16T12:04:59ZJunxiao Shi
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Code review</i></li><li><strong>Assignee</strong> set to <i>Junxiao Shi</i></li><li><strong>Start date</strong> deleted (<del><i>12/19/2017</i></del>)</li><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li></ul><p><a href="https://gerrit.named-data.net/4684">https://gerrit.named-data.net/4684</a></p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230252018-04-16T16:03:52ZDavide Pesavento
<ul></ul><p>What does "fixed-width unsigned integer" mean anyway? Does the spec say how it should be encoded (its endianness for instance)?</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230262018-04-16T20:17:16ZDavide Pesavento
<ul></ul><p>And why don't we change the protocol spec instead of the code? It doesn't seem that Sequence has uses where it may need to be changed in place (thus potentially changing the encoding length), so nonNegativeInteger shouldn't be slower in any noticeable way.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230272018-04-16T20:22:30ZJunxiao Shi
<ul></ul><blockquote>
<p>nonNegativeInteger shouldn't be slower in any noticeable way.</p>
</blockquote>
<p>It is slower because decoder contains branching instructions. The decoder is still readNonNegativeInteger for now, but I plan to change it to read uint64 after a release.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230282018-04-16T20:29:34ZDavide Pesavento
<ul></ul><p>I bet the difference is not even measurable. And even in the fixed-width case, you still need at least one branch to check that the size is what you expect.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230322018-04-16T20:44:37ZJunxiao Shi
<ul></ul><p>Having less branching (one) is always faster than having more branching (four). It will be measurable when executed millions of times.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230362018-04-16T21:00:21ZDavide Pesavento
<ul></ul><p>Only one branch is ever taken. The difference is in the number of comparisons.</p>
<p>Regardless, we're not having this argument. It would apply to many other things that <em>are</em> in the protocol, some much worse than nonNegativeInteger.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230642018-04-18T15:49:46ZJunxiao Shi
<ul></ul><p>20180418 NFD call decides that this Change can proceed.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230932018-04-20T09:45:04ZJunxiao Shi
<ul></ul><blockquote>
<p>just to double check, the spec doesn't mandate a specific endianness, that's also a per-link decision I guess</p>
</blockquote>
<p>I see no reason to mandate a specific endianness. ndn-cxx uses big endian to be compatible with prior versions, but other links can choose differently.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230982018-04-20T19:55:39ZJunxiao Shi
<ul></ul><p>NFD's LpReliability assumes Ack is NNI. This is updated in <a href="https://gerrit.named-data.net/4693">https://gerrit.named-data.net/4693</a>.<br>
Test case for LpReliability is somewhat simplified because Ack numbers don't make a difference in piggybacking calculation.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=230992018-04-20T20:23:41ZJunxiao Shi
<ul></ul><p>I’m thinking about a better idea on calculating the size of a piggybacked field:</p>
<pre><code>size_t sizeofAck = lp::Packet::estimateSize<lp::AckField>(ack);
</code></pre>
<p>This can avoid the errorprone field size calculation in NFD’s LinkService.</p>
<p>However, I’d rather do this under a different issue.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=231282018-04-29T11:29:05ZDavide Pesavento
<ul><li><strong>Status</strong> changed from <i>Code review</i> to <i>In Progress</i></li><li><strong>% Done</strong> changed from <i>100</i> to <i>90</i></li></ul><p>Moving back to "in progress" since the decoder is still using NNI format and will be changed to fixed-width after the upcoming release.</p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=231302018-04-29T16:18:17ZJunxiao Shi
<ul><li><strong>Related to</strong> <i><a class="issue tracker-3 status-5 priority-2 priority-default closed" href="/issues/4598">Task #4598</a>: Stop accepting NonNegativeInteger as NDNLP sequence number</i> added</li></ul> ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=231322018-04-29T16:18:44ZJunxiao Shi
<ul></ul><blockquote>
<p>will be changed to fixed-width after the upcoming release.</p>
</blockquote>
<p>That shall be a separate issue. <a class="issue tracker-3 status-5 priority-2 priority-default closed" title="Task: Stop accepting NonNegativeInteger as NDNLP sequence number (Closed)" href="https://redmine.named-data.net/issues/4598">#4598</a></p>
ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=231352018-04-29T16:27:48ZDavide Pesavento
<ul><li><strong>Blocks</strong> <i><a class="issue tracker-3 status-5 priority-2 priority-default closed" href="/issues/4564">Task #4564</a>: Release 0.6.2</i> added</li></ul> ndn-cxx - Bug #4403: LpPacket Sequence number field incorrect encodinghttps://redmine.named-data.net/issues/4403?journal_id=231372018-04-29T16:28:16ZDavide Pesavento
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Closed</i></li><li><strong>% Done</strong> changed from <i>90</i> to <i>100</i></li></ul><p>Closing then.</p>