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] Pass planning scene monitor into cpp interface #1849

Merged
merged 2 commits into from
Jan 22, 2020

Conversation

davetcoleman
Copy link
Member

@davetcoleman davetcoleman commented Jan 21, 2020

  • Separate LOGNAME into each file
  • Add unit test

I need to pass in the PSM from a larger application. Previously it required creating a new PSM even in the same process

@davetcoleman davetcoleman force-pushed the jog_arm_psm branch 2 times, most recently from 37a9fbe to 2363b4b Compare January 21, 2020 22:09
@davetcoleman davetcoleman changed the title [jog_arm] Pass planning scene monitor into cpp interface - WIP [jog_arm] Pass planning scene monitor into cpp interface Jan 21, 2020
- Separate LOGNAME into each file
- Add unit test
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

The image is still rather large: 4032x3024. Please reduce to something like 1200x.. or 1024x.. Thanks.

@gavanderhoorn
Copy link
Contributor

The image is still rather large: 4032x3024. Please reduce to something like 1200x.. or 1024x.. Thanks.

was this comment meant for moveit/moveit.ros.org#401?

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Nice cleanup. Some minor comments remaining.


double velocity_scale_coefficient = -log(0.001) / parameters_.collision_proximity_threshold;
// Copy the planning scene's version of current state into new memory
robot_state::RobotState current_state(getLockedPlanningSceneRO()->getCurrentState());
Copy link
Contributor

Choose a reason for hiding this comment

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

Great! Better decoupling of PSM.

planning_scene_monitor::PlanningSceneMonitorPtr planning_scene_monitor_;
}; // class TestJogCppInterface

TEST_F(TestJogCppInterface, PlanningSceneTest)
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests are not very informative yet, but they are a start 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

I realized need to do huge refactors of jog_arm to make it testable (separate ROS and simplify threading)


TEST_F(TestJogCppInterface, InitTest)
{
moveit_jog_arm::JogCppApi jog_cpp_interface(planning_scene_monitor_);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about inserting a ROS sleep for a second or so to give the threads some time to actually process data?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should add arbitrary sleeps until its proven needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the thread is immediately stopped due to immediate destruction of JogCppApi.
Adding a sleep would give the thread same time to cycle.

Copy link
Member Author

Choose a reason for hiding this comment

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

i added sleep

@davetcoleman
Copy link
Member Author

@rhaschke i force pushed changes to my feature branch, but just noticed you had some kind of formatting changes that got squashed, and i can't restore. what were they?

@davetcoleman davetcoleman merged commit cd16a50 into moveit:master Jan 22, 2020
@davetcoleman davetcoleman deleted the jog_arm_psm branch January 22, 2020 18:23
@rhaschke
Copy link
Contributor

I force pushed changes to my feature branch

That's why one should always use --force-with-lease 😉
I can recover. Will have a look asap. Thanks for the note.

@davetcoleman
Copy link
Member Author

That's a cool trick I hadn't heard of!

@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
* [jog_arm] Pass planning scene monitor into cpp interface

- Separate LOGNAME into each file
- Add unit test

* Add PR feedback
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request May 12, 2020
* [jog_arm] Pass planning scene monitor into cpp interface

- Separate LOGNAME into each file
- Add unit test

* Add PR feedback
tylerjw pushed a commit to PickNikRobotics/moveit that referenced this pull request May 20, 2020
* [jog_arm] Pass planning scene monitor into cpp interface

- Separate LOGNAME into each file
- Add unit test

* Add PR feedback
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