UMDBPP / CCSDS_Xbee

Code implementing a simplified user interface for sending CCSDS packets over xbees.
3 stars 0 forks source link

Use util functions for setting/getting fields from packets #8

Closed ascended121 closed 8 years ago

ascended121 commented 8 years ago

I committed a full set of setter/getter functions (based off of Nick's util functions) for the Primary and Telemetry Secondary headers for interacting with the packet so that we only do the casting in one place (one of the util functions). I updated SendTlmMsg to use these new functions.

The rest of the functions in CCSDS_xbee.cpp need to be updated to use these functions. 'Getters/Setters should also be written for the Command Secondary header.

nmrossomando commented 8 years ago

There is more pointer trickiness here. I'm doing the best I can to make sure it's smooth.

Basically, the getters can internally use the getPrimaryHeader(), getTlmHeader(), getCmdHeader() functions, but the setters can't. It's not safe to return a pointer from a function (Hello segfaults!). Therefore those functions have to return a copy. (I can't remove those altogether, as 900Relay uses them.)

nmrossomando commented 8 years ago

All done. 900Relay should be updated to use the new library, but it isn't time critical.

ascended121 commented 8 years ago

If I'm understanding correctly... returning a pointer from a function is allowed, but it dangerous to return the pointer to a local variable because you can't be sure what the compiler is going to do with that memory after the end of the function execution, correct? For these functions, it looks like you're taking that risk with the getters because reading a random piece of memory won't cause a problem, whereas writting to it probably would. Am I understanding correctly?

nmrossomando commented 8 years ago

Right on the first part.

Not quite on the getters: they don't need a pointer at all, they're happy with a copy of the data. So that's what they use. And it falls out of scope as soon as the function returns, so the memory hit isn't bad.

ascended121 commented 8 years ago

This implementation is probably fine... I'm just intellectually curious now...

Why not have the field getters/setters declare the header pointer and have the getPrimaryHeader, getTlmHeader, getCmdHeader take that in as an argument and fill it? Then the pointer wouldn't be owned by getPrimaryHeader, getTlmHeader, getCmdHeader and would last for the duration that the field getter/setter needed it, correct?

ascended121 commented 8 years ago

Also, apparently declaring a local variable static will cause the compiler to reserve its space in memory at compile-time... so you wouldn't have to worry about it being something else after the function returned. Would that be another solution here?

nmrossomando commented 8 years ago

Both would be, I believe. The former is probably the right way to do this. The reason I went with this implementation was just for backward compatibility (since we're a day away from launch). But in future we can change that.

ascended121 commented 8 years ago

No worries and I completely agree... I was just trying to figure out what was going on

nmrossomando commented 8 years ago

Yup, all good!