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

RosSubChannelElement should be created as a shared_ptr #75

Closed

Conversation

guihomework
Copy link
Contributor

completely fixes #61

@meyerj
Copy link
Member

meyerj commented Aug 18, 2016

The member variable tmp is already a shared pointer (actually a boost::intrusive_ptr) in rtt_rostopic_ros_msg_transporter.hpp:257, so the change in this PR should actually not make a difference.

Only intrusive_ptrs allow assignment from raw pointers like in this case. shared_ptrs with an external reference counter indeed would have to be constructed explicitly before the assignment to tmp (or using make_shared).

Did you observe a difference after having applied this patch? Actually the test added in #66 (only in branch toolchain-2.9) should verify that the subscriber was actually shutdown after the port has been disconnected. The toolchain-2.9 branch is fully backwards-compatible and you can also use it for toolchain 2.8 in ROS indigo.

@guihomework
Copy link
Contributor Author

Dear @meyerj I will re do my own tests on 2.8.3 first and then on 2.9 if you say it should not change anything, because without my patch on 2.8.3 I could still see the rtt node being displayed as subscriber with rostopic info.
In order to test the code change I know I have to clean completely my build space since the typekits must be rebuilt to integrate the changes in this header file.
stay tuned

@guihomework
Copy link
Contributor Author

@meyerj
After cleaning my build space AND devel space (using catkin tools) it this time worked.
I had cleaned only my build space only , applying or removing my patch and I could definitely see changes, now with the devel space cleaned as well your patch is sufficient. I suppose the fact that code is in headers affects building with catkin somehow.

sorry for the noise.

@guihomework guihomework deleted the B#61_unregister_subscribers branch August 18, 2016 13:17
@meyerj
Copy link
Member

meyerj commented Aug 18, 2016

I suppose the fact that code is in headers affects building with catkin somehow.

Yeah, that's kind of tricky and the main reason why orocos-toolchain/rtt#85 has been introduced in toolchain 2.9. Actually you can keep your build-space to save some time, but you should always clear the install-space (and devel-space in case you are using catkin) before rebuilding without this include directories patch because otherwise your compiler will prefer the installed headers from previous builds.

@guihomework
Copy link
Contributor Author

Okay, it makes sense now.
I am saving so much time building my workspace without rtt-ros-integration (since the released version works for me).
thanks for the nice support.

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

2 participants