Broken pid tests

[adolfo-rt]

The pid_tests target has been broken for a while now, and it fails with the following message:

control_toolbox/test/pid_tests.cpp:170:33: error: passing ‘const control_toolbox::Pid’ as ‘this’ argument of ‘control_toolbox::Pid::Gains control_toolbox::Pid::getGains()’ discards qualifiers [-fpermissive]

The offending code is:

const Pid pid4(pid3);
Pid::Gains g3 = pid4.getGains();

It's calling the non-const method getGains() of the const object pid4.

Does it actually make sense to have a const Pid instance?. It would seem that you can't do much with it anyway. If there is indeed need for accessing the gains from a const instance, an API addition would be required. I tested this and it works (tests pass), but is pretty ugly:

const Pid::Gains& Pid::getGains() const
{
  return *(const_cast<realtime_tools::RealtimeBuffer<Gains>& >(gains_buffer_).readFromRT());
}

Didn't take the time to see if a cleaner implementation is possible. Still, I'd avoid it if there is no real need.

@davetcoleman could you take a look at it?. You wrote that part of the test and worked on the pid implementation back in July.

[adolfo-rt]

Bump.

Tests have been broken for many months but the release process was so far unaffected. Now pre-release tests are failing because of this issue (as they should).

@davetcoleman, I'm assigning you this issue. In the meantime, I'll remove this block of code from the test code , which is the troublemaker.

I'd appreciate it if we could give closure to this issue soon.

[davetcoleman]

Roger, will do.

[davetcoleman]

Waiting on answer for catkin tools testing catkin/catkin_tools#72

[adolfo-rt]

In the meantime, you can workaround the issue by calling ctest directly on the build folder

ctest -VV -R control_toolbox

(very verbose, match test name regex)

[davetcoleman]

I don't think they are built though. I get:

1: /bin/sh: 1: /home/dave/ros/ws_ros_control/devel/lib/control_toolbox/pid_tests: not found

[adolfo-rt]

Try make tests from the build folder.

I have not yet used catkin_tools, so I can't provide insights on that front, but if a feature is missing, you can still fallback to catkin_make, right?.

[davetcoleman]

I tracked down the problem - this commit 17e41b5 removed the feature that allowed that test to pass. When I reverted it I was able to build and test the entire ros_control suite.

The thing is - I don't know why I removed the getGains() const function. There must have been some bug or problem. @hsu do you recall why?

[hsu]

I vaguely remember it was not working properly due to some race condition, but I can't find any evidence supporting that statement except my [memory|imagination].

[davetcoleman]

Well no one has missed it except this test, so I vote we not re-introduce it and instead just remove that test (which it has already been commented out). I don't see a need for that functionality at this point anyway.

[adolfo-rt]

Copy that, you can then close the issue. Thanks for looking into this.