search

openhumanoids / oh-distro

An integrated humanoid control, planning and perception system. Developed by MIT and the University of Edinburgh for the Boston Dynamics Atlas and the NASA Valkyrie humanoid robots
http://drc.mit.edu/
Other
117 stars 45 forks source link

Removed IHMC hard-coded params. #111

Closed simalpha closed 8 years ago

simalpha commented 8 years ago

Added IHMC parameters to list of control related params in footstep_params_t. Depends on https://github.com/RobotLocomotion/director/pull/220.

patmarion commented 8 years ago

see comment at https://github.com/RobotLocomotion/director/pull/220

patmarion commented 8 years ago

So this branch has been updated to introduce ihmc_control_params_t, which is not compatible with the pending Director footsteps driver branch, if I understand correctly. Perhaps this a temporary measure until the Director UI can be reviewed?

simalpha commented 8 years ago

I updated the branch on Director as well. Refer to https://github.com/openhumanoids/director/commit/b7bf986427ab4b4821c5d92ab09f8e825a5b93e9

patmarion commented 8 years ago

I see. I recognize you guys are busy getting things working and I don't want to interfere with your progress, but this one in particular is a little tricky/subtle, and probably worth discussing in more depth. How come you decided to move away from using footstep params and instead added the control parameters message?

patmarion commented 8 years ago

Perhaps it would be helpful for me to explain my perspective-

when @rdeits redesigned the footstep/walking messages, we strived to remove all state from our planners. In this pull request, I see some parameter state in the lcm2ros translator which could lead to incorrect behavior. Under this scheme, if the translator is restarted, its state will not be consistent the parameter state displayed in Director.

By using footstep parameters, we guarantee that all parameters are encoded into the footstep lcm message, thus the correct behavior is guaranteed. This enables use cases such as multiple instances of Director making footstep requests, restarting processes, and log playback without depending on specific message sequences/ordering.

mauricefallon commented 8 years ago

Yes, you are correct Pat. The parameters would be different in the way you described.

My thinking was that:

  1. The IHMC footstep parameters are not used in footstep planning i.e. they are not used by @rdeits .
  2. The footstep positions are planned as usual by the footstep planner and these parameters are just added to them for IHMC's controller for execution. So bundling them into the planning query wasn't needed and gives the impression they are used in the planning.
  3. Adding the parameters will change the LCM type, which I didn't want to do.

Putting them in the message was @simalpha 's original design. I'm happy to go back to it. I also was very keen on keeping all stepping parameters together.

patmarion commented 8 years ago

Do you foresee additional changes to footstep_params_t to support new features in the future? The last time we modified this message type was May 2015. When this message type changes it invalidates our log files. @rdeits has support for historical lcm types, where it searches (at run time) for previous lcm types, so there is that fallback.

However, maybe we can discuss a possible designs to support many possible backends, rather than hardcoding names like bdi, drake, and ihmc into message types. As one possible design, @rdeits suggested we could use a string encoding like JSON. That would mean subscribers of the message, like the lcm2ros translator, would need a JSON parser, but then it would be easier to add parameters and have a more explicit backward compatibility and versioning.

mauricefallon commented 8 years ago

No, I don't see more changes occurring. (But I didn't want to change the type now either)

Yes, string encoding is the right solution. There is no reason that adding parameters should make things backwardly compatible. I'd be in favour of this.