Skip to content
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

diff_drive_controller prev_time not set #495

Open
PaddyCube opened this issue Jul 12, 2021 · 1 comment
Open

diff_drive_controller prev_time not set #495

PaddyCube opened this issue Jul 12, 2021 · 1 comment
Labels

Comments

@PaddyCube
Copy link
Contributor

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants