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

[plugins][maissensor] Fix discrepancy of MAIS analog sensor between simulation and real robot #502

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

xEnVrE
Copy link
Contributor

@xEnVrE xEnVrE commented Jul 21, 2020

Fixed mismatched behavior, with respect to the real robot, of MAIS analog sensors output

Output is constrained to be between 0 and 255, the former is reached when the joint is >= maximum value, the latter when the joint is <= its minimum value

Maximum and minimum values are obtained from Gazebo hw limits.

Fixes #476

@pattacini pattacini requested a review from traversaro July 21, 2020 06:46
Copy link
Member

@randaz81 randaz81 left a comment

Choose a reason for hiding this comment

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

I suspect this will break CER simulation, where mais output is expressed in degrees and not 0-255.. Investigation is required before merging...

@pattacini
Copy link
Member

Can we revise these modifications in such a way that they can be made configurable?
We need to fix the MAIS handling on iCub quite urgently.

@xEnVrE
Copy link
Contributor Author

xEnVrE commented Jul 21, 2020

Can we revise these modifications in such a way that they can be made configurable?

If ok, I can add a parameter to specify the format of the output. And, not to break the current status, to have by default the output in degrees.

I though that only icub robots were using this plugin also because there was a hardcoded

which, btw, I changed to 15 within this PR as that is the number of channels we are expecting on icub robots (according to the icub wiki). [edit by @randaz81: see related issue https://github.com/robotology-legacy/icub-gazebo-legacy/issues/81]

@randaz81
Copy link
Member

randaz81 commented Jul 21, 2020

If urgent, a parameter is ok as a temporary fix because, unfortunately, I think we need some time to investigate an optimal solution.
Other possibilities could be, just for example, a conversion performed in the wrapper layer (this kind of operation is typically done in the RawToUser layer of yarp), or a modification of the CER model (using a different/new plugin and not the maisSensorDriver, please note that on the real R1 a different hardware is used).
PS: I also have a bad feeling that using 0-255 values in icub, will break some ROS compatibility, but I'm not sure if this feature is used by someone.

@traversaro
Copy link
Member

Thanks @xEnVrE for the PR and @randaz81 for the review. @xEnVrE if you can resolve the CHANGELOG conflict, then I can merge, thanks!

…real robot, of mais analog sensorsoutput

Output is constrained to be between 0 and 255, the former is reached when the joint is at its maximum value, the latter when the joint is at its minimum value
@randaz81
Copy link
Member

After some additional reasoning, I come out with the idea that the fixes suggested by Nicola are right: the plugin in gazebo must reflect as much as possible the implementation on the real hardware. In this case iCub is ok, while R1 implementation is contrived and it needs some refactoring. Please proceed with this PR, I'll take care of the fixes required on R1.

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.

4 participants