Closed dinies closed 3 years ago
Hi Nice work ! It is good to be able to use inria_wbc with other robots !
A few remarks
I would prefer to move the has_floating_base
parmeter in the CONTROLLER
section
The PARAMS
section was made to be used with https://github.com/resibots/inria_wbc/blob/67cdc1d50dd0b851a22f50289701d47d9d8d1db8/src/controllers/controller.cpp#L256
(to create a Controller::Params
from yaml)
I think that the etc
folder will grow quickly, maybe we can add talos
and franka
subfolders in it ?
As you did, it is good to start behavior and controller file names with the robot name.
An other option could be to also create talos
and franka
subfolders but for now it seems light enough not to do that
I have seen that you have created an almost empty franka_pos_tracker.cpp
.
If you do not have plans to add things in it in the future, it would be better to usepos_tracker.cpp
directly
What do you think about that ?
Hello, I will try to make a case for every point and then I will wait for additional opinions and guidance.
I think it is fine like this yes I don't know if @jbmouret or @ivanbergonzani has any opinion on the subfolder question ?
As of now I don't fell the need to refactor n subdolders (but would be cleanest). If the number of files increase than it would become the best way. However I think we must try to keep inria_wbc light and instead create additional projects that links to it without adding toouch stuff here.
Still the refactor could be the subject of a different PR.
If we start having franka + tiago + talos + icub, we will need folders in etc. We could have files for each robots in different repos, but this might fragment too much (a difficult tradeoff).
However, I think this kind of refactoring is best done when it is needed (no need to anticipate too much), and it's a different PR (we should first check that we can run code without the floating base and it works fine).
Ok then for now I will (probably tomorrow):
I have done the modifications and tested the executables after merging the latest changes in devel.
@dalinel @ivanbergonzani should we merge? then we can work on a cleaning PR to make directories, more generic behaviors, etc.
This contribution aims to extend the usage of the library to robots that don't have a floating base (eg. manipulators). There is a new parameter now specified in the main conf yaml files (eg walk_on_spot.yaml) that specifies explicitly if the robot is supposed to have a floating base or not. This addition could also be avoided resulting in giving a default flag value in the controller class controller.hpp .cpp
This new parameter affects the logic of controller class in multiple instances, the two most relevant ones are:
In controller.cpp:79 the constructor used if the robot has not floating base allows to choose the suitable robotwrapper constructor ( see tsid/src/robots/robot-wrapper.cpp) In tasks.cpp:167 again there is a flag for the presence of the floating base which, this time, is inferred by the nv and na values ( also clear in tsid/src/robots/robot-wrapper.cpp that they reflect that property unequivocally ) After that the other things that where added are an executable src/robot_dart/test_cartesian_controller_franka.cpp that was build mimicking test_controller then an extension of the controller class franka_pos_tracker.hpp .cpp a behavior specific for the franka and the configuration files needed.