gazebo_yarp_imu: add multipleanalogsensors interface #443
Conversation
|
cc @nunoguedelha @prashanthr05 can you have a look? Thanks! |
|
Travis is failing due to #442 (TL;DR: we need to use YARP devel in the Travis script). |
Nicogene
force-pushed
the
addMasInterfaces
branch
from
fc9fa40 to
1bee4a9
Compare
|
This path has been successfully tested with these patches: robotology-legacy/icub-gazebo-legacy#71 and https://github.com/robotology/icub-main iCubGui + inertial
iCubGui + WBD
|
|
Thanks a lot @Nicogene ! Do you think it is possible to keep the plugin compatible with the old configuration data? As we have a few models around that assume the old format and we also keep models in a repo that does not have |
|
For using the legacy behavior, we can just check if |
|
I think the documentation could include how to write a sample configuration file accounting for the new features and the usual table for specifying the required parameters must be updated. Otherwise, the code lgtm! |
Nicogene
force-pushed
the
addMasInterfaces
branch
from
1bee4a9 to
5e1ad79
Compare
|
Changes requested addressed, I updated the documentation and I added a fallback open in case you have an old version of the ini file |
| yarp::sig::Vector m_imuData; //buffer for imu data | ||
| yarp::os::Stamp m_lastTimestamp; //buffer for last timestamp data | ||
| yarp::os::Semaphore m_dataMutex; //mutex for accessing the data | ||
| mutable yarp::os::Semaphore m_dataMutex; //mutex for accessing the data | ||
| std::string m_sensorName{"sensor_imu_gazebo"}; |
There was a problem hiding this comment.
Gazebo has a Sensor::Name method, I think we can use that one for both the sensor and the frame, at least for now. We can read it in the Load and then pass it to the device via the yarp parameters.
There was a problem hiding this comment.
Sorry, I think it makes more sense (for eventual consistency with the real robot) to use the ::Name method, not the ScopedName that will contains also the model name and the world name.
There was a problem hiding this comment.
I used the _sensor->Name() check line 145 of IMU.cc, is it the one you are referring to?
There was a problem hiding this comment.
You are right, I was confused by the old code that used ScopedName instead, thanks!
Nicogene
force-pushed
the
addMasInterfaces
branch
from
0c90a82 to
50fdb35
Compare
| if (!m_imuDriver.open(m_parameters)) { | ||
| yError() << "GazeboYarpIMU Plugin Load failed: error in opening yarp driver"; | ||
| } | ||
| return; |
There was a problem hiding this comment.
In this case, I assume we are inducing a wrong behavior with the return, by not attaching itself to the analog server?
Could you also please add tests for the addition of back-compatibility?
There was a problem hiding this comment.
Actually inside this if statement there is the code as it was before, then if you have the old ini file it will open as device the ServerInertial and as subdevice the GazeboYarpIMU. No attach is required in this case as before.
There was a problem hiding this comment.
Ah okay..! Understood, what I mentioned is not necessary since we are dealing with ServerInertial. Thanks @Nicogene !
|
A few Travis build fails due to robotology/yarp#2086 , but the rest is green. @prashanthr05 if you can approve, then we can merge. |
This PR add the multiple analog interface to the
gazebo_yarp_imuplugin.Please review code.
(TESTED)