Closed jpardeiro closed 4 years ago
Overall this looks reasonable. I'd ask that you remove the whitespace only changes so we don't diverge as much for future merges. It's definitely cleaner and for new code we ask that all whitespace be stripped.
At a slightly higher conceptual level I'd ask the question of whether we should have the enable flag vs make a 2nd action for backwards compatibility. In general the enable flag on the node for the velocity control doesn't make a lot of sense. If anything it should be in the message payload to be self contained. However I think that it might be simply easier to suggest that we simply choose to add this field as a fork for the upcoming Kinetic release instead of trying to add backwards compatibility layers for the already released systems. It will keep the code simple but give people known checkpoint for the change.
Since we already did a kinetic release, what is the plan to add in these features?
We didn't fork turtlebot_apps for kinetic, so we could fork and merge in this PR, breaking users of the action on Kinetic.
I like the idea of making a new Action message, due to it preserving backwards compatibility, but it doesn't seam ideal.
Could just have it be robust to a default constructed velocity field, i.e. still use the old parameter as the default velocity, and if 0.0
is detected as incoming, then use the default parameter.
I know this is not the theoretically correct solution - it is changing the message api in a release and also relying on a default constructed value of 0.0. However, this solution is much less complicated and will help it move a non-critical update forwards (kinetic only perhaps?)
@tfoote It's a shame ROS messages don't do optional fields. That would make backwards compatibility so much easier and also allow compatibility to various other messaging systems.
If the user want to specify the velocity the navigation stack for the turtlebot allows it. New field added in the action message.
@tfoote I addressed most of your comments. I agree in all of them, I just tried to keep compatibility with the current developments. The smoother is disabled by default in the current stable Indigo branch, if you want I can enable it again and update my PR.
If these changes are going to be included in the new Kinetic release, should I modify my pull request to point to a different branch?