Change thrusterData to a pointer

[Yume27]

• Tickets addressed: bsk-952
• Review: By commit
• Merge strategy: Merge (no squash)

Description

Changed the thruster config message in thrusterDynamicEffector to a pointer, duplication of thruster config messages is avoided, and parameters can be easily modified during simulation.

Verification

Unit test was done with test_ThrusterDynamicsUnit.py

Documentation

N/A

Future work

N/A

[juan-g-bonilla]

Have you thought about using std::shared_ptr instead of a raw pointer? It'd allow you to share the thrusterData between different models and with Python, and it'll get automatically cleared once no one is using it. By creating these objects in Python and passing them as raw pointers to C++, you risk use-after-free (segfault if you're lucky!). If the Python objects go out of scope, they will get garbage collected (so the C++ object will be cleared from memory), but your models will still have pointers to them! We've had this problem in the past for standalone messages created in Python functions that never get returned.

Take a look at how gravityModel gets handled in gravityEffector. It's all very transparent to users, and you don't have to worry about handling its memory. You only need to add:

%include <std_shared_ptr.i>
%shared_ptr(THRSimConfig)

to the SWIG interface file before you %include "simulation/dynamics/_GeneralModuleFiles/THRSimConfig.h". A better approach would be to create a THRSimConfig.i file an %import -ing that instead of %include.

[schaubh]

Howdy @juan-g-bonilla, thanks for the suggestion. I think I'd like to try this, but in a separate branch. We did the RW configuration messages as raw pointers as well. If we change this, I'd like to change both RW and thrusters at once. I like the benefits you list for having better memory management if something is connected to these configuration message objects.

[schaubh]

Mm, looking at the unit tests, we are getting a lot of segfault errors. I'm assuming we have several scenarios where the thruster info is copied to the BSK effector and then not retained. With the raw pointers this leads to seg faults.

@Yume27 , could you please try out @juan-g-bonilla's suggestion above for this branch? Right now the tests are not passing. Please be sure to test all BSK tests on your computer as well before pushing a PR.

[schaubh]

The BSK tests are not all passing. See Juan's suggestion on using smart pointers.

Also, be sure to document these changes by updating the Release Notes, and update the thruster RST documentation.

[Yume27]

Mm, looking at the unit tests, we are getting a lot of segfault errors. I'm assuming we have several scenarios where the thruster info is copied to the BSK effector and then not retained. With the raw pointers this leads to seg faults.

@Yume27 , could you please try out @juan-g-bonilla's suggestion above for this branch? Right now the tests are not passing. Please be sure to test all BSK tests on your computer as well before pushing a PR.

Do you recommend trying what @juan-g-bonilla suggested rather than adapting each example script that was affected by the change?

[schaubh]

Let's try @juan-g-bonilla's suggestion to see if this helps here.