Conversation
destogl
left a comment
destogl
left a comment
There was a problem hiding this comment.
I looks good. I think this go into very nice direction. I also found another error I did in initial description.
| @@ -119,27 +120,31 @@ Note: | |||
| </hardware> | |||
| <joint name="joint1"> | |||
| <classType>ros2_control_components/MultiInterfaceJoint_MultiWrite</classType> | |||
| <stateInterfaceType>position</stateInterfaceType> | ||
| <param name="min_position_value">${-PI}</param> | ||
| <param name="max_position_value">${PI}</param> | ||
| <stateInterfaceType name="position"> |
There was a problem hiding this comment.
This would be this optional limits for the stateInterfaceType.
Nevertheless, I did error here, I think here we should have sensor and not joint component (tag in L306))
| <param name="max_value">1</param> | ||
| </joint> | ||
| </ros2_control> | ||
| <ros2_control name="2DOF_System_Robot_ForceTorqueSensor" type="sensor"> |
| @@ -367,16 +377,10 @@ Note: | |||
| <classType>ros2_control_components/IMUSensor</classType> | |||
| <stateInterfaceType>velocity</stateInterfaceType> | |||
| <stateInterfaceType>acceleration</stateInterfaceType> | |||
There was a problem hiding this comment.
I would keep the values here as an option.
| </sensor> | ||
| <sensor name="sensor2"> | ||
| <classType>ros2_control_components/2DImageSensor</classType> | ||
| <stateInterfaceType>image</stateInterfaceType> |
| @@ -119,27 +120,31 @@ Note: | |||
| </hardware> | |||
| <joint name="joint1"> | |||
| <classType>ros2_control_components/MultiInterfaceJoint_MultiWrite</classType> | |||
There was a problem hiding this comment.
Can you rename this to:
"MultiInterfaceMultiWriteJoint"
destogl
left a comment
destogl
left a comment
There was a problem hiding this comment.
When we are already renaming stuff than we could introduce RRBot immediately :)
| <param name="min_value">-0.5</param> | ||
| <param name="max_value">0.5</param> | ||
| </commandInterfaceType> | ||
| <commandInterfaceType name="position" min="-1" max="1"/> |
There was a problem hiding this comment.
You added here the parameters as attributes and above as child elements. Did you do this on purpose?
Does this mean that MultiInterfaceJoint have min and max values as mandatory attributes?
There was a problem hiding this comment.
I'd always consider min-max to be optional
There was a problem hiding this comment.
I am not sure if this is not confusing for the users. Because in some cases is attribute and in some case is child element. I understand why and when, but a new user could have issues with it.
| <ros2_control name="2DOFSystemRobotPositionOnly" type="system"> | ||
| <hardware> | ||
| <classType>ros2_control_demo_hardware/2DOF_System_Hardware_Position_Only</classType> | ||
| <classType>ros2_control_demo_hardware/2DOFSystemHardwarePositionOnly</classType> |
There was a problem hiding this comment.
Proposal: rename this to RRBotSystemPositionOnlyHardware
This is inline with ros-controls/ros2_control_demos#37 so it could increase understanding of the examples and demos
|
|
||
| ```xml | ||
| <ros2_control name="2DOF_System_Robot_Position_Only" type="system"> | ||
| <ros2_control name="2DOFSystemRobotPositionOnly" type="system"> |
There was a problem hiding this comment.
Proposal: rename this to RRBotSystemPositionOnly
|
|
||
| ```xml | ||
| <ros2_control name="2DOF_System_Robot_MultiInterface" type="system"> | ||
| <ros2_control name="2DOFSystemRobotMultiInterface" type="system"> |
There was a problem hiding this comment.
Proposal: rename this to RRBotSystemMultiInterface
| <ros2_control name="2DOFSystemRobotMultiInterface" type="system"> | ||
| <hardware> | ||
| <classType>ros2_control_demo_hardware/2DOF_System_Hardware_MultiInterface</classType> | ||
| <classType>ros2_control_demo_hardware/2DOFSystemHardwareMultiInterface</classType> |
There was a problem hiding this comment.
Proposal: rename this to RRBotSystemMultiInterfaceHardware
| </transmission> | ||
| </ros2_control> | ||
| <ros2_control name="2DOF_Modular_Robot_joint2" type="actuator"> | ||
| <ros2_control name="2DOFModularRobotJoint2" type="actuator"> |
There was a problem hiding this comment.
Proposal: rename this to RRBotModularJoint2
| </joint> | ||
| </ros2_control> | ||
| <ros2_control name="2DOF_System_Robot_Position_Sensor_joint1" type="sensor"> | ||
| <ros2_control name="2DOFSystemRobotPositionSensorJoint1" type="sensor"> |
There was a problem hiding this comment.
Proposal: rename this to RRBotModularPositionSensorJoint1
| </joint> | ||
| </ros2_control> | ||
| <ros2_control name="2DOF_System_Robot_Position_Sensor_joint2" type="sensor"> | ||
| <ros2_control name="2DOFSystemRobotPositionSensorJoint2" type="sensor"> |
There was a problem hiding this comment.
Proposal: rename this to RRBotModularPositionSensorJoint2
|
|
||
| ```xml | ||
| <ros2_control name="2DOF_Modular_Robot_Wrist" type="system"> | ||
| <ros2_control name="2DOFModularRobotWrist" type="system"> |
There was a problem hiding this comment.
Proposal: rename this to RRBotModularWrist
|
|
||
| ```xml | ||
| <ros2_control name="2DOF_Modular_Robot_joint1" type="actuator"> | ||
| <ros2_control name="2DOFModularRobotJoint1" type="actuator"> |
There was a problem hiding this comment.
Proposal: rename this to ActuatorModularJoint1
| <param name="min_value">-0.5</param> | ||
| <param name="max_value">0.5</param> | ||
| </commandInterfaceType> | ||
| <commandInterfaceType name="position" min="-1" max="1"/> |
There was a problem hiding this comment.
I am not sure if this is not confusing for the users. Because in some cases is attribute and in some case is child element. I understand why and when, but a new user could have issues with it.
bmagyar
force-pushed
the
min_max_to_attribute
branch
from
4e550f4 to
c0f1497
Compare
3 participants
Please note that I'm also proposing to remove limits when only state interfaces are declared. We can extend the design later but I don't think state interfaces need limiting at this stage.
Resolves #23