Save calibration result

[Tacha-S]

Change Summary

Save the latest position to a file when updating sensor values. Load the previous position from a file when servo/calibration is set to false.

• fix #80

Details

Impacts

References

Additional Information

[jsupratman13]

Why not execute calibration via rosservice?

[Tacha-S]

Why not execute calibration via rosservice?

To keep backward compatibility. And looking ahead to ROS2.

[nyxrobotics]

Looks good to me, would you like to check, @jsupratman13 ?

[jsupratman13]

Looks good to me, would you like to check, @jsupratman13 ?

yes

[Tacha-S]

Second, I might have missed this, but wouldn't it be better to save the calibration result after finishing calibration instead of continuing to save the calibration results?

We already tried, but we can't do it because the hardware interface does not call destructors.

Third, and it might relate to the second statement or I'm testing this incorrectly, but if you calibrate vesc -> command vesc to move somewhere -> stop and restart vesc hw interface then vesc assumes the current position as 0

What are the saved values?

[jsupratman13]

We already tried, but we can't do it because the hardware interface does not call destructors.

Completely forgot about this behavior. How about before this line? https://github.com/sbgisen/vesc/blob/4b29211c1faa04797178e5f4a805ac2b87b8e098/vesc_hw_interface/src/vesc_servo_controller.cpp#L248

[Tacha-S]

please check the comment https://github.com/sbgisen/vesc/pull/81#discussion_r1156848764

[jsupratman13]

What are the saved values?

~~Varies, depending on where the arm is after I stopped the hardware interface. In my case, if I stopped the hardware interface right after calibration I get a value of -0.00016667. But if I stopped the hardware interface 0.1 m away after calibration I get a value of -0.0979~~

Actually, I'm getting an uncalibrated behavior in another situations so I need to check on this a little bit more. FYI, I'm testing this via position_control_sample.launch

[Tacha-S]

Varies, depending on where the arm is after I stopped the hardware interface. In my case, if I stopped the hardware interface right after calibration I get a value of -0.00016667. But if I stopped the hardware interface 0.1 m away after calibration I get a value of -0.0979

It seems to be working correctly.

[jsupratman13]

Alright I'm a little bit lost for a moment and I'm probably testing this incorrectly. Run me through the exact steps to using this feature (Correct CLI, files to edit etc, when to restart hw interface, and the expected behavior)

[Tacha-S]

1. add servo/calibration_result_path to position_control_sample.launch.
2. launch it.
3. move servo to some position after calibration.
4. shutdown the node.
5. add <rosparam command="load" file="calibration_result_path"/> to vesc_hw_interface_node and add servo/calibration: false to position_control_sample.launch.
6. launch again.
7. done.

[jsupratman13]

1. add servo/calibration_result_path to position_control_sample.launch.

   2. launch it.

   3. move servo to some position after calibration.

   4. shutdown the node.

   5. add <rosparam command="load" file="calibration_result_path"/> to vesc_hw_interface_node and add servo/calibration: false to position_control_sample.launch.

   6. launch again.

   7. done.

Tried the above method but didn't seem to show calibrated behavior. Instead, vesc_hw_interface uses its current position as zero position.

[Tacha-S]

Oh..., vesc_hw_interface call write() before calling read(). @nyxrobotics @jsupratman13 Can I swap this? https://github.com/sbgisen/vesc/blob/4b29211c1faa04797178e5f4a805ac2b87b8e098/vesc_hw_interface/src/vesc_hw_interface_node.cpp#L38-L44

[jsupratman13]

Sure, but make sure all the mode works with the changes above

[Tacha-S]

I tested position and velocity_duty mode.

[jsupratman13]

Oh..., vesc_hw_interface call write() before calling read(). @nyxrobotics @jsupratman13 Can I swap this?

https://github.com/sbgisen/vesc/blob/4b29211c1faa04797178e5f4a805ac2b87b8e098/vesc_hw_interface/src/vesc_hw_interface_node.cpp#L38-L44

@Tacha-S Apologies, I didn't consider this before, but you might want to make sure your PR works without switching the read and write. It might work here, but there is no guarantee that any third-party main node (the node with controller_manager with/without combine hardware interface) will follow this particular read -> write order.

[Tacha-S]

http://wiki.ros.org/ros_control/Tutorials/Create%20your%20own%20hardware%20interface

there is no guarantee that any third-party main node

Do you have any reference?

Apologies, I didn't consider this before, but you might want to make sure your PR works without switching the read and write.

If write() is executed before read(), even if a vesc packet has been received, joint state will not be updated and target_position will be overwritten with 0.

[jsupratman13]

http://wiki.ros.org/ros_control/Tutorials/Create%20your%20own%20hardware%20interface Do you have any reference?

The documentation above says nothing about the order, just that the developers are responsible for extracting states and sending commands. While the example given is probably the recommended order, by no means is this enforced, and its entirely up to the developers to decide what order to be. One such example is this repository (though I don't believe there is any restriction in the order)

It's your job to make sure the pos, vel and eff variables always have the latest joint state available, and you also need to make sure that whatever is written into the cmd variable gets executed by the robot. To do this, you could for example add a read() and a write() function to your robot class. In your main(), you'd then do something like this...

[jsupratman13]

If write() is executed before read(), even if a vesc packet has been received, joint state will not be updated and target_position will be overwritten with 0.

I understand, hence the reason why I'm a little bit hesitant to constrain the read/write order. One might argue that read -> write is the recommended / future trend order, and if there is a reference or evidence to support this I might be more willing.

[Tacha-S]

So you are saying that you do not like the order to be fixed.

Is it acceptable to make the any order only for the sake of appearance on the hardware interface, so that hardware interface can't write unless hardware interface can get the state of the vesc first internally?

[jsupratman13]

So you are saying that you do not like the order to be fixed.

Hesitant but not unwilling. Like I mentioned before, for example, if there is a reference that ros (or ros2) hardware interface is leaning toward read->write order, then imposing fixed order is ok.

Is it acceptable to make the any order only for the sake of appearance on the hardware interface, so that hardware interface can't write unless hardware interface can get the state of the vesc first internally?

Before appearances, it's important to note that the optimal order for reading from and writing to hardware will depend on the specific application and the hardware being used. We designed vesc_hw_interface to be usable with the combined_hardware_interface, allowing users/developers to combine vesc_hw_interface with their own system. As of now, vesc_hw_interface can read or write first either way (since the actual order is processed internally), but this luxury may not be applicable to other systems. Thus I wanted to keep this part as flexible as possible.

But as you said, 「any order only for the sake of appearance 」 is not the best practice for designing the hardware interface; instead, and I believe this is your intent, it should be done through careful consideration of application and hardware. But for this case, the PR is trying to impose order because target_position_ will be overwritten with 0. From personal experience, it's not a good idea to directly control the variable (indirectly) registered to joint_OOO_interface. You could have something like a target_position_sub_ to internally command pid control and update this variable freely or if there is a change in command but this is a kind of a different issue.

@nyxrobotics any thoughts on this?

[Tacha-S]

https://github.com/sbgisen/vesc/blob/4b29211c1faa04797178e5f4a805ac2b87b8e098/vesc_hw_interface/src/vesc_servo_controller.cpp#L196-L199

Is the line not controled the sensing position directly?

[jsupratman13]

lgtm. fix the readme and I'll approve it