Pause service for rosbag player by ayrton04 · Pull Request #949 · ros/ros_comm

ayrton04 · 2016-12-14T16:26:59Z

We've recently required the ability to pause playback in rosbag play via service calls within code. The changes include the addition of a spinOnce() call within doPublish() so that we can process the service request. I'm not sure if having callbacks in the Player class is acceptable, or if this functionality would be useful for anyone else, but I thought it worth a PR.

ayrton04 · 2016-12-14T16:28:18Z

tools/rosbag/src/player.cpp

+              }
+              else
+              {
+                ros::WallDuration shift = ros::WallTime::now() - paused_time_;


I realize this logic is duplicated in the keyboard input processing below, and can put it in a method if preferred.

+1 for merging methods

tappan-at-git · 2016-12-22T14:41:59Z

I think having the service be namespaced is a good call, even if we need node_handle_ to be at the base namespace of the node. Otherwise you'll have issues if you run more than one rosbag record instance in the same namespace. Tom has rightly pointed out that 1) this is for playback, not recording, and 2) having a service namespaced under an anonymous node is ridiculously unhelpful.

That said, it may be good to rename the service to something a little less generic if we can't conveniently namespace it. rosbag_pause could be a good match with the existing style.

mikepurvis · 2017-01-10T19:33:02Z

tools/rosbag/src/player.cpp

+
+  if (res.success)
+  {
+    res.message = (requested_pause_state_ ? "Pause" : "Resume") + std::string(" requested.");


Wondering if this should be past tense? Eg, Bag playback is now paused/resumed.

mikepurvis · 2017-01-10T19:38:07Z

tools/rosbag/src/player.cpp

+    requested_pause_state_(false),
    terminal_modified_(false)
 {
+  pause_service_ = node_handle_.advertiseService("pause", &Player::pauseCallback, this);


Obviously a different service name here.

Perhaps another option might be:

ros::NodeHandle private_node_handle('~'); pause_service_ = private_node_handle.advertiseService("pause", &Player::pauseCallback, this);

This should get you a rosbag/pause service name, but it will be trivial to rename/remap it from roslaunch or wherever else.

I wasn't sure what the best route was here. The problem with a private node handle is that if you're running rosbag play outside a launch file, it will by default be given an anonymous name, and so calling the service is a pain (e.g., rosservice call /play_1484077899473435649/pause). As I couldn't envisage a use case wherein we'd be running more than one rosbag play instance, I figured a service in the global namespace would work.

However, I also can't really see using this outside of a launch file, and even if we did, we could assign a name via the command line. I'll move it back to the private node handle.

mikepurvis · 2017-01-10T19:39:02Z

This seems simple and straightforward. I'd support a merge if the service name can be sorted out.

…service using a private node handle.

mikepurvis · 2017-01-13T20:29:15Z

LGTM.

jspricke

Thanks! +1 apart from some comments.

jspricke · 2017-01-13T21:44:31Z

tools/rosbag/src/player.cpp

    terminal_modified_(false)
 {
+  ros::NodeHandle private_node_handle("~");
+  pause_service_ = private_node_handle.advertiseService("pause", &Player::pauseCallback, this);


maybe name it "pause_playback"?

OK, can do.

jspricke · 2017-01-13T21:45:37Z

tools/rosbag/include/rosbag/player.h


    ros::NodeHandle node_handle_;

+    ros::ServiceServer pause_service_;


I guess this changes the ABI, but I don't think anyone is relying on that @dirk-thomas ?

If we can avoid breaking the ABI than we should for sure aim for that. While we don't "guarantee" it we try to make an effort where possible.

OK. Shall I make this static?

I think ABI would be maintained if all the new fields (and method) were appended to the class rather than inserted.

However, IMO this is not worth it for the loss in source readability— there are unlikely to be external packages linking to this class.

OK, I can go either way. Let me know what you'd all prefer.

leave it as it is.

jspricke · 2017-01-13T21:45:53Z

tools/rosbag/src/player.cpp

+              }
+              else
+              {
+                ros::WallDuration shift = ros::WallTime::now() - paused_time_;


+1 for merging methods

…n a method

mikepurvis · 2017-01-21T19:54:03Z

Looks great— thanks Tom!

* Adding pause service to rosbag Player class * Adding std_srvs dependency * Making response more informative * Using private node handle to advertise service * Removing private namespace from node handle * PR feedback: changing response message to past tense and advertising service using a private node handle. * PR feedback: changing service name and placing pause handling logic in a method

ayrton04 added 4 commits December 13, 2016 14:55

Adding pause service to rosbag Player class

d5dc6b6

Adding std_srvs dependency

e151c0b

Making response more informative

425ea10

Using private node handle to advertise service

9aca7bb

ayrton04 commented Dec 14, 2016

View reviewed changes

dirk-thomas changed the title ~~Pause service~~ Pause service for rosbag player Dec 14, 2016

Removing private namespace from node handle

2824921

mikepurvis reviewed Jan 10, 2017

View reviewed changes

mikepurvis added the hitlist label Jan 10, 2017

PR feedback: changing response message to past tense and advertising …

6d8346e

…service using a private node handle.

jspricke reviewed Jan 13, 2017

View reviewed changes

ayrton04 added 2 commits January 18, 2017 10:10

PR feedback: changing service name and placing pause handling logic i…

89e618a

…n a method

Merging in upstream changes

1296433

mikepurvis merged commit bd7e89c into ros:kinetic-devel Jan 21, 2017

mikepurvis mentioned this pull request Feb 13, 2017

Implemented rate-control-topic and rate-control-max-delay. #947

Merged

dirk-thomas mentioned this pull request Mar 2, 2017

changes between 1.11.20 and 1.12.7 for backporting #1008

Merged

dirk-thomas mentioned this pull request Jul 14, 2017

improve roswtf tests #1101

Merged


		ros::NodeHandle node_handle_;

		ros::ServiceServer pause_service_;

Conversation

ayrton04 commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tappan-at-git commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ayrton04 Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikepurvis commented Jan 10, 2017

Uh oh!

mikepurvis commented Jan 13, 2017

Uh oh!

jspricke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikepurvis commented Jan 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tappan-at-git commented Dec 22, 2016 •

edited

Loading

ayrton04 Jan 10, 2017 •

edited

Loading