feat: restructure joint getters and support continuous joints #247
Conversation
|
I am planning to update the bindings in a separate commit/PR once everything related to these changes has been fully validated |
| const robot_model::Model& robot_model | ||
| ) { | ||
| auto ctrl = ControllerFactory<JointState>::create_controller(type, parameters, robot_model.get_number_of_joints()); | ||
| auto ctrl = |
There was a problem hiding this comment.
Sorry I keep being confused about this, the configuration dimension is the number of variables necessary to describe the joint state, right? I'm not sure that our idea of controllers is compatible with anything else than revolute and prismatic joint or how would an impedance controller look like for a continuous joint? It doesn't work because the two numbers are coupled no?
There was a problem hiding this comment.
yes, it would fail. I only supported forward kinematics and velocities, so everything else is still incompatible (theoretically and in practice).
So perhaps we should completely fail exactly in this place, but indeed here it should also be number of joints, not configuration parameters
| const auto& joint = this->robot_model_.joints[i]; | ||
| std::size_t idx_q = joint.idx_q(); | ||
| switch (joint_types[i - 1]) { | ||
| case JointType::CONTINUOUS: { |
There was a problem hiding this comment.
Assuming we have a URDF with floating or planar joints, does it construct? Is there a chance we get to this switch case with that URDF?
There was a problem hiding this comment.
yes it would construct. And with the condition I have in the constructor where I don't run the QP if we have limited support, we could indeed get here I think.
So in general there is still a question of where we should fail the construction of the model, or if we directly provide a separate model for mobile robots and fail everything else in this for example
There was a problem hiding this comment.
I think it might make sense to have the current model throw on construction if there are joints that are not revolute or prismatic -> this way our controllers can stay the way they are. Then we make a derived class (we still prefer most of the logic to be in the current class, e.g. the getters are valid here) that can handle mobile robots which is just a different way of saying we support continuous joints as well but we should still throw on construction for planar and floating? What do you think?
There was a problem hiding this comment.
yeah I think that's going to be cleaner (and addresses the performance concerns with the if-else). I'll create a commit for this so that we can see what this looks like
There was a problem hiding this comment.
we should also perhaps consider a robot model factory that controllers/hardware can use, but that may imply we need to make a cleaner interface. That's why in a sense I was also considering
RobotModel
/ \
RobotArmModel MobileRobotModel
There was a problem hiding this comment.
A factory could also work even though I have a hard time seeing where we'd need that kind of inheritance since it feels like a controller is always just one or the other. But also an option
There was a problem hiding this comment.
It might actually also be unfavorable to derive a MobileRobotModel from the current RobotModel because one could maybe mess up things through pointer injection? Idk
There was a problem hiding this comment.
yup, all valid. My point is, before we have a pinocchio model somewhere we technically don't know what robot we have. But sure, a controller may explicitly have to use the correct class, but then technically we'd have to manage this in controllers that support both
Description
This PR solves the issue by properly converting the configuration space where needed such that the interface is the same on our end (state representation only knows joints by names), but internally we handle kinematics by unrolling this to the correct configuration space (i.e., continuous joints need to be sin and cos of the joint angle). That is currently only propagate to the minimum amount of places that would allow continuous joints to exist in models.
Review guidelines
Estimated Time of Review: 15 minutes
Checklist before merging: