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

Proposed new messages to replace IMU message. #101

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

Proposed new messages to replace IMU message. #101

Show file tree
Hide file tree
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
4 changes: 4 additions & 0 deletions sensor_msgs/msg/AngularVelocity.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
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:

common_msgs/sensor_msgs/msg/Imu.msg

Lines 5 to 8 in 20a833b

# 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

14 changes: 14 additions & 0 deletions sensor_msgs/msg/Attitude.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# 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.

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?

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?

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/Quaternion orientation
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.

3 changes: 3 additions & 0 deletions sensor_msgs/msg/LinearAcceleration.msg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.