TcPacketBase bug and improvement #81

New Issue

muellerr · 2020-05-15T20:04:25+02:00

muellerr commented

2020-05-15 20:04:25 +02:00

Some improvements and possible bugx i found when coding the tc injector, which packs telecommands in the software and sends them to a packet distributor (the tc injector is propably a viable framework feature too, might add that in separate pull request..):

An additional function to set data and data length at once would be nice.
Also, calculating the full packet size with a given application data size before initializing it allows storing telecommands directly into the TC store by using getFreeElement(), so a function for that purpose would be helpful.
I also think there is a bug in the tcPacketBase::initializeTcPacket()
function. The length field should be set to the actual length minus 1, the minus 1 is missing.

Some improvements and possible bugx i found when coding the tc injector, which packs telecommands in the software and sends them to a packet distributor (the tc injector is propably a viable framework feature too, might add that in separate pull request..): - An additional function to set data and data length at once would be nice. - Also, calculating the full packet size with a given application data size before initializing it allows storing telecommands directly into the TC store by using getFreeElement(), so a function for that purpose would be helpful. - I also think there is a bug in the tcPacketBase::initializeTcPacket() function. The length field should be set to the actual length minus 1, the minus 1 is missing.

muellerr added the

bug

feature

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

muellerr referenced a pull request that will close this issue

2020-05-15 20:04:45 +02:00

TMTC Packet Base improvements #80

gaisser commented

2020-05-18 15:37:52 +02:00

I think, i have found that bug before. But I don't see the issue right now :-O I'm getting old.

gaisser self-assigned this 2020-05-18 15:42:26 +02:00

muellerr commented

2020-05-18 16:06:04 +02:00

The second point is actually done by TcPacketStored, which I completely missed. But i guess a static member function would still be nice..

muellerr commented

2020-05-18 16:43:22 +02:00

I made a convencience API change for TcPacketStored:

ACK now has a default value of 1111 (ACK_FULL). I think this will be used in 99% of all cases anyway (right?..)
New order: service, subservice, APID, SSQ, data, size, ACK

I made a convencience API change for TcPacketStored: - ACK now has a default value of 1111 (ACK_FULL). I think this will be used in 99% of all cases anyway (right?..) - New order: service, subservice, APID, SSQ, data, size, ACK

gaisser commented

2020-05-25 15:26:00 +02:00

There are TCs with no step ACKs.

muellerr commented

2020-05-25 15:42:25 +02:00

Ok. if anything it makes it more clear that ACK isnt a boolean value, by hinting to the custom defined types (ACK_FULL, ACK_STEP...).

Also, memcpy is performed by the setAppData / setSourceData functions now to copy the supplied data. In its current form, the TcPacketBase and TmPacketBase are difficult to use in their own as soon as application and source data come into play. the addition of a few custom helper functions would help to decouple and simplify usage without needing Tc/TmPacketStored. However, maybe range checks should be performed then.

Ok. if anything it makes it more clear that ACK isnt a boolean value, by hinting to the custom defined types (ACK_FULL, ACK_STEP...). Also, memcpy is performed by the setAppData / setSourceData functions now to copy the supplied data. In its current form, the TcPacketBase and TmPacketBase are difficult to use in their own as soon as application and source data come into play. the addition of a few custom helper functions would help to decouple and simplify usage without needing Tc/TmPacketStored. However, maybe range checks should be performed then.

gaisser added this to the Release Apollo-Sojus-Test-Project (ASTP) 0.0.1 milestone 2020-10-27 13:56:34 +01:00

gaisser closed this issue

2020-10-29 13:17:36 +01:00

Sign in to join this conversation.