Use robots' based configuration

gergondet commented 1 year ago

As introduced in https://github.com/jrl-umi3218/mc_rtc/pull/327

This moves robot's specific configuration the robots section of the configuration then:

we check that values are provided for a given robot (the fully minimal configuration for a given robot is to specify the frame name for the chest orientation task)
we load this section into the main config
we load the specified overwrite keys

This was tested on HRP4CR (in simulation) with the configuration introduced by https://github.com/isri-aist/mc_hrp4cr/commit/17175bf3aaa14fab386e3147363cf1eb1a4a2b09

We could get rid of the ConfigUtils.h header but I don't know if it's used in other projects downstream

mmurooka commented 1 year ago

Thank you for the PR.

I still find it convenient to overwrite init_pos and FSM transitions by overwriting configurations (an example) and I am against dropping that feature. I haven't looked at the changes in detail yet, but this PR seems to drop that feature.

mmurooka commented 1 year ago

Also, I prefer the order of overwrite to be first the robot's specific configuration, then the user-specified overwrite keys. If the changes in jrl-umi3218/mc_rtc#327 only allow access to the robot's specific configuration after the construction of the FSM controller, I think we cannot use the changes in jrl-umi3218/mc_rtc#327 as is for my preferred use case...

gergondet commented 1 year ago

I still find it convenient to overwrite init_pos and FSM transitions by overwriting configurations (an example) and I am against dropping that feature. I haven't looked at the changes in detail yet, but this PR seems to drop that feature.

init_pos overwrite works with the changes introduced in this PR (if they are part of the robot's specific configuration not of an override key)

For the FSM overwriting approach this indeed does not work but I feel that -- as it is used in the sample you linked -- it would be better adressed by having a Meta-like state that lets you pick a motion (or automatically picks it).

I prefer the order of overwrite to be first the robot's specific configuration, then the user-specified overwrite keys

It's the same in this PR.

Note: we can also bring the overwrite concept to mc_rtc controller construction parameters, the override keys would be used to search for files under the same folder as the robots' specific configuration files. How does that sound?

mmurooka commented 1 year ago

init_pos overwrite works with the changes introduced in this PR (if they are part of the robot's specific configuration not of an override key)

Changing init_pos in user-specified overwrite is also done in Motion6DoF. (Since Motion6DoF is being rewritten to the new MultiContactController, I would like to keep what Motion6DoF can do as much as possible.)

For the FSM overwriting approach this indeed does not work but I feel that -- as it is used in the sample you linked -- it would be better adressed by having a Meta-like state that lets you pick a motion (or automatically picks it).

I don't know if I understand the meaning of this correctly, but since I had prepared so many overwrite keys, I think it makes more sense to overwrite only the necessary ones that are specified.

Note: we can also bring the overwrite concept to mc_rtc controller construction parameters, the override keys would be used to search for files under the same folder as the robots' specific configuration files. How does that sound?

If this satisfies the following use case (as currently done in Motion6DoF), it is OK.

Both robot specific and user-specified overwrites can change init_pos, robots, the FSM state configuration, and the FSM transition. (For example, robots are overwritten here)
The configuration can be overwritten in cascade by supplying multiple overwrite keys.
The same overwrite key can specify different overwrite contents for each controller.
It may not be possible to put the overwrite contents of multiple overwrite keys in the same file, but this is probably acceptable.

mmurooka commented 1 year ago

In BaselineWalkingController's CI, the following file is copied to ~/.config/mc_rtc/controllers/BaselineWalkingController.yaml to override the robot-specific overwrite contents. It would not be easy to maintain this as it is, but an equivalent feature is needed. https://github.com/isri-aist/BaselineWalkingController/blob/d059894ff30787c30fb965796b0c7d9b6120e677/.github/workflows/config/WalkingWithFootstepPlanner.yaml#L50-L53

gergondet commented 1 year ago

I don't know if I understand the meaning of this correctly, but since I had prepared so many overwrite keys, I think it makes more sense to overwrite only the necessary ones that are specified.

Sorry this was unclear, please allow me to elaborate.

Please let me know if I'm wrong but what I understand from these configuration files is that each overwrite key correspond to a given motion (with the addition of the NoFeedback overwrite for some specific motions)

From looking through some of those files, they specify two things:

a specific state to implement a given multi-contact motion
a specific transition map to go from Initial to the specified stated
extra robots to load

I believe there is simper solutions to those than the configuration overwrite. This solution also has a major drawback since as far as I can tell you could not simply "play" two motions back to back.

a specific state to implement a given multi-contact motion

You can populate the states folder along the controller installation directory to load all those states.

a specific transition map to go from Initial to the specified stated

I think this would be better to have a selector state to pick the motion you want to perform, e.g.

- transitions:
  - [Initial, ClimbStairs, Motion::ClimbStairs, Auto]
  - [Initial, TraverseField, Motion::TraverseField, Auto]

This would require a little extra works to trigger a motion automatically but that would be on the same level as switching the OverwriteConfigKeys entry.

extra robots to load

This could be the duty of the _Base state, to load (and unload at the end) extra robots for a given motion

Finally, I think the NoFeedback special case can be handled by inserting a NoFeedbackBase state that the specific motions that require this option can handle.

In BaselineWalkingController's CI, the following file is copied

Not that this specific scenario also works with this PR if you simply exchange OverwriteConfigList with robots

mmurooka commented 1 year ago

Thank you for the explanation. I still have the feeling that overwriting all configurations in one batch would be a simpler solution in total, but I think I can go ahead with your solution :)

Before, I used to describe the configuration for each robot in each entry block of the configuration like this, but in my experience this was more difficult to manage.

you could not simply "play" two motions back to back

Multiple motion states were executed in succession like this.

I think this would be better to have a selector state to pick the motion you want to perform This would require a little extra works to trigger a motion automatically

How can I trigger it? The nice thing about overwrite was that it could be done with a single line comment in-out. Is it as easy as that?

And I guess I'll have to give up overwriting init_pos, right? Often these changes are left as local changes directly in the yaml file and left out of the git commit. The overwrite system was a good way to avoid this.

Let me ask a few more questions.

This could be the duty of the _Base state, to load (and unload at the end) extra robots for a given motion

Is there already a function to add or remove robots in the FSM configuration?

Not that this specific scenario also works with this PR if you simply exchange OverwriteConfigList with robots

Which of the following two lines takes precedence? Currently, the latter takes precedence.

https://github.com/isri-aist/BaselineWalkingController/blob/d059894ff30787c30fb965796b0c7d9b6120e677/etc/BaselineWalkingController.in.yaml#L282

https://github.com/isri-aist/BaselineWalkingController/blob/d059894ff30787c30fb965796b0c7d9b6120e677/.github/workflows/config/WalkingWithFootstepPlanner.yaml#L50-L53

gergondet commented 1 year ago

I still have the feeling that overwriting all configurations in one batch would be a simpler solution in total, but I think I can go ahead with your solution

I agree this is overall a little simpler but I think adding support for a new robot by simply dropping a file in the right location without interfering with other parts of the controller is worth the extra work.

Multiple motion states were executed in succession like this.

My bad, I should have though about this.

How can I trigger it? The nice thing about overwrite was that it could be done with a single line comment in-out. Is it as easy as that?

Yes. I can imagine:

AutoplayMotions: ["Motion1", "Motion2"]

And I guess I'll have to give up overwriting init_pos, right? Often these changes are left as local changes directly in the yaml file and left out of the git commit. The overwrite system was a good way to avoid this.

I think if we want to handle robots in a specific motion plan we can also handle init_pos

Is there already a function to add or remove robots in the FSM configuration?

No, but it's simple (did not try to compile but that should work):

// in start
auto robots = plan("robots", std::map<std::string, mc_rtc::Configuration>{});
for(const auto & [name, rconfig] : robots)
{
  auto rm = mc_rbdyn::RobotLoader::get_robot_module(rconfig("module"));
  auto & robot = ctl.loadRobot(*rm, name);
  if(rconfig.has("init_pos"))
  {
    robot.posW(rconfig("init_pos");
    ctl.realRobot(name).posW(rconfig("init_pos"));
  }
}
// in teardown
for(const auto & [name, rconfig] : robots)
{
  ctl.removeRobot(name);
}

Which of the following two lines takes precedence? Currently, the latter takes precedence.

Should be the latter.

With this PR you can also simply put:

FootManager:
  enableArmSwing: false

In $HOME/.config/mc_rtc/controllers/BaselineWalkingController/jvrc1.yaml

mmurooka commented 1 year ago

Thank you for taking part in this long discussion :)

AutoplayMotions: ["Motion1", "Motion2"]

Sorry, could you elaborate this? What does AutoplayMotions and Motion1 and Motion2 mean respectively?

I think if we want to handle robots in a specific motion plan we can also handle init_pos No, but it's simple (did not try to compile but that should work):

If I wanted this feature in both BaselineWalkingController and MultiContactController, would I copy this code to both controllers? That doesn't look so good.

In $HOME/.config/mc_rtc/controllers/BaselineWalkingController/jvrc1.yaml

If the default jvrc1.yaml is already installed in $HOME/.config/mc_rtc/controllers/BaselineWalkingController/jvrc1.yaml and I want to overwrite part of it for a specific motion (or overwrite key), what should I do?

Probably it would be best if you could provide examples of how the configurations of Motion6DoF and BWC's CI could be changed to achieve the same thing that is currently being done.

mmurooka commented 1 year ago

BWC's CI

Let me elaborate on this. If you download Artifacts from https://github.com/gergondet/BaselineWalkingController/actions/runs/4041541739 and see BWC-video-PreviewControlZmp-OfflineMpc-WalkingWithFootstepPlanner.mp4, you will see the robot swinging its arms during walking. This is not the expected behavior (i.e., the configuration for CI is not reflected correctly). The arm swinging during walking sometimes causes collision between the arms and an obstacle, causing the robot to fall over. Therefore, I disabled arm swing only in the WalkingWithFootstepPlanner test case.

This CI is not triggered by the PR, which is discussed in https://github.com/isri-aist/BaselineWalkingController/pull/1 and https://github.com/isri-aist/BaselineWalkingController/pull/2

mmurooka commented 1 year ago

Sorry for the delay. I have merged this.

BWC's CI

I have confirmed (locally) that this can be handled by https://github.com/isri-aist/BaselineWalkingController/commit/59b8daac7ada6cce95ef871e697e8f2172e04980

mmurooka commented 1 year ago

I have just noticed that there is a problem in BaselineWalkingController.yaml due to this PR change, where two robots entries are defined as follows, and the first one is ignored.

https://github.com/isri-aist/BaselineWalkingController/blob/39d2b28c03068b760a534fac62f8d4a259bc1e4a/etc/BaselineWalkingController.in.yaml#L20-L22

https://github.com/isri-aist/BaselineWalkingController/blob/39d2b28c03068b760a534fac62f8d4a259bc1e4a/etc/BaselineWalkingController.in.yaml#L271-L272

While working on a similar change (https://github.com/mmurooka/LocomanipController/commit/a4be0993c84b95c552c36d302027b67e3ea57ab6) for LocomanipController, I encountered a fatal problem: the following obj robot was not added.

https://github.com/isri-aist/LocomanipController/blob/fe9cbfdfebb13704062696bd43d617d28c64dcc4/etc/LocomanipController.in.yaml#L22-L28

Is it sufficient to just merge the two robot entries into one? I think the first robot entry lists the robots to appear in the controller, while the second robot entry defines a robot-specific configuration for all possible robots, so they seem to have different purposes...

gergondet commented 1 year ago

I have just noticed that there is a problem in BaselineWalkingController.yaml due to this PR change, where two robots entries are defined as follows, and the first one is ignored.

This is not specific to the issue but rather to have two separate section with the same name:

a:
  value: 0
# other stuff
a:
  another-value: 1

It's ill-formed and the implementation will choose or the other. The solution is to put everything under the robots section.

Is it sufficient to just merge the two robot entries into one? I think the first robot entry lists the robots to appear in the controller, while the second robot entry defines a robot-specific configuration for all possible robots, so they seem to have different purposes...

Yes. For a cleaner separation it's better to put robot's specific values in a separate file that will get loaded at runtime under the robots section.

mmurooka commented 1 year ago

@gergondet Thank you. I fixed it in the above commit.

In the robots section entry, can you tell me how the parser distinguishes between robots that are "added to the controller" and robots that are "just describing a configuration"? Is it with or without the module entry?

gergondet commented 1 year ago

Is it with or without the module entry?

Yes. If an entry in robots does not have a module then mc_rtc will not look at it unless it matches the name of an already loaded robot.

isri-aist / BaselineWalkingController

Use robots' based configuration #12