Conversation
|
Can this PR be targeted against the branch of #121? Then the diff is somewhat easier to read. |
Karsten1987
left a comment
Karsten1987
left a comment
There was a problem hiding this comment.
Can you elaborate on what hardware means to you here? I am missing the definition of system in here as there's a SystemInfo struct.
| <actuator name="motor1"> | ||
| <mechanicalReduction>1</mechanicalReduction> | ||
| </actuator> |
There was a problem hiding this comment.
I though we're treating only joints and no actuators? It's up to the implementation of the joint to deal with actuators.
The mechanicalReducation could then be placed within a transmission tag inside the joint.
There was a problem hiding this comment.
It's up to the implementation of the joint to deal with actuators.
No.
As we discussed multiple times now, the joint to actuator relationship is not one to one. Transmissions cannot be completely handled by joints, they are a separate entity, not handled by either actuators or joints but some Hardware Abstraction Layer (whatever we call that now).
That being said, I think that these examples can probably omit transmissions and only define what the components require.
|
|
||
| urdf_xml_tail_ = | ||
| R"( | ||
| <transmission name="tran1"> |
There was a problem hiding this comment.
why is the transmission the top tag here?
| <hardware> | ||
| <classType>ros2_control_demo_hardware/DemoRobotHardwareMinimal</classType> | ||
| <param name="write_for_sec">2</param> | ||
| <param name="read_for_sec">2</param> | ||
| </hardware> |
There was a problem hiding this comment.
what is hardware here? And what is write_for_sec and read_for_sec? Where is this being used?
|
|
||
| valid_urdf_ros2_control_joints_only_ = | ||
| R"( | ||
| <ros2_control name="MinimalRobot" type="robot"> |
There was a problem hiding this comment.
what other types than robot can be there?
hardware_interface/include/hardware_interface/utils/component_parser.hpp
Outdated
Show resolved
Hide resolved
| if (!ros2_control_it) { | ||
| throw std::runtime_error("no " + std::string(kROS2ControlTag) + " tag"); | ||
| } | ||
| attr = ros2_control_it->FindAttribute("name"); |
There was a problem hiding this comment.
I think this section could be made more readable if you had a helper function that's something along these lines:
string get_attr_value(const tinyxml2::XMLElement * some_it, string attribute_name)
{
const tinyxml2::XMLAttribute * attr;
attr = some_it->FindAttribute(attribute_name);
if (!attr) {
throw std::runtime_error("no attribute name in " + attribute_name + " tag");
}
return some_it->Attribute(attribute_name);
}
then you can simplify this parsing bit to:
// shorter version of getting the attribute value
system.type = get_attr_value(ros2_control_it, "type");
I'm not sure this applies to all cases but it seems like a fair bit could use it
| <actuator name="motor1"> | ||
| <mechanicalReduction>1</mechanicalReduction> | ||
| </actuator> |
There was a problem hiding this comment.
It's up to the implementation of the joint to deal with actuators.
No.
As we discussed multiple times now, the joint to actuator relationship is not one to one. Transmissions cannot be completely handled by joints, they are a separate entity, not handled by either actuators or joints but some Hardware Abstraction Layer (whatever we call that now).
That being said, I think that these examples can probably omit transmissions and only define what the components require.
| <joint name="joint1"> | ||
| <interfaceName>position</interfaceName> | ||
| </joint> | ||
| <param name="can_read">True</param> |
There was a problem hiding this comment.
what is can_read in this context?
| HARDWARE_INTERFACE_PUBLIC | ||
| virtual | ||
| return_type read(std::vector<double> & data) = 0; |
There was a problem hiding this comment.
as mentioned yesterday, it is not really clear to me what read does here. Imagine I have a robot with an integrated sensor. First off, how do I know which sensor to address here and their size of data? You can imagine that different sensors have different types of data to return.
|
|
||
| return_type | ||
| virtual | ||
| write(const std::vector<double> & data) = 0; |
There was a problem hiding this comment.
this vector data comes from the resource manager? Or which entity is allocating the memory for the data?
|
I hope we clarified all the comments here in the discussion on ros-controls/roadmap#22 I am closing this in favor of #127 so that we do not have a dependency on #121. |
* Added new version of parser. Files checkout from #122 * SensorHardware can handle multiple sensor types
3 participants
the second part of #101
Depends on #121