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

Adds enable_metrics flag to Sensor. #665

Merged
merged 3 commits into from
Aug 17, 2021

Conversation

francocipollone
Copy link
Contributor

Signed-off-by: Franco Cipollone [email protected]

🎉 New feature

Related to gazebosim/gz-sensors#146 (comment)

Summary

<enable_metrics> attribute was added to Sensor.

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 🏰 citadel Ignition Citadel label Aug 12, 2021
@francocipollone francocipollone force-pushed the francocipollone/performance_metrics branch 2 times, most recently from 2b809e9 to c9196da Compare August 12, 2021 04:10
@chapulina chapulina requested a review from scpeters August 12, 2021 18:08
@scpeters
Copy link
Member

I recommend looking at #429 as an example for updating the specification files in the sdf/ folder and adding tests

@francocipollone
Copy link
Contributor Author

I recommend looking at #429 as an example for updating the specification files in the sdf/ folder and adding tests

Thanks, I will check.

@francocipollone
Copy link
Contributor Author

francocipollone commented Aug 16, 2021

Hey @scpeters , some updates:

updating the specification files in the sdf/ folder

I've updated only the 1.7 specification file but I found that in other PRs the specification is updated for several versions.
I didn't find information about which version I should target.

Should I add it to the 1.5 and 1.6 too?

and adding tests

I've added only unit test for the addition of the new attribute.
I realized that there is no integration test involving the sdf file test/sensors.sdf
Should I create the integration test for sensor and test the sensor specification?
EDITED: It is being used at link_dom.cc, I will update it

@francocipollone francocipollone force-pushed the francocipollone/performance_metrics branch 2 times, most recently from 297341d to 3035e0f Compare August 16, 2021 13:27
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.

Thanks, @francocipollone , LGTM!

I didn't find information about which version I should target.

libSDFormat9 supports the 1.7 spec, so I think adding to that is enough.

@chapulina chapulina merged commit 135c8eb into sdf9 Aug 17, 2021
@chapulina chapulina deleted the francocipollone/performance_metrics branch August 17, 2021 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants