-
Notifications
You must be signed in to change notification settings - Fork 90
Configure prescribed motion effector for select effector branching #1134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configure prescribed motion effector for select effector branching #1134
Conversation
1301065 to
0d84979
Compare
f0b70b8 to
77aa7a1
Compare
20788df to
983aa58
Compare
andrewmorell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR! A lot of added functionality and the scenarios demonstrating it are really cool. I like the sequence of calling updateContributions() then addPrescribedMotionContributions() to augment the backsub matrices. I also like how you dual use the same backsub matrices for the dependent state effectors and then the prescribed motion module.
I have two broad recommendations besides the specific code callouts below.
- Consider being more explicit with inline documentation where new variables are defined in .cpp files since they don't get the nice definitions that variables defined in the .h file do. Your linked papers are great for explaining what math goes on under the hood, but doesn't help someone perusing the code figure out how this math interacts with all of the existing Basilisk architecture choices and frame ambiguity.
- I feel like these new features could benefit from a guardrail or two that stops you from trying to setup configurations not yet allowed. When I try adding a non-compatible state effector to prescribed motion, I get an indirect error from the dynParamManager during sim initialization. A default error in the stateEffector base class' linkInPrescribedMotionProperties() method that says this combination isn't set up would be cleaner.
src/simulation/dynamics/prescribedMotion/prescribedMotionStateEffector.cpp
Show resolved
Hide resolved
src/simulation/dynamics/spinningBodies/spinningBodiesOneDOF/spinningBodyOneDOFStateEffector.cpp
Outdated
Show resolved
Hide resolved
src/simulation/dynamics/spinningBodies/spinningBodiesOneDOF/spinningBodyOneDOFStateEffector.cpp
Show resolved
Hide resolved
src/simulation/dynamics/spinningBodies/spinningBodiesTwoDOF/spinningBodyTwoDOFStateEffector.cpp
Outdated
Show resolved
Hide resolved
src/simulation/dynamics/spinningBodies/spinningBodiesTwoDOF/spinningBodyTwoDOFStateEffector.cpp
Outdated
Show resolved
Hide resolved
src/simulation/dynamics/spinningBodies/spinningBodiesTwoDOF/spinningBodyTwoDOFStateEffector.cpp
Outdated
Show resolved
Hide resolved
...nearTranslationalBodies/linearTranslationBodiesOneDOF/linearTranslationOneDOFStateEffector.h
Outdated
Show resolved
Hide resolved
schaubh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR overall. Thanks @andrewmorell for having gone through this so carefully. I agree with Andrew's comment that we need the same safeguards implemented as he has with his effector branching. This way if an effector is added that doesn't support your feature, then a BSK Error is thrown with a user-readable message as to why this is not working. Should be simple to mimic what Andrew did on his branch.
5a56ff4 to
f40daf3
Compare
f40daf3 to
8c2c93a
Compare
|
@schaubh I believe this PR is ready to go, I addressed @andrewmorell and your comments. |
andrewmorell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good on my side!
schaubh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go. Thanks for addressing the feedback and adding the guards against adding an effector which is not compatible.
Description
This PR addresses 5 issues related to adding the capability to connect other state effectors to the prescribed motion module rather than the spacecraft hub.
stateEffectorclass for attachment of state effectors to the prescribed motion effector.prescribedMotionmodule.prescribedMotionmodule to enable the new branching capability.prescribedMotionmodule.spinningBodyOneDOF,spinningBodyTwoDOF, andlinearTranslationOneDOFstate effectors for optional attachment to the prescribed motion effector.test_PrescribedMotionStateEffectorBranchingto the prescribed motion module. Each of these three state effectors are separately attached to the prescribed motion effector and independently verified.examplesfolder. The first is a rotational branching scenario calledscenarioPrescribedMotionWithRotationBranching. The second is a translational branching scenario and is calledscenarioPrescribedMotionWithTranslationBranching.bskReleaseNotes.Verification
A new test script
test_PrescribedMotionStateEffectorBranchingis added to theprescribedMotionmodule that checks the changes made in this PR.Documentation
The prescribed motion module documentation is updated to discuss these new features.
Future work
Other state effectors will be configured for attachment to the prescribed motion effector in the future.