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

Charles padawan #125

Open
wants to merge 7 commits into
base: kinetic-devel
Choose a base branch
from

Conversation

jlurgo
Copy link
Collaborator

@jlurgo jlurgo commented Jan 17, 2020

No description provided.

@eborghi10 eborghi10 self-requested a review January 20, 2020 11:03
@eborghi10
Copy link
Member

@jlurgo add a description to the PR and change the name of the title.
Also, your branch is outdated and has conflicts with the latest version.

Copy link
Member

@eborghi10 eborghi10 left a comment

Choose a reason for hiding this comment

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

@jlurgo I made some comments. If you have doubts let me know.

<xacro:macro name="dock" params="width">
<link name='back'>
<visual name='visual'>
<origin xyz="0 0 0" rpy="0 0 0" />
Copy link
Member

Choose a reason for hiding this comment

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

Origin tags with all their components in 0 are not necessary since its the default value, so please remove them.

</link>
<link name='base'>
<visual name='visual'>
<origin xyz="0 ${width/2 - width/6} ${-(width * 2/3)/2}" rpy="0 0 0" />
Copy link
Member

Choose a reason for hiding this comment

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

Since rpy=0 0 0, you can use this:

<origin xyz="0 ${width/2 - width/6} ${-(width * 2/3)/2}"/>

<child link="base" />
</joint>
</xacro:macro>
<xacro:dock width="0.15">
Copy link
Member

Choose a reason for hiding this comment

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

Think about this like a library, where you create a function (in this case, the dock macro) and in the same place you use it. That's not correct. What you should do is to create a new file using this macro.

@@ -0,0 +1,13 @@
<launch>
Copy link
Member

Choose a reason for hiding this comment

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

This file is not necessary. Add the dock to the empty world and spawn it with an environment variable DOCK. You will also need to declare the initial pose for each environment.

@@ -1,7 +1,8 @@
<launch>
<arg name="env" default="empty"/>
<arg name="localization" default="$(optenv LOCALIZATION none)"/>
Copy link
Member

Choose a reason for hiding this comment

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

These changes are not related to your PR.

@@ -0,0 +1,80 @@
#include <ros/ros.h>
Copy link
Member

Choose a reason for hiding this comment

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

Is this the plugin that you're using to detect whether the robot is docker or not?

@eborghi10 eborghi10 mentioned this pull request Jun 12, 2020
2 tasks
@eborghi10 eborghi10 added this to the Simulation milestone Jun 12, 2020
@eborghi10 eborghi10 linked an issue Jun 12, 2020 that may be closed by this pull request
2 tasks
@eborghi10 eborghi10 added the enhancement New feature or request label Jun 12, 2020
@eborghi10 eborghi10 marked this pull request as draft June 15, 2020 16:01
@eborghi10 eborghi10 marked this pull request as ready for review August 1, 2020 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Charging dock model needed
2 participants