Magic value present in many SDNV codec functions

[BrianSipos]

The value 10 appears many times in Ltp.cpp related to the maximum size of an encoded SDNV. This makes understanding the purpose of the "magic value" more difficult.

I recommend to make a global declaration of the semantics of that value in Sdnv.h such as:

/// The largest possible encoding of a 64-bit value
constexpr int SdnvMaximumEncodedSize = 10;

and then search-and-replace "10" with "SdnvMaximumEncodedSize" (and where applicable "11" with "1 + SdnvMaximumEncodedSize" and similar).

[tphnicholas]

Is the issue stale/are there other plans on refactoring, or should I go ahead and pick this up?

If so, there is also a discrepancy at: https://github.com/nasa/HDTN/blob/97116f5b42fa9a8772eca6ef40f3286ed15ce2b5/common/ltp/src/Ltp.cpp#L319

should the comment be corrected to show space for 3 + (2 conditional) SDNVs (service_id/offset/length + reception claim pair when the segment is a checkpoint), instead of the current 5 + 2?

[briantomko]

@tphnicholas I was actually just working on this issue on my side branch, which should be released soon. I also took a look at line 319 and the code comment is incorrect (looks like I copied and pasted from report_segment_t line 193. I will fix that comment. Good catch! The code is actually correct because it's 5 sdnvs if it's a checkpoint and 3 otherwise, per LTP RFC:

If the data segment is a checkpoint, the segment MUST additionally include the following two serial numbers (checkpoint serial number and report serial number) to support efficient retransmission. Data segments that are not checkpoints MUST NOT have these two fields in the header and MUST continue on directly with the client service data.

[tphnicholas]

@briantomko I see, just making sure I parsed the draft correctly, not too familiar with the protocols involved yet.

I'm interested in learning about them and your implementation, feel free to update the issues with any low hanging fruit when it comes to refactoring, would be a good way to get familiarized with both.

[briantomko]

@tphnicholas Will definitely keep you posted with new issues. Glad you are willing to help. Let me know if you want me to walk through the LTP code with you sometime and explain how it works.

[tphnicholas]

@briantomko From the little I browsed around when looking up this issue, the code seems well documented so there shouldn't be too many questions after reading up on the protocol specifications and experimenting around with tests.

Of course any other resources you might've prepared would be appreciated.

[briantomko]

This issue has been resolved by the merge of commit 924e8f8e1374966db57f7fb86cdab259909392b5