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

Added config and launch files for raw-mini #752

Closed
wants to merge 15 commits into from

Conversation

flg-vs
Copy link
Contributor

@flg-vs flg-vs commented Feb 28, 2018

related to ipa320/cob_common#250

@@ -0,0 +1,9 @@
<launch>
<node name="rplidarNode" pkg="rplidar_ros" type="rplidarNode" output="screen">
Copy link
Member

Choose a reason for hiding this comment

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

this is a new dependency to rplidar_ros. We should add this to the package.xml and CMakeLists.txt. A requirement for new dependencies is that they have to be released for indigo and kinetic. If not, we cannot add this node to cob_bringup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rplidar_ros is already present in the package.xml file and is available for both indigo and kinetic.

Copy link
Member

Choose a reason for hiding this comment

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

oh sorry, didn't see that.

floweisshardt
floweisshardt previously approved these changes Mar 1, 2018
@@ -0,0 +1,19 @@
## joint_names
joint_names: [fr_caster_rotation_joint, fl_caster_rotation_joint, br_caster_rotation_joint, bl_caster_rotation_joint, fr_caster_r_wheel_joint, fl_caster_r_wheel_joint, br_caster_r_wheel_joint, bl_caster_r_wheel_joint]
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a leftover from raw --> the whole file can be removed until we transition to ros-control

<arg name="robot" value="$(arg robot)"/>
<arg name="config_file" default="$(arg pkg_hardware_config)/robots/$(arg robot)/config/velocity_smoother.yaml"/>
<arg name="raw_cmd_vel_topic" default="velocity_smoother/command"/>
<arg name="smooth_cmd_vel_topic" default="twist_controller/command"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation is inconsistent (here tabs, spaces in other files). unify to spaces since they are already in the majority of the files

Copy link
Member

@fmessmer fmessmer left a comment

Choose a reason for hiding this comment

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

robot should also be added to cob_supported_robots for launch file checks!

@fmessmer
Copy link
Member

fmessmer commented Apr 16, 2018

I'm sorry to say, but in contrary to #752 (comment) all launch files in cob_bringup use TABS for indentation...
please change accordingly

<param name="serial_baudrate" type="int" value="115200"/>
<param name="frame_id" type="string" value="laser_link"/>
<param name="inverted" type="bool" value="false"/>
<param name="angle_compensate" type="bool" value="true"/>
Copy link
Member

Choose a reason for hiding this comment

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

please add one level of indentation for the param lines...they are under node

@@ -0,0 +1,9 @@
<launch>
<node name="rplidarNode" pkg="rplidar_ros" type="rplidarNode" output="screen">
Copy link
Member

Choose a reason for hiding this comment

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

  • is the sensor really called rplidar_mini? or does the mini come from the raw_mini? please name the launch file according to the sensor model name
  • this name is not really conform with ROS-convention...use rplidar_driver instead?
  • also me mainly use the following order pkg, type, name here

@fmessmer
Copy link
Member

fmessmer commented Apr 16, 2018

after fixing the comments above, simulation needs to be tested, too

therefore, the comments in https://github.com/ipa320/cob_common/pull/250 need to be considered

finally, the PRs should be merged "bottom-up", i.e. merge ipa320/cob_supported_robots#17 and https://github.com/ipa320/cob_common/pull/250, re-trigger travis for this PR, fix if needed until all tests pass, then merge...

@fmessmer
Copy link
Member

fmessmer commented Apr 23, 2018

you might also want to consider a simpler naming scheme - as I hear you are planning more than one raw mini...use maybe raw-m1, raw-m2,...for example

or are you planning to only have one config for all of them...than you cannot modify anything on just one raw mini

@fmessmer fmessmer mentioned this pull request Jan 30, 2019
@jabbenseth jabbenseth mentioned this pull request Jan 27, 2020
@fmessmer
Copy link
Member

superseded by #798

@fmessmer fmessmer closed this Jan 30, 2020
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.

4 participants