ros-infrastructure / rep

ROS Enhancement Proposals
http://www.ros.org/reps/rep-0000.html
149 stars 136 forks source link

Update Rep 145 (P II) #372

Open SteveMacenski opened 1 year ago

SteveMacenski commented 1 year ago

Continuation of https://github.com/ros-infrastructure/rep/pull/186 reflecting comments in the discussion:

Should I add myself and Tully as coauthors?

peci1 commented 1 year ago

I understand that unbiasing angular velocity measurements is much more common than unbiasing acceleration, but the sole names of the topics do not actually indicate which one of these was unbiased... One possible interpretation is that e.g. if you only compensate gyros, then you would have topics imu_unbiased/angular_velocity and imu_biased/linear_acceleration. That would make sense, but it would make it harder to use the topics further. Also, it's not obvious what parts of the aggregate message are unbiased. Apart from this confusion, I like the approach this PR settled on.

I would also add a sketch of the relationship between the 4 defined namespaces. As I understand it, each IMU driver should publish imu_raw, but the subscribers cannot do any kind of assumptions about the data (unbiasing, attitude, grav comp). Then the role of the driver is to republish each of these data streams to the correct namespaces. Do I get it right? It might also help to add a few notes about how to add the missing links - i.e. using a bias removal node, using IMU filter etc.

I think the unbiasing tools could be a part of standard ROS packages (possibly in imu_tools). If you'd like to get a reference implementation, just ping me, we have a nice node for that. It is not yet public, but I aim at publishing it to Noetic.

SteveMacenski commented 1 year ago

I understand that unbiasing angular velocity measurements is much more common than unbiasing acceleration, but the sole names of the topics do not actually indicate which one of these was unbiased...

Maybe what isn't clear is that not every topic needs to exist under every namespace if it isn't possible or completed. Just as an IMU need not publish all of the messages in the section above, depending on what it has and what is relevant. Since we've established already the concept that not all those topics may exist to begin with, the indirection of namespaces is just applying that same rule but to sorted data.

If you buy into that view of it, I can add a clarifying note about how not every topic needs to exist.

Then the role of the driver is to republish each of these data streams to the correct namespaces. Do I get it right? i.e. using a bias removal node, using IMU filter etc.

Maybe, maybe not. There could be external tools (like the filters) that are unbiasing the data and they publish the imu_unbiased or imu_compensated topics, not the drivers. I don't think specifying who in particular must publish this data is a good restriction, but I agree that having some examples or illustrations of what we mean could help.

I have only a cursory understanding of IMU calibration (e.g. working next to people in grad school that focused on it, but not myself :laughing: ) but this is a topic that you appear to know quite alot about. Would you be open to providing some language for added descriptions to add to the biased/unbiased namespaces? I can fill in more detail on grav comp and raw.

peci1 commented 1 year ago

I have only a cursory understanding of IMU calibration but this is a topic that you appear to know quite alot about. Would you be open to providing some language for added descriptions to add to the biased/unbiased namespaces?

Well, I wouldn't call myself an IMU calibration expert, but I read something about it when implementing state estimation for our robots. I'll have a look at it in a week or two.

SteveMacenski commented 1 year ago

@peci1 hi, just pinging you back here :smile:

peci1 commented 1 year ago

Thanks, I must admit I forgot about this. I won't have time for it today, but tomorrow I'll have a look at it ;)

peci1 commented 1 year ago

My attempt at improving the bias and gravity compensation sections is in https://github.com/SteveMacenski/rep/pull/1 . The text seems a bit heavy to me, but I was not able to make it more concise.

I have also ditched the imu_biased namespace in favor of just imu. As it is not a problem to try to remove bias from an already unbiased topic, it does not matter whether data on the imu topic are unbiased or not. It seems to me that imu and imu_biased were very similar in many regards, in getting rid of one of them helps to simplify the REP.

SteveMacenski commented 1 year ago

Huh, somehow I wasn't notified about that. I wonder what else I haven't seen.... scary thoughts

I have also ditched the imu_biased namespace in favor of just imu.

I like that. I think what you did there makes sense - IMU is the base, everything else should be specified relative. It does make me wonder if we need both imu and imu_raw, they appear to be the same thing functionally. Do we really need both?

Note a comment I made in your PR: https://github.com/SteveMacenski/rep/pull/1#discussion_r1140868747

ros-discourse commented 1 year ago

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-03-16/30432/1

peci1 commented 1 year ago

It does make me wonder if we need both imu and imu_raw, they appear to be the same thing functionally.

The difference is that imu_raw might have gravity compensated or not. imu should always include gravity. Maybe it needs better wording? The idea is that the driver can just relay data from a serial link/Ethernet to imu_raw, and then a postprocessing step turns it into imu.

Let's have a look at the two cases:

  1. The IMU publishes gravity-compensated data. In this case, imu_raw has gravity compensated data and you need a node that adds back gravity and creates topic imu (angular velocities and magnetometer can be copied directly to imu). You can directly republish the accelerations to imu_compensated.

  2. If the IMU publishes data with gravity, imu and imu_raw can be the same, so you just create a republisher. Or you don't even need to expose the imu_raw topic and can directly publish to imu. Creating imu_compensated would require estimating the gravity vector and subtracting it from imu.

No generic processing node should access imu_raw. It doesn't know what it finds there. imu_raw should only be used by the low-level processing node in a particular IMU's driver.

SteveMacenski commented 1 year ago

Apologies, this is on a hiatus for me given the recent changes in employment. Need to baton down the hatches a bit and revisit things like this once I know what's going on

ros-discourse commented 1 year ago

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

https://discourse.ros.org/t/driver-for-witmotion-imu-sensors-released/30602/2

ros-discourse commented 1 year ago

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

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-4-20-2023/31087/1

ros-discourse commented 9 months ago

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

https://discourse.ros.org/t/future-of-ros-2-gps-support/33297/53

clalancette commented 7 months ago

We did a review of this in the ROS 2 weekly meeting, and here are some of the notes from that:

ros-discourse commented 6 months ago

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

https://discourse.ros.org/t/ros-2-meeting-minutes-2024-02-15/36221/1