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 dependency on clearpath_sensors #128

Closed
wants to merge 1 commit into from
Closed

Conversation

civerachb-cpr
Copy link
Contributor

It looks like we're missing the dependency on clearpath_sensors, which can result in the generator failing if this package isn't already installed on the system.

Resolves clearpathrobotics/clearpath_simulator#66

Should be cherry-picked into Jazzy.

@civerachb-cpr civerachb-cpr requested a review from a team as a code owner December 9, 2024 17:29
@civerachb-cpr civerachb-cpr requested review from mhosmar-cpr and roni-kreinin and removed request for a team December 9, 2024 17:29
Copy link
Contributor

@roni-kreinin roni-kreinin left a comment

Choose a reason for hiding this comment

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

Disregard the approval. clearpath_common should not depend on sensors.

@roni-kreinin
Copy link
Contributor

Simulator should be the one depending on sensors if this is referring to clearpathrobotics/clearpath_simulator#66

@roni-kreinin
Copy link
Contributor

clearpath_sensors contains launch and config file only for physical sensors, not simulated. It should only be installed on physical robots, hence why it is under the clearpath_robot repo. We may need to move some of these common generator components into the robot and gz generators instead.

@civerachb-cpr
Copy link
Contributor Author

This is sounding like a much bigger set of structural changes to the packages. Should I schedule a meeting to go over how we want to tackle this?

My first thought is to just create a new clearpath_sensors_config package inside clearpath_common and have both clearpath_generator_common and clearpath_sensors refer to those config files. We're obviously using the physical sensor configurations to inform the simulation.

@roni-kreinin
Copy link
Contributor

Yes it would be worth talking over.

@civerachb-cpr civerachb-cpr deleted the sensors-depend branch December 11, 2024 16:53
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.

Unable to move the robot
2 participants