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

Added throttle to jogarm accel limit warning #2141

Merged
merged 2 commits into from
Jun 5, 2020

Conversation

JStech
Copy link
Contributor

@JStech JStech commented Jun 5, 2020

Description

Minor change to add throttle to warning message, which is currently printed for every joint in every iteration of the control loop.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Jun 5, 2020

Codecov Report

Merging #2141 into master will increase coverage by 0.10%.
The diff coverage is 55.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2141      +/-   ##
==========================================
+ Coverage   57.72%   57.82%   +0.10%     
==========================================
  Files         328      328              
  Lines       25735    25735              
==========================================
+ Hits        14856    14882      +26     
+ Misses      10879    10853      -26     
Impacted Files Coverage Δ
...rm/include/moveit_jog_arm/collision_check_thread.h 100.00% <ø> (ø)
...veit_jog_arm/include/moveit_jog_arm/jog_arm_data.h 100.00% <ø> (ø)
...veit_experimental/moveit_jog_arm/src/jog_calcs.cpp 70.93% <28.57%> (ø)
...ntal/moveit_jog_arm/src/collision_check_thread.cpp 80.59% <47.61%> (ø)
...rimental/moveit_jog_arm/src/jog_interface_base.cpp 72.79% <76.92%> (ø)
...raint_samplers/src/default_constraint_samplers.cpp 82.54% <0.00%> (-0.37%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 47.98% <0.00%> (+0.09%) ⬆️
.../move_group_interface/src/move_group_interface.cpp 48.07% <0.00%> (+0.21%) ⬆️
... and 8 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 8e2ca7f...4c44246. Read the comment docs.

Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up my oversight! Here's a suggestion to take the PR to the next level, the best it could be (I think...).

There are two collision-checking algorithms, stop_distance and threshold_distance. See collision_check_thread.h. I think we should only print this warning if the current method is stop_distance. (threshod_distance doesn't need the acceleration limit)

To make that change, I think you could move the CollisionCheckType enum from collision_check_thread.h to jog_arm_data.h (so both threads have access to it). Or make a new header file just for the enum. Then change these lines of code to something like:

else if (parameters_.collision_check_type == K_STOP_DISTANCE)
{
  ROS_ERROR_STREAM_ONCE_NAMED(LOGNAME, "...");
}

But I would approve the PR without this change, too.

moveit_experimental/moveit_jog_arm/src/jog_calcs.cpp Outdated Show resolved Hide resolved
@JStech
Copy link
Contributor Author

JStech commented Jun 5, 2020

@AndyZe I'll let you put those improvements in another PR.

@tylerjw
Copy link
Member

tylerjw commented Jun 5, 2020

@davetcoleman please help. The old travis integration is marked as required even though this is passing the new travis setup. I'd like to merge this PR but I can't.

@davetcoleman
Copy link
Member

I've switched it to the new travis.

Why does Codecov really consider a single line edit as a decrease of .1%??

@tylerjw tylerjw merged commit d03bb75 into moveit:master Jun 5, 2020
@tylerjw
Copy link
Member

tylerjw commented Jun 5, 2020

Why does Codecov really consider a single line edit as a decrease of .1%??

Small changes are weird with codecov, no idea why.

tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jun 24, 2020
* added throttle to jogarm accel limit warning
* switched to ERROR_ONCE
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jun 24, 2020
* added throttle to jogarm accel limit warning
* switched to ERROR_ONCE
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jun 24, 2020
* added throttle to jogarm accel limit warning
* switched to ERROR_ONCE
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jun 24, 2020
* added throttle to jogarm accel limit warning
* switched to ERROR_ONCE
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jun 24, 2020
* added throttle to jogarm accel limit warning
* switched to ERROR_ONCE
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jun 29, 2020
* added throttle to jogarm accel limit warning
* switched to ERROR_ONCE
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request Jul 1, 2020
* added throttle to jogarm accel limit warning
* switched to ERROR_ONCE
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.

5 participants