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

Fix race condition #22

Open
wants to merge 2 commits into
base: kinetic-devel
Choose a base branch
from
Open

Fix race condition #22

wants to merge 2 commits into from

Conversation

marcoesposito1988
Copy link

As soon as a subscriber is started, it is able to receive messages and call the registered callback. However unlikely, this could cause the callback to access parameters and publishers which have not been inited yet. This race condition can cause obscure segmentation faults.

@bmagyar
Copy link
Collaborator

bmagyar commented Sep 12, 2016

Hi there,

Please remove the commit "Ignore KDevelop files" from this PR.

The dynamic reconfigure removal is a good catch!

Has this race condition you mention ever appeared for you? Nothing should happen with callbacks until ros::spinOnce() is called later in the code, hence I find it unlikely to get into a race condition.

@marcoesposito1988
Copy link
Author

Hi @bmagyar,

ok, I will remove the commit.

Not yet on this version of the code, but I am also trying to update the ArUco package and I got a segmentation fault. So I started poking around in the code and this jumped to my eyes. Even if this might not cause errors in this case, it is less error-prone and more future-proof to perform the initialization in this order (param reading -> publisher creation -> subscriber registration). Later modification to the code might introduce dependencies of the publishers from the params for example, and that would induce very subtle errors. So I thought of opening this PR.

As soon as a subscriber is started, it is able to receive messages and call the registered callback. However unlikely, this could cause the callback to access parameters and publishers which have not been inited yet. This race condition can cause obscure segmentation faults.
@bmagyar
Copy link
Collaborator

bmagyar commented Sep 12, 2016

Why do you need to update the aruco library? I thought of it before too but frankly it just didn't seem worth to go into that.

ROS Kinetic has OpenCV3 that comes with Aruco inside, the new version of this package will be pretty minimal. I intend to simplify the nodes too, probably only keeping one that's configurable.

@marcoesposito1988
Copy link
Author

Oh I didn't know about the OpenCV integration! Yes, that is a good point.

I wanted to give it a shot because I also incur in issue #20. Markers are not detected as soon as they are further away than a certain distance from the camera. This even if the image is very good.

@marcoesposito1988
Copy link
Author

marcoesposito1988 commented Sep 12, 2016

BTW: I got the new version working! And it indeed works for longer distances, it appears.

Interested in a new PR?

P.S.: works, but not yet correctly.

@Flacedoo
Copy link

Flacedoo commented Feb 2, 2017

Is there a solution yet for the problem that markers are not recognized from a further than a specific distance? I ran a simulation, and when the camera is further away than 1m from a 6.5cm tag, it hasn't got any chance to detect it. Other libraries like apriltag has no problem at all with more than double the distance!

@bmagyar
Copy link
Collaborator

bmagyar commented Feb 2, 2017 via email

@bmagyar bmagyar changed the base branch from indigo-devel to kinetic-devel April 20, 2018 09:00
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.

3 participants