-
Notifications
You must be signed in to change notification settings - Fork 248
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
Make sure published messages are acknowledged for play mode #951
Conversation
Signed-off-by: Barry Xu <barry.xu@sony.com>
The failure on ament_uncrustify is related to file
|
@fujitatomoya @emersonknapp |
@Barry-Xu-2018 will do in a couple of days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I only saw a couple comments that might use a bit of tweaking, though I don't think it's enough to hold up the review over.
Since you requested @fujitatomoya and @emersonknapp as reviewers, specifically, I'll wait on their judgement rather than attempt merging for now.
@@ -229,6 +229,30 @@ void Player::play() | |||
std::lock_guard<std::mutex> lk(ready_to_play_from_queue_mutex_); | |||
is_ready_to_play_from_queue_ = false; | |||
ready_to_play_from_queue_cv_.notify_all(); | |||
|
|||
// Wait for all published messages are acknowledged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Wait for all published messages are acknowledged | |
// Wait for all published messages to be acknowledged |
// Timeout for waiting all published messages acknowledged | ||
// negative means published messages need not to be acknowledged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Timeout for waiting all published messages acknowledged | |
// negative means published messages need not to be acknowledged | |
// Timeout for waiting for all published messages to be acknowledged. | |
// Negative values means that published messages do not need to be acknowledged. |
@jhdcs since i do not have any authorization on this repository, you can just go ahead instead of my comments if you are good to go. (i will do the review as community reviewer 😃 ) |
ros2bag/ros2bag/verb/play.py
Outdated
def not_negative_int(arg: str) -> int: | ||
value = int(arg) | ||
if value < 0: | ||
raise ArgumentTypeError(f'Value {value} is less than zero.') | ||
return value | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why dont we have this along with check_positive_float
as common function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to my understanding, the input parameter of check function is fixed (only arg: str
, and this parameter is passed by argparse module).
We cannot use this parameter to specify what to do, such as check positive
or not_negative
.
So I have no idea how to use one common function to implement check_positive_float
and not_negative_int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was not clear on this, i was just saying to move this function where check_positive_float
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was just saying to move this function where check_positive_float is.
Get it.
RCLCPP_WARN( | ||
get_logger(), | ||
"--wait-for-all-acked is invaild for topic '%s' since reliability of QOS is BestEffort.", | ||
topic.name.c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 100 of 200 topics come true with this if statement, there will be 100 warning lines here. which btw, i think it is likely to happen. do we need to print this warning for each topic? how about printing warning once that --wait-for-all-acked is invalid for topics with reliability of QOS is BestEffort.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if 100 of 200 topics come true with this if statement, there will be 100 warning lines here.
Yes. It is poor user experience.
But I think topic information is useful for user.
How about change as below: (only print one warning for all invalid topics)
-wait-for-all-acked is invalid for below topics with reliability of QOS is BestEffort
xxx, xxx, xxx, xxx, xxx
ros2bag/ros2bag/verb/play.py
Outdated
@@ -93,6 +101,17 @@ def add_arguments(self, parser, cli_name): # noqa: D102 | |||
parser.add_argument( | |||
'--start-offset', type=check_positive_float, default=0.0, | |||
help='Start the playback player this many seconds into the bag file.') | |||
parser.add_argument( | |||
'--wait-for-all-acked', type=not_negative_int, default=0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default behavior would be okay with wait forever
to avoid possible problem to miss the last message? we would like to have consensus on this. CC: @emersonknapp @jhdcs
IMO, we would want to have specific timeout like 1 sec instead of wait forever
, because if anything happens, wait forever
will be not good for user experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.
I think this option should be disabled by default, since most cases don't need this feature.
What do you think ?
if (!pub.second->wait_for_all_acked(timeout)) { | ||
RCLCPP_ERROR( | ||
get_logger(), | ||
"Failed to wait all published messages acknowledged for topic %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this happens only when timeout
, so how about the following? I think timeout is different from failure or exception.
"Failed to wait all published messages acknowledged for topic %s", | |
"Timeout to wait all published messages acknowledged for topic %s", |
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did another review pass, since several changes were made. Again, just had some comment/documentation suggestions.
'--wait-for-all-acked', type=check_not_negative_int, default=-1, | ||
help='Wait until all published messages are acknowledged by all subscribers or until ' | ||
'the timeout elapses in millisecond before play is terminated. ' | ||
'Especially for the case of sending message with big size in a short time. ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this flows a little better, though I tend to be a bit wordy.
'Especially for the case of sending message with big size in a short time. ' | |
'Especially useful when sending many messages with large sizes in a short timeframe.' |
if (!pub.second->wait_for_all_acked(timeout)) { | ||
RCLCPP_ERROR( | ||
get_logger(), | ||
"Timeout to wait all published messages acknowledged for topic %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this reads a little better.
"Timeout to wait all published messages acknowledged for topic %s", | |
"Timed out while waiting for all published messages to be acknowledged for topic %s", |
} catch (std::exception & e) { | ||
RCLCPP_ERROR( | ||
get_logger(), | ||
"Failed to wait all published messages acknowledged for topic %s : %s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another small wordage suggestion.
"Failed to wait all published messages acknowledged for topic %s : %s", | |
"Exception occurred while waiting for all published messages to be acknowledged for topic %s : %s", |
|
||
RCLCPP_WARN( | ||
get_logger(), | ||
"--wait-for-all-acked is invaild for below topics since reliability of QOS is BestEffort.\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor change here.
"--wait-for-all-acked is invaild for below topics since reliability of QOS is BestEffort.\n" | |
"--wait-for-all-acked is invaild for the below topics since reliability of QOS is BestEffort.\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments.
Please help to check again @jhdcs
Signed-off-by: Barry Xu <barry.xu@sony.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Did CI from https://ci.ros2.org get run on this PR? |
Oh crud, I thought it had! Crud, let me see how to undo that... |
Not sure I did that right... Sorry, first time merging a PR... |
@clalancette thanks for checking on this one. after all, everything looks okay! |
Address #571
After test with the latest code for rolling (use fastdds), #571 (follow steps to reproduce in #571) cannot reproduce.
Note that synchronous is set as default for fastdds recently(ros2/rmw_fastrtps@e7c749a). But even if I use async mode, #571 also cannot reproduce.