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

Proposed new messages to replace IMU message. #101

Open
wants to merge 1 commit into
base: jade-devel
Choose a base branch
from

Conversation

tfoote
Copy link
Member

@tfoote tfoote commented Mar 11, 2017

From https://groups.google.com/forum/#!topic/ros-sig-drivers/2NvgNBOjcFQ

Also connected to ros-infrastructure/rep#95

I'm opening this for more review and to restart the discussion.

I see that we need a few things to be implemented before this can be merged.

  • homogenize this with the REP 145 proposal, REP: 145 ros-infrastructure/rep#95
  • Implement some python methods to convert forward and backwards between the old IMU message and the trio of new messages. And a node that can used to convert each way.
  • Provide an IMU transport lib (in c++ and python) that will provide the necessary synchronization helper methods to handle the parallel transport in a way that keeps things synchronized.

int8 ENU=1
int8 NED=2
int8 ENU_FLOATING=3
int8 NED_FLOATING=4
Copy link
Member

Choose a reason for hiding this comment

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

There'd be a comment here explaining what these mean and what their relationship is to sensor_frame?

float64[9] orientation_covariance # Row major about x, y, z axes

uint8 reference_frame_type # NED, ENU, NED_FLOATING, ENU_FLOATING
string sensor_frame # The coordinate frame in the system of the observation.
Copy link

Choose a reason for hiding this comment

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

Isn't this duplicating info from header?

Copy link

Choose a reason for hiding this comment

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

No. From https://groups.google.com/g/ros-sig-drivers/c/2NvgNBOjcFQ/m/LfG6fhFbIBwJ:

Overview of frames for AHRS

/map # semi-global reference
|
---> ahrs_fix_ned/enu
|
---> odom
       | ---> base_link
                 |------> imu_link (the frame in which the ahrs sensor is mounted, possibly anywhere)

The sensor_frame should always be the frame of the IMU (/imu_link in this example). The way I understood the original proposal, the header.stamp should be the reference frame, like this:

  • if reference_frame_type == ENU then header.stamp = ahrs_fix_enu
  • if reference_frame_type == NED then header.stamp = ahrs_fix_ned
  • if reference_frame_type == ENU_FLOATING then header.stamp = ahrs_fix_enu_floating
  • if reference_frame_type == NED_FLOATING then header.stamp = ahrs_fix_ned_floating

... where the TF frames ahrs_fix_enu_floating and ahrs_fix_ned_floating don't exist (= are not connected to the /map frame). This is a bit weird, but the "floating" reference frames mean that the attitude is relative to some arbitrary reference frame (usually the startup pose of the sensor), and the transformation between /map and the startup pose is not known.

Copy link

Choose a reason for hiding this comment

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

Something along these lines should probably added as a comment to this message.

@@ -0,0 +1,3 @@
std_msgs/Header header
geometry_msgs/Vector3 linear_acceleration
float64[9] linear_acceleration_covariance # Row major x, y z
Copy link

Choose a reason for hiding this comment

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

Maybe consider adding bool is_gravity_compensated? Defaulting to false sounds good as that is the old behavior.

int8 ENU=1
int8 NED=2
int8 ENU_FLOATING=3
int8 NED_FLOATING=4
Copy link

Choose a reason for hiding this comment

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

Define these right above reference_frame_type to make it more clear these belong to it?

int8 ENU=1
int8 NED=2
int8 ENU_FLOATING=3
int8 NED_FLOATING=4
Copy link

Choose a reason for hiding this comment

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

Prefix the constants with REFERENCE_FRAME_?

std_msgs/Header header

geometry_msgs/Vector3 angular_velocity
float64[9] angular_velocity_covariance # Row major about x, y, z axes
Copy link

Choose a reason for hiding this comment

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

We should probably copy the comment from the sensor_msgs/Imu message here:

# If the covariance of the measurement is known, it should be filled in (if all you know is the
# variance of each measurement, e.g. from the datasheet, just put those along the diagonal)
# A covariance matrix of all zeros will be interpreted as "covariance unknown", and to use the
# data a covariance will have to be assumed or gotten from some other source

# This message represents 3D attitude

int8 ENU=1
int8 NED=2
Copy link

Choose a reason for hiding this comment

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

Why not enforce one of these?

Now all subscribers should handle this. I would like it better when there is only one definition within the ROS ecosystem.

Copy link

Choose a reason for hiding this comment

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

Can you clarify how the enforcing should be done?

Copy link

Choose a reason for hiding this comment

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

Stating in the message header-comment that ENU convention should be used.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/if-you-work-with-imu-data-share-sample-and-get-thanked/30203/4

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.

6 participants