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.

Sign up for GitHub

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

Jump to bottom

Update the REP 145 with respect to discussed new messages #186

Closed
wants to merge 1 commit into from
Closed

Update the REP 145 with respect to discussed new messages #186

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions rep-0145.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,20 +94,22 @@ Topics
The following topics are expected to be common to many devices - an IMU device driver is expected to publish at least one. Note that some of these topics may be also published by support libraries, rather than the base driver implementation. All message types below are supplemented with a std_msgs/Header, containing time and coordinate frame information.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is to the above section Transformation, but github doesn't allow to place a comment there. I never understood the idea of this paragraph. If I transform both the sensor frame and the reference frame, isn't the resulting data transform always identity? I imagine applying a transform to IMU data is like taking the IMU and rotating its body. I.e. only changing the sensor frame transform regarding the same reference frame. How would I rotate ENU by 90 degrees yaw?

Copy link

@mintar mintar Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree this could be phrased better.

Here's a relevant email by @paulbovbel (the original author of REP 145) that probably explains his thinking when he wrote that paragraph: https://groups.google.com/g/ros-sig-drivers/c/Fb4cxdRqjlU/m/MwS3uZDyfkoJ

I suspect this paragraph is mixing two things, but I'm not 100% certain:

  • NED -> ENU conversion: You have to change body and world frames and the orientation.
  • transformation into a different body frame (e.g., from imu_link to base_link): You only have to change body frame and orientation; the world frame stays the same.



* `imu/data_raw` (sensor_msgs/Imu)
* `imu/angular_velocity` (sensor_msgs/AngularVelocity)

- Sensor output grouping accelerometer (`linear_acceleration`) and gyroscope (`angular_velocity`) data.
Copy link
Contributor

Choose a reason for hiding this comment

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

This becomes obsolete and requires new wording.


* `imu/data` (sensor_msgs/Imu)
* `imu/attitude` (sensor_msgs/Attitude)

- Same as `imu/data_raw`, with an included quaternion orientation estimate (`orientation`).
- Sensor output containing information about the attitude
Copy link

Choose a reason for hiding this comment

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

Suggested change
- Sensor output containing information about the attitude
- Sensor output containing information about the attitude estimation (roll, pitch and yaw).


* `imu/mag` (sensor_msgs/MagneticField)
Copy link

Choose a reason for hiding this comment

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

Hey, where did imu/mag go? We still need that one!

* `imu/linear_acceleration` (sensor_msgs/LinearAccleration)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `imu/linear_acceleration` (sensor_msgs/LinearAccleration)
* `imu/linear_acceleration` (sensor_msgs/LinearAcceleration)

Copy link

Choose a reason for hiding this comment

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

Maybe move linear_acceleration up one bullet point, so it's right behind angular_velocity? Makes more sense to group these two together, because those are usually the raw sensor data, and attitude is computed from those, so it shouldn't be between them. We could also move attitude right to the end (after angular_velocity, linear_acceleration and mag).


- Sensor output containing magnetometer data.
- The sensor output of the measured linear accleration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- The sensor output of the measured linear accleration.
- The sensor output of the measured linear acceleration.



All message types provide a covariance matrix (see REP 103 [1]_) alongside the data field (`*_covariance`). If the data's covariance is unknown, all elements of the covariance matrix should be set to 0, unless overridden by a parameter. If a data field is unreported, the first element (`0`) of the covariance matrix should be set to `-1`.
All message types provide a covariance matrix (see REP 103 [1]_) alongside the data field (`*_covariance`). If the data's covariance is unknown, all elements of the covariance matrix should be set to 0, unless overridden by a parameter. If a data field is unreported, the first element (`0`) of the covariance matrix should be set to `NaN`.
The timestamp for these messages sould reflect the best estimate of when the observation was made.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The timestamp for these messages sould reflect the best estimate of when the observation was made.
The timestamp for these messages should reflect the best estimate of when the observation was made.

It is expected that downstream users may want to use data from multiple of these topics.
To that end data from a unified solution should make sure to publish the data with the same timestamp so that an ``ExactTimeSynchronizer`` ``message_filter`` can be used to subscribe to the set of all the data.

Namespacing
Copy link
Contributor

@peci1 peci1 Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This comment is to Reference implementation section, but github doesn't allow adding a comment there. The reference implementation has long had a serious bug in transformation of attitude (ros-perception/imu_pipeline#8). Wouldn't it be good to fix the reference before accepting this PR? A fix is provided, but not merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed by ros-perception/imu_pipeline#15 .

'''''''''''
Expand Down