diagnostic_updater swallows first character of name parameter

[PaulBouchier]

This bug cost me a few hours some years ago when I first ran into it (and didn't get to the bottom of it). It just cost me a few more, when a colleague reviewing my code that reads DiagnosticStatus messages in my custom DiagnosticAnalyzer asked why the name of the message I was matching to was missing the first character.

The problem is at line 542 of diagnostic_updater.h. It appears there was an original intent to remove the leading "/" from the node name, which provides the default intializer for the first part of the message name. However, if I provide a custom initializer name for the diagnostic_updater, line 542 removes its first character.

One can ask, "why specify the name"? No special reason - I didn't realize the normal procedure is to just instantiate the updater without arguments and it would choose the node name for the first part of diagnostic_updater message names. There's no documentation that says how the message name is generated. Using the default constructor initializers will be my fix.

This is not a major bug, but at a minimum it ought to be documented. I think it's probably too dangerous to change the behavior - it would break the hack I previously put in by adding a space in front of the updater name. A less dangerous fix might be to change the default intializer to ros::this_node.getName().substr(1) and remove the .substr at line 542, but that would still break hacks that pre-padded the name.

It is disappointing that there's no tutorial - only an example.cpp that is quite unprofessional in its variable/method names & data (Ex. stat.add("Stupidicity of this updater", 1000.);). I would be willing to turn example.cpp into a C++ tutorial, but would welcome a reviewer, or maybe a python co-contributor. example.cpp does seem to have most of the words you'd want in a wiki tutorial. This is where we could explain the bug/feature in the constructor. Also, I've seen somewhere where compilable code had tutorial text embedded in it such that you could check that it builds and also have it serve as a tutorial, but I don't remember where.

Thoughts?

[trainman419]

Yeah; the examples written by the original author are not very professional. As it stands now, I'm keeping this package in maintenance mode and I don't have a lot of time to do active development or write new tutorials, but if you have time to write tutorials on the ROS wiki I would be happy to review them.

For this specific bug, I would welcome a patch that conditionally removes the first character of node_name_ if it's a /, and otherwise leaves node_name_ unmodified.