Documentation PRs?

[tphnicholas]

How do you feel about documentation for the internals of the project's main components? Starting with LTP, while the RFCs cover the high-level logic, the implementation could perhaps benefit from some structured documentation to get up to speed quicker/make the impact of changes across shared state of sub-components more obvious.

Is the implementation/interface stable enough at this point for documentation to make sense?

If so, what's your input on structure/content?

[briantomko]

@tphnicholas I believe LTP to be stable enough for documentation. My last update to LTP contained some optimizations as well as a new feature to allow the option to have sessions keep the Red Part session data on disk instead of memory (for long delays with high rates and many simultaneous sessions), which was pretty invasive to the code. There's currently no plans in the near term for any major updates, so any documentation PRs would be excellent and helpful!

[tphnicholas]

Would you prefer keeping the format of a short description per function + inline comments where the implementation could use some clarifications/details? As seen in:

HDTN/common/ltp/src/LtpRandomNumberGenerator.cpp

Lines 33 to 37 in 91eafd5

	//return a hardware random generated number with:
	// - bit 63..56 (8 bits of engineIndex)
	// - bit 55 set to 0 to leave room for incrementing without rolling into the engineIndex
	// - bit 54..16 a random part (39 bits of randomness)
	// - bits 15..0 a 16-bit incremental part (never 0) (to prevent a birthday paradox) which rolls around (1,2,..,65534,65535,1,2,..)

HDTN/common/ltp/src/LtpRandomNumberGenerator.cpp

Line 56 in 91eafd5

inc += (inc == 0); //must be non-zero when uint16 rolls back around

Or looking for something more structured and descriptive akin to doxygen-style comments?

[briantomko]

@tphnicholas The goal for documentation would be to have doxygen-style comments in the header files only. We definitely want both function comments as well as an overall class comment, and at the top of the file below the copyright stuff a file comment denoted by * @section DESCRIPTION. Any detailed comment that may exist in a .cpp file (that are above the function) should probably be moved to the header file, unless of course the class is an implementation-only class defined within the .cpp file itself. Any inline comments within a .cpp file should be kept if they are explaining details about what a specific line or block of code is doing. A good example of what we're looking for is https://github.com/nasa/HDTN/blob/master/common/util/include/TokenRateLimiter.h

Also, just curious how you heard about HDTN or what got you interested in the project.

Thanks

[tphnicholas]

@briantomko Thanks for the example, should clear things up, I'll see to a PR for one of the smaller/less involved classes sometime in the next couple of days to get some reviewing going.

The interest was coincidental, was reading up on communication through EM waves out of curiosity -> wondered what the variables in space comms were -> found DTN -> looked for an actively developed/maintained implementation -> found HDTN -> found your paper https://ntrs.nasa.gov/citations/20220011407 interesting.

The goal for now is to become familiar with the implementation while reading through the RFCs for each layer of the stack, getting involved with documenting/refactoring would complement my learning and hopefully add some value to the project.

[briantomko]

@tphnicholas That's great to hear. As always, let me know if you have any questions with the code or the project.

[tphnicholas]

@briantomko In response to #37, if you have no additional requests/input on style/format, I can start working through the core LTP modules + any small utility classes they might be referencing, for a PR sometime in the following week.

[briantomko]

@tphnicholas The style/format looked good. As with the "Circular Index Buffer" class, if you see any functions that should be const feel free to make them const. If they should be noexcept feel free to do that as well. The functions in the "Circular Index Buffer" could be declared as const noexcept. I'll be on the lookout for the next PR. Thanks!

[tphnicholas]

@briantomko Is LtpTimerManager due a refactoring, specifically the mechanism around handling timer deletion/manager destruction with m_timerIsDeletedPtr and m_activeSerialNumberBeingTimed that seems less than ideal with all the tracking postconditions? If so, what is your input?

If not, the docs based on the current state of the class are ready and will be included in the next PR regardless.

[briantomko]

@tphnicholas LtpTimerManager was difficult in that it had to be able to be deleted whether or not it had a timer within the external io_service. If the LtpTimerManager was deleted and then the timer completion callback got called, all the member variables would be considered deleted, so it must look at the heap allocated flag bound to its callback function to determine this. I couldn't think of a better way to solve this at the time. Always open to suggestions.

[tphnicholas]

@briantomko Alright, should be finishing up the LTP docs PR over the weekend, will keep LtpTimerManager as is and look into it after.

[tphnicholas]

@briantomko If explicitly-defined trivial destructors, copy/move constructors and copy/move assignment operators are left in for consistency, should I be explicitly defaulting them instead (with = default)?

e.g. LtpEngine::cancel_segment_timer_info_t could be made trivially copyable to avoid UB on:

HDTN/common/ltp/src/LtpEngine.cpp

Line 33 in 6b8d7ef

memcpy(this, data, sizeof(cancel_segment_timer_info_t));

[briantomko]

@tphnicholas Let's keep the behavior as-is for now. The memcpy there was to allow emplace construction from an array of bytes. Thanks

[briantomko]

@tphnicholas I'm looking to merge some LTP performance updates with a little refactoring to some of the functions. Just wondering if I should hold off until your branch is ready to merge so I can rebase on top of your branch and resolve conflicts?

[tphnicholas]

@briantomko I am just finishing up ~10 functions left for LtpEngine, the PR should be up within the next few hours if you want to hold off for today and it covers everything in the common/ltp/include directory that was missing documentation except for Ltp.h.

If they are changes to function logic and not overall class structure and only contained in a few functions, shouldn't be too much work rebasing and correcting the docs locally on my end if you are looking to push before that.

[briantomko]

@tphnicholas Thanks I will hold off and look for your PR today.

[briantomko]

@tphnicholas I rebased and pushed up my ltp updates.
Just fyi many of these parameters in multiple functions were changed from:

@param constBufferVecs The vector of data buffers to send.
@param underlyingDataToDeleteOnSentCallbackVec The vector of underlying data buffers shared pointers.
@param underlyingCsDataToDeleteOnSentCallbackVec The vector of underlying client service data to send shared pointers.

to:

std::shared_ptr<std::vector<UdpSendPacketInfo> >& udpSendPacketInfoVecSharedPtr

I haven't updated the docs to reflect that.

[tphnicholas]

@briantomko I will be keeping track of LTP changes and doc mismatches to eventually give it a second pass, now looking to get a high-level understanding of the HDTN modules and how they interact with each other to be able to run some of the examples/simulations, is the natural progression to start working upwards from hdtn_one_process?

What is the development status for the HDTN modules, specifically router, scheduler and storage? Any of those stable enough for docs?

[briantomko]

@tphnicholas Since LTP and the other convergence layers would sort of be the lowest level, the next level up would be:

common/outduct_manager which is used by module/egress which is used by hdtn_one_process

in parallel to that:

common/induct_manager which is used by module/ingress which is used by hdtn_one_process

The induct and outduct managers are definitely stable. The ingress and egress and storage modules (which communicate over ZeroMQ) are undergoing only slight modifications. hdtn_one_process is merely a wrapper around all these modules if hdtn is being run in non-distributed mode (the general use case). Otherwise, these modules can be run as there own distributed processes if desired (rare use case), which would be suited for a use case such as one module per Raspberry Pi.

[tphnicholas]

@briantomko I'll see about setting up a simulation environment and starting on the documentation for the base in/outduct classes, the in/outduct_managers and the LTP in/outduct + bundle sink/source implementations this week, but should take some time since I need to review the relative RFCs and skim through the rest of the convergence layers.

Would you like me to add some user-facing docs for common/bpcodec/include/app_patterns/ and common/bpcodec/apps/ while we're at it?

[briantomko]

@tphnicholas That would be great! thanks!