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

ForceTorque system: write WrenchMeasured to ECM #2494

Merged
merged 22 commits into from
Aug 14, 2024

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Jul 25, 2024

🎉 New feature

Part of #2268 and #2391, requires gazebosim/gz-sensors#449 and targets the branch for #2487

Summary

This adds a new WrenchMeasured component and writes to it on the force-torque sensor entities in the ECM. The ForceTorque sensor configuration is changed to call sensor updates even when the gz-transport topic has no subscribers, so that the latest wrench data will be in the ECM. This also adds Model::ModelCount and Model::ModelByName APIs to gz/sim/Model.hh to support the new test.

Test it

Build from source and run ctest -R INTEGRATION_force_torque_system

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Specifying an integer value in a <gz:system_priority> tag
for a system will control the order in which System PreUpdate
and Update callbacks are executed, with lower values executing
first.

The PriorityPrinter example plugin is added to illustrate
the feature.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Set gz:system_priority to positive value for force_torque
system in example and test worlds.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Currently the system skips the sensor update step when
there are no gz-transport subscribers, but to support
writing to the ECM, the system should always update
when there is new sensor data.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Add ModelByName and ModelCount APIs. These are needed
for the updated ForceTorque test.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
* Alphabetize headers
* Rename test using gz-transport topics
* Add const to some local variables

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters marked this pull request as ready for review July 28, 2024 08:19
@scpeters scpeters changed the title Write WrenchMeasured data to ECM ForceTorque system: write WrenchMeasured to ECM Jul 28, 2024
@scpeters
Copy link
Member Author

gazebosim/gz-sensors#449 has been merged and nightlies rebuilt, rerunning CI now

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
Base automatically changed from scpeters/system_execution_order_ionic to main August 1, 2024 21:05
// note: gz-sensors does its own throttling. Here the check is mainly
// to avoid doing work in the ForceTorquePrivate::Update function
bool needsUpdate = false;
for (const auto &[sensorEntity, sensor] : this->dataPtr->entitySensorMap)
{
if (sensor->NextDataUpdateTime() <= _info.simTime &&
sensor->HasConnections())
if (sensor->NextDataUpdateTime() <= _info.simTime)
Copy link
Contributor

@iche033 iche033 Aug 1, 2024

Choose a reason for hiding this comment

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

does it make sense for users to indicate that they want data to be written to ECM by creating a components::WrenchMeasured component? here we would just check for the presence of that component, e.g.

    if (sensor->NextDataUpdateTime() <= _info.simTime &&
        (sensor->HasConnections() ||
        _ecm.Component<components::WrenchMeasured>(sensorEntity) != nullptr))

Probably not going to make much perf difference for this system since the sensor Update call should be relatively fast and same for writing force torque values to ECM. It would be nice to keep things consistent if we do decide to port this functionality to other sensors, e.g. contact or rendering sensors

Copy link
Member Author

Choose a reason for hiding this comment

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

ooh, I like that. Let me give that a try

Copy link
Member Author

Choose a reason for hiding this comment

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

This allows users to enable writing the sensor
data to the ECM by creating the component.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member Author

scpeters commented Aug 2, 2024

I noticed a python test failing in #2500 and realized I didn't set priority on its test file, so I've done so in ba5408f

I'm guessing it will fail, since the python test implicitly assumes that its on_pre_update_cb will execute after the ForceTorque::PreUpdate, but that is now changed. I think the python test should be updated to account for this ordering, which shouldn't be too hard

The test's PreUpdate callback assumes that it executes
after the ForceTorque::PreUpdate, so just move it to
Update to gurantee it. Also fix a spelling error in
the callback variable names.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member Author

scpeters commented Aug 2, 2024

I noticed a python test failing in #2500 and realized I didn't set priority on its test file, so I've done so in ba5408f

I'm guessing it will fail, since the python test implicitly assumes that its on_pre_update_cb will execute after the ForceTorque::PreUpdate, but that is now changed. I think the python test should be updated to account for this ordering, which shouldn't be too hard

the test should be fixed by 1f76c6a, though it's hard for me to run these tests on my machine, so I'll wait for CI to confirm

Copy link

codecov bot commented Aug 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.07%. Comparing base (098085b) to head (9a7785e).
Report is 75 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2494      +/-   ##
==========================================
+ Coverage   65.95%   66.07%   +0.12%     
==========================================
  Files         327      329       +2     
  Lines       31319    31801     +482     
==========================================
+ Hits        20655    21012     +357     
- Misses      10664    10789     +125     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iche033
Copy link
Contributor

iche033 commented Aug 5, 2024

looks good to me.

I noticed some windows tests were failing. I think they are flaky as I've seen a couple of them fail before. I just triggered another build to double check.

@scpeters scpeters merged commit 2f4d8c3 into main Aug 14, 2024
10 of 11 checks passed
@scpeters scpeters deleted the scpeters/forcetorque_write_ecm branch August 14, 2024 06:29
scpeters added a commit that referenced this pull request Aug 27, 2024
The system priority for the force-torque system was
explicitly set using XML tags in several test worlds
in #2494. These XML tags are no longer needed since #2500
was merged, so remove them now.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters added a commit that referenced this pull request Aug 27, 2024
Backport nested model accessor APIs from #2494.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters added a commit that referenced this pull request Aug 27, 2024
The system priority for the force-torque system was
explicitly set using XML tags in several test worlds
in #2494. These XML tags are no longer needed since #2500
was merged, so remove them now.

Also fix some outdated comments

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
scpeters added a commit that referenced this pull request Aug 27, 2024
* Model.hh: add ModelByName and ModelCount APIs
  Backport nested model accessor APIs from #2494.
* Add WrenchMeasured component
* Physics.cc: fix typo in comment
* sensor_TEST.py: move PreUpdate callback to Update
  The test's PreUpdate callback assumes that it executes
  after the ForceTorque::PreUpdate, so just move it to
  Update to gurantee it. Also fix a spelling error in
  the callback variable names.
* fix spelling in on_post_update_cb
* backport minor fixes to force-torque test
* Include System.hh from some systems, add comment

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
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
Projects
Status: Highlights
Development

Successfully merging this pull request may close these issues.

4 participants