ROS Resources: Documentation | Support | Discussion Forum | Index | Service Status | ros @ Robotics Stack Exchange
Ask Your Question
0

Subscriber to array type overwriting data from different publishers!

asked 2019-01-12 19:44:35 -0500

Hypomania gravatar image

updated 2019-01-12 19:48:16 -0500

Hi,

I have been witnessing some bizarre behaviour with my very simple node.

I have a custom message that contains uint8[] type (it's 8 elements long). I have a subscriber that subscribes to a topic of the custom message and stores it in a global variable:

uint16* leddar_distance_cm_1 = NULL;

void leddarCallback(const perception_msgs::Leddar8::ConstPtr& msg) {
  if (msg->leddar_id == 1) {
      leddar_distance_cm_1 = const_cast<uint16_t*>(&(msg->distance[0]));
  }

  for (uint8_t i = 0; i < 8; i++) {
      ROS_INFO("VALUE: [%d]", leddar_distance_cm_1[i]);
  }
}

int main(int argc, char** argv) {
  ros::init(argc, argv, "collision_avoidance_node");
  ros::NodeHandle n;

  ros::Subscriber leddar_sub = n.subscribe("perception/leddars", 1000, leddarCallback);

  ros::spin();

  return 0;
}

I then have 2 publishers publishing onto that topic using rostopic pub. When I try to print the stored array from the callback I get this (bottom right window):

image description

As you can see the two publishers seem to be interfering with each other and somehow overwritting leddar_distance_cm_1 pointer repeatedly. This doesn't happen with any non-pointer types from the message such as char or int.

I even tried creating 2 different subscribers for each publisher, same issue! As if both publishers are overwriting the same memory address (which they in fact are). Does this have anything to do with queues? I have a feeling that I am not reading the array correctly. I have spent over 8 hours looking at this and I can't figure out what the problem is, if there is an obvious mistake could someone please tell me what it is?

edit retag flag offensive close merge delete

1 Answer

Sort by ยป oldest newest most voted
3

answered 2019-01-12 23:53:20 -0500

ahendrix gravatar image

tl;dr: Your leddar_distance_cm_1 should be std::vector<uint8_t> leddar_distance_cm_1;

Your current code is taking the address of a temporary object (msg->distance[0]) and storing it in a pointer ( leddar_distance_cm_1) after that object goes out of scope and is destroyed at the end of the callback.

After the callback is done, leddar_distance_cm_1 is now pointing to unused memory, and the operating system is free to re-use that memory for whatever it wants. You are getting VERY LUCKY that it chooses to re-use that memory for the next message.

Instead of taking the address of the lidar data, your callback needs to copy it out of the message before that message is destructed. You could do this by hand with a for loop, but since the data is already stored in a std::vector<uint8_t>, just copying the data into another std::vector<uint8_t> will do all the hard work for you.

edit flag offensive delete link more

Comments

Really appreciate your answer!

I copied this approach from a different package, does that mean it's not safe to use C arrays in messages? What if I use memcpy? I read that fixed size arrays in a msg are boost::array type, how would one utilize these?

Hypomania gravatar image Hypomania  ( 2019-01-13 07:06:21 -0500 )edit
1

Either the package that you copied this from is also broken, or they've done something slightly different that doesn't use the pointer after the message has been destructed.

ahendrix gravatar image ahendrix  ( 2019-01-13 12:19:33 -0500 )edit
1

You could copy the message data using memcpy, but you'd also need to make sure that the destination array was allocated and was the right size, and you'd need to copy both the data array and its size. Using std::vector or boost::array does this for you, so you're less likely to write a bug.

ahendrix gravatar image ahendrix  ( 2019-01-13 12:21:49 -0500 )edit
1

The msg wiki page describes the C++ equivalent types used to represent arrays in messages. If you message was fixed-length, your array type would be boost::array<uint8_t, 8>

ahendrix gravatar image ahendrix  ( 2019-01-13 12:23:09 -0500 )edit

@ahendrix, gotcha, the package I copied it from was pointing to an argument (of a pointer type) inside an object's function: https://github.com/astuff/kvaser_inte... , line 216. What happens after that I am not sure.

Hypomania gravatar image Hypomania  ( 2019-01-13 12:50:28 -0500 )edit

@ahendrix, the call is here: https://github.com/astuff/kvaser_inte... , line 107. It's a low-level CAN driver wrapper that sends CAN messages upon successful callback. Not exactly sure how it works as it's still pointer to pointer assignment.

Hypomania gravatar image Hypomania  ( 2019-01-13 12:53:07 -0500 )edit

What I don't understand is: I mentioned earlier that "I even tried creating 2 different subscribers for each publisher, same issue! As if both publishers are overwriting the same memory address (which they in fact are)." and I still had the same issue, even though they had different mem. addresses..

Hypomania gravatar image Hypomania  ( 2019-01-13 12:54:47 -0500 )edit

@ahendrix, P.S.: 2 different subscribers, 2 separate topics, 2 separate publishers, still overwriting each other. Would the same issue happen with this package? I am going to be using 2 publishers for that subscriber so now I am worried :(

Hypomania gravatar image Hypomania  ( 2019-01-13 12:59:14 -0500 )edit

Question Tools

1 follower

Stats

Asked: 2019-01-12 19:44:35 -0500

Seen: 432 times

Last updated: Jan 12 '19