Skip to content

[RFC 96] EBus and ROS2 support for sensor configurations #894

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

Conversation

arturkamieniecki
Copy link
Contributor

@arturkamieniecki arturkamieniecki commented Apr 22, 2025

What does this PR do?

This PR enables runtime modification of sensor configuration using EBuses and ROS2 messages.
It add a new toggle to all sensors. If it is enabled, the sensor will create two topics. Get configuration and set configuration. Each of these components will use JSON to communicate.

How was this PR tested?

By modifying the configuration of the components.

@arturkamieniecki arturkamieniecki changed the title Ak/sensor configurations EBus support for sensor configurations Apr 22, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds runtime sensor configuration support via EBuses and introduces new configuration structures across multiple sensor components. Key changes include:

  • Introduction of configuration structs and Bus overrides for GNSS, Contact, and Camera sensor components.
  • Refactoring of ROS2SensorComponentBase to support a configuration template parameter.
  • Updates to include paths and adjustments to Lidar sensor configuration with mutable model lists.

Reviewed Changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Gems/ROS2Sensors/Code/Source/Imu/ImuSensorConfiguration.cpp Updated include path to use ROS2Sensors module.
Gems/ROS2Sensors/Code/Source/GNSS/ROS2GNSSSensorComponent.{h,cpp} Added configuration struct and overrides for GNSS sensor.
Gems/ROS2Sensors/Code/Source/ContactSensor/ROS2ContactSensorComponent.{h,cpp} Introduced configuration struct and interface for the Contact sensor.
Gems/ROS2Sensors/Code/Source/Camera/ROS2CameraSensorComponent{.h,.cpp} Refactored sensor activation and configuration handling; added dedicated configuration method.
Gems/ROS2Sensors/Code/Include/ROS2Sensors/Sensor/ROS2SensorComponentBase.h Updated base component to be templated with a configuration type and integrated ConfigurationBus support.
Gems/ROS2Sensors/Code/Include/ROS2Sensors/Lidar/LidarSensorConfiguration.h Modified include paths and changed m_availableModels from const to mutable.
Gems/ROS2Sensors/Code/Include/ROS2Sensors/Configuration/ConfigurationBus.h Added a new configuration Bus interface.

@byrcolin byrcolin added the sig/simulation Categorizes an issue or PR as relevant to SIG Simulation label Apr 22, 2025
@jhanca-robotecai jhanca-robotecai force-pushed the feature/rfc_96_split_ros2 branch from 6bf2af4 to 3144a50 Compare April 24, 2025 06:47
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
@arturkamieniecki arturkamieniecki force-pushed the ak/SensorConfigurations branch from 918d155 to 0b6463e Compare April 24, 2025 07:49
Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
@arturkamieniecki arturkamieniecki marked this pull request as ready for review April 24, 2025 07:57
@arturkamieniecki arturkamieniecki requested review from a team as code owners April 24, 2025 07:57
@arturkamieniecki arturkamieniecki changed the title EBus support for sensor configurations EBus and ROS2 support for sensor configurations Apr 24, 2025
Copy link
Contributor

@michalpelka michalpelka left a comment

Choose a reason for hiding this comment

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

I think reusing std_msgs/string is not acceptable approach.
I support some internal API for adjusting sensors but ROS 2 messages should be used for sensors' data.

@jhanca-robotecai jhanca-robotecai changed the title EBus and ROS2 support for sensor configurations [RFC 96] EBus and ROS2 support for sensor configurations May 6, 2025
@arturkamieniecki arturkamieniecki force-pushed the ak/SensorConfigurations branch from 46c2726 to 43dd331 Compare May 13, 2025 10:44
jhanca-robotecai added a commit to RobotecAI/o3de-extras that referenced this pull request May 19, 2025
jhanca-robotecai added a commit to RobotecAI/o3de-extras that referenced this pull request May 19, 2025
@jhanca-robotecai
Copy link
Contributor

I reused some code from this PR and did it differently: #914

I created separate sensor configuration buses for each sensor type, i.e.:

  • CameraConfigurationRequestBus
  • ImuConfigurationRequestBus
  • LidarConfigurationRequestBus
  • WheelOdometryConfigurationRequestBus

ContactSensor, GNSS, and Odometry do not have any configuration. An advantage of this approach is a clear separation of the buses. Instead of having SensorConfigurationBus and ConfigurationBus, which are quite difficult to understand, each sensor would have a SensorConfigurationBus to configure basic parameters (available in every sensor) and a dedicated bus for parameters specific to that sensor.

Additionally, it simplifies the integration with ScriptCanvas/Lua if needed and is more O3DE way. Buses have calls to configure the whole sensor at once or to configure single parameters. This solution is much more flexible, as it allows the use of the same pipeline (getting the whole configuration via JSON, using O3DE serialization, and setting the configuration) or to easily create services for changing one parameter only. Finally, it does not require creating fake empty configuration structures just to ensure each sensor can override setters/getters (we have 4 sensors that can be configured and 3 that cannot).

@nick-l-o3de
Copy link
Contributor

Do we want to close this and use 914, or are both required?

@jhanca-robotecai
Copy link
Contributor

Do we want to close this and use 914, or are both required?

This one will be closed in favor of #914 if accepted. I will take care of that after the other one is merged, thanks!

jhanca-robotecai added a commit that referenced this pull request Jun 3, 2025
* Remove SensorHelper from API
* Move interface TypeIDs to a separate header
* Add ImuSensorConfigurationRequestBus
* Add WheelOdometryConfigurationRequestBus
* Add CameraConfigurationRequestBus
* Add LidarConfigurationRequestBus
* Fix camera and imu configuration (reconfigure correctly)
* Add WheelOdometryConfiguration and serialization (test fails) as in (#894)
* Extend LidarConfigurationRequestBus (reconfigure params correctly)
* Fix tests: reshuffle files in ROS2 cmake files
* Port changes from (#894)

Signed-off-by: Jan Hanca <[email protected]>
@jhanca-robotecai
Copy link
Contributor

Closing this PR in favor of #914

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.

7 participants