-
Notifications
You must be signed in to change notification settings - Fork 14
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
Code does not follow ROS design conventions #3
Comments
Here are my remarks:
ROS Enhancement Proposals suggests lowercase alphanumerics and _ separators, so instead of
Are you suggesting to add namespaces into the launch file?
There are two params
That makes sense to me. |
Correct!
Yes. So you remove the hard coded namespaces from the driver (
(of course, without forcing them in the global namespace by hardcoding the initial
You're looking up those parameters in the global namespace by performing the lookup using the global nodehandle here: Line 315 in 2fee735
Lines 328 to 329 in 2fee735
Instead, you should do the lookup of node specific parameters in the node's local namespace with a local nodehandle: ros::NodeHandle n_p("~"); When they are in the node's namespace, there's no need to prefix the parameter names with By the way, there is a built in way to use defaults too: http://wiki.ros.org/roscpp/Overview/Parameter%20Server#Getting_Parameters
Good :) Also take note of the units specified in the I can make a PR for you probably over the weekend. |
I tried to keep it somewhat backward compatible. I think only the change in parameter namespace may break an application that doesn't use the included launch file. |
This node does not follow the conventions and rules about:
sensor_msgs/IMU
message: https://www.ros.org/reps/rep-0145.htmlAlso, it's good practice to separate a library for interfacing with the hardware from the ROS wrapper, implemented as a C++ class.
Would you be interested in a refactor to make your driver follow these conventions? It might break existing setups, but as there is no release yet, that doesn't seem like too much of a problem.
The text was updated successfully, but these errors were encountered: