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

[jog_arm] Fix valid cmd flags #2013

Merged
merged 2 commits into from Apr 8, 2020
Merged

[jog_arm] Fix valid cmd flags #2013

merged 2 commits into from Apr 8, 2020

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented Apr 6, 2020

Switching back and forth between joint commands and Cartesian commands could be buggy sometimes. The change at line 203 of jog_calcs.cpp fixes that.

Some flags were renamed and their logic was flipped to help readability

@AndyZe
Copy link
Member Author

AndyZe commented Apr 6, 2020

@JStech @henningkayser

@AndyZe AndyZe changed the title Fix valid cmd flags [jog_arm] Fix valid cmd flags Apr 6, 2020
@codecov-io
Copy link

Codecov Report

Merging #2013 into master will increase coverage by 0.06%.
The diff coverage is 67.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2013      +/-   ##
==========================================
+ Coverage   51.82%   51.88%   +0.06%     
==========================================
  Files         319      317       -2     
  Lines       24959    24618     -341     
==========================================
- Hits        12934    12773     -161     
+ Misses      12025    11845     -180     
Impacted Files Coverage Δ
...veit_jog_arm/include/moveit_jog_arm/jog_arm_data.h 100.00% <ø> (ø)
...erimental/moveit_jog_arm/src/jog_cpp_interface.cpp 11.49% <0.00%> (ø)
...veit_experimental/moveit_jog_arm/src/jog_calcs.cpp 75.31% <100.00%> (+0.63%) ⬆️
...erimental/moveit_jog_arm/src/jog_ros_interface.cpp 84.81% <100.00%> (ø)
.../include/moveit/py_bindings_tools/py_conversions.h 0.00% <0.00%> (-86.67%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 75.33% <0.00%> (-2.25%) ⬇️
...ls/include/moveit/py_bindings_tools/gil_releaser.h
...s/include/moveit/py_bindings_tools/serialize_msg.h
...anning_scene_monitor/src/current_state_monitor.cpp 52.82% <0.00%> (+1.53%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d989220...83b46b7. Read the comment docs.

@henningkayser
Copy link
Member

@AndyZe Couldn't this potentially lead to sudden jerks in the motion when switching from one mode to another? In general, I think it might make sense to make this a little more explicit, i.e. setting the mode via CPP-interface, or only switching mode after repeated commands via ROS-interface

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Any straight-forward way to test this? Switching modes is more of an edge case for now I assume, but would it be possible to mix joint and cartesian commands without much delay?

@AndyZe
Copy link
Member Author

AndyZe commented Apr 7, 2020

@AndyZe Couldn't this potentially lead to sudden jerks in the motion when switching from one mode to another? In general, I think it might make sense to make this a little more explicit, i.e. setting the mode via CPP-interface, or only switching mode after repeated commands via ROS-interface

Well, the user can already request abrupt changes in motion. In Cartesian mode, the commands get filtered, but there isn't a filter in joint mode. A joint command filter might be something to add for the future.

Yeah, an explicit mode is a good idea, although a pain to implement and test for C++ and the ROS interface.

Edit - I looked through the code to see how the lowpass filters get applied. They are used in both Cartesian and joint control mode, so commands from the user should get smoothed regardless. So, I don't think an explicit mode is necessary. It's nice to keep the jogger agnostic to the source of incoming commands.

I'll make some plots to verify the smoothing actually happens when transitioning from Cartesian -> joint control.

Copy link
Contributor

@JStech JStech left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for tracking this down

@AndyZe
Copy link
Member Author

AndyZe commented Apr 7, 2020

Here are some plots of transitions from joint to Cartesian commands in opposite directions. And Cartesian commands to joint commands. Looks pretty smooth to me, I don't think more work on that is necessary.

joint_to_cartesian_command

cartesian_to_joint_command

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

@AndyZe great work! Tested this and didn't find any issues with it.

@henningkayser henningkayser merged commit 18715e1 into moveit:master Apr 8, 2020
@tylerjw tylerjw mentioned this pull request May 8, 2020
20 tasks
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request May 12, 2020
* Rename the 'zero command flag' variables for readability
* Reset flags when incoming commands timeout
* Remove debug line, clang format
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request May 12, 2020
* Rename the 'zero command flag' variables for readability
* Reset flags when incoming commands timeout
* Remove debug line, clang format
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request May 20, 2020
* Rename the 'zero command flag' variables for readability
* Reset flags when incoming commands timeout
* Remove debug line, clang format
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.

None yet

4 participants