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

YarpSensorBridge Implementation #106

Merged
merged 6 commits into from
Sep 18, 2020

Conversation

prashanthr05
Copy link
Collaborator

@prashanthr05 prashanthr05 commented Sep 13, 2020

This PR adds the YarpSensorBridge implementation that refactors out our traditional way of getting sensor measurements into a library. The current implementation supports measurements read from

  • a remote control board remapper implementing encoders interface
  • a multiple analog sensor remapper implementing accelerometers, gyroscopes, magnetometers, orientation sensors, and force-torque sensors interface
  • generic IMU sensors through a server inertial
  • analog FT sensors
  • cartesian wrenches opened as generic sensors considered to have dimension size 6

It needs to be configured with a PolyDriverList of the relevant devices that are available in the network, and a configuration setup that informs the SensorBridge about the devices and the underlying sensor interfaces.

Implementation roadmap

  • Configure step

  • Attach drivers step

  • Read sensors

  • Advance and Getters (missing implementation for image getters, Eigen Ref conversions of YARP Image needs to be understood).

  • Integrated testing with a YARP device (reading one IMU, two cartesian wrenches, and a remote control board remapper takes about an average of 50 microseconds). The device closed without any segmentation faults. On comparing IMU measurements with ports and those given by the bridge, they matched (except for those converted to radians). Should I add the test device to BLF?

  • IMPORTANT CHANGE In ISensorBridge Interface class, I made the getter implementations for the sensors as optional by changing them from being pure virtual functions to virtual functions. This allows the developer to decide what they want to implement rather than polluting their API with unnecessary and unused methods.

Points to discuss,

  • P.S. The current monolithic Impl source code design could be further refactored into multiple Impl classes for each SensorTypes. However, for starters, let's begin with this.

  • Caution for a possible design flaw I Inherited the YarpSensorBridge from the Advanceable class as well. However, I have a feeling that to poll the measurements at each advance step and have a huge set of measurements stored internally would be expensive than polling for the specific measurement whenever the user calls the getXXX() method on a very specific sensor of a specific measurement type. Also since within the advance() step all the sensor reads are done serially (no sense of parallelism in internal reads as well), this might cause a huge overhead in the pipeline. I hope this doesn't turn out to be a huge design flaw in the future. If it does, we might have to reuse the read individual sensor methods in either a parallel way or to call the individual sensor reads while the user calls the getters.
    (Imagine a single instance of SensorBridge configured with all sensor interfaces including cameras and multiple interfaces!)

  • Dealing with YARP images, I am using internal storage of yarp::sig::Image and dynamic casting it to the derived classes of yarp::sig::ImageOf<yarp::sig::PixelRgb>, yarp::sig::ImageOf<yarp::sig::PixelFloat> and yarp::sig::FlexImage for reading RGB Images from IFrameGrabber interface, depth images from IRGBDSensor interface and RGB images from IRGBDSensor interface. We need to understand if it's a good way to do so.

@prashanthr05 prashanthr05 force-pushed the feature/YarpSensorBridge branch 2 times, most recently from fef8401 to c8561a2 Compare September 13, 2020 16:47
@prashanthr05 prashanthr05 marked this pull request as ready for review September 15, 2020 20:33
Copy link
Member

@GiulioRomualdi GiulioRomualdi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments except #106 (comment)

/**
* Attach to cartesian wrench interface
*/
bool attachCartesianWrenchInterface(const yarp::dev::PolyDriverList& devList)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

@prashanthr05
Copy link
Collaborator Author

prashanthr05 commented Sep 18, 2020

The configure methods must be modified in view with https://github.com/dic-iit/bipedal-locomotion-framework/pull/103/files.

This means no need to specify BipedalLocomotion::GenericContainer::VectorResizeMode while getting parameters from the parameter handler.

This is why the CI is failing.

@prashanthr05
Copy link
Collaborator Author

The configure methods must be modified in view with https://github.com/dic-iit/bipedal-locomotion-framework/pull/103/files.

This means, no need to specify BipedalLocomotion::GenericContainer::VectorResizeMode while getting parameters from the parameter handler.

This is why the CI is failing.

Done in 4f9b30c

@prashanthr05
Copy link
Collaborator Author

prashanthr05 commented Sep 18, 2020

@GiulioRomualdi I've addressed your review comments.

I have also opened relevant issues for addressing important points left behind in this PR.

@GiulioRomualdi
Copy link
Member

Thank you @prashanthr05. If you agree I can merge the PR as it is. Let me know if you want to reorganize the commits

…ensorBridge

Missing implementation for image getters - eigen conversions of yarp images must be implemented
@prashanthr05
Copy link
Collaborator Author

Thank you @prashanthr05. If you agree I can merge the PR as it is. Let me know if you want to reorganize the commits

I've squashed some minor commits and left some other commits which modify changes from other PRs to keep the history of changes informative.

Please feel free to merge the PR once the CI passes.

@GiulioRomualdi
Copy link
Member

Merging!

@GiulioRomualdi GiulioRomualdi merged commit c055c48 into ami-iit:master Sep 18, 2020
@prashanthr05 prashanthr05 deleted the feature/YarpSensorBridge branch September 18, 2020 15:43
@prashanthr05
Copy link
Collaborator Author

prashanthr05 commented Sep 18, 2020

cc @GiulioRomualdi an example usage of this with respect to the YARP device implementation is available in
prashanthr05@f310861

The only difference with respect to the usage in RFModule would be the opening of the relevant devices and populating a PolyDriverList, which with a YARP device is possible to do within the configuration files and use yarprobotinterface attach to get a PolyDriverList.

This will be useful for your controller implementation.

Let me know if I should open a PR for that device.

@GiulioRomualdi
Copy link
Member

cc @GiulioRomualdi an example usage of this with respect to the YARP device implementation is available in
prashanthr05/bipedal-locomotion-framework@f310861

Really thank you!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants