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

Add force torque sensor #393

Merged
merged 2 commits into from
Nov 23, 2020
Merged

Conversation

nlamprian
Copy link
Contributor

Addresses gazebosim/gz-sensors#25

Signed-off-by: Nick Lamprianidis [email protected]

@nlamprian nlamprian force-pushed the nlamprian/force_torque branch from 7915ac4 to 5462681 Compare October 19, 2020 08:35
Signed-off-by: Nick Lamprianidis <[email protected]>
EXPECT_EQ(sdf.get(), ft.Element().get());

// The ForceTorque::Load function is tested more thouroughly in the
// link_dom.cc integration test.
Copy link
Member

Choose a reason for hiding this comment

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

wow, that test is actually wrong because the force_torque sensor is only supposed to be attached to a joint, not a link. we can merge this as is, but at some point we will need to correct test/sdf/sensors.sdf

here's an example from osrf/gazebo:

@scpeters
Copy link
Member

This PR is straight-forward; it exposes the existing data fields from the specification. It does raise some questions for me about the fact that force_torque sensors should only be attached to joints, not links, but this is not clearly stated in the specification; it is not reflected in our example files, and we don't check for that anywhere

I'll open an issue to discuss this further.

@scpeters
Copy link
Member

This PR is straight-forward; it exposes the existing data fields from the specification. It does raise some questions for me about the fact that force_torque sensors should only be attached to joints, not links, but this is not clearly stated in the specification; it is not reflected in our example files, and we don't check for that anywhere

I'll open an issue to discuss this further.

#421

@codecov-io
Copy link

Codecov Report

Merging #393 (d35bac9) into master (5a91081) will decrease coverage by 0.18%.
The diff coverage is 64.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #393      +/-   ##
==========================================
- Coverage   87.71%   87.52%   -0.19%     
==========================================
  Files          61       62       +1     
  Lines        9467     9542      +75     
==========================================
+ Hits         8304     8352      +48     
- Misses       1163     1190      +27     
Impacted Files Coverage Δ
src/Sensor.cc 79.54% <35.71%> (-2.53%) ⬇️
src/ForceTorque.cc 70.96% <70.96%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a91081...d35bac9. Read the comment docs.

@scpeters scpeters merged commit 1014a21 into gazebosim:master Nov 23, 2020
@azeey azeey mentioned this pull request Oct 18, 2021
azeey pushed a commit to azeey/sdformat that referenced this pull request Oct 27, 2021
Signed-off-by: Nick Lamprianidis <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
azeey pushed a commit that referenced this pull request Oct 28, 2021
Signed-off-by: Nick Lamprianidis <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-10/1228/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

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