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

This PR aims to deal with the control or not of the upper body. #19

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from

Conversation

MaximilienNaveau
Copy link
Collaborator

@MaximilienNaveau MaximilienNaveau commented Apr 6, 2023

In this PR there is a fix for the upper body, namely about this issue: #18
For now the upper body is controlled in torques but make the left wrists controlled and the nan measured in the torques set to zero.

The changes provides 2 things:

  • parametrization of the talos_params roslaunch to activate/deactivate leg/arm/wrists/head/gripper, etc...
  • the safe evaluation of joint sensor data when they are not provided

One can now create a simple node to start the lf on robot like so:
my_lfc_starting_node.sh:

ssh robot-name:~ 'roslaunch linear_feedback_controller talos_linear_feedback_controller.launch simulation:=false default_params:=false activate_right_leg:=true activate_right_leg:=true activate_torso:=true'

This line will allow you to use your own parameters or the default ones and to activate/de-activate the limbs of the robot you are interested in.

Copy link
Collaborator

@ComePerrot ComePerrot left a comment

Choose a reason for hiding this comment

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

I tested this code in simulation and didn't have any problem.
However, I didn't manage to reproduce the error related to the lack of torque sensors on the joints of the wrist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the moving joint names list should be handled by the main demo package (i.e. not the linear_feedback_controller).
The MPC has a similar parameter which means that, for now, it is duplicated between the two packages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these params are available via ROS params. So the controller can also read them from there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what the best solution is for this file.
Having a custom version in every demo package, which sets up the robot correctly, may be the most straightforward way to go.
On the other hand, having a way to automatically parametrize the controlled joints using the moving_joint_names list could prevent mistakes where only the parameter file is updated and not the launch file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we setup the whole body by default? or the lower body only?
This should be considered as an example roslaunch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whole body should be default in my opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the most important change of the PR in my opinion.
I am not sure it is necessary to replace the NaNs in the eigen_sensor_msg_ by zeros.
In any case, I think a warning should be added when a value is modified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes I will flag once with an error message when this happens with a comprehensive error.

@@ -483,6 +488,9 @@ void LinearFeedbackController::acquireSensorAndPublish(
eigen_sensor_msg_.joint_state.effort(i) =
getJointMeasuredTorque(pin_to_hwi_[i]) -
in_torque_offsets_[pin_to_hwi_[i]];
if (std::isnan(eigen_sensor_msg_.joint_state.effort(i))) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So here we should keep sending NaNs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see why we should send NaNs here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question I was asking is, should we remove the NaNs from the measures if there are any?
In my opinion we should either send a value that has been measured/estimated or send a NaN (and not just replace a NaN by 0).

My question was mainly to know if sending NaNs could lead to unwanted behavior in the computation of the MPC (in that case we should change them to 0).
There might be some NaNs in the effort measurement when the wrist are controlled because there are no torque sensors on those joints. In my case I know it won't change anything because I don't use the effort measurements at all.

Copy link
Collaborator

@edantec edantec left a comment

Choose a reason for hiding this comment

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

I did not test the draft but Come did, so I assume the PR is good to go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whole body should be default in my opinion.

@@ -483,6 +488,9 @@ void LinearFeedbackController::acquireSensorAndPublish(
eigen_sensor_msg_.joint_state.effort(i) =
getJointMeasuredTorque(pin_to_hwi_[i]) -
in_torque_offsets_[pin_to_hwi_[i]];
if (std::isnan(eigen_sensor_msg_.joint_state.effort(i))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see why we should send NaNs here.

@MaximilienNaveau MaximilienNaveau changed the title draft: This PR aims to deal with the control or not of the upper body. This PR aims to deal with the control or not of the upper body. Jun 29, 2023
@MaximilienNaveau
Copy link
Collaborator Author

I think a second pass is welcome here!
I need to test this on the robot but it will be in 2 weeks.
In the mean time you could give a feedback on the API.

Copy link
Collaborator

@edantec edantec left a comment

Choose a reason for hiding this comment

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

Seems good to me.

@@ -92,7 +92,7 @@ in the same terminal, once the robot is in the default pose, kill (`ctrl+C`) the

- Terminal 3: Spawn the linear feedback controller:
```
reset && source install/setup.bash && roslaunch linear_feedback_controller talos_linear_feedback_controller.launch simulation:=true default_params:=true
reset && source install/setup.bash && roslaunch linear_feedback_controller talos_linear_feedback_controller.launch simulation:=true default_params:=true activate_right_leg:=true activate_right_leg:=true activate_torso:=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implies that default_params has arm joints deactivated. It is not quite intuitive, although I don't know how we can do better.

@MaximilienNaveau
Copy link
Collaborator Author

MaximilienNaveau commented Jun 29, 2023 via email

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