-
Notifications
You must be signed in to change notification settings - Fork 190
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
base: jade-devel
Are you sure you want to change the base?
Conversation
int8 ENU=1 | ||
int8 NED=2 | ||
int8 ENU_FLOATING=3 | ||
int8 NED_FLOATING=4 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
thenheader.stamp = ahrs_fix_enu
- if
reference_frame_type == NED
thenheader.stamp = ahrs_fix_ned
- if
reference_frame_type == ENU_FLOATING
thenheader.stamp = ahrs_fix_enu_floating
- if
reference_frame_type == NED_FLOATING
thenheader.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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
# This message represents 3D attitude | ||
|
||
int8 ENU=1 | ||
int8 NED=2 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 |
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.