Refactor TMTC Stack, improve test framework #655
Labels
No Label
API Change
Breaking API Change
bug
build
cosmetics
Documentation
duplicate
feature
help wanted
hotfix
invalid
question
Refactor
Tests
wontfix
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Blocks
Depends on
#662 WIP: CFDP Router and CFDP Handlers
fsfw/fsfw
#667 Refactor Local Pool API
fsfw/fsfw
#656 Expand SerializeIF
fsfw/fsfw
#659 returnvalue namespace
fsfw/fsfw
#677 SerialBufferAdapter refactoring
fsfw/fsfw
Reference: fsfw/fsfw#655
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "mueller/refactor-tmtc-stack"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
MessageQueueMock
significantly. Most of these mocks are also necessary to unittest more complex classes like the CommandingServiceBasePusServiceBase
PusServiceBase
to ensure better testabilityPusServiceBase
now takes a composite configuration struct calledPsbParams
TmFunnel
, which need to manipulate certain fields of a PUS TMwhich is otherwise fine, a special
PusTmZeroCopyWriter
class can be used. This class currently only exposes the two functions required: Updating the sequence count and the error control field.PusPacketChecker
and theCcsdsPacketChecker
. The CCSDS distributor will perform the CCSDS packet check while the PUS distributor performs the PUS check. By default, the CCSDS checker will not perform any APID checking, but the PUS checker will. It is possible to specify a custom CCSDS checker. This allows to set a custom checker which will verify that the APID is equal to the PUS APID or a hypothetical CFDP APID. The CFDP checker will not make assumptions about how the CFDP packets are wrapped and then has an implementation independent of CCSDS SpacePacket.These changes also introduce a lot more interfaces to increase the flexbility of the code base. They are also composition-centric, which gets rid of the complicated inheritance trees and the diamond inheritance issue of the former packet stack.
This PR pushes the code coverage to ~65 %.
Full list of added interfaces
SpacePacketIF
: Generic CCSDS space packet fields. This interface provides all methods based on 4 core abstract methods which return the packet ID, packet sequence control, data length and version asuint16_t
oderuint8_t
PusIF
: Generic PUS fields: Version, service and subservice. This interface impliesSpacePacketIF
PusTmIF
: Generic PUS C TM fields, for example the fields of the secondary header. This interface impliesPusIF
PusTcIF
: Generic PUS C TC fields, for example the fields of the secondary header. This interface impliesPusIF
ReadablePacketIF
: Returns whole packet in raw format. Useful for zero-copy classesRawUserDataReaderIF
: Returns user data like TC app data or TM source data in raw format. Useful for zero-copy classesCreatorDataIF
: Returns user data in form of a tagged union which allows to specify user data as a serializable (implementingSerializeIF
) or in raw format (pointer + size). Useful for high-level packet creatorsMigration Guide
Framework Objects
The
VerificationReporter
object will not be created automatically now,so a call like this in the user object factory will probably become necessary
This creates the reporter with the default object ID
VERIFICATION_REPORTER
which is used by other objects if no explicit reporter is set.
Pus Service Base API
Calls like this:
do look like this now
Static Framework IDs
Some static IDs are written in capital letter now to specify that those are statics
which should remain constant after setting them once.
For example, the following
PusServiceBase
and theVerificationReporter
IDsmight be interesting to set in the
Factory::setStaticFrameworkObjectIds
function, assuming that all the objects were created and set up properly:Data Pool Manager Subscription
Calls like this:
look like this now
TM Funnel Implementation
Existing funnel implementations looked like this
They now look like this, with the time reader being explicitely set
Changed Interfaces
TimeStamperIF
: Remove hardcoded timestamp size and implySerializeIF
. This allows more flexibility in the used timestampOther changes
TimeStamper
toCdsShortTimeStamper
. This is more explicit.WIP
PusTmZeroCopyWriter
classWIP: Refactor TMTC Stackto Refactor TMTC Stack, improve test frameworkRefactor TMTC Stack, improve test frameworkto WIP: Refactor TMTC Stack, improve test frameworkWIP: Refactor TMTC Stack, improve test frameworkto Refactor TMTC Stack, improve test frameworkRefactor TMTC Stack, improve test frameworkto WIP: Refactor TMTC Stack, improve test frameworkWIP: Refactor TMTC Stack, improve test frameworkto Refactor TMTC Stack, improve test frameworkThe changes in this PR are used inside the EIVE OBSW now
I gave up after 81 files. I cannot review this in a sense that I feel satisfied by the review. This is way to large for a proper review.
@ -16,1 +13,3 @@
this->messageSize = this->HEADER_SIZE + size;
: messageSize(MessageQueueMessage::HEADER_SIZE + size) {
if (size <= MessageQueueMessage::MAX_DATA_SIZE) {
std::memcpy(internalBuffer + MessageQueueMessage::HEADER_SIZE, data, size);
You need to avoud this here? Is this a good idea?
clang-tidy does not like using virtual member functions in the constructor
@ -13,7 +13,6 @@ ReturnValue_t MessageQueueSenderIF::sendMessage(MessageQueueId_t sendTo,
MessageQueueMessageIF* message,
MessageQueueId_t sentFrom, bool ignoreFault) {
return MessageQueue::sendMessageFromMessageQueue(sendTo, message, sentFrom, ignoreFault);
return returnvalue::OK;
Oh this was a dumb one.
@ -148,4 +148,2 @@
// Init our dummy packet and correct endianness of object ID before
// sending it back.
WiretappingPacket TmPacket(DeviceHandlerMessage::getDeviceObjectId(reply), data);
TmPacket.objectId = EndianConverter::convertBigEndian(TmPacket.objectId);
API change or bug fix?
The previous API used a function which simply copies the TM data with
memcpy
. The new one uses this functionwhere
DataWithObjectIdPrefix
implements SerializeIF so there is no need to tweak the endianness manually.Local pool specific code will be moved into a separate PR
@ -35,3 +34,1 @@
data.data_field.service_subtype = subtype;
TmPacketMinimal testPacket((uint8_t*)&data);
testPacket.setAPID(apid);
mintm::MinimalPusTm data{};
What does mintm mean?
minimal tm. I can rename it to minTm:: if that helps..
I think that minTm is better.
@ -27,3 +30,3 @@
private:
uint8_t subService;
uint8_t subService{};
{} vs = 0 ?
that was auto-generated. apparently clion prefers this..
Ok.
This PR leaks https://egit.irs.uni-stuttgart.de/fsfw/fsfw/pulls/668/files
This is intentional. This PR requires the features of #668 , I only packaged #668 after using it at two places in the code. It might be worth a thought to replace it to an API similar to #671
@ -12,3 +12,3 @@
* addTimeStamp may be called in parallel from a different context.
*/
class TimeStamperIF {
class TimeStamperIF : public SerializeIF, public TimeStampIF {
Is the TimeStamper Serializable? Or is the product of the timestamp serializable?
Whatever implements TimeStamperIF should also be serializable.
Refactored, now named
TimeWriterIF
Approving this for merge as to keep up with eive.