search

pal-robotics / aruco_ros

Software package and ROS wrappers of the Aruco Augmented Reality marker detector library
MIT License
449 stars 307 forks source link

Fix race condition #22

Open marcoesposito1988 opened 8 years ago

marcoesposito1988 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.

bmagyar commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

Hi guys,

Yes, please do send in a new version on a PR against kinetic-devel. I've just created the branch!

On 2 February 2017 at 11:14, Flacedoo notifications@github.com wrote:

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!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pal-robotics/aruco_ros/pull/22#issuecomment-276917990, or mute the thread https://github.com/notifications/unsubscribe-auth/ADXH4X-WoCwrYsIeR-aiDN_5lgDFH4o7ks5rYayBgaJpZM4J6hdH .