Skip to content

Publish singularity condition to topic#695

Merged
AndyZe merged 2 commits intomoveit:mainfrom
nbbrooks:improve-singularity-deubgging
Sep 28, 2021
Merged

Publish singularity condition to topic#695
AndyZe merged 2 commits intomoveit:mainfrom
nbbrooks:improve-singularity-deubgging

Conversation

@nbbrooks
Copy link
Copy Markdown
Contributor

@nbbrooks nbbrooks commented Sep 22, 2021 •
edited
Loading

Description

Publish singularity condition to ~/servo_server/condition to help understand servo behavior around singularities

This is broken out from #620

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

@nbbrooks nbbrooks requested a review from AndyZe September 22, 2021 14:43
@nbbrooks nbbrooks force-pushed the improve-singularity-deubgging branch from 51f31bd to ab4c21b Compare September 22, 2021 14:49
@nbbrooks nbbrooks changed the title Improve knowledge of servo behavior around singularities Publish singularity condition to topic Sep 22, 2021
felixvd
felixvd approved these changes Sep 22, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@felixvd felixvd left a comment

Choose a reason for hiding this comment

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

Debug data good.

nbbrooks
nbbrooks commented Sep 22, 2021
View reviewed changes
@nbbrooks nbbrooks mentioned this pull request Sep 22, 2021
Merged
8 tasks
@codecov
Copy link
Copy Markdown

codecov bot commented Sep 22, 2021 •
edited
Loading

Codecov Report

Merging #695 (e07655a) into main (eec02c7) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head e07655a differs from pull request most recent head 76dde4e. Consider uploading reports for the commit 76dde4e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #695      +/-   ##
==========================================
+ Coverage   54.17%   54.19%   +0.02%     
==========================================
  Files         192      192              
  Lines       20182    20184       +2     
==========================================
+ Hits        10931    10936       +5     
+ Misses       9251     9248       -3
Impacted Files Coverage Δ
moveit_ros/moveit_servo/src/servo_calcs.cpp 70.64% <100.00%> (+0.12%) ⬆️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 75.85% <0.00%> (+1.14%) ⬆️

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 eec02c7...76dde4e. Read the comment docs.

tylerjw
tylerjw approved these changes Sep 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

one small nit, otherwise I see no issue with this.

moveit_ros/moveit_servo/src/servo_calcs.cpp

// Publish status
status_pub_ = node_->create_publisher<std_msgs::msg::Int8>(parameters_->status_topic, ROS_QUEUE_SIZE);
condition_pub_ = node_->create_publisher<std_msgs::msg::Float64>("~/condition", ROS_QUEUE_SIZE);
Copy link
Copy Markdown
Member

@tylerjw tylerjw Sep 22, 2021

Choose a reason for hiding this comment

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

Would you mind moving the name of this topic into a constant in the anonymous namespace? We generally try to avoid string constants and other magic numbers in the code.

Copy link
Copy Markdown
Contributor Author

@nbbrooks nbbrooks Sep 22, 2021

Choose a reason for hiding this comment

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

will do, thanks

@nbbrooks nbbrooks force-pushed the improve-singularity-deubgging branch from e07655a to 1063ab4 Compare September 24, 2021 23:14
@AndyZe AndyZe merged commit e6835c2 into moveit:main Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tylerjw tylerjw tylerjw approved these changes

@AndyZe AndyZe AndyZe approved these changes

+1 more reviewer

@felixvd felixvd felixvd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nbbrooks @tylerjw @felixvd @AndyZe