-
Notifications
You must be signed in to change notification settings - Fork 1
feat: restructure joint getters and support continuous joints #247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: breaking
Are you sure you want to change the base?
Changes from all commits
715e36b
c86033b
c8c1f64
5db83cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -196,6 +196,10 @@ class Model { | |
| */ | ||
| std::vector<pinocchio::CollisionPair> generate_joint_exclusion_list(); | ||
|
|
||
| protected: | ||
| Eigen::VectorXd | ||
| expand_joint_positions_to_full_configuration(const state_representation::JointPositions& joint_positions) const; | ||
|
|
||
| public: | ||
| /** | ||
| * @brief Construct with robot name and path to URDF file | ||
|
|
@@ -307,10 +311,22 @@ class Model { | |
|
|
||
| /** | ||
| * @brief Getter of the number of joints | ||
| * @return the number of joints | ||
| * @return the number of joints (minus pinocchio's universe root joint and fixed joints) | ||
| */ | ||
| unsigned int get_number_of_joints() const; | ||
|
|
||
| /** | ||
| * @brief Getter of the degrees of freedom (dof) of the model | ||
| * @return the number of degrees of freedom (dof) of the model | ||
| */ | ||
| unsigned int get_dof() const; | ||
|
|
||
| /** | ||
| * @brief Getter of the configuration dimension of the model | ||
| * @return the number of configuration dimensions of the model | ||
| */ | ||
| unsigned int get_configuration_dimension() const; | ||
|
|
||
| /** | ||
| * @brief Getter of the joint frames from the model | ||
| * @return the joint frames | ||
|
|
@@ -321,7 +337,7 @@ class Model { | |
| * @brief Getter of the types of the current model's joints | ||
| * @return a vector of joint types | ||
| */ | ||
| std::vector<JointType> get_joint_types(); | ||
| std::vector<JointType> get_joint_types() const; | ||
|
|
||
| /** | ||
| * @brief Getter of the frames from the model | ||
|
|
@@ -651,7 +667,15 @@ inline std::optional<std::reference_wrapper<const std::string>> Model::get_urdf_ | |
| } | ||
|
|
||
| inline unsigned int Model::get_number_of_joints() const { | ||
| return this->robot_model_.nq;// ! todo: this is actually incorrect, that gives dofs | ||
| return this->robot_model_.njoints - 1; | ||
| } | ||
|
|
||
| inline unsigned int Model::get_dof() const { | ||
| return this->robot_model_.nv; | ||
| } | ||
|
|
||
| inline unsigned int Model::get_configuration_dimension() const { | ||
| return this->robot_model_.nq; | ||
| } | ||
|
|
||
| inline std::vector<std::string> Model::get_joint_frames() const { | ||
|
|
@@ -726,7 +750,7 @@ struct JointTypeVisitor : public boost::static_visitor<JointType> { | |
| } | ||
| }; | ||
|
|
||
| inline std::vector<JointType> Model::get_joint_types() { | ||
| inline std::vector<JointType> Model::get_joint_types() const { | ||
| std::vector<JointType> joint_types; | ||
| JointTypeVisitor classifier; | ||
| for (int j = 1; j < this->robot_model_.njoints; ++j) {// skips the first joint (universe) | ||
|
|
@@ -736,4 +760,34 @@ inline std::vector<JointType> Model::get_joint_types() { | |
| } | ||
| return joint_types; | ||
| } | ||
|
|
||
| inline Eigen::VectorXd | ||
| Model::expand_joint_positions_to_full_configuration(const state_representation::JointPositions& joint_positions) const { | ||
| if (this->get_number_of_joints() == this->get_configuration_dimension()) { | ||
| return joint_positions.get_positions(); | ||
| } | ||
| Eigen::VectorXd q(this->get_configuration_dimension()); | ||
| const auto& joint_types = this->get_joint_types(); | ||
| const Eigen::VectorXd& positions = joint_positions.get_positions(); | ||
| std::size_t joint_input_idx = 0; | ||
| for (int i = 1; i < this->robot_model_.njoints; ++i) { | ||
| const auto& joint = this->robot_model_.joints[i]; | ||
| std::size_t idx_q = joint.idx_q(); | ||
| switch (joint_types[i - 1]) { | ||
| case JointType::CONTINUOUS: { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| double angle = positions[joint_input_idx++]; | ||
| q[idx_q + 0] = std::cos(angle); | ||
| q[idx_q + 1] = std::sin(angle); | ||
| break; | ||
| } | ||
| default: { | ||
| for (int j = 0; j < joint.nq(); ++j) { | ||
| q[idx_q + j] = positions[joint_input_idx++]; | ||
| } | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| return q; | ||
| } | ||
| }// namespace robot_model | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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