gPTP code comments

This page is a public site to politely record comments on the gPTP code base

Documentation Request

Module	Function	Submitter comment	Reviewer comment	Action

Highly Suspicious

Module	Function	Submitter comment	Reviewer comment	Action
avbts_message.hpp	FollowUpTLV::toByteString()	memcpy(byte_str, this, sizeof(*this)) operation looks weird (AndrewE)
avbts_osnet.hpp	LinkLayerAddress::toOctetArray and LinkLayerAddress(uint8_t *)	Method does not verify if pointer is valid. Might segfault(DavidC)	It should be fixed	Issue #241
avbts_osnet.hpp	InterfaceName::~InterfaceName()	destructor does not delete this->name created at the constructor (DavidC)	It should be fixed	Issue #242
avbts_osnet.hpp	InterfaceName::toString	if string is null, strncpy might fail silently and the method returns successfully (DavidC)	It should be fixed	Issue #241
PTPMessageCommon	PTPMessageCommon::buildCommonHeader	correctionField assumes Little-endian Machine. We should be able to detect endianness and set value accordingly (DavidC)	It should be fixed	Issue #243

Could Be Improved

Description	Submitter comment	Reviewer comment	Action
Cleanup the message build/parse code – Could be much smaller and simpler using struct to represent message	by ChrisH
Inconsistent Port Identity and Clock Identity code – in some code clock identity is treated as a string in others as a C++ object	by ChrisH
Inconsistent timestamp mathematics – mixture of C++ operator overloading and macros	by ChrisH
Timer API design and usage could be simplified	by ChrisH
Change “low-level” APIs to conform to those in AVnu docs	by ChrisH
Change #defines in code to conform to IEEE standards doc	by ChrisH
Remove duplicate struct gPtpTimeData defns	by AndrewE
Magic numbers at ieee1588Port constructor	by DavidC	It should be fixed	Issue #129
Method IEEE1588Port::recoverPort doesnt do anything. Just returns.	by DavidC
Method IEEE1588Port::addForeignMaster not implemented.	by DavidC
Method IEEE1588Port::removeForeignMaster not implemented.	by DavidC
Method IEEE1588Port::removeForeignMasterAll not implemented.	by DavidC
Method IEEE1588Port::getParentLastSyncSequenceNumber not implemented.	by DavidC
Method IEEE1588Port::setParentLastSyncSequenceNumber not implemented.	by DavidC
Magic number being added to messageType	by DavidC	It should be fixed	Issue #129
Improve code clarity at buildPTPMessage	by DavidC
Improve code clarity at buildPTPMessage	by DavidC
Improve code clarity at buildPTPMessage	by DavidC
Magic number at buildPTPMessage	by DavidC	It should be fixed	Issue #129
Remove commented code at buildPTPMessage if not in use	by DavidC
Duplicated define at OUTSTANDING_MESSAGES from OUTSTANDING_MESSAGES	by DavidC
ClockIdentity set() doesn't have matching get(). getIdentityString() should have matching setIDentityString()	by DavidC
Should msg pointer be const at IEEE1588Port::setLastSync(PTPMessageSync *msg) ?	by DavidC
HWTimestamper_final not used	by DavidC
Should we remove unused code as for instance scaledNs ?	by DavidC
HWTimestamper_get_extderror not in use.	by DavidC
Remove HWTimestamper_get_extclk_offset. It was a hack to get a specific board working.	by DavidC

Minor Wishes

Description	Submitter comment	Reviewer comment	Action
Encapsulate the formatting of data into byte buffers/byte streams into a small library of functions	Levi
[daemon_cl.cpp] Indent code between lines 273 and 323 (open-avb-next @ 56f990a)	DavidC
add comments describing difference between linux_hal_common.cpp and linux_hal_genric.cpp	AndrewE
ieee1588port.cpp has different indentation throughout the file. Some functions are using 2 spaces and some are using 1 tab of 4 spaces.	DavidC
Evaluate commented code at [IEEE1588Port::becomeSlave] (https://github.com/AVnu/Open-AVB/blob/open-avb-next/daemons/gptp/common/ieee1588port.cpp#L944)	DavidC
Evaluate commented code at PTPMessageCommon::buildCommonHeader	DavidC
Remove the double semi-colon at PTPMessageCommon::getTimestampCounterValue. Probably a typo.	DavidC