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

Add Noise to Slotcar Plugin #118

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add Noise to Slotcar Plugin #118

wants to merge 2 commits into from

Conversation

arjo129
Copy link
Member

@arjo129 arjo129 commented Mar 8, 2024

New feature implementation

Implemented feature

This should help simulate real world scenarios where robots might report noisy location.

To use publish a gz.sim.SensorNoise message to /slotcar/translation_noise.

Note: This feature only works on GZ-SIM

arjo129 added 2 commits March 8, 2024 16:11
This should help simulate real world scenarios where robots might report
noisy location.

To use publish a gz.sim.SensorNoise message to `/slotcar/translation_noise`.

Note: This feature only works on GZ-SIM

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@@ -27,6 +27,8 @@ find_package(rmf_fleet_msgs REQUIRED)
find_package(rmf_building_map_msgs REQUIRED)
find_package(rmf_robot_sim_common REQUIRED)

find_package(ignition-cmake2 REQUIRED)
Copy link
Member Author

Choose a reason for hiding this comment

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

No Idea why I need this here but oh-well. I guess I can move the sdf code into the header and then not need it?

@luca-della-vedova
Copy link
Member

Going forward my preference would be to just stick to gz plugins and avoid the additional layer of complexity added by moving the logic to a common class, after all there won't be a gz_classic implementation soon anyway.

@arjo129
Copy link
Member Author

arjo129 commented Apr 24, 2024

Makes sense. For immediate term though I need to update slotcar common cause most of the logic is there.

@luca-della-vedova
Copy link
Member

I had a typed review but it's gone :|

From the top of my head:

  • There seems to be a callback / subscription for the translation_noise but none for the rotation_noise, can we have one of that as well? Otherwise it seems it might be impossible to set.
  • Can we have also some basic docs in the plugin's README to show how to use this feature, i.e. what CLI command can be used to set the translation and rotation noise?
  • After looking at the code closer I agree that keeping the split is a bit easier now, with the refactor done hopefully we can get rid of the rmf_robot_sim_common package "soon".

The rest looks alright (if I can test it!), there are many ways to approach this (parameter in SDF or set through a service, service that sets for all the models vs one per robot) but they all have their pros and cons and don't really have feelings strong enough to advocate for a different way to do it.

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