Skip to content
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

Fixes to base estimator squashed in one single commit. #805

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

G-Cervettini
Copy link
Contributor

No description provided.

Copy link
Member

@GiulioRomualdi GiulioRomualdi left a comment

Choose a reason for hiding this comment

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

There is somenthing not clear to me:

  1. I was expecting the class computing the base estimation only from one foot
  2. Another class that combines the left and the right feet classes for doing the switching

Comment on lines +19 to +22
// YARP
#include <yarp/sig/Vector.h>
#include <yarp/os/BufferedPort.h>
#include <yarp/os/Network.h>
Copy link
Member

Choose a reason for hiding this comment

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

This dependency shouldn't be there.

Comment on lines +224 to +228

bool m_isModelSet{false};
bool m_isInitialized{false};
bool m_isInputSet{false};
bool m_isOutputValid{false};
Copy link
Member

Choose a reason for hiding this comment

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

You should add the documentation for this

Comment on lines +253 to +257
// Eigen::Matrix3d stepRot = m_state.footPose_R.rotation().inverse() * m_state.footPose_L.rotation();
// Eigen::Vector3d stepRotRPY = toXYZ(stepRot);
// manif::SO3d stepRotManif = manif::SO3d(stepRotRPY(0), stepRotRPY(1), stepRotRPY(2));
// manif::SE3d T_walkRot(m_noTras, stepRotManif);
// manif::SE3d temp = T_walkTras * T_walkRot * m_T_walk * m_T_yawDrift;
Copy link
Member

Choose a reason for hiding this comment

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

You can remove these comments

@GiulioRomualdi
Copy link
Member

I will work on this starting from Monday since I will need it for running some test on the robot :)

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

Successfully merging this pull request may close these issues.

2 participants