-
Notifications
You must be signed in to change notification settings - Fork 37
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
YarpSensorBridge Implementation #106
Conversation
fef8401
to
c8561a2
Compare
...Interface/YarpImplementation/include/BipedalLocomotion/RobotInterface/YarpSensorBridgeImpl.h
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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)
...Interface/YarpImplementation/include/BipedalLocomotion/RobotInterface/YarpSensorBridgeImpl.h
Outdated
Show resolved
Hide resolved
...Interface/YarpImplementation/include/BipedalLocomotion/RobotInterface/YarpSensorBridgeImpl.h
Outdated
Show resolved
Hide resolved
...Interface/YarpImplementation/include/BipedalLocomotion/RobotInterface/YarpSensorBridgeImpl.h
Outdated
Show resolved
Hide resolved
...Interface/YarpImplementation/include/BipedalLocomotion/RobotInterface/YarpSensorBridgeImpl.h
Outdated
Show resolved
Hide resolved
...Interface/YarpImplementation/include/BipedalLocomotion/RobotInterface/YarpSensorBridgeImpl.h
Show resolved
Hide resolved
/** | ||
* Attach to cartesian wrench interface | ||
*/ | ||
bool attachCartesianWrenchInterface(const yarp::dev::PolyDriverList& devList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
src/RobotInterface/include/BipedalLocomotion/RobotInterface/ISensorBridge.h
Outdated
Show resolved
Hide resolved
This is mainly to avoid allocation errors while passing around EigenTypes using STL containers through functions
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 This is why the CI is failing. |
0593bcf
to
4f9b30c
Compare
Done in 4f9b30c |
@GiulioRomualdi I've addressed your review comments. I have also opened relevant issues for addressing important points left behind in this PR. |
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
4882805
to
29fe5b5
Compare
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. |
Merging! |
cc @GiulioRomualdi an example usage of this with respect to the YARP device implementation is available in 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 This will be useful for your controller implementation. Let me know if I should open a PR for that device. |
Really thank you!!!! |
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
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 theAdvanceable
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 ofyarp::sig::ImageOf<yarp::sig::PixelRgb>
,yarp::sig::ImageOf<yarp::sig::PixelFloat>
andyarp::sig::FlexImage
for reading RGB Images fromIFrameGrabber
interface, depth images fromIRGBDSensor
interface and RGB images fromIRGBDSensor
interface. We need to understand if it's a good way to do so.