Add velocity control mode #15
Conversation
Thanks for this nice PR..
Is this really necessary? Or you did it for safety? |
|
For the sole sake of safety I added this limitation to user-friendliness. I still wonder if it is a good idea. |
It allows for easier reading and to set a default interface mode.
|
You're right for the fatal that should exit. For the errors related to command interface type (position/velocity), the joints concerned are disabled. Is that not enough ? Do we need to give such a strong signal as to crash the whole program ? |
There was a problem hiding this comment.
I like how this code is becoming. Good job @dogoepp! Some very minor comments...
src/dynamixel_control_hw.cpp
Outdated
| } | ||
| catch (XmlRpc::XmlRpcException& e) { | ||
| ROS_FATAL_STREAM("Exception raised when parsing the configuration: " << e.getMessage()); | ||
| return 1; |
There was a problem hiding this comment.
Most probably naive question, but just making sure. This should be enough to not allow incorrect configurations right? I.e., nothing is needed in the dynamixel_hardware_interface.hpp file. Just making sure..
There was a problem hiding this comment.
What is an incorrect configuration for you ?
The above code should only accept well-structures configuration. However, it does not prevent the addition of unused parameters (which are then ignored).
If a parameter is expected to be an integer but is of an other type, an exception is raised and causes the program to stop. Saddly, the exception message is not informative. I don't know how to improve this, except by splitting this try/catch in more, smaller ones.
There was a problem hiding this comment.
The above code should only accept well-structures configuration.
That's what I correct configuration is for me..
If a parameter is expected to be an integer but is of an other type, an exception is raised and causes the program to stop.
And that's what an incorrect configuration is for me. 😄
However, it does not prevent the addition of unused parameters (which are then ignored).
This is ok.
Saddly, the exception message is not informative. I don't know how to improve this, except by splitting this try/catch in more, smaller ones.
I think we should not bother too much for this.. It is not of that big importance..
Great! This is ok for me then!
|
|
||
| // enable torque output on the servo and set its configuration | ||
| // including max speed | ||
| _enable_and_configure_servo(_servos[i], hardware_mode); |
There was a problem hiding this comment.
If I am not mistaken, when the hardware_mode is OperatingModel::unknown we should not enable the servo but from what I see we still set_torque_enable(1) in this function (_enable_and_configure_servo) no matter the mode. Of course, I might be missing something..
In this case, we are talking about actuators which operating mode (position or velocity) is not declared, improperly declared or does not match the hardware setting.
|
Looks good. Thanks ! Merging |
Here a new control mode is supported : sending commands to servos in velocity command mode (wheel in Robotis' jargon).
The mode is detected in the servos but we require that the user declares the desired control mode with a ros parameter, to be sure to detect wrong assumption on the current control mode of a servo.