PR2 / pr2_controllers

The controllers that run in realtime on the PR2 and supporting packages.
46 stars 34 forks source link

remove redundant/incorrect code. #377

Closed goretkin closed 2 years ago

goretkin commented 9 years ago

This does not change any behavior, but it removes misleading code.

there is no need for an initialized_ flag, because the controller manager will call starting to indicate the first iteration of a control cycle.

The controller already does the correct thing in starting. If instead the code in starting were removed, and the initialized_ flag kept, then this position controller would (incorrectly) jump to the previous set point if it is stopped and started (for example, if you hit and release the run-stop), since the initialized_ flag was only being cleared in the constructor.

goretkin commented 9 years ago

The history of the file doesn't go back enough to see what the original intentions were with the initialized_ flag

https://github.com/PR2/pr2_controllers/commits/hydro-devel/robot_mechanism_controllers/src/joint_position_controller.cpp

but if there's a way to see it, I'd be interested.

TheDash commented 9 years ago

Thanks @goretkin for your continuing efforts. As such, any changes made will have to be tested on a real robot to ensure that things are running the same.

Can you confirm that this is the case?

As well, perhaps do a search for the initialize_ parameter. Another class, script, or program might be using it to check if the robot has been initialized.

goretkin commented 9 years ago

I've run it on the PR2 I have access to without any changes that I can note, but of course it would be nice for someone else to confirm. In this case the change seems to be relatively benign, but ideally there would be something like unit tests using the gazebo simulator. That's out of my league for now, though!

The initialized_ is declared as a private member variable, so I think only classes than inherit from JointPositionController could access it.

v4hn commented 2 years ago

replaced by #403 :cat2: