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 franco.c@ekumenlabs.com

🎉 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 added this to Inbox in Core development via automation Aug 12, 2021
@chapulina chapulina moved this from Inbox to In review in Core development Aug 12, 2021
@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
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
Signed-off-by: Franco Cipollone <franco.c@ekumenlabs.com>
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
Core development automation moved this from In review to Done Aug 17, 2021
@chapulina chapulina moved this from Done to Highlights in Core development Aug 18, 2021
@chapulina chapulina moved this from Highlights to Done in Core development Aug 20, 2021
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
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.

None yet

4 participants