Open-loop odometry #39

efernandez · 2014-07-18T12:36:01Z

The purpose of this PR is to support open loop odometry when there are no encoders.
It basically adds:

open_loop param
extends Odometry class to update using the last linear and angular velocities received by the controller, i.e. set the odometry twist directly with then and integrate the position (x, y)

po1 · 2014-07-18T12:38:05Z

diff_drive_controller/src/diff_drive_controller.cpp

@@ -108,7 +108,8 @@ static bool getWheelRadius(const boost::shared_ptr<const urdf::Link>& wheel_link
 namespace diff_drive_controller{

  DiffDriveController::DiffDriveController()
-    : command_struct_()
+    : open_loop_(false)


It is better style to add new parameters at the bottom. This can help prevent ABI breaks.

I tried to put it together with other odometry related attributes in the header file.
It just happened to be before command_struct_, so I had to put it at the beginning.

no problem. This is not really a library anyway, so nothing builds against this API.

efernandez · 2014-07-18T15:37:50Z

Nice refactoring @bmagyar
👍

Now we use period instead of time for the odometry integration.
@po1 What do you think?
I add @adolfo-rt to the loop.

adolfo-rt · 2014-07-18T15:51:48Z

diff_drive_controller/include/diff_drive_controller/odometry.h

+     * \param time    Current time
+     * \return true if the odometry is actually updated
+     */
+    void update_open_loop(double linear, double angular, const ros::Duration &period);


updateOpenLoop

+1
also, same as above for update()

lucamarchionni · 2014-07-18T15:54:10Z

Period is constant in every control loop execution.
It happens that there is jitter and using time differences in between control loop executions could be more accurate.

adolfo-rt · 2014-07-18T15:58:03Z

Could you provide a brief summary of the PR, to better match the code with the expectations of the PR? (and to make sure that no extra changesets made it through by mistake).

Also, please mention API additions and changes. This PR changes public interfaces, but the affected classes might be internal.

po1 · 2014-07-18T16:53:37Z

diff_drive_controller/include/diff_drive_controller/odometry.h

    /// Current pose:
    double x_;        //   [m]
    double y_;        //   [m]
    double heading_;  // [rad]

+    /// Current velocity (for open loop mode):


after Bence's refactoring it's now for both open loop and closed loop

po1 · 2014-07-18T17:07:44Z

@bmagyar thanks for the refactoring, but the title of the commit is misleading, since you also changed the behavior (time -> period)

efernandez · 2014-07-19T07:40:32Z

@adolfo-rt The purpose of this PR was originally to support open loop odometry when there are no encoders.
It basically adds:

open_loop param
extends Odometry class to update using the last linear and angular velocities received by the controller, i.e. set the odometry twist directly with then and integrate the position (x, y)

Some refactoring was done later that basically simplifies setting the odometry message and tf with the odometry values (especially the getLinear and getAngular methods).

We also changed the behaviour to use period instead of time, but that might go to another PR IMHO.
@lucamarchionni Here we're basically asking what should we use. period to filtered out the jitter (as it's constant), or time`?

efernandez · 2014-07-19T07:42:58Z

@adolfo-rt This PR only affects internal classes. It adds updateOpenLoop method and a few attributes to the class.

po1 · 2014-07-19T09:32:07Z

It also adds a rosparam, that is external API that has to be documented.
If this is to be merged upstream, and it should be, then a unit test exercising the new feature would be welcome.

efernandez · 2014-07-20T08:06:35Z

@po1 I think the simple test I've added should be enough.
Could you give it a try?

efernandez · 2014-07-21T13:13:16Z

Testing on REEM-H2.
With open_loop it's easy to see that the odometry has no noise, while in close loop it has. :)
This is normal and expected, and it's a signal that the changes are OK.
Anyway, I still have to test a little more, moving the robot, and also using close loop, to verify that there's no regression.

efernandez · 2014-07-21T13:25:59Z

It works, both open and close loops.
@lucamarchionni @po1 @bmagyar @adolfo-rt Any other comment or something missed before merging?

po1 · 2014-07-21T13:35:27Z

all good for me

efernandez · 2014-07-21T13:43:18Z

After talking with @lucamarchionni we have decided that it's better to use the time difference instead of the period.
I'll do and test that.

po1 · 2014-07-21T13:47:04Z

@efernandez I'd be curious to have a short summary of the motivation for reverting to time

efernandez · 2014-07-21T14:51:11Z

I've gone back to time for the odometry computations.

I've also detected a potential bug with the timestamp_ not initialized correctly.
Therefore, I've added an init method in a separate commit.

@po1 @bmagyar Could you give it a look?
I'll test it on the robot.

efernandez · 2014-07-21T14:54:46Z

BTW, all (8/8) unit tests passed.

bmagyar · 2014-07-21T15:10:44Z

The reason for using time is that period is fixed, the theoretical update cycle of the controller, while the update function might be called with slight differences. This could potentially introduce small jitter in the controller, while another reason for not changing could be IfItAintBrokeDontFixIt.

The init function definitely makes things better, now we won't have a big bump upon the first update. This wasn't causing problems on REEM but might do on other platforms, better be safe.

efernandez · 2014-07-21T15:11:40Z

Last changes tested on REEM-H2 robot.
Everything looks OK.

@po1 I've followed @lucamarchionni recommendation to use the time because it's more accurate, especially if the real time system doesn't really follow the period.

@bmagyar @adolfo-rt If you find it right now, I'll merge it.

bmagyar · 2014-07-21T15:29:06Z

I'd squash the 2 commits about cosmetics, and rebase the rest, but the contents are fine.

bmagyar · 2014-07-21T15:29:10Z

+1

efernandez · 2014-07-21T15:42:56Z

Cosmetic changes squashed into one.

I don't feel like merging my own PR...

po1 · 2014-07-21T17:51:05Z

@bmagyar @efernandez IIRC that was exactly the motivation to use period in the first place. The jitter introduced by the controller in the odometry will be amplified in the computation of the velocities, which would be way smoother if you used the a priori knowledge about the frequency of the controller, which is period. I understand that it works that way for now, but I seem to remember that it works the other way around in our walking controller, which is far more sensible to jitter...

@efernandez I suggest you give a try to both options, especially on a system that has a very low resolution odometry, if you see what I refer to.

In the mean time, let's merge this and open more PRs if needed.

po1 · 2014-07-21T17:52:08Z

@bmagyar in the future, please don't label a commit as 'cosmetic' if it contains changes in the API (even internal) and the logic.

Open-loop odometry

adds open loop odometry

a482cea

po1 reviewed Jul 18, 2014
View reviewed changes

adolfo-rt reviewed Jul 18, 2014
View reviewed changes

po1 reviewed Jul 18, 2014
View reviewed changes

po1 changed the title ~~adds open loop odometry~~ Open-loop odometry Jul 19, 2014

bmagyar and others added 3 commits July 21, 2014 17:39

Cosmetic refactoring

a8fea93

adds open loop test

b97b942

adds odometry init to reset timestamp and accs

1f846b6

po1 added a commit that referenced this pull request Jul 21, 2014

Merge pull request #39 from efernandez/open_loop_odometry

077ba0b

Open-loop odometry

po1 merged commit 077ba0b into pal-robotics-forks:hydro-devel Jul 21, 2014

efernandez deleted the open_loop_odometry branch July 22, 2014 09:24

bmagyar mentioned this pull request Oct 11, 2014

Diff drive open loop odom ros-controls/ros_controllers#136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Open-loop odometry #39

Open-loop odometry #39

efernandez commented Jul 18, 2014

po1 Jul 18, 2014

efernandez Jul 18, 2014

po1 Jul 18, 2014

efernandez commented Jul 18, 2014

adolfo-rt Jul 18, 2014

po1 Jul 18, 2014

lucamarchionni commented Jul 18, 2014

adolfo-rt commented Jul 18, 2014

po1 Jul 18, 2014

po1 commented Jul 18, 2014

efernandez commented Jul 19, 2014

efernandez commented Jul 19, 2014

po1 commented Jul 19, 2014

efernandez commented Jul 20, 2014

efernandez commented Jul 21, 2014

efernandez commented Jul 21, 2014

po1 commented Jul 21, 2014

efernandez commented Jul 21, 2014

po1 commented Jul 21, 2014

efernandez commented Jul 21, 2014

efernandez commented Jul 21, 2014

bmagyar commented Jul 21, 2014

efernandez commented Jul 21, 2014

bmagyar commented Jul 21, 2014

bmagyar commented Jul 21, 2014

efernandez commented Jul 21, 2014

po1 commented Jul 21, 2014

po1 commented Jul 21, 2014

Open-loop odometry #39

Open-loop odometry #39

Conversation

efernandez commented Jul 18, 2014

po1 Jul 18, 2014

Choose a reason for hiding this comment

efernandez Jul 18, 2014

Choose a reason for hiding this comment

po1 Jul 18, 2014

Choose a reason for hiding this comment

efernandez commented Jul 18, 2014

adolfo-rt Jul 18, 2014

Choose a reason for hiding this comment

po1 Jul 18, 2014

Choose a reason for hiding this comment

lucamarchionni commented Jul 18, 2014

adolfo-rt commented Jul 18, 2014

po1 Jul 18, 2014

Choose a reason for hiding this comment

po1 commented Jul 18, 2014

efernandez commented Jul 19, 2014

efernandez commented Jul 19, 2014

po1 commented Jul 19, 2014

efernandez commented Jul 20, 2014

efernandez commented Jul 21, 2014

efernandez commented Jul 21, 2014

po1 commented Jul 21, 2014

efernandez commented Jul 21, 2014

po1 commented Jul 21, 2014

efernandez commented Jul 21, 2014

efernandez commented Jul 21, 2014

bmagyar commented Jul 21, 2014

efernandez commented Jul 21, 2014

bmagyar commented Jul 21, 2014

bmagyar commented Jul 21, 2014

efernandez commented Jul 21, 2014

po1 commented Jul 21, 2014

po1 commented Jul 21, 2014