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

Updated Driver #1

Merged
merged 4 commits into from
Apr 17, 2015
Merged

Updated Driver #1

merged 4 commits into from
Apr 17, 2015

Conversation

decabyte
Copy link
Contributor

Hello there, we are using the ntpd_driver in one of our AUVs here in Heriot-Watt University and we did some changes in the driver that we would like to share back with the community.

The main changes are:

  • removal of the transport hints to allow this driver to work with the nmea_serial_driver, this is because the driver uses python and the UDP compatibility among roscpp and rospy is not guaranteed
  • fixed indentation according to the ROS guidelines
  • added the time_ref node parameter to avoid remapping the input topic (this has been done without changing the original behaviour of the node)

The code has been tested with Ubuntu 14.04.02 LTS (both x86 and x86-64). Hope this can help others to adopt the use of this nice driver to keep the bot's properly in sync.

@vooon
Copy link
Owner

vooon commented Apr 17, 2015

I don't like to change indentation, and please give me a note about rospy-roscpp UDP incompatibility.
I used UDP in hope to reduce latency and avoid resends of old timestamps.

But parameter for topic name is a good idea, but better name it time_ref_topic.

@vooon
Copy link
Owner

vooon commented Apr 17, 2015

Hmm, also can you give me some notes why remap is bad?

@decabyte
Copy link
Contributor Author

@vooon the UDP implementation in rospy is not finished yet. Several ROS Answers questions pointed that. Speaking about the latency I'm confident that the limiting factor is the rate of the GPS driver. In this case nmea_serial_driver is handling GPS units with 10Hz or 1Hz rates and according to their implementation the standard transport is working fine. We tested this solution in field by synchronizing multiple hosts using a network of ntp servers inside the same vehicle. A good trick to avoid having old timestamps moving around is to limit the queue size of the subscriber.

Here are some links about the UDP:

I agree with you about the name of the topic. We use this convention for our software too. I didn't changed it to avoid breaking the compatibility.

About the remap, I don't think is bad per se, I've seen a lot of packages implementing the change of a topic name with a parameter. So you can always dump your configuration from the rosparam server as save it for debugging purposes. The remap instead is done when launching the node.

The indentation style is taken from http://wiki.ros.org/CppStyleGuide. These of course are just a recommendation and they can be optionally dropped.

vooon added a commit that referenced this pull request Apr 17, 2015
@vooon vooon merged commit 327fd0a into vooon:master Apr 17, 2015
@vooon
Copy link
Owner

vooon commented Apr 17, 2015

I don't like this 2-space indent, but it's hard to split your changes in style and adding param (please avoid that mixing in future, that common practice in many projects, not just mine).
Decided to merge as-is.

@vooon
Copy link
Owner

vooon commented Apr 17, 2015

I agree with you about the name of the topic. We use this convention for our software too. I didn't changed it to avoid breaking the compatibility.

Compatibility with what? There no param before... I will change that.

@decabyte
Copy link
Contributor Author

@vooon yes I agree. Sorry for the indentation change. 👍

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.

None yet

2 participants