TMTC Packet Base improvements #80

muellerr · 2020-05-15T20:02:19+02:00

muellerr commented

2020-05-15 20:02:19 +02:00

fixes #81. includes possible bugfix in initializeTcPacket()

muellerr added the

bug

feature

labels 2020-05-15 20:03:25 +02:00

muellerr reviewed 2020-05-15 20:05:24 +02:00

tmtcpacket/pus/TcPacketBase.cpp Outdated

						
				@ -73,3 +80,3 @@

					initSpacePacketHeader(true, true, apid, sequenceCount);

					memset(&tcData->data_field, 0, sizeof(tcData->data_field));

					setPacketDataLength(sizeof(tcData->data_field) + CRC_SIZE);

					setPacketDataLength(sizeof(PUSTcDataFieldHeader) + CRC_SIZE - 1);

muellerr commented

2020-05-15 20:05:24 +02:00

Bug here. I think a minus one was missing: the value is the data length field should be the actual size of the data field minus 1.

muellerr commented

2020-05-18 16:52:12 +02:00

redordered CTOR argument order for TcPacketStored

muellerr commented

2020-05-19 15:08:07 +02:00

There is still a bug in here, setApplicationData and setData() of Tc/TmPacketBase are (very!) confusing to use on their own.

I added new setAppData, setSourceData functions which perform the memcpy normally performed by TmPacketStored and TcPacketStored. If a framework user wants to create TM/TC by itself without using the store, this is possible inutitively now.

There is still a bug in here, setApplicationData and setData() of Tc/TmPacketBase are (very!) confusing to use on their own. I added new setAppData, setSourceData functions which perform the memcpy normally performed by TmPacketStored and TcPacketStored. If a framework user wants to create TM/TC by itself without using the store, this is possible inutitively now.

muellerr changed title from ~~mueller_tcPacketBase~~ to WIP: mueller_tcPacketBase

2020-05-19 16:56:08 +02:00

muellerr changed title from ~~WIP: mueller_tcPacketBase~~ to mueller_tcPacketBase

2020-05-19 19:07:23 +02:00

muellerr changed title from ~~mueller_tcPacketBase~~ to TMTC Packet Base improvements

2020-05-20 12:40:47 +02:00

gaisser requested changes 2020-10-27 13:36:28 +01:00

gaisser left a comment

I think we should see the base classes as a Helper for parsing.
It does not contain the data itself and does not know anything about the available size.
Therefore, a TcPacketStored and the Tm equivalent are the way to go for users.
Hence, setters after construction in the BaseClasses are all potentials difficult bugs.

I think we should see the base classes as a Helper for parsing. It does not contain the data itself and does not know anything about the available size. Therefore, a TcPacketStored and the Tm equivalent are the way to go for users. Hence, setters after construction in the BaseClasses are all potentials difficult bugs.

tmtcpacket/pus/TcPacketBase.cpp Outdated

						
				@ -52,0 +51,4 @@

					tcData = (TcPacketPointer*) pData;

				}

				void TcPacketBase::setAppData(uint8_t * appData, uint16_t dataLen) {

gaisser commented

2020-10-27 13:06:19 +01:00

The getter is called "getApplicationData", we should keep the names the same.

tmtcpacket/pus/TcPacketBase.cpp Outdated

						
				@ -52,0 +52,4 @@

				}

				void TcPacketBase::setAppData(uint8_t * appData, uint16_t dataLen) {

					memcpy(&tcData->appData, appData, dataLen);

gaisser commented

2020-10-27 13:30:24 +01:00

As tcData->appData is only a "fake" pointer of which we are not able to check the length, this operation is dangerous and there is no warning about it. TcPacketStored is the way the user does not have to thing about that. The whole class is only intended as Parsing Helper for a existing Packet or it is hidden behind a safe operation as in TcPacketStored.

I'm not sure if we should allow setting of anything here after the constructor.
Even the setData function is interessting maybe we could make that protected as its only used in TcPacketStored.

As tcData->appData is only a "fake" pointer of which we are not able to check the length, this operation is dangerous and there is no warning about it. TcPacketStored is the way the user does not have to thing about that. The whole class is only intended as Parsing Helper for a existing Packet or it is hidden behind a safe operation as in TcPacketStored. I'm not sure if we should allow setting of anything here after the constructor. Even the setData function is interessting maybe we could make that protected as its only used in TcPacketStored.

tmtcpacket/pus/TcPacketStored.cpp Outdated

						
				@ -14,1 +11,3 @@

						TcPacketBase(NULL) {

				TcPacketStored::TcPacketStored(uint8_t service, uint8_t subservice,

						uint16_t apid, uint8_t sequence_count, const uint8_t* data,

						size_t size, uint8_t ack ) :

gaisser commented

2020-10-27 13:31:35 +01:00

The order of arguments is changed. This is an API Change. The APID has no default argument, so changing the order does not seem useful.

tmtcpacket/pus/TmPacketBase.cpp Outdated

						
				@ -103,2 +103,4 @@

				}

				void TmPacketBase::setSourceData(uint8_t* sourceData, size_t sourceSize) {

					memcpy(getSourceData(), sourceData, sourceSize);