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

Update Extern library to the latest XDA (2022.0.0) #38

Merged

Conversation

HosameldinMohamed
Copy link
Contributor

@HosameldinMohamed HosameldinMohamed commented Mar 7, 2023

Refer to #26 for more details.

The code is tested with MTi-680G, and the overall behavior seems to be the same as in v0.2.2.

The PR adds also a copy of the example code provided by Xsens. Renamed to exampleScanDevicesAndReceiveData.cpp. It installs an executable called xsensExample.

# Authors: Silvio Traversaro <silvio.traversaro@iit.it>
# CopyPolicy: Released under the terms of the LGPLv2.1 or later, see LICENSE

# The code contained in this directory is imported directly from the src_cpp example
# in the MT Software Suite, version 4.8.2, available from https://www.xsens.com/mt-software-suite/
# in the MT Software Suite, version 2022.0.0, available from https://www.xsens.com/mt-software-suite/
Copy link
Member

Choose a reason for hiding this comment

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

The old URL is now broken, can you update it with the new URL from which you got the code?

Copy link
Contributor Author

@HosameldinMohamed HosameldinMohamed Mar 7, 2023

Choose a reason for hiding this comment

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

Updated is 0a495b0

@@ -288,7 +339,8 @@ class yarp::dev::XsensMT : public yarp::dev::IGenericSensor,
bool m_isSensorMeasurementAvailable{false};

// Interface exposed by the Xsens MT Software suite
DeviceClass m_xsensDevice;
XsControl* m_xsensControl;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
XsControl* m_xsensControl;
XsControl* m_xsensControl;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0a495b0

@traversaro
Copy link
Member

traversaro commented Mar 7, 2023

It would be great to actually test on an XSens MTi-300 sensor mounted on an iCub 2.7, do you know the state of iCubGenova04 to see if we can try there? Otherwise we can ask to @Nicogene if he has any idea of how to get one of those devices to test.

@HosameldinMohamed
Copy link
Contributor Author

It would be great to actually test on an XSens MTi-300 sensor mounted on an iCub 2.7, do you know the state of iCubGenova04 to see if we can try there? Otherwise we can ask to @Nicogene if he has any idea of how to get one of those devices to test.

Yes, iCubGenova04 is actually available. I'll give it a test!.

@HosameldinMohamed
Copy link
Contributor Author

HosameldinMohamed commented Mar 7, 2023

I wanted to suggest removing the m_sensorThread and the sensorReadLoop, because the reading and buffering (Also the mutual exclusion) are handled by the user class CallbackHandler (inheriting XsCallback).

With this version of XDA, it seems the thread in the YARP device is redundant.

See the relevant information from the SDK documentation:

image

I wanted to have a separate PR for this, but if you think it's ok to propose it in this PR, no probs!

@HosameldinMohamed
Copy link
Contributor Author

HosameldinMohamed commented Mar 7, 2023

Also, how to handle the position data, in case the device has this capability, and if it's available.

The update seems straightforward when using mulitpleAnalogSensor interfaces, but with IGenericSensor, it will introduce a breaking change, because the structure of the YARP port may change.

@traversaro
Copy link
Member

I wanted to have a separate PR for this, but if you think it's ok to propose it in this PR, no probs!

I would go for a separate PR!

@traversaro
Copy link
Member

Also, how to handle the position data, in case the device has this capability, and if it's available.

The update seems straightforward when using mulitpleAnalogSensor interfaces, but with IGenericSensor, it will introduce a breaking change, because the structure of the YARP port may change.

In general, I would develop a custom Network Wrapper Server YARP devices that exposes the required information in the desired form in a YARP port that publishes yarp::sig::Vector-like. In this way, we can just re-use it across different devices, for example both here and for the realsense devices, see robotology/yarp-device-realsense2#7 . The alternative solution is to modify every hardware device to stream the data as you prefer, but as you imagine this is not scalable.

@traversaro
Copy link
Member

Also, how to handle the position data, in case the device has this capability, and if it's available.
The update seems straightforward when using mulitpleAnalogSensor interfaces, but with IGenericSensor, it will introduce a breaking change, because the structure of the YARP port may change.

In general, I would develop a custom Network Wrapper Server YARP devices that exposes the required information in the desired form in a YARP port that publishes yarp::sig::Vector-like. In this way, we can just re-use it across different devices, for example both here and for the realsense devices, see robotology/yarp-device-realsense2#7 . The alternative solution is to modify every hardware device to stream the data as you prefer, but as you imagine this is not scalable.

As a starting point, you can copy the code of the multipleanalogsensorsserver YARP device ( https://github.com/robotology/yarp/tree/master/src/devices/multipleanalogsensorsserver ) and just modify the publishing logic to write the data as you prefer.

@HosameldinMohamed
Copy link
Contributor Author

As a starting point, you can copy the code of the multipleanalogsensorsserver YARP device ( https://github.com/robotology/yarp/tree/master/src/devices/multipleanalogsensorsserver ) and just modify the publishing logic to write the data as you prefer.

I moved the discussion to #39

@HosameldinMohamed
Copy link
Contributor Author

It would be great to actually test on an XSens MTi-300 sensor mounted on an iCub 2.7, do you know the state of iCubGenova04 to see if we can try there? Otherwise we can ask to @Nicogene if he has any idea of how to get one of those devices to test.

Yes, iCubGenova04 is actually available. I'll give it a test!.

I tested this branch with the Xsens MTi-300 mounted on iCubGenova04, the PC runs Ubuntu 20.04 at the moment.

Compiling and running the device goes with no issues. Also checked the YARP port.

Terminal contents for reference

icub@icub-head:~$ yarpdev --device inertial --name /icub/xsens_inertial --period 0.0005 --subdevice xsensmt --serial /dev/ttyXsens --xsensmt_period 0.005
[DEBUG] |yarp.dev.PolyDriver|inertial| Parameters are (device inertial) (name "/icub/xsens_inertial") (period 0.000500000000000000010408) (serial "/dev/ttyXsens") (single_threaded 1) (subdevice xsensmt) (xsensmt_period 0.00500000000000000010408)
[WARNING] |yarp.devices.ServerInertial| The 'inertial' device is deprecated in favour of 'multipleanalogsensorsremapper' + 'multipleanalogsensorsserver' + 'IMURosPublisher'.
[WARNING] |yarp.devices.ServerInertial| The old device is no longer supported, and it will be deprecated in YARP 3.7 and removed in YARP 4.
[WARNING] |yarp.devices.ServerInertial| Please update your scripts.
[DEBUG] |yarp.devices.ServerInertial| Subdevice xsensmt
[DEBUG] |yarp.dev.PolyDriver|xsensmt| Parameters are (device xsensmt) (name "/icub/xsens_inertial") (period 0.000500000000000000010408) (serial "/dev/ttyXsens") (single_threaded 1) (subdevice xsensmt) (xsensmt_period 0.00500000000000000010408)
[WARNING] xsensmt -  Parameter "sensor_name" not set. Using default value  " sensor_imu_xsensmt " for this parameter.
[WARNING] xsensmt -  Parameter "frame_name" not set. Using the same value as "sensor_name" for this parameter.
[INFO] xsensmt: Opening serial port /dev/ttyXsens with baud rate 115200 and output period 0.0050 seconds.
[DEBUG] Device:  MTi-300-2A8G4 , with ID:  03782070  opened.
[INFO] xsensmt: Putting device into configuration mode.
[INFO] xsensmt: Found a device with id:  03782070  @ port:  /dev/ttyXsens , baudrate:  4098  .
[INFO] xsensmt: Device:  MTi-300-2A8G4  opened.
[INFO] xsensmt: Configuring the device of type 03782070.
[INFO] xsensmt: Putting device into measurement mode.
[INFO] |yarp.dev.PolyDriver|xsensmt| Created device <xsensmt>. See C++ class yarp::dev::XsensMT for documentation.
[INFO] |yarp.devices.ServerInertial| No ROS group found in config file ... skipping ROS initialization.
[WARNING] xsensmt: Missing orientation message, skipping orientation update.
[INFO] |yarp.os.Port|/icub/xsens_inertial| Port /icub/xsens_inertial active at tcp://10.0.0.2:10002/
[INFO] |yarp.devices.ServerInertial| Server Inertial : no ROS initialization required
[INFO] |yarp.devices.ServerInertial| Starting server Inertial thread
[INFO] |yarp.dev.PolyDriver|inertial| Created wrapper <inertial>. See C++ class ServerInertial for documentation.
[DEBUG] |yarp.devices.ServerInertial| Writing an Inertial measurement.
[DEBUG] |yarp.dev.Drivers|inertial| ===============================================================
[DEBUG] |yarp.dev.Drivers|inertial| == Options checked by device:
[DEBUG] |yarp.dev.Drivers|inertial| ==
[DEBUG] |yarp.dev.Drivers|inertial| device=inertial
[DEBUG] |yarp.dev.Drivers|inertial| id [inertial]
[DEBUG] |yarp.dev.Drivers|inertial|     Id assigned to this device
[DEBUG] |yarp.dev.Drivers|inertial| ==
[DEBUG] |yarp.dev.Drivers|inertial| ===============================================================
[INFO] |yarp.os.Port|/icub/xsens_inertial/quit| Port /icub/xsens_inertial/quit active at tcp://10.0.0.2:10003/
[INFO] |yarp.dev.Drivers|inertial| device active in background...
[INFO] |yarp.dev.Drivers|inertial| device active in background...
[INFO] |yarp.dev.Drivers|inertial| device active in background...
[INFO] |yarp.dev.Drivers|inertial| device active in background...
[INFO] |yarp.dev.Drivers|inertial| device active in background...
[INFO] |yarp.dev.Drivers|inertial| device active in background...
[INFO] |yarp.dev.Drivers|inertial| device active in background...

It's worth noting that the sensor MTi-300 returns the following values:

isMti=  true
isGnss=  false
isImu=  false
isVru=  false
isAhrs=  true

While the MTi-680G returns:

isMti=  true
isGnss=  true
isImu=  false
isVru=  false
isAhrs=  false

@HosameldinMohamed
Copy link
Contributor Author

HosameldinMohamed commented Mar 7, 2023

When terminating the YARP dvice (ctrl-c) I get a segmentation fault though

Comment on lines 270 to 274
// yDebug("xsensmt: Closing Xsens port %s.", m_portInfo.portName().toStdString().c_str());
// m_xsensControl->closePort(m_portInfo.portName().toStdString());
//
// yDebug("xsensmt: Freeing Xsens control object.");
// m_xsensControl->destruct();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are cleanup lines after closing the device, but I noticed they are being executed at a very early stage. And I couldn't understand why!

Copy link
Contributor Author

@HosameldinMohamed HosameldinMohamed Mar 7, 2023

Choose a reason for hiding this comment

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

Handled in dbd12e7 with @traversaro

Basically XsensMT::close() is called twice, once because it's called in the deconstructor of XsensMT and the second after sending a close signal to the YARP device.

The deconstructor is called as soon as XsensMT is constructed, probably even before open() is called.

We initialized XsControl* m_xsensControl{nullptr} to a nullptr, and called m_xsensControl.destruct() only if the m_xsensControl doesn't return nullptr. After destructing it, the pointer value is set to nullptr again.

@@ -125,99 +123,130 @@ bool XsensMT::open(yarp::os::Searchable &config)

m_portInfo = XsPortInfo(comPortString, XsBaud::numericToRate(baudRate));

m_xsensControl = XsControl::construct();
Copy link
Member

Choose a reason for hiding this comment

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

Pay attention to use 4-spaces and not tabs, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was the code copied from Xsens!

Fixed in a24afea and dbd12e7

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for jumping in 😅 Can we fetch directly the code online?

Copy link
Contributor Author

@HosameldinMohamed HosameldinMohamed Mar 7, 2023

Choose a reason for hiding this comment

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

Hi @GiulioRomualdi!

Yes by following https://www.movella.com/support/software-documentation and downloading MT software suite.

You can extract it and follow the readme files to further extract and compile the SDK.

Let me know if you need more info!

See #26 (comment) for more details!

Copy link
Member

Choose a reason for hiding this comment

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

No, I was mentioned using the CMake. Instead of copy paste the library here is it possible to write a cmake machinery that fetches the code? (not related to the scope of this PR)

Copy link
Member

Choose a reason for hiding this comment

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

No, there is no (official) public URL from which the code can be downloaded, if you go in their website you need to fill a form.

@HosameldinMohamed
Copy link
Contributor Author

@traversaro Ready for review! Thanks!

xsensmt/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Just a minor change.

Co-authored-by: Silvio Traversaro <silvio@traversaro.it>
@traversaro traversaro merged commit 8a195bb into robotology:master Mar 8, 2023
@traversaro
Copy link
Member

Thanks!

@HosameldinMohamed HosameldinMohamed deleted the update/2022.0.0_latest branch March 8, 2023 14:46
@GiulioRomualdi
Copy link
Member

Thank you @HosameldinMohamed for the effort this will also help us with the new imu on ergocub

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.

3 participants