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

Implement joint torque control device and friction estimation through PINN #866

Merged
merged 17 commits into from
Sep 4, 2024

Conversation

isorrentino
Copy link
Collaborator

No description provided.

@isorrentino isorrentino self-assigned this Jul 22, 2024
@isorrentino isorrentino force-pushed the friction_estimation branch 5 times, most recently from 329ceef to e33f3b6 Compare July 22, 2024 10:09
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.

I would avoid to add both the onnx and the config files here. Onnx can be saved on huggingface and the config file in robots' configuration

CHANGELOG.md Outdated
@@ -227,7 +228,6 @@ All notable changes to this project are documented in this file.
- Fix `QPTSID` documentation (https://github.com/ami-iit/bipedal-locomotion-framework/pull/634)
- Fix error messages in `QPTSID` class (https://github.com/ami-iit/bipedal-locomotion-framework/pull/639)
- Fix compilation failure when using CMake 3.26.1 and pybind11 2.4.3 (https://github.com/ami-iit/bipedal-locomotion-framework/pull/643)
- Fixed changelog checker (https://github.com/ami-iit/bipedal-locomotion-framework/pull/856)
Copy link
Member

Choose a reason for hiding this comment

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

This line shouldn't be removed

/**
* @file JointTorqueControlDevice.h
* @authors Ines Sorrentino
* @copyright 2023 Istituto Italiano di Tecnologia (IIT). This software may be modified and
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @copyright 2023 Istituto Italiano di Tecnologia (IIT). This software may be modified and
* @copyright 2024 Istituto Italiano di Tecnologia (IIT). This software may be modified and

Comment on lines 42 to 56
struct CouplingMatrices
{
Eigen::MatrixXd fromJointTorquesToMotorTorques;
Eigen::MatrixXd fromMotorTorquesToJointTorques;
Eigen::MatrixXd fromJointVelocitiesToMotorVelocities;
void reset(int NDOF)
{
fromJointTorquesToMotorTorques = Eigen::MatrixXd::Identity(NDOF, NDOF);
fromMotorTorquesToJointTorques = Eigen::MatrixXd::Identity(NDOF, NDOF);
fromJointVelocitiesToMotorVelocities = Eigen::MatrixXd::Identity(NDOF, NDOF);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Could you comments these?

Comment on lines 61 to 64
double kt; ///< motor torque to current gain
double kfc; ///< friction compensation weight parameter
double kp; ///< proportional gain
double maxCurr; ///< maximum current
Copy link
Member

Choose a reason for hiding this comment

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

Comments are like

double kt /**< motor torque to current gain */

Comment on lines 1177 to 1180
double now = yarp::os::Time::now();

std::lock_guard<std::mutex> lock(globalMutex);
if (now - timeOfLastControlLoop >= this->getPeriod())
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 use blf clock and use std::chrono::nanoseconds for handling the time

@@ -7,8 +7,8 @@ BSD-3-Clause license. -->
<param name="robot">ergocub</param>
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid to touch this config file in this PR

@@ -26,7 +26,7 @@ BSD-3-Clause license. -->
</group>

<group name="ExogenousSignals">
<param name="vectors_collection_exogenous_inputs">("Walking", "GridTracking","cmw")</param>
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid to touch this config fil here

Comment on lines 90 to 82
yError("PassThroughControlBoard error: %d devices passed to attachAll, but only 1 is "
"supported. If you want to connect several device to a single "
"PassThroughControlBoard, please use a controlboardremapper device in the middle.\n",
p.size());
return 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 may use blf::log()->error()

Comment on lines 64 to 73
bool legacyOption = false;
legacyOption = legacyOption || config.check("proxy_remote");
legacyOption = legacyOption || config.check("proxy_local");

if (legacyOption)
{
yError("PassThroughControlBoard error: legacy option proxy_remote and proxy_local are not "
"supported anymore. Please use the yarprobotinterface to compose devices.\n");
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed.

What do you think @traversaro ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, it was a leftover from the copy&paste from controlboardwrapper2 or similar I guess.

@GiulioRomualdi
Copy link
Member

Hi @isorrentino let me know if It is ready for a review

@isorrentino
Copy link
Collaborator Author

isorrentino commented Aug 19, 2024

Hi @isorrentino let me know if It is ready for a review

Hi @GiulioRomualdi, it is ready for review.

@isorrentino
Copy link
Collaborator Author

Hi @GiulioRomualdi do you have time to check if everything is ok now?

@GiulioRomualdi
Copy link
Member

@GiulioRomualdi
Copy link
Member

Waiting for the CI

@GiulioRomualdi
Copy link
Member

843889f should fix the issue

@isorrentino
Copy link
Collaborator Author

Thanks

@GiulioRomualdi GiulioRomualdi merged commit 7ffa8df into ami-iit:master Sep 4, 2024
10 checks passed
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.

3 participants