0
$\begingroup$

Rosanswers logo

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?


Originally posted by Hypomania on ROS Answers with karma: 120 on 2019-01-12

Post score: 0

$\endgroup$

1 Answer 1

0
$\begingroup$

Rosanswers logo

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.


Originally posted by ahendrix with karma: 47576 on 2019-01-13

This answer was ACCEPTED on the original site

Post score: 3


Original comments

Comment by Hypomania on 2019-01-13:
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?

Comment by ahendrix on 2019-01-13:
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.

Comment by ahendrix on 2019-01-13:
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.

Comment by ahendrix on 2019-01-13:
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>

Comment by Hypomania on 2019-01-13:
@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_interface/blob/master/src/linuxcan.cpp, line 216. What happens after that I am not sure.

Comment by Hypomania on 2019-01-13:
@ahendrix, the call is here: https://github.com/astuff/kvaser_interface/blob/master/src/kvaser_can_bridge.cpp, 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.

Comment by Hypomania on 2019-01-13:
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..

Comment by Hypomania on 2019-01-13:
@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 :(

Comment by ahendrix on 2019-01-13:
There are a lot of differences between that code and yours. Instead of trying to explain all of them and possibly giving the wrong impression of the C++ rules, I suggest you go through a few tutorials about object scope and lifetime in C++

Comment by ahendrix on 2019-01-13:
All of these google results look similar; I suggest you go through and find one that makes sense to you: https://www.google.com/search?q=C%2B%2B+object+lifetime+tutorial&oq=C%2B%2B+object+lifetime+tutorial&aqs=chrome..69i57.5719j0j1&sourceid=chrome&ie=UTF-8

Comment by Hypomania on 2019-01-13:
@ahendrix, thank you again, very much appreciated. I am still new to C++ so RAII is pretty much out of my scope but I am getting there!

$\endgroup$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.