Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactoring sensor parsing #84

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rhaschke
Copy link
Contributor

@rhaschke rhaschke commented Mar 17, 2016

This PR is an attempt to rework sensor parsing towards more modularity as requested e.g. in #28. My guidelines were the following (according to the previous discussions in #82 and #28):

  • Don't touch the udf::ModelInterface. Particularly, I didn't put the list of parsed sensors into that data structure as suggested in use SensorBase class as base class for sensors #28. The rationale is the following: The ModelInterface defines the kinematic structure of the robot, which is kind of universal and doesn't require specific data types (other than the existing ones) to describe it. On the other hand, there is a whole universe of sensors out there, each requiring its own data type and parser. So sensors are obviously highly domain-specific and we will need a plugin concept to handle them in a generic fashion.
  • Accordingly, I was thinking about a separate sensor parser, that returns a list of successfully parsed <sensor> tags in the URDF. This generic parser handles common fields as defined in the original Sensor class. In order to handle the multitude of potential sensor descriptions, I propose to pass application-specific sensor parsers to that method via a map from sensor type to the associated parser instance.
  • The list of available sensor parsers should be loaded via the pluginlib mechanism of ROS. However, as liburdfdom should be ROS-agnostic, corresponding functions are provided in the urdf package only (Sensor parsing urdf#5).
  • The association of XML sensor elements to appropriate parsers is done via the sensor type. I have chosen to identify the sensor type by the first non-standard tag below <sensor>. This maintains backward compatibility to potentially existing ray and camera sensor descriptions:
    <sensor name="camera1" update_rate="20">
      <parent link="link1"/>
      <origin xyz="0 0 0" rpy="0 0 0"/>
      <camera>
        <image width="640" height="480" hfov="1.5708" format="RGB8" near="0.01" far="50.0"/>
      </camera>
    </sensor>
    Alternatively, the type could be specified via an attribute of the <sensor> tag.
  • As there might evolve different parsers and descriptions with identical sensor type names in the future, we might need a mechanism to explicitly specify a parser from a specific package.

Accordingly, these commits:

  • merge use SensorBase class as base class for sensors #28 - renaming VisualSensor to SensorBase (previously only two sensor types could be handled, camera and ray).
  • refactor the existing parsing functions parseCamera() and parseRay() functions into appropriate parser classes, derived from a new interface class, urdf::SensorParser.
  • defining a utility function parseAttribute() I could simplify the parsing functions for camera and ray dramatically.
  • A usage example can be found in the unittest.

@rhaschke
Copy link
Contributor Author

This PR goes along with ros/urdfdom_headers#22.

@rhaschke
Copy link
Contributor Author

rhaschke commented Feb 8, 2019

@scpeters @clalancette Would be really great to get some feedback on this nearly two-years-old PR - I just rebased nth time.
Sensor (camera or ray) parsing is broken since ages (since parent's link name isn't correctly parsed, see 87c1cb0) and it shouldn't harm to consider this extension 😉

rhaschke and others added 4 commits September 22, 2022 18:01
- According to http://wiki.ros.org/urdf/XML/sensor, the parent's link name isn't an attribute
  'parent_link_name', but an attribute 'link' in the <parent/> element
- Gracefully handle missing <parent> element (instead of segfaulting)
- moved function declarations into new header files (instead of repeating them in .cpp files)
- export symbols of link.h and pose.h
- and install those files as well (to be re-used by other software)
- parse sensor-specific content with a SensorParser (extension point)
- implemented parseCamera() and parseRay() as SensorParsers (in visual_sensor_parsers.*)
- added unittest test_sensor_parsing
@rhaschke
Copy link
Contributor Author

@clalancette, another 2.5 years passed w/o progress on this. Is there any chance this can get merged?

Move basic attribute parsing routine to utils.h
This is required for parsers loaded via pluginlib to avoid premature unloading of the lib.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants