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

Fixed wheel diameter in URDF #230

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Fixed wheel diameter in URDF #230

merged 1 commit into from
Mar 20, 2024

Conversation

JesusSilvaUtrera
Copy link
Contributor

This is just a test pull request to start getting used to the workflow, just changed the value of the wheel diameter to be coherent with the wheel radius in the YAML.

I have reviewed other modules and there is n sense in loading the YAML into the URDF because none of the paramters in it are used in this xacro file, so regarding that this is the only place the wheel_diameter is used it would not be bad to hardcode it.

Copy link
Member

@jballoffet jballoffet left a comment

Choose a reason for hiding this comment

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

Thanks @JesusSilvaUtrera for contributing to Andino!

Please notice that the DCO check is failing because the commit is not signed. See https://github.com/Ekumen-OS/andino/pull/230/checks?check_run_id=22870646833 for instructions on how to fix it.

On the other hand, I would appreciate it if you could use the PR description template, which is useful so as to avoid missing any important information (such as linking this PR to an issue).

Thanks!

@jballoffet
Copy link
Member

On the other hand, how was this tested? Were you able to run the simulation? If so, I would appreciate it if you could attach a screenshot or gif where the changes can be depicted. Thanks!

Signed-off-by: JesusSilvaUtrera <[email protected]>
Copy link
Collaborator

@francocipollone francocipollone left a comment

Choose a reason for hiding this comment

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

LGTM!

@francocipollone
Copy link
Collaborator

On the other hand, how was this tested? Were you able to run the simulation? If so, I would appreciate it if you could attach a screenshot or gif where the changes can be depicted. Thanks!

Adding to this: The change will be only noticed in the odometry calculation and velocity of the wheels. Not visual change here.

@JesusSilvaUtrera
Copy link
Contributor Author

@jballoffet I was able to run the simulation and teleoperate the robot in Gazebo using 'teleop_twist_keyboard', but as this change is minimal and it can be easily appreciated I did not include any evidence, sorry. In future PRs I will ensure to do this, thanks Javi for the feedback!

@JesusSilvaUtrera
Copy link
Contributor Author

@francocipollone I could add the output of an 'echo' in the odometry or velocity topics if you want

@francocipollone
Copy link
Collaborator

@francocipollone I could add the output of an 'echo' in the odometry or velocity topics if you want

Not needed! LGTM for me . Thanks for contributing 😃

Copy link
Member

@jballoffet jballoffet left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @JesusSilvaUtrera for contributing to Andino!

@jballoffet jballoffet merged commit dfa1378 into Ekumen-OS:humble Mar 20, 2024
4 checks passed
@JesusSilvaUtrera JesusSilvaUtrera deleted the jesus/#93_fix_wheel_size branch March 20, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants