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

Use ROS 2 clock for sensor triggering #807

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

michalpelka
Copy link
Contributor

@michalpelka michalpelka commented Dec 10, 2024

What does this PR do?

This PR fixes two minor things in sensors triggering:

How was this PR tested?

It was cherry picked against stable and tested in response to #804.
I've used test branch that logs sensor triggers (https://github.com/RobotecAI/o3de-extras/tree/mp/trigger_noted)
And script to visualize : https://gist.github.com/michalpelka/3f2c66f16d20ba714c6389d5cd173032

Without filter:
NoFiltering
With filtering
WithMedianFilter

You can see that triggers fro camera (green dots) are spaced evenly for code using median filter.
Note this is minor issue, since it can cause:

  • too low frequency use case without frame limiter and request for high FPS ROS 2 camera.
  • most simulation are using \clock topic so inconsistency in wall time is not that big deal.

@michalpelka michalpelka marked this pull request as ready for review December 10, 2024 13:22
@michalpelka michalpelka requested review from a team as code owners December 10, 2024 13:22
@byrcolin byrcolin added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Dec 10, 2024
@michalpelka michalpelka added sig/simulation Categorizes an issue or PR as relevant to SIG Simulation and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 11, 2024
Copy link
Contributor

@jhanca-robotecai jhanca-robotecai left a comment

Choose a reason for hiding this comment

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

The code works correctly for me, only minor comments/nitpicks, mostly about the naming. I would strongly suggest keeping it more consistent. I found the following in your codebase:

  • SimulationTime
  • SimTimestamp
  • SimTime
  • SimulationLoopTime

Gems/ROS2/Code/Source/Sensor/Events/PhysicsBasedSource.cpp Outdated Show resolved Hide resolved

AZStd::deque<float> m_simulationLoopTimes;
builtin_interfaces::msg::Time m_lastSimulationTime;
float m_currentMedian = 1.f/60.0f;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
float m_currentMedian = 1.f/60.0f;
float m_currentMedian = 1.0f / 60.0f;

Btw. I would also rename the variable (ROS2SystemComponent->currentMedian?), maybe m_simTimeMedian?

Gems/ROS2/Code/Source/Sensor/Events/TickBasedSource.cpp Outdated Show resolved Hide resolved

#include <ROS2/ROS2Bus.h>
#include <ROS2/Utilities/ROS2Conversions.h>
#include <AzCore/std/numeric.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this header needed?

Gems/ROS2/Code/Source/Sensor/Events/PhysicsBasedSource.cpp Outdated Show resolved Hide resolved
Co-authored-by: Jan Hanca <[email protected]>
Signed-off-by: Michał Pełka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/simulation Categorizes an issue or PR as relevant to SIG Simulation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent way to trigger sensors in ROS2 Gem
3 participants