This PR fixes issue 59 of the Python version of the DQ Robotics

[juanjqo]

@dqrobotics/developers

Hi @mmmarinho,

This PR initializes some variables to match the Matlab implementation. Specifically, I initialized the attributes damping, gain, system_reached_stable_region_, stability_threshold, stability_counter, and stability_counter_max of the class DQ_KinematicController.

This PR fixes the issue 59 in Python by setting the damping to 1e-3 in the class DQ_ClassicQPController(as in the Matlab version).

Furthermore, I added a default damping in the constructor of the class DQ_ClassicQPController to match the implementation of the class DQ_ClassicQPController.m.

Minimal Example

#include <iostream>
#include <dqrobotics/robots/KukaLw4Robot.h>
#include <dqrobotics/robot_control/DQ_ClassicQPController.h>
#include <dqrobotics/solvers/DQ_PROXQPSolver.h>

using namespace DQ_robotics;

int main()
{
    auto robot = std::make_shared<DQ_SerialManipulatorDH>(KukaLw4Robot::kinematics());
    auto solver = std::make_shared<DQ_PROXQPSolver>();
    auto controller = DQ_ClassicQPController(robot, solver);
    auto damping = controller.get_damping();
    std::cout<<damping<<std::endl;
}

Output

0.001

Output of the current version of DQ Robotics (MacOS)

0

Please let me know if additional modifications are required.

Best regards,

Juancho

[mmmarinho]

Thanks, @juanjqo. My major concern is the re-initialization of things already being initialized in the constructor.

[mmmarinho]

Hi @juanjqo and @bvadorno ,

I'm quoting the reply by @juanjqo here because if we resolve that conversation all the discussion will be lost (AFAIK).

@bvadorno no, they do not match. Unlike the C++ implementation, the Matlab version of DQ_KinematicController.m does not initialize the attributes in the constructor. Instead, they are defined directly in the class. Furthermore, the constructor is public in DQ_KinematicController.m and protected in DQ_KinematicController.h

PS. @bvadorno, In one of our internal DQ robotics meetings, we decided to modify the Matlab version to match the C++ one. This is, initializing the attributes in the constructor. May I follow that plan?

If I may add to this discussion, given that I wasn't part of those meetings.

A bazillion years ago (more accurately 12 years or so), the C++ library had a C++98-ish style, hence in-class initialization was not possible. Please put into context that it inherited from even earlier code and C++11 compilers were still not widespread. For instance,

cpp/DQ_kinematics.h

Line 17 in ba29d76

matrix <double> dq_kin;

In-class initialization has been added to compilers for C++11 and beyond. In-class initialization is preferred in C++11 as well, no doubt. It leads to less repetition and could be less error-prone (if users are observant of the style). However, because constructor initialization isn't wrong, the original style was followed to keep the files consistent across the library (if we do in-class initialization somewhere, I'm sure it's rare).

My initial point when flagging the in-class initialization of the damping factor was simply that the initialization must happen all in the same place. It's fine to move it out of the constructor and have all initialization done in class. It is also reasonable to move the new one to the constructor. What is confusing is to have part of it in class and part of it in the constructor.

[bvadorno]

Thanks for the additional context, @mmmarinho. Since I'm stuck with C++98, having the context of the functionalities that came after is always a mix of delight (learning cool things) and despair (feeling like a dinosaur).

@juanjqo, since both are correct, let's stick with what we decided in person. I don't remember the reason for our decision anymore, but I'm quite sure we had a good reason (we probably concluded that changing MATLAB would be easier).

Kind regards,
Bruno

[mmmarinho]

Hi @juanjqo,

From what I can see, the current implementation is following the MATLAB format while keeping to the C++ requirements. This seems good to go.

[mmmarinho]

This seems to be properly matching MATLAB.

https://github.com/dqrobotics/matlab/blob/74b5ffc535dd08724436713b9846fc9e54e03525/robot_control/DQ_KinematicController.m#L83

[mmmarinho]

This seems to be properly following MATLAB.

https://github.com/dqrobotics/matlab/blob/74b5ffc535dd08724436713b9846fc9e54e03525/robot_control/DQ_ClassicQPController.m#L37