Standardize vesc speed to m/s conversion.

[ValerioMagnago]

Before this commit the vesc_to_odom node and the ackermann_to_vesc_node were using a different formula for converting m/s to erpm.

The conversion from m/s to erpm is done here with the following formula:

servo_msg.data = steering_to_servo_gain_ * cmd->drive.steering_angle + steering_to_servo_offset_;

Inverting this formula leads to:

(servo_msg.data - steering_to_servo_offset_)/steering_to_servo_gain_  = cmd->drive.steering_angle ;

so for consistency, it would be better to remove the minus sign from the conversion in the vesc to odom node and use the vesc tool to configure the vesc to provide the information with the correct direction.

[ValerioMagnago]

To add a bit more in the ROS2 branch, the sign inversion from the rpm ( as reported in #9 ) is removed see (

vesc/vesc_driver/src/vesc_packet.cpp

Line 250 in 153998d

return static_cast<double>(v) / 1.0;

) .... while on the main is still there ( https://github.com/f1tenth/vesc/blob/main/vesc_driver/src/vesc_packet.cpp#L178 ) .
In this cleaning attempt also removing this other inversion makes sense.

As a remark, anyway, the user can change the sign of the gain in the ackerman package (we have 4 2 for the transform from ackermann to vesc and 2 for transforming back) or (maybe cleaner) modify the motor direction from the vesc tool.

[ValerioMagnago]

@JWhitleyWork any feedback here?

[JWhitleyWork]

I'm OK with this in-theory. However, this is a breaking change because it could damage existing vehicles if they unkowingly upgrade, which I think is concern enough to leave it out. This would bug me too but I think the safety/property damge concern is enough to leave it as-is. If others disagree, I'm cool with changing it but we need to make notices in a lot of places.

[ValerioMagnago]

Sorry I have just realized that in my first comment, I was wrongly referring to steering gain when the correct word is speed gain.

The ros2 version already differs from the ros1 version in the sign of the rpm (https://github.com/f1tenth/vesc/blob/main/vesc_driver/src/vesc_packet.cpp#L172) so in the past, you already somehow decided in favour of having a cleaner conversion of the data from the vesc. For this reason, upgrading from ros1 to ros2 will already face the user a problem in porting the configurations.
The PR here tries to finalize this attempt and makes the data more intuitive: with a correctly configured vesc (i.e. forward control return a forward spinning engine) this PR allows to share of the configuration between the Ackermann to vesc and vesc to odom. In the current situation inverting the sign of the speed gain is required.

Anyway, I understand your view. Since the ROS2 support was recently added maybe not many people are using it but I cannot quantify the number and I cannot quantify the impact of this change.

If you think that the risk is too big feel free to close this PR.

[ValerioMagnago]

@JWhitleyWork should we then close this?

[JWhitleyWork]

Yes, closing. Thank you for the work and feedback.