search

ros-drivers / rosserial

A ROS client library for small, embedded devices, such as Arduino. See: http://wiki.ros.org/rosserial
519 stars 524 forks source link

Eliminate non-stdint.h types from Time, Duration classes #169

Open mikepurvis opened 9 years ago

mikepurvis commented 9 years ago

The time.h and duration.h headers should specify their member variables using the types defined in stdint.h.

@tonybaltovski mentioned to me today that publishing a Duration from a 32-bit MCU back to the PC was causing a synchronization issue in his project.

tonybaltovski commented 9 years ago

Thanks @mikepurvis. In addition, is there a reason why a signed long is being using for in ros/duration.h rather than unsigned? EDIT: I just checked the ros duration message which is the same.

PaulBouchier commented 9 years ago

ros time at http://wiki.ros.org/roscpp/Overview/Time defines that duration can be negative. It notes that ros::Time and ros::Duration have identical representations (signed), though obviously time can't be negative.

PaulBouchier commented 9 years ago

As an interesting side note on the signed vs. unsigned issue: at work the coding standard calls for most variables like ints to be signed, even if the valid value range for the variable can never be negative (e.g time). The thinking is that if you use unsigned you're operating close to the boundary (0), where a simple mistake could cause roll-under, and you can't detect it with a <0 test. Conversely, if you use int for variables that should never be negative, it is an easily detectable error if they do become negative. FYI FWIW

tonybaltovski commented 9 years ago

Thanks @PaulBouchier, there seems to be case where this can be an issue with time + duration = time if the duration is negative but normalizeSecNSecUnsigned() will throw an error. Good note nonetheless.

PaulBouchier commented 9 years ago

Mike - can you check me on this - the logic in rosserial_client/src/ros_lib/time.cpp looks wrong:

Time& Time::operator +=(const Duration &rhs) { sec += rhs.sec; nsec += rhs.nsec; normalizeSecNSec(sec, nsec); return *this; }

If duration is negative nsec+= rhs.nsec is addition of an unsigned and signed int anad the signed int will be promoted to unsigned, making it a very large number. The result may or may not overflow, depending on the value of nsec. (There's a 50% chance it will not overflow since nsec recycles so frequently). If it doesn't overflow (duration nsec is more negative than time nsec is positive), normalizeSecNsec will treat nsec as a positive number which could be up to about 4 billion nsec (4 sec) which will be added to sec, which is wrong. Tony didn't say what error is "thrown" - since it's an arduino I assume he means a wrong answer. I'm hoping this won't happen to sec because he shouldn't be adding a duration that is more negative than Time is positive, or he'd be asking for a negative Time result - suitable only for Dr Who.

mikepurvis commented 9 years ago

Hmm. Maybe the Time and Duration classes could get some tests, similar to the float64 stuff:

https://github.com/ros-drivers/rosserial/tree/indigo-devel/rosserial_client/test

Would happily merge a PR for this, and obviously to fix any deficiencies uncovered by said test(s).

chuck-h commented 8 years ago

This time-adding logic has worked for me:

https://gist.github.com/chuck-h/e265b65a525f4f69de58