Skip to content

Added load functions #26

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Added load functions #26

wants to merge 20 commits into from

Conversation

chongwilliam
Copy link

Functions for adding/removing loads at a link.

Copy link
Collaborator

@mikael-jorda mikael-jorda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add unit tests in tests/Tests_Sai2Model.cpp

@@ -0,0 +1,129 @@
// 02-update_model:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update or remove those comments

robot_with_load->setQ(q);
robot_with_load->updateModel();

// tests
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an example, not a test. In the example, you want to show how to use the functions related to this feature: how to add a load and how to remove the load, maybe adding several loads and showing the effect on the model. But a single model to which you add a load and then remove it and compare the gravity and mass matrix by printing them for example is good.

And then you want to make a unit test case (or several if needed) in the tests/Tests_Sai2Model.cpp file where you can make the comparisons you make here

RigidBodyDynamics::Body body = RigidBodyDynamics::Body(-load_params.mass, \
RigidBodyDynamics::Math::Vector3d(load_params.com_pos), \
RigidBodyDynamics::Math::Matrix3d(-load_params.inertia));
// erase existing body, add body with same name, and remove body again
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do all that ? It seems weird to have to add a body with negative mass to the model. Can't we just erase the body ?

* @param inertia load inertia measured about the parent joint frame
* @param body_name unique body name for identifier
*/
void addLoad(const std::string& link_name,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a Transform3d to set the position and orientation of the fixed body in parent instead of just the position ? That way we can use the inseria of the added body in its own inertial frame which is easier to use in general

const Vector3d& com_pos,
const Matrix3d& inertia,
const std::string& body_name) {
if (_load_names_to_load_mass_map.find(body_name) != _load_names_to_load_mass_map.end()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add a check to make sure that the link name exists, and other ones to make sure the mass and inertia are physically valid (positive mass and positive definite inertia)

const double& mass,
const Vector3d& com_pos,
const Matrix3d& inertia,
const std::string& body_name) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call that load_name instead of body_name. It is cleaner

_load_names_to_load_mass_map.insert(std::make_pair(body_name, LinkMassParams(mass, com_pos, inertia, link_name)));
}

void Sai2Model::Sai2Model::removeLoad(const std::string body_name) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load_name instead of body_name to be consistent with the function name (removeLoad)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants