Skip to content

Conversation

jhanca-robotecai
Copy link

This is my code review of #1 . The main goal was to make the text more formal (RFC style) and to make it easier to read by people less familiar with Simulation Interfaces. The changes are focused around the text - they do not really change the meaning of the RFC.

Additionally, the definitions of all ROS 2 interfaces were removed from this RFC, as they are not relevant. The links are left in the text.

Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
Copy link

@PawelLiberadzki PawelLiberadzki left a comment

Choose a reason for hiding this comment

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

Reasonable propositions, that organize this document. I made few remarks for discussion, and few nitpicks.

| SCOPE_STATE | Move all spawned entities to initial poses cached in `ROS 2 Entities manager`. |
| SCOPE_TIME | New call using `ROS2Bus` |
## SetSimulationState service

Choose a reason for hiding this comment

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

It may be worth noting here or under GetSimulationState, what will be the initial state? According to simulation interfaces RFC simulation starts as PAUSED.

Moreover, I would at least consider, what is before this? I guess the ROS 2 simulation interfaces become available only when simulation is initialized, and the state is first set to PAUSED.

jhanca-robotecai and others added 2 commits March 17, 2025 15:29
Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
Co-authored-by: Michał Pełka <michal.pelka@robotec.ai>
Co-authored-by: Paweł Liberadzki <pawel.liberadzki@robotec.ai>
Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
Signed-off-by: Jan Hanca <jan.hanca@robotec.ai>
@jhanca-robotecai jhanca-robotecai merged commit ea295f1 into rb/sim_interfaces Mar 17, 2025
@jhanca-robotecai jhanca-robotecai deleted the jh/sim_interfaces_changes branch March 17, 2025 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants