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

Make joint encoder acceleration reading optional #876

Conversation

LoreMoretti
Copy link
Contributor

It targets #875

@LoreMoretti LoreMoretti changed the title make joint encoder acceleration reading optional Make joint encoder acceleration reading optional Aug 13, 2024
@LoreMoretti LoreMoretti marked this pull request as ready for review August 13, 2024 15:00
@GiulioRomualdi
Copy link
Member

That's fine. Notice that the logger will still try to get the acceleration

@LoreMoretti
Copy link
Contributor Author

LoreMoretti commented Aug 14, 2024

That's fine. Notice that the logger will still try to get the acceleration

That's right, thanks @GiulioRomualdi for pointing this out.

Maybe I could add a configuration parameter for the logger? Now the sensorBridge in the logger has the following configuration parameters:

<param name="check_for_nan">false</param>
<param name="stream_joint_states">true</param>
<param name="stream_inertials">true</param>
<param name="stream_cartesian_wrenches">false</param>
<param name="stream_forcetorque_sensors">true</param>
<param name="stream_pids">false</param>
<param name="stream_motor_PWM">false</param>
<param name="stream_temperatures">true</param>

Maybe I can add stream_joint_accelerations. The logic would be that this parameter has effect only if it is set to false, and in this case we will not stream joint accelerations. Otherwise (i.e. if not present or set to true) the acceleration will be streamed.

I see that there is already stream_joint_states, and adding stream_joint_accelerations could look a bit redundant or ambiguous, but I could not figure out other way to achieve the final goal yet.

If you have better suggestions let me know!

@GiulioRomualdi
Copy link
Member

A possibility is also to add a parameter directly to sensor bridge. Simular to readIMU. Something like readJointAcceleration

@LoreMoretti
Copy link
Contributor Author

A possibility is also to add a parameter directly to sensor bridge. Simular to readIMU. Something like readJointAcceleration

Not sure I got it. Could you expand it a bit?

By chance it is what I have done here?

@traversaro
Copy link
Collaborator

A possibility is also to add a parameter directly to sensor bridge. Simular to readIMU. Something like readJointAcceleration

Not sure I got it. Could you expand it a bit?

By chance it is what I have done here?

I think @GiulioRomualdi was referring to the possibility of setting that attribute using a configuration parameter in

, so that the measure can be disabled by just adding a parameter in the configuration file, without the need to call a method, that requires instead to modify the code of who is using the sensorbridge. Perhaps for consistency with other parameters the parameter name could be stream_joint_accelerations or something like that?

@LoreMoretti
Copy link
Contributor Author

Ok, I got it now thanks.

So what I can do is:

  1. Remove the method that I have added
  2. Change the data member
    bool readJointEncoderAcceleration{true}; /**< flag to enable reading joint accelerations from
    name to streamJointAccelerations and set it from a config file parameter called stream_joint_accelerations.

@traversaro
Copy link
Collaborator

Yes, I think that is what @GiulioRomualdi was referring to.

This commit also removes any member function used to disable
the streaming of joitn accelerations.
@GiulioRomualdi
Copy link
Member

Thank you @LoreMoretti

@GiulioRomualdi GiulioRomualdi merged commit 75386bb into ami-iit:master Aug 19, 2024
11 checks passed
LoreMoretti added a commit to isorrentino/bipedal-locomotion-framework that referenced this pull request Aug 27, 2024
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.

3 participants