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

Joint Force-Torque Sensor #144

Merged
merged 19 commits into from
Sep 22, 2021
Merged

Joint Force-Torque Sensor #144

merged 19 commits into from
Sep 22, 2021

Conversation

chawladevansh
Copy link

🎉 New feature

Closes #25

Summary

Joint force-torque sensor implementation.

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Jul 15, 2021
@mjcarroll mjcarroll self-requested a review July 15, 2021 18:43
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

I just noticed that this is a draft, anyhow I just made some comments

include/ignition/sensors/ForceTorqueSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/ForceTorqueSensor.hh Outdated Show resolved Hide resolved
include/ignition/sensors/ForceTorqueSensor.hh Show resolved Hide resolved
src/ForceTorqueSensor.cc Outdated Show resolved Hide resolved
src/ForceTorqueSensor.cc Show resolved Hide resolved
src/ForceTorqueSensor.cc Show resolved Hide resolved
src/ForceTorqueSensor.cc Show resolved Hide resolved
test/integration/force_torque_plugin.cc Outdated Show resolved Hide resolved
test/integration/force_torque_plugin.cc Outdated Show resolved Hide resolved
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 20, 2021
@azeey azeey changed the base branch from ign-sensors5 to main September 22, 2021 03:13
@azeey azeey marked this pull request as ready for review September 22, 2021 03:31
@azeey azeey requested a review from iche033 as a code owner September 22, 2021 03:31
Signed-off-by: Addisu Z. Taddese <[email protected]>
@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #144 (b49a430) into main (fb97d96) will increase coverage by 0.37%.
The diff coverage is 85.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
+ Coverage   75.52%   75.90%   +0.37%     
==========================================
  Files          25       27       +2     
  Lines        2644     2747     +103     
==========================================
+ Hits         1997     2085      +88     
- Misses        647      662      +15     
Impacted Files Coverage Δ
src/ForceTorqueSensor.cc 85.29% <85.29%> (ø)
include/ignition/sensors/ForceTorqueSensor.hh 100.00% <100.00%> (ø)

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 fb97d96...b49a430. Read the comment docs.

@azeey azeey added 🏯 fortress Ignition Fortress and removed 🏢 edifice Ignition Edifice labels Sep 22, 2021
Signed-off-by: Addisu Z. Taddese <[email protected]>
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

DCO is failing, but we can fix that when merging the PR.

include/ignition/sensors/ForceTorqueSensor.hh Show resolved Hide resolved
src/ForceTorqueSensor.cc Outdated Show resolved Hide resolved
test/integration/force_torque.cc Outdated Show resolved Hide resolved
if (frame == "child")
{
EXPECT_EQ((rotChildInSensor.Inverse() * force) + forceNoiseMean,
ignition::msgs::Convert(msg.force()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer to get to the expectations on a test in a different way from the source code's implementation. For example, I think this test doesn't really help us find problems with the implementation:

// Code
int sum(int _a, int _b)
{
  return _a * _b;
}

// Test
EXPECT_EQ(a * b, sum(a, b));

A better test would be:

// Values obtained with a calculator
EXPECT_EQ(3, sum(1, 2));

I don't think the current test needs to be changed. Just a general comment.

@mjcarroll mjcarroll requested a review from ahcorde September 22, 2021 15:28
@mjcarroll mjcarroll merged commit d7f8345 into main Sep 22, 2021
@mjcarroll mjcarroll deleted the force-torque branch September 22, 2021 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joint force-torque sensor
5 participants