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 PerformanceSensorMetrics proto message. #172

Merged
merged 2 commits into from
Aug 2, 2021

Conversation

francocipollone
Copy link
Contributor

🎉 New feature

Related to gazebosim/gz-sim#557
Related to gazebosim/gz-sensors#146

Summary

Adds a message to publish performance metrics of the sensors.

/// \brief This message contains information about the performance of
/// a sensor in the world.
/// If the sensor is a camera then it will publish the frame per second (fps).
message PerformanceSensorMetrics
{
  /// \brief Sensor name
  string name                 = 1;

  /// \brief The update rate of the sensor in real time.
  double real_update_rate     = 2;

  /// \brief The update rate of the sensor in simulation time.
  double sim_update_rate      = 3;

  /// \brief The nominal update rate defined to the sensor.
  double nominal_update_rate  = 4;

  /// \brief If the sensor is a camera then this field should be filled
  /// with average fps in real time.
  double fps                  = 5;
}

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

@codecov
Copy link

codecov bot commented Jul 21, 2021

Codecov Report

Merging #172 (6237f9d) into ign-msgs5 (e32a8bc) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           ign-msgs5     #172   +/-   ##
==========================================
  Coverage      84.38%   84.38%           
==========================================
  Files              7        7           
  Lines            807      807           
==========================================
  Hits             681      681           
  Misses           126      126           

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 e32a8bc...6237f9d. Read the comment docs.

Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Are we suppose to fill all the fields when publishing this message? If the answer is no, consider to use StringMsg and Double instead, as we can check if the field has been set when parsing the message.

@francocipollone
Copy link
Contributor Author

Are we suppose to fill all the fields when publishing this message? If the answer is no, consider to use StringMsg and Double instead, as we can check if the field has been set when parsing the message.

Thanks for the suggestion. All but fps probably should always contain information. fps is the one that will be optional given that it will depend on the nature of the sensor.

I will address it, thanks!

Signed-off-by: Franco Cipollone <[email protected]>
@francocipollone francocipollone force-pushed the francocipollone/performance_metrics branch from d2dbc19 to 6237f9d Compare July 26, 2021 12:38
Copy link
Collaborator

@caguero caguero left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@caguero caguero merged commit 47dae0f into ign-msgs5 Aug 2, 2021
@caguero caguero deleted the francocipollone/performance_metrics branch August 2, 2021 17:48
@caguero caguero mentioned this pull request Oct 25, 2021
@peci1
Copy link
Contributor

peci1 commented Dec 22, 2021

What is the difference between fps and real_update_rate? Is it so that fps is averaged over some interval?

@peci1
Copy link
Contributor

peci1 commented Dec 22, 2021

Also, another interesting metric might be how long did the Update() function take. This way, it'd be easy to determine which sensors slow down the simulation the most. It might become a bit less informative in case some rendering pipeline parallelism comes into play, but still I think it'd be nice.

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.

3 participants