Skip to content

[pepper_robot] disable odom with param#26

Open
Naoki-Kameyama wants to merge 1 commit into
ros-naoqi:masterfrom
Naoki-Kameyama:disable_odom_with_param
Open

[pepper_robot] disable odom with param#26
Naoki-Kameyama wants to merge 1 commit into
ros-naoqi:masterfrom
Naoki-Kameyama:disable_odom_with_param

Conversation

@Naoki-Kameyama

Copy link
Copy Markdown

Because I'd like to use Visual SLAM with Pepper.

Pleas merge this PR after ros-naoqi/naoqi_bridge#58

@mikaelarguedas

Copy link
Copy Markdown
Member

closing/opening to trigger new travis job

@k-okada

k-okada commented Sep 6, 2016

Copy link
Copy Markdown
Member

I think we have to release naoqi_bridge
I also think we should set true as default value until someone provide (v)slam code/launch in this repository.

@mikaelarguedas

Copy link
Copy Markdown
Member

yes we'll need to wait for naoqi_bridge to be released indeed.
Agreed for the default value, I think it should be set to false as in the parent PR.
@Naoki-Kameyama could you update your PR with the default value set to true ?

@mikaelarguedas

Copy link
Copy Markdown
Member

thanks @Naoki-Kameyama
Waiting until next naoqi_bridge to squash and merge this

@Naoki-Kameyama
Naoki-Kameyama force-pushed the disable_odom_with_param branch from f233062 to 00bfd61 Compare September 6, 2016 01:28
<arg name="nao_ip" default="$(optenv NAO_IP 127.0.0.1)" />
<arg name="nao_port" default="$(optenv NAO_PORT 9559)" />
<arg name="namespace" default="pepper_robot" />
<arg name="use_odometry" default="True" />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Naoki-Kameyama Is this should be true instead of True?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for pointing it out, it seems that the same comment made by @k-okada a few days ago doesn't show up on this PR so thanks for reposting it.

@Naoki-Kameyama

Copy link
Copy Markdown
Author

Thank you for pointed.

@Naoki-Kameyama

Naoki-Kameyama commented Sep 13, 2016

Copy link
Copy Markdown
Author

This PR is required to release naoqi_bridge ros-naoqi/naoqi_bridge#59.

@yoneken

yoneken commented Sep 21, 2016

Copy link
Copy Markdown

His claim is seems to be satisfied now! ros/rosdistro#12759

@Karsten1987

Copy link
Copy Markdown
Contributor

what about extending this config file and then set the parameter manually once vslam is available?

@Karsten1987

Copy link
Copy Markdown
Contributor

any updates on this?

@suryaambrose

Copy link
Copy Markdown
Member

@Karsten1987 I did not get your point (extending the config file, which is in a different project).

Otherwise, lgtm

@mikaelarguedas

Copy link
Copy Markdown
Member

@nlyubova same thing as #37 (comment), this new argument should be added to naoqi_driver.launch

@nlyubova

Copy link
Copy Markdown
Member

I would agree with @Karsten1987 about the confi file and this is already a parameter in this config file in Naoqi Driver. And here you propose to pass it as a parameter in a launch file to the same Naoqi Driver, so it means that in Naoqi Driver there are will be at least two entries for this parameter (in a launch file and in a config file), so I am not sure if it is a good option or not. @suryaambrose do you think it is ok?

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.

8 participants