Skip to content
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

Port prescribedRot1DOF module to C++ with coast profiler #597

Merged
merged 20 commits into from
Mar 6, 2024

Conversation

leahkiner
Copy link
Contributor

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

Description

This PR ports the existing prescribedRot1DOF C module to a C++ module. The primary motivation for this migration is to provide a significantly simpler public api for the module. This simpler interface make the module easier to setup and use, and makes it much hard to use in an incorrect fashion.

This PR is a refactor of a previously opened PR #555 which was later closed in favor of this migration to C++. As a result, this PR implements the features added in the previous PR (in their order) and then cleans up elements made redundant by the move to C++. As a first step the PR ports the existing module directly to a C++ class. Then a new coast feature is added where a period of zero acceleration is added between the two acceleration segments when the user sets the ramp duration via setCoastRampDuration. The final significant change to the module is that all class variables are made private, with only the needed accessor methods provided.

Verification

The same module test is employed here with only modifications to account for the simplified module interface.

Documentation

The documentation from prescribedRot1DOF module has been added and updated.

Future work

The prescribedRot1DOF module is now deprecated. A general Basilisk module deprecation facility needs to be added so that we can mark an entire module as deprecated.

@leahkiner leahkiner added the enhancement New feature or request label Feb 9, 2024
@leahkiner leahkiner self-assigned this Feb 9, 2024
@leahkiner leahkiner linked an issue Feb 9, 2024 that may be closed by this pull request
@leahkiner
Copy link
Contributor Author

Note that PR #576 must be merged prior to this PR (This PR will not pass the checks until #576 is merged)

@leahkiner leahkiner marked this pull request as ready for review February 9, 2024 22:13
@leahkiner leahkiner changed the title Refactor/refactor prescribed rotation1 dof Port prescribedRot1DOF module to C++ with coast profiler Feb 9, 2024
@leahkiner leahkiner marked this pull request as draft February 12, 2024 21:40
@leahkiner leahkiner force-pushed the refactor/refactor-prescribedRotation1DOF branch 3 times, most recently from b4070b2 to f2ecdeb Compare February 12, 2024 22:36
@leahkiner leahkiner marked this pull request as ready for review February 12, 2024 22:38
@leahkiner leahkiner force-pushed the refactor/refactor-prescribedRotation1DOF branch 8 times, most recently from 63be960 to 66c5428 Compare February 14, 2024 22:06
@schaubh schaubh force-pushed the refactor/refactor-prescribedRotation1DOF branch from 66c5428 to 924b147 Compare February 17, 2024 20:09
@schaubh
Copy link
Contributor

schaubh commented Feb 18, 2024

The prescribedMotion dynamic effector test must be updated to use the new profiler motion, not the depreciated prescribedRot1DOF module.

Copy link
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there. Please

  • integrate the RST documentation fix marked with [squash]
  • update the prescribed motion dynamic effector unit test to use the new profiler, not not depreciated prescribedRot1DOF module

@leahkiner leahkiner force-pushed the refactor/refactor-prescribedRotation1DOF branch from 924b147 to 870a335 Compare February 19, 2024 18:44
@leahkiner
Copy link
Contributor Author

@schaubh Thank you for your feedback, I just pushed your requested changes.

@leahkiner leahkiner force-pushed the refactor/refactor-prescribedRotation1DOF branch 2 times, most recently from 473051a to 9ec3353 Compare February 19, 2024 19:38
@schaubh schaubh self-requested a review February 19, 2024 19:53
@leahkiner leahkiner force-pushed the refactor/refactor-prescribedRotation1DOF branch from 9ec3353 to ed5af4c Compare February 19, 2024 20:34
Copy link
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Good to go.

@leahkiner leahkiner force-pushed the refactor/refactor-prescribedRotation1DOF branch from ed5af4c to 9c80ffa Compare March 6, 2024 16:59
@patkenneally patkenneally merged commit 6f12949 into develop Mar 6, 2024
2 checks passed
@patkenneally patkenneally deleted the refactor/refactor-prescribedRotation1DOF branch March 6, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add rotation with coast option to the prescribedRot1DOF module
3 participants