Renaming getCurrentVelocity and getCurrentPose to address #163

[mhpanah]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	(#163)
Primary OS tested on	(Ubuntu 18.04)
Robotic platform tested on	(HP Desktop)

---

Description of contribution in a few bullet points

Addressing first TODO items of issue #163.

• Renamed getCurrentVelocity to getOdometry which contains both odometry pose and twist.
• Renamed getCurrentPose to getGlobalLocalizerPose. This way we know this is the estimated pose from the localizer and not the odometry.

---

Future work that may be required in bullet points

• Do the remaining of the TODO's.

[SteveMacenski]

I think the distinction is pretty semantic, what brought upon this? It's everyone's understanding that a pose is always the output of state estimation and it just makes the lines longer

[mhpanah]

@SteveMacenski Here the intention was to distinguish between the pose that was estimated by AMCL and Odometry. But, the odom pose is usually estimated with KF and its not raw measurements from wheel encoder. So I agree that this name change does not satisfy the original intend. Maybe a better name for this method would be getCurrentAMCLPose or getAMCLPose ?

[SteveMacenski]

@mhpanah The odometry is the distance / current speed travelled from the original frame in which it started in. Typically this will be the location when the robot started but doesn't necessarily have to be. The pose estimation is always in the global frame (by convention, or else it wouldnt really be a pose-estimation) and can be created by a number of clients like ORB-SLAM, AMCL, other slams, other fusion techniques, etc.

So AMCL should definitely not appear, as many people don't use AMCL, its a good starting point but has issues. I think what you're getting at is a global pose estimate or a global localizer, but AMCL just provides a frame correction based on global landmarks, I wouldn't consider this part of the pose estimation directly but something running in parallel to deal with growing error.
But the 'global localizer' will generally be looked at as the robot pose at this snap shot in time before the sensor fusion comes through to dead reckon. The GetOdometry() clearly distinguishes between a global pose and the odometry. I dont think any additional clarification is needed past getCurrentPose(). If anything, I would make that function take in an optional argument (defaulted to "map") that allows you to get the pose in different frames.

i.e. getCurrentPose("map"), getCurrentPose("odom"), getCurrentPose("some_weird_frame_because_I_can").

[mhpanah]

@SteveMacenski Pose estimation is the problem of determining the pose (position and orientation) of objects relative to some coordinate system. One can estimate robots pose through wheel encoders and/or IMU with an estimator (EKF, UKF, etc.) assuming the initial pose is known with respect to a reference frame of interest. Other methods such as AMCL, ORB-SLAM, etc. as you suggested are also can be used for pose estimation. In the former the accuracy degrades over time because of drift, wheel slippage, model uncertainty such as friction, etc. while this is also true in the latter methods but accuracy should be improved since these methods are also observing external features, landmarks, etc. in addition to robot’s internal states.

I agree, now that the function is renamed to getOdometry(), I think it’s fair to conclude that no further clarification is needed for getCurrentPose(). One can get odom pose from getOdometry and the localizer pose from getCurrentPose(). Being able to get either of these two poses are important since usually Odom poses are updated at much faster rates. I still think getCurrentEstimatedPose() is more descriptive terminology but I’m okay with leaving the Estimated part out :) Later we might include more functionality to robot’s class to get ground truth pose in simulation for various reasons such as testing stack in simulation, tuning parameters, and comparing different localizer performance in simulations, etc. in which having currentPose and currentEstimatedPose makes sense.

[SteveMacenski]

The getCurrentPose() (or similar) shouldn't just be the pose given by the global landmark solution, it should be from TF the current live pose. How people implement GetRobotPosition() today is through a TF listener getting map (or eq.) -> base link (or eq.) not getting just what the last thing AMCL/SLAM/Fusion pumped out potentially 5-30 seconds ago

Under the hood, getOdometry() and getPose() should be pretty similar unless you want to distinguish getOdometry differently (which you have and I agree is an interesting use case) to be the odometry reported by the implementation of the base controller, vs the pose in odometry frame in TF (which may differ? don't know why it would - depends if looking at raw or fused odometry).

If I can call getRobotPose() 100ms apart, I should expect to see motion even if AMCL hasnt triggered again to give me a landmark based estimation - this is why robot_pose_ekf + robot_localization + all the major SLAM's update TF with the state of their frames.

If you would like to have another method, perhaps getLastLocalizerPose() or eq. to present the last pose the global localizer like SLAM, AMCL, fused sources, then that would also be interesting - I also dont know how I'd use it but if I had it I think I'd find a reason

[mhpanah]

I agree. The user should be able to have access to both AMCL/SLAM pose with timestamp and also through TF listener and we should be able to provide both from robot class. What we have so far in the robot class is the bare minimum that should definitely be extended and re-designed.

[SteveMacenski]

So if this is for the last global position correction package's position, I would like to have it renamed to reflect that - the getCurrentPose() would be confusing and later when we added TF (or eq.) based positioning we'd be changing the underlying code running from an end user

I would suggest getLastGlobalPoseCorrection() getLastLandmarkUpdatePose() or something

[mhpanah]

Let's brainstorm on all these ideas in one of the WG meetings and then update #163.

[SteveMacenski]

Sounds good

[mhpanah]

Renamed getCurrentPose to getGlobalLocalizerPose for now until team decides what to do and what should be included in the Robot class.

[ghost]

Rather than change all the actions to use the global localizer pose, lets leave them using the correct one. Then in the robot class, implement a getCurrentPose that just calls getGlobalLocalizerPose and add a TODO saying it needs to use the transform method found in costmap.

That way, we only have one place to change to use the correct pose method.

Maybe open an issue to remind us to implement that function as well.

[mhpanah]

I can do that but we still need to change it in other places since the type will be different.

[ghost]

You could just return getGlobalLocalizerPose

[ghost]

This is such a long typename. Adding the line using geometry_msgs::msg::PoseWithCovarianceStamped in the class would let you shorten these names here and in the CPP file.

[mhpanah]

I won't be using the using in this PR to be consistent. However, you are correct we should consider using.

[orduno]

Looks good