-
Notifications
You must be signed in to change notification settings - Fork 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
Implement dofs, get / set joint control mode in Ignition Gazebo, refactor PID #104
Implement dofs, get / set joint control mode in Ignition Gazebo, refactor PID #104
Conversation
10c6d4b
to
cc46f76
Compare
to take into account the new capability to set the JointControlMode
8dcba75
to
551bd20
Compare
@@ -71,6 +74,8 @@ class gympp::gazebo::IgnitionRobot : public gympp::Robot | |||
|
|||
bool setJointPosition(const JointName& jointName, const double jointPosition) override; | |||
bool setJointVelocity(const JointName& jointName, const double jointVelocity) override; | |||
bool setJointControlMode(const JointName& jointName, | |||
const JointControlMode controlMode) override; |
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.
Not a big problem for the simulation runtimes, but for real robot runtimes it may be useful to be able to set also a first reference when you switch control mode. If there is the possibility of setting the joint position setpoints synchronously with the joint control mode, then there is no problem.
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.
Excellent catch @traversaro. Right now is not possible to set the new reference synchronously (nor before changing control mode).
I see two solutions:
-
Make this a warning to allow users to set a reference before changing control mode:
gym-ignition/ignition/src/IgnitionRobot.cpp
Lines 483 to 487 in 551bd20
if (jointControlMode(jointName) != JointControlMode::Position) { gymppError << "Cannot set the position target of joint '" << jointName << "' not controlled in Position" << std::endl; return false; } -
Get the current joint positions before switching to
Position
mode and use them as a reference. In this way, if there's a small delay between the switch of the control mode and the setting of the first reference, the PIDs are already in working state.
Do you have any comment on these two, or a third preferable option?
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.
Get the current joint positions before switching to Position mode and use them as a reference. In this way, if there's a small delay between the switch of the control mode and the setting of the first reference, the PIDs are already in working state.
In some cases, this is not going to work (for example, if you do that on a KUKA robot with RSI, the controller will fault due to the difference between the current reference and the current position, that are different due to static error).
Do you have any comment on these two, or a third preferable option?
To be honest, I am not enthusiastic of either solution (mainly because it is not clear for the user) for me, it is ok to just take note of this problem, and understand how to solve it in the future.
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.
In some cases, this is not going to work (for example, if you do that on a KUKA robot with RSI, the controller will fault due to the difference between the current reference and the current position, that are different due to static error).
I didn't get something. How could the user provide a reference more precise than the current state read by sensors? In any case there will be a mismatch, right?
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.
Opened a new issue to track this #107
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.
I didn't get something. How could the user provide a reference more precise than the current state read by sensors? In any case there will be a mismatch, right?
We can discuss f2f, but basically in general the reference is different from the measured one if there is static error. If the static error is not negligiible, switching the reference from the old reference to the new reference creates a discontinuity.
@@ -333,6 +325,11 @@ bool IgnitionRobot::valid() const | |||
return true; | |||
} | |||
|
|||
size_t IgnitionRobot::dofs() const | |||
{ | |||
return jointNames().size(); |
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.
Just to know, are we tracking somewhere that the joint contained in jointNames
has 1 and only 1 DOFs?
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.
There are a lot of asserts in the code, not here specifically. I think we're well covered, but we should document (yet another thing to remember) that we only support 1 DoF joints.
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.
(yet another thing to remember)
Yet another thing for which to open issues. : )
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.
I converted an old issue to collect all these links to TODO(cument) stuff #37
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.
Small curiosity, why you need to update git?
I am still puzzled to be honest. Have a look at this log:
It downloads the tarball, instead of creating a local git repository. For the python versioning we use setuptools-scm that, by reading the git history, creates a PEP440 compliant release. If there's no git repo, it fails to get the version:
|
This PR adds the following capabilities to robots simulated in Ignition Gazebo:
Fixes partially #65