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

Reject enabling IK outside safety radius #279

Merged
merged 56 commits into from
Apr 3, 2024

Conversation

DrDab
Copy link
Contributor

@DrDab DrDab commented Nov 7, 2023

This prevents MissionControlProtocol from setting Globals::armIKEnabled if end effector breaches safety radius.

@DrDab DrDab requested a review from abhaybd November 7, 2023 01:36
@DrDab DrDab marked this pull request as draft November 7, 2023 01:47
@DrDab DrDab marked this pull request as ready for review November 28, 2023 02:16
@DrDab DrDab marked this pull request as draft November 28, 2023 02:26
Copy link
Member

@abhaybd abhaybd left a comment

Choose a reason for hiding this comment

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

Mostly look good, just some refactoring needed!

src/network/MissionControlProtocol.cpp Outdated Show resolved Hide resolved
src/network/MissionControlProtocol.cpp Outdated Show resolved Hide resolved
src/network/MissionControlProtocol.cpp Outdated Show resolved Hide resolved
src/network/MissionControlProtocol.cpp Outdated Show resolved Hide resolved
src/Globals.cpp Outdated Show resolved Hide resolved
Copy link
Member

@abhaybd abhaybd left a comment

Choose a reason for hiding this comment

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

The changes since my last review look good, but the main architectural issues haven't been resolved. Take a look at my previous comments, the one about the factory function and such, and try to see what you can do there. Let me know if you have any questions! We can talk about it at the next meeting if it helps.

@DrDab DrDab requested a review from abhaybd January 23, 2024 16:02
@DrDab DrDab marked this pull request as ready for review January 26, 2024 01:39
Copy link
Member

@abhaybd abhaybd left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just left some comments on initialization logic!

Have you tested this in simulation? I suspect this doesn't work right now. Also, can you add to the docs in every method in the controller class to clarify what the behavior is if the controller is uninitialized? I.e. you should mention if these methods should not be called on an uninitialized controller. Also, modify the class documentation to clarify what it means to be uninitialized, and clarify that the constructor creates it in that state.

src/control/PlanarArmController.h Outdated Show resolved Hide resolved
src/control/PlanarArmController.h Outdated Show resolved Hide resolved
src/control/PlanarArmController.h Outdated Show resolved Hide resolved
src/control/PlanarArmController.h Show resolved Hide resolved
tests/control/PlanarArmControllerTest.cpp Outdated Show resolved Hide resolved
@abhaybd abhaybd requested a review from huttongrabiel April 2, 2024 22:56
Copy link
Contributor

@huttongrabiel huttongrabiel left a comment

Choose a reason for hiding this comment

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

lgtm

@DrDab
Copy link
Contributor Author

DrDab commented Apr 3, 2024

Tested on sim with test cases:

  • Initialize IK within safety radius (succeeded as expected)
  • Initialize IK outside safety radius (failed as expected)
  • Re-initialize IK within safety radius after initial failure (succeeded as expected)
  • Re-initialize IK within safety radius after initial success (succeeded as expected)
  • Re-initialize IK outside safety radius after initial success (failed as expected)

@DrDab DrDab dismissed abhaybd’s stale review April 3, 2024 21:35

Tested on simulator

@DrDab DrDab merged commit 1f32925 into master Apr 3, 2024
3 checks passed
@DrDab DrDab deleted the rejectEnableIKOutsideSafetyRadius branch April 3, 2024 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants