search

gazebosim / gz-sim

Open source robotics simulator. The latest version of Gazebo.
https://gazebosim.org
Apache License 2.0
685 stars 262 forks source link

Publish performance metrics #557

Open chapulina opened 3 years ago

chapulina commented 3 years ago

Performance metrics have been recently added to Gazebo classic, see Gazebo: Tutorial: Performance metrics.

It would be interesting to publish the same metrics from Ignition simulations, so that users can:

chapulina commented 3 years ago

For reference, here's the PR where the feature was implemented in Gazebo classic: https://github.com/osrf/gazebo/pull/2819. The new message should be added to ign-msgs.

And it's important to note that the original PR introduced a deadlock that was fixed in https://github.com/osrf/gazebo/pull/2917. Something to be aware of when implementing the same here.

This PR can target Citadel.

francocipollone commented 3 years ago

Hey, a couple of comments/question:

For reference this is the proto message to be published:

Click to see the proto message description ```proto message PerformanceMetrics { /// \brief This message contains information about the performance of /// each 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 Optional fps data. If the sensor is a camera then this field should be filled /// with average fps in real time. double fps = 4; } /// max_step_size x real_time_update_rate sets an upper bound of /// real_time_factor. If real_time_factor < 1 the simulation is /// slower than real time. double real_time_factor = 1; /// Each sensor in the world will create a PerformanceSensorMetrics /// message publishing information about the performance. repeated PerformanceSensorMetrics sensor = 2; } ```

  1. Who should publish the information? In Gazebo Classic we have a SensorManager class and feels correct to publish from there given that the performance metrics are mainly based on the performance of the sensors. However, in Ignition, the ownership of the sensors isn't centralized.

    a. It seems right to let SimulationRunner be the one that publishes the PerformanceMetrics information, given that it is already publishing WorldStatistics msg at /world/<name>/stats, and besides, if another metric(non related to sensor) is needed to be added to the PerformanceMetric message probably from there we can manage it. Note: I was thinking that If we implement it in the SimulationRunner there is the possibility to extend the WorldStatistics msg with the PerformanceMetrics information instead of having two different topics that are being advertised with "stats"

    b. It is tempting to use the Sensor System to publish this information given that it gathers all the RenderingSensors and probably makes no sense to print information about sensors non-rendering sensors like Altimeter that has a more trivial implementation than RenderingSensors. However, for consistency, I believe that it is a message that we will want to publish even when we don't have the Sensors System loaded and for all the sensors.

  2. How accessing the ignition::sensors::Sensors instances? The Sensor System owns the sensors instances(through a ignition::sensors::Manager instance). It will depend on from where we are publishing the metrics how directly we can get the info:

    • (1a)Publishing from SimulationRunner: There is no easy way to get the Sensors (Probably I am missing something)
    • (1b)Publishing from Sensor System: Sensor system owns them so it is easy to get a pointer to the sensors and call the methods that ignition::sensors::Sensor's API provides to get the necessary information to create the metric message.

  3. Calculating the sensor metrics In Gazebo Classic, the Sensor's interface defines a method called LastMeasurmentTime that allows us, combined with the realTime of the simulation, to calculate the real_update_rate and the sim_update_rate for each sensor. In ignition we have ignition::sensors::Sensor::NextUpdateTime that returns the time of the next generation of data of the sensor, so I haven't deeply analyzed it but probably I could get the metric from there. Otherwise, I will need to extend the interface a bit.

chapulina commented 3 years ago

These are all very good questions!

n Ignition, the ownership of the sensors isn't centralized

One idea that crossed my mind is that maybe each sensor could publish its own metrics. This logic could live in ignition::sensors::Sensors. The downside is that the information would be spread across multiple topics, instead of aggregated in a single one. But on the upside, users can easily subscribe just to the sensor metrics they're interested in and don't need to parse the message to find the sensor they care about.

Would that be easier than doing it from the SimulationRunner or a system?

instead of having two different topics that are being advertised with "stats"

Yeah I agree that this duplication is not good. For this task, I'd be ok completely dropping the real_time_factor field and just publishing the sensor metrics. Then the stats topic continues being the authority on RTF. This would be nice so we don't end up in the messy situation that Gazebo classic is in, with multiple sources of RTF.

francocipollone commented 3 years ago

One idea that crossed my mind is that maybe each sensor could publish its own metrics. This logic could live in ignition::sensors::Sensors. The downside is that the information would be spread across multiple topics, instead of aggregated in a single one. But on the upside, users can easily subscribe just to the sensor metrics they're interested in and don't need to parse the message to find the sensor they care about.

Would that be easier than doing it from the SimulationRunner or a system?

Yes that's would be easier. ~I am just thinking that from Sensor we don't know about simTime, so we wouldn't be able to get both the sim_update_rate metric, only real_update_rate. Am I right?~

francocipollone commented 3 years ago

The common::time that sensors::Sensor::Update() method is receiving corresponds to the simulation time.

https://github.com/ignitionrobotics/ign-sensors/blob/ca0dab3e513ccf24e9f60e2d4ae1ec82fcc37091/src/Sensor.cc#L243-L267

Given that this variable comes from this called(In the case of the Sensors System)

https://github.com/ignitionrobotics/ign-gazebo/blob/7cc8c095db61f45f5677372006d0e304fe4b6515/src/systems/sensors/Sensors.cc#L264

So, having said that, given that we receive the simTime we could obtain the simUpdateRate of the sensor and in order to get the realUpdateRate we could just use std::chrono::steady_clock::now() calls to calculate the real time between updates.

This all could be implemented within sensors::Sensor

Unless I am missing something I think that we have a clear path to follow.

One last thing. What about adding also to the metrics message the information about the "nominal" update rate of the sensor?

https://github.com/ignitionrobotics/ign-sensors/blob/ca0dab3e513ccf24e9f60e2d4ae1ec82fcc37091/include/ignition/sensors/Sensor.hh#L118

chapulina commented 3 years ago

What about adding also to the metrics message the information about the "nominal" update rate of the sensor?

I like that idea :+1:

I think that we have a clear path to follow.

Great, thanks!

francocipollone commented 3 years ago

I like that idea +1

:smiley:

Some updates: I've made progress here:

The only thing left is related to the fps field of the message. This field makes sense when the sensor is a CameraSensor.

CameraSensor::RenderingCamera() method returns a pointer to rendering::Camera and from there I could call the rendering::Camera::AvgFPS method.

But RenderingCamera() isn't in the Sensor's API so from Sensor I can't get that information.

chapulina commented 3 years ago

One idea would be to make the PublishMetrics function virtual, so sensors like cameras could implement their own function.

Another thing that stands out to me is that it would be nice to be able to disable the metrics publishing. For now we can just add the API to ign-sensors, and in the future there could be a way to set it from SDF or ign-gazebo's CLI.

francocipollone commented 3 years ago

One idea would be to make the PublishMetrics function virtual, so sensors like cameras could implement their own function.

Another thing that stands out to me is that it would be nice to be able to disable the metrics publishing. For now we can just add the API to ign-sensors, and in the future there could be a way to set it from SDF or ign-gazebo's CLI.

Thanks for the suggestions! :+1:

francocipollone commented 3 years ago

I thought that rendering::Camera already has an AvgFPS method but I'd got confused with gazebo classic, my mistake.

ignition::rendering::Camera doesn't provide api to get the average fps from rendering. Adding this method would imply modifying:

@chapulina If you consider it is ok I could go for it.

chapulina commented 3 years ago

Ah I see. Since we're short on time for this task, how about we leave FPS as future work and just focus on update rate for now?

francocipollone commented 3 years ago

Ah I see. Since we're short on time for this task, how about we leave FPS as future work and just focus on update rate for now?

Great, sounds reasonable.

I leave below some next steps based on things discussed in the issue: