-
Notifications
You must be signed in to change notification settings - Fork 99
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
Distance segfault #642
Distance segfault #642
Conversation
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
Signed-off-by: Nate Koenig <nate@openrobotics.org>
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.
Quick comments, haven't ran it yet
# docker is run | ||
RUN mkdir -p subt_ws/src \ | ||
&& cd subt_ws/src \ | ||
&& git clone https://github.com/osrf/subt -b distance_segfault |
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.
Reminder to reset this branch before merging.
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 guess we didn't want this part merged as is, right?
subt_ign/src/GameLogicPlugin.cc
Outdated
@@ -576,6 +590,13 @@ GameLogicPlugin::~GameLogicPlugin() | |||
this->dataPtr->finished = true; | |||
if (this->dataPtr->publishThread) | |||
this->dataPtr->publishThread->join(); | |||
|
|||
if (this->dataPtr->bagThread) |
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.
Since this is already done in Finish
it may not be necessary to duplicate here.
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.
Actually when running this, I get a segfault because it's trying to join the thread twice for some reason, I have a fix inbound.
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.
Look at bc03832
subt_ign/src/GameLogicPlugin.cc
Outdated
this->dataPtr->rosRecorder.reset(new rosbag::Recorder(recorderOptions)); | ||
this->dataPtr->bagThread.reset(new std::thread( | ||
&GameLogicPluginPrivate::RosBag, this->dataPtr.get())); |
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->dataPtr->rosRecorder.reset(new rosbag::Recorder(recorderOptions)); | |
this->dataPtr->bagThread.reset(new std::thread( | |
&GameLogicPluginPrivate::RosBag, this->dataPtr.get())); | |
this->dataPtr->rosRecorder = std::make_unique<rosbag::Recorder>(recorderOptions); | |
this->dataPtr->bagThread = std::make_unique<std::thread>([&](){ | |
this->dataPtr->rosRecorder->run(); | |
}); |
I think you can save yourself a member function here.
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 also did this in bc03832 since I was in there.
Signed-off-by: Michael Carroll <michael@openrobotics.org>
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 you are good with my changes then LGTM. I tested via both /subt/finish
service as well as just terminating the launch process and both ended with valid rosbags.
I think just the branch needs to be updated in the Dockerfile before merging.
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 PR resolves an important issue and, in consideration of the code freeze, has been thoroughly tested to make sure it does not cause any new issues. This is approved to merge.
This PR attempts to resolve #631 and fixes ROS bag recording on the simulation server. A docker image based on this is available using osrf/subt-virtual-testbed:cloudsim_sim_issue_631.