personalrobotics / aikido

Artificial Intelligence for Kinematics, Dynamics, and Optimization
https://personalrobotics.github.io/aikido/
BSD 3-Clause "New" or "Revised" License
213 stars 30 forks source link

Controller Switching through RosRobot #621

Closed RKJenamani closed 1 year ago

RKJenamani commented 2 years ago

Builds on top of #614. Adds corresponding controller operations (load, start, stop) to the executors operations (register, activate, deactivate). Corresponding hardware mode switching takes place if a joint mode controller is specified.


Before creating a pull request

Before merging a pull request

egordon commented 1 year ago

@RKJenamani please resolve conflicts

egordon commented 1 year ago

So when I was talking about ownership of the Client objects, I wasn't talking necessarily about getting rid of the setters. You completely got rid of the ability to modify any parameters about the controller service clients. You can have shared_ptr to both mode and controller objects and still have them owned by the RosRobot class.

Here is what I would recommend (and I'll review to this effect): (1) Keep separate setters for both mode and controller manager service clients. But don't have the argument just be a shared_ptr (or keep it as a way for maximum customization, but don't make it the only option, see below) (2) The only parameter that matters for the controller_manager service clients is the namespace (i.e. the prefix before "cotnroller_manager/*"). So have those clients default to nullptr (which disables controller switching) and have the setter just ask for the namespace (default "") (3) Have the setter for the mode switch that takes takes in the 2 necessary parameters: controller name and string list of joints. I would completely get rid of the "joint_mode" string default (since that is definitely non-standard). (4) Have a single boolean flag in registerSubrobot that, if true, forwards mode and controller clients, and otherwise starts them as nullptr.

egordon commented 1 year ago

With that said, I think this is actually okay to merge as-is (and move the above to an issue / future PR) just to keep our PRs small and easy to review.

I'll make sure this doesn't break Gen2 demos and then we can approve and merge

egordon commented 1 year ago

Does not break Feeding Demo, will merge after CI is fixed (see #631 )

codecov[bot] commented 1 year ago

Codecov Report

Merging #621 (af841f3) into master (bd2e675) will decrease coverage by 0.89%. The diff coverage is 3.38%.

:exclamation: Current head af841f3 differs from pull request most recent head b8267e2. Consider uploading reports for the commit b8267e2 to get more accurate results

@@            Coverage Diff             @@
##           master     #621      +/-   ##
==========================================
- Coverage   73.01%   72.12%   -0.89%     
==========================================
  Files         221      223       +2     
  Lines        7978     8083     +105     
==========================================
+ Hits         5825     5830       +5     
- Misses       2153     2253     +100     
Impacted Files Coverage Δ
...e/aikido/control/ros/RosJointModeCommandClient.hpp 0.00% <0.00%> (ø)
...ontrol/ros/detail/RosJointCommandExecutor-impl.hpp 0.00% <ø> (ø)
include/aikido/robot/Robot.hpp 100.00% <ø> (ø)
src/control/ros/RosJointGroupCommandClient.cpp 0.00% <ø> (ø)
src/control/ros/RosJointModeCommandClient.cpp 0.00% <0.00%> (ø)
src/control/ros/RosTrajectoryExecutor.cpp 0.00% <ø> (ø)
src/robot/Robot.cpp 18.89% <10.25%> (-0.99%) :arrow_down:
src/io/util.cpp 84.61% <0.00%> (-2.06%) :arrow_down:
src/control/ros/Conversions.cpp 80.16% <0.00%> (+0.08%) :arrow_up:
include/aikido/robot/util.hpp 90.90% <0.00%> (+36.36%) :arrow_up: