Code does not follow ROS design conventions

[rokusottervanger]

This node does not follow the conventions and rules about:

• Package naming: https://www.ros.org/reps/rep-0144.html
• Naming conventions for drivers: https://ros.org/reps/rep-0135.html
• Parameter namespacing: http://wiki.ros.org/Parameter%20Server
• IMU topics and using the sensor_msgs/IMU message: https://www.ros.org/reps/rep-0145.html

Also, 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.

[horverno]

Here are my remarks:

Package naming: https://www.ros.org/reps/rep-0144.html

ROS Enhancement Proposals suggests lowercase alphanumerics and _ separators, so instead of duro-ros i think eg duro_ros would be ok

Naming conventions for drivers: https://ros.org/reps/rep-0135.html

Are you suggesting to add namespaces into the launch file?

Parameter namespacing: http://wiki.ros.org/Parameter%20Server

There are two params duro_address and duro_port, it seems ok, but clearly I have missed something, can you explain please?

IMU topics and using the sensor_msgs/IMU message: https://www.ros.org/reps/rep-0145.html

That makes sense to me.

[rokusottervanger]

Here are my remarks:

Package naming: https://www.ros.org/reps/rep-0144.html

ROS Enhancement Proposals suggests lowercase alphanumerics and _ separators, so instead of duro-ros i think eg duro_ros would be ok

Correct!

Naming conventions for drivers: https://ros.org/reps/rep-0135.html

Are you suggesting to add namespaces into the launch file?

Yes. So you remove the hard coded namespaces from the driver (/gps/duro), and just publish topics in the global namespace:

/fix
/imu
/mag
/status_flag
/status_string

(of course, without forcing them in the global namespace by hardcoding the initial /). Then push the node to a group with a ns in the launch file (which is up to the user, in the end).

Parameter namespacing: http://wiki.ros.org/Parameter%20Server

There are two params duro_address and duro_port, it seems ok, but clearly I have missed something, can you explain please?

You're looking up those parameters in the global namespace by performing the lookup using the global nodehandle here: https://github.com/szenergy/duro_gps_driver/blob/2fee735d8e0f6c04572d81c21fe3c641f0ba40e5/src/duro.cpp#L315 https://github.com/szenergy/duro_gps_driver/blob/2fee735d8e0f6c04572d81c21fe3c641f0ba40e5/src/duro.cpp#L328-L329

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 duro_ anymore either, so you can just call them ip_address and port or something like that.

By the way, there is a built in way to use defaults too: http://wiki.ros.org/roscpp/Overview/Parameter%20Server#Getting_Parameters

IMU topics and using the sensor_msgs/IMU message: https://www.ros.org/reps/rep-0145.html

That makes sense to me.

Good :) Also take note of the units specified in the sensor_msgs/MagneticField message. Those should be in Tesla, not microTesla.

I can make a PR for you probably over the weekend.

[rokusottervanger]

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.