Skip to content

Comments

Add simulation clock publisher#2

Open
PedroDeSanti wants to merge 7 commits intomainfrom
feature/clock
Open

Add simulation clock publisher#2
PedroDeSanti wants to merge 7 commits intomainfrom
feature/clock

Conversation

@PedroDeSanti
Copy link
Member

Add a publisher for the MuJoCo simulation time (d->time)

Copy link

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

Adds support for publishing the MuJoCo simulation clock to ROS 2 and updates documentation and build files to include the new dependency.

  • Introduce a clock_publisher in Ros2Plugin and publish builtin_interfaces::msg::Time in compute()
  • Update README.md to document the Simulation Clock under published topics and show usage examples
  • Add builtin_interfaces to CMakeLists.txt dependencies

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/ros2_plugin.hpp Added include for builtin_interfaces::msg::Time and clock_publisher member
src/ros2_plugin.cpp Created clock_publisher in create_sensor_publishers and publish time in compute
README.md Expanded published topics section with “Simulation Clock” and example
CMakeLists.txt Added find_package(builtin_interfaces) and listed it as a dependency
Comments suppressed due to low confidence (3)

src/ros2_plugin.cpp:57

  • To integrate with ROS 2’s time synchronization (when use_sim_time is enabled), consider publishing on the global /clock topic or making the clock topic name configurable, so other nodes can pick up simulation time correctly.
this->create_publisher<builtin_interfaces::msg::Time>(this->ros_namespace + "clock", this->qos);

README.md:30

  • The anchor link #️-published-topics may not match GitHub’s autogenerated header ID for ### ⬅️ Published Topics. Update the link to match the actual header (e.g., remove emoji or adjust casing) so it resolves correctly.
-  - [⬅️ Published Topics](#️-published-topics)

CMakeLists.txt:31

  • You’ve added builtin_interfaces to CMake dependencies; ensure the same dependency is declared in package.xml so the package manifest stays in sync and builds reliably.
find_package(builtin_interfaces REQUIRED)

@GabrielCosme
Copy link
Member

Fiz algumas refatorações extras no plugin:
Removi o take do código, já que existiam alguns problemas resultantes da sua implementação:

  • O callback ainda estava sendo chamado, por mais que vazio, mas não havia muita otimização em relação a isso.
  • As mensagens eram removidas da fila, o que não causa tanto problema, já que elas são removidas apenas da fila do subscriber, de forma que a fila do publisher se mantém inalterada.
  • Aparentemente o take pode gerar algum problema em comunicações entre processos diferentes, mas isso também precisa ser investigado melhor.

Em resumo, não acho que faça tanta diferença tirar o take, uma outra opção é desativar os callbacks como explicado aqui. Acho que vale a pena fazer um teste comparando a performance nas duas situações.

Alterei os parâmetros de configuração: Depois de entender melhor as filas e o QoS, percebi que não faz sentido ter uma fila de tamanho diferente de 1, já que o tamanho da fila determina só a parte referente ao plugin, e não afeta as filas do node que está se comunicando com o simulador. Além disso deixei a reliability dos publishers fixada em reliable, já que isso permite suporte a ambas configurações, e mantive a opção do usuário escolher a reliability do subscriber, mas deixando o best_effort como padrão, já que também mantem a compatibilidade parcial com outras configurações: https://docs.ros.org/en/rolling/Concepts/Intermediate/About-Quality-of-Service-Settings.html

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.

2 participants