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

image_transport: 'Republish' as a nodelet #73

Closed

Conversation

furushchev
Copy link

Ported republish node as a nodelet, and changed to call the nodelet from republish executable.

This PR requires and is an application of #72

@skohlbr
Copy link

skohlbr commented Jun 13, 2018

+1, Just searched if this is available somewhere and was close to implementing myself.

// Use Publisher::publish as the subscriber callback
typedef void (Publisher::*PublishMemFn)(const sensor_msgs::ImageConstPtr&) const;
PublishMemFn pub_mem_fn = &Publisher::publish;
sub_ = it_->subscribe(
Copy link

Choose a reason for hiding this comment

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

This might yield a warning from the ReconfigureServer of the CompressedSubscriber. Since a new subscriber is created before the old one is destroyed, the new Server wants to advertise an existing service. My solution would be, to manually destroy the sub_ object in unsubscribe().

Copy link
Author

Choose a reason for hiding this comment

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

@Sefrin Thanks for the review! I could reproduced the bug,

# Terminal 1
roscore &
rosrun uvc_camera uvc_camera_node &
rosrun image_transport republish in:=image_raw compressed out:=foo
# Terminal 2
rostopic hz /foo/compressed
^C
rostopic hz /foo/compressed

And I could see the error message on the terminal 1:

furushchev@dango:~/ros/kinetic_test/src/image_common/image_transport/src [republish-nodelet]$ rosrun image_transport republish in:=image_raw compressed out:=foo
[ INFO] [1533828252.220730375] [/image_republisher_1533828252119766128:ros.nodelet]: Initializing nodelet with 8 worker threads.
[ WARN] [1533828257.226403951] [/image_republisher_1533828252119766128:ros.image_transport./image_republisher_1533828252119766128]: This node/nodelet subscribes topics only when subscribed.
[ERROR] [1533828263.122396843] [/image_republisher_1533828252119766128:ros.roscpp]: Tried to advertise a service that is already advertised in this node [/image_republisher_1533828252119766128/compressed/set_parameters]

Copy link

@Sefrin Sefrin left a comment

Choose a reason for hiding this comment

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

There's a small bug that might produce warnings from the dynamic reconfigure server. I did not test this solution but I encountered this bug, when I rewrote the republish node for myself so that it stops subscribing when no one is listening.

@furushchev
Copy link
Author

furushchev commented Aug 9, 2018

@skohlbr I updated this PR to ensure the shutdown method destructs the subscriber impl object which holds dynamic_reconfigure server object.
I tested on the same commands as the reproduced and confirmed that the error message now disappears.
Could you double check if you don't mind?

@Sefrin
Copy link

Sefrin commented Aug 9, 2018

Thanks for the fast reply! This change might yield errors if we call methods (getTopic etc) on the subscriber after calling shutdown. I'm not sure if that causes trouble somewhere. I think it might be an option to just create a boost::shared_ptr<Subscriber> sub_; for the subscriber object in the RepublishNodelet and call sub_->shutdown() and sub_.reset() in unsubscribe.. But maybe there is a better solution..

If we want to fix this completely, we might need to change the CompressedSubscriber plugin (http://docs.ros.org/lunar/api/compressed_image_transport/html/compressed__subscriber_8cpp_source.html). Maybe we can override shutdown there and let it destroy the reconfigure_server_ object...

@Sefrin
Copy link

Sefrin commented Aug 9, 2018

Confirmed! Fixed in this PR: ros-perception/image_transport_plugins#25
With that, you can reverse the changes made to this PR..

@furushchev
Copy link
Author

@Sefrin Thanks too! That is indeed a far better solution. I reverted the change now.

@knorth55
Copy link

What is the status of this PR?

@ahcorde
Copy link
Collaborator

ahcorde commented Jan 18, 2024

This PR is targeting hydro which is EOL Feel free to reopen this PR if the problem persists

@ahcorde ahcorde closed this Jan 18, 2024
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.

5 participants