diff_drive_controller prev_time not set

[PaddyCube]

Hello,

I'm using diff_drive_controller for a two-wheeled robot and discovered that it didn't respect acceleration limits. I was curious about this and after digging around, I might came across a bug inside the example code found here: https://github.com/ros-controls/ros_control/blob/5db3baaa71c9dcd8a2fadb3b2c0b5085ea49b3a1/hardware_interface/mainpage.dox

In main.cpp, there is a control loop like this:

    // Setup for the control loop.
    ros::Time prev_time = ros::Time::now();
    ros::Rate rate(10.0); // 10 Hz rate
    //
    while (ros::ok())
    {
        // Basic bookkeeping to get the system time in order to compute the control period.
        const ros::Time     time   = ros:Time::now();
        const ros::Duration period = time - prev_time;

        robot.read();
        cm.update(time, period);
        root.write();

        // All these steps keep getting repeated with the specified rate.
        rate.sleep();
    }

So outisde the while loop, you set prev_time to actual time. Inside the loop, you use the current time - prev_time to calculate the period. But you never set prev_time inside the loop which results in an increasing period which confuses me and as well my PID crontroller.

Shouldn't there be something like prev_time = time right before rate.sleep(); ? If I add it, acceleration gets used as it should. I wonder why this is missing here as this package is widely used with success.

[matthew-reynolds]

Shouldn't there be something like prev_time = time

Yup, you're right, there should be. Would appreciate a PR adding that line!

I wonder why this is missing here as this package is widely used with success.

That document is fairly new, it was part of a round of updating documentation at the end of last year. Looks like that line just got missed when that document was added. I suspect no one noticed this since a) there are a lot of examples out there on how to use the framework, most of which are older than this doc and have had the issues ironed out, and b) it's a fairly small omission so I suspect anyone who saw this doc just noticed it fixed it in their own code.