Convert all remaining actions to new framework

[AstroStucky]

Linked Issues:

Jira Ticket 🎟️ OCEANWATER-904

Summary of Changes

• All supported actions now use the new action framework
• Stow/Unstow now use the same mixin class as most other arm actions
• At some point within this branch arm action feedbacks were broken. They are now fixed.
• While porting GuardedMove, a bunch of logic that appeared to do nothing was removed/simplified.
• Fixed a bug that caused RRTstar planning actions to break Unstow/Stow/ArmMoveJoint(s) actions called after them.

Tests

Source devel/setup.bash in every terminal you perform the following tests in.

arm_check_action.test

Run rostest ow_sim_tests arm_check_action.test and verify all units pass.

sample_collection.test

Run rostest ow_sim_tests sample_collection.test and verify all units pass.

ow_plexil Regression Test

First terminal Run any sim environment roslaunch ow atacama_y1a.launch
Second terminal Run the ReferenceMission1 roslaunch ow_plexil ow_exec.launch plan:=ReferenceMission1.plx

Test the 2 actions not covered in rostests

First terminal Boot up any sim environment (feel free to use the one from the previous test)
Second terminal

1. Make antenna mast shines its lights onto the lander body rosrun ow_lander antenna_pan_tilt.py 0.0 1.0
2. Change the intensity of both lights rosrun ow_lander light_set_intensity.py left 0.0 then rosrun ow_lander light_set_intensity.py right 0.5

Help Messages

Ensure all help messages of all clients work and that the language is clear. Here's a single-line command that will print them all, or feel free to do so manually.

roscd ow_lander/scripts; for script in antenna_pan_tilt.py guarded_move.py arm_move_joint.py arm_move_joints.py camera_capture.py light_set_intensity.py deliver.py dig_circular.py dig_linear.py discard.py stop.py dock_ingest_sample.py stow.py grind.py unstow.py; do echo -e "\033[0;33m=== HELP MESSAGE FOR ${script} ===\033[0m"; ./$script --help; done

The orange text is just a separator and is not part of the help messages.

Caveat

Whenever simulation shuts down you will now see the following message

[ERROR] Could not stop controller 'limbs_controller' since it is not running

It comes from Gazebo. The limbs controller is not actually used by anything as best as I can tell, but I also do not wish to remove its implementation before I understand it better. I made some attempts to suppress the message, but none were successful. It does not appear to break anything. I will make an entry in our Caveats section once this is merged.
It turns out this was already happening in noetic-devel, but is not listed in our Caveats. I'll go ahead and make an entry for this in my documentation branch.

[kmdalal]

First and second try each had one failure, which were approximately the same:

[ow_sim_tests.rosunit-arm_check_action/test_02_guarded_move][FAILURE]-----------
-0.10109258250951786 not less than or equal to -0.10730500000000001 : y-coordinate is out of acceptable range

Third test had 2 failures, the above and this one:

[ow_sim_tests.rosunit-arm_check_action/test_04_dig_circular][FAILURE]-----------
2.2538438803223033 not greater than or equal to 2.303171327437262 : x-coordinate is out of acceptable range

I have not tried any more.

[kmdalal]

All tests pass except the one noted above. Moving on to the code for now.

[AstroStucky]

First and second try each had one failure, which were approximately the same:

[ow_sim_tests.rosunit-arm_check_action/test_02_guarded_move][FAILURE]-----------
-0.10109258250951786 not less than or equal to -0.10730500000000001 : y-coordinate is out of acceptable range

My most recent commit changed the frame in which guarded move provides its "final" Result parameter, so it makes perfect sense this now consistently fails. I will update the expected value configuration.

[ow_sim_tests.rosunit-arm_check_action/test_04_dig_circular][FAILURE]-----------
2.2538438803223033 not greater than or equal to 2.303171327437262 : x-coordinate is out of acceptable range

My explanation for this failure is not as clean cut. Considering it deviated by an entire 5 cm, I read this as indicating a real bug, but maybe a sporadic one. Something to keep an eye on next retest.

I will be back to provide updated expected values for GuardedMove later this morning.

[AstroStucky]

Expected values for GuardedMove have been updated to be in the correct frame. arm_check_action.test, should work fine, but please do watch your tests closely in case there are obvious diversions off the expected trajectory. Our rostests only verify the final position and duration, but do not verify the trajectory.

[kmdalal]

The arm check test now fully passes.

Just one request: add a descriptive sentence at the top of every .py file, even if you think the filename is descriptive enough. About half of them don't have obvious meanings to me.

Also, I think it would have been good to delay renaming the action client scripts, since one more renaming will be needed (I'd like to drop the _client suffix), but this is okay.

Great work!

[AstroStucky]

Just one request: add a descriptive sentence at the top of every .py file, even if you think the filename is descriptive enough. About half of them don't have obvious meanings to me.

Can do!

Also, I think it would have been good to delay renaming the action client scripts, since one more renaming will be needed (I'd like to drop the _client suffix), but this is okay.

I am all for this. What if I just did the full rename right now in this PR? So stow_client.py will just become stow.py, and same for all the rest.

[kmdalal]

I am all for this. What if I just did the full rename right now in this PR? So stow_client.py will just become stow.py, and same for all the rest.

That sounds fine, actually. This will need a user documentation update, however, which you can do on a branch of the wiki.

[AstroStucky]

That sounds fine, actually. This will need a user documentation update, however, which you can do on a branch of the wiki.

Done. Unfortunately it is now harder to discern action client scripts from other scripts in the ow_lander/scripts directory, but this problem should get better as we remove the old ROS Services code.

Out of scope for this PR, but I am considering merging all action servers into a single executable. So arm_action_servers.py, dock_ingest_sample_server.py, camera_capture_server.py, ect, ect would all become lander_action_servers.py.

[kmdalal]

Done. Unfortunately it is now harder to discern action client scripts from other scripts in the ow_lander/scripts directory, but this problem should get better as we remove the old ROS Services code.

I think the net change was positive.

Out of scope for this PR, but I am considering merging all action servers into a single executable. So arm_action_servers.py, dock_ingest_sample_server.py, camera_capture_server.py, ect, ect would all become lander_action_servers.py.

That sounds like a good idea. Feel free to add it to this PR if it's not hard or complicated. Lower priority than implementing unified actions though.

[kmdalal]

I just realized another round of script renaming may be in order when we rename all the actions to their unified name. But let's hold off any more renaming for now.

[AstroStucky]

That sounds like a good idea. Feel free to add it to this PR if it's not hard or complicated. Lower priority than implementing unified actions though.

Done. I think this is cleaner.

[AstroStucky]

@kmdalal There is one new test section I would like you to complete. The LightSetIntensity was not tested by the tests you have already performed.

[kmdalal]

The additional test (pan/tilt, lights) passed and the filename changes are good (for now).

[kmdalal]

@AstroStucky , since this PR is a prerequisite for subsequent action work that both you and I potentially want to tackle this week, and @anthonykoutroulis is out this week, you can merge it now if it's done on your end. I think it has been sufficiently reviewed.