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

[pepper_robot] disable odom with param #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Naoki-Kameyama
Copy link

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

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

@mikaelarguedas
Copy link
Member

closing/opening to trigger new travis job

@k-okada
Copy link
Member

k-okada commented Sep 6, 2016

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
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
Member

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

@@ -3,6 +3,7 @@
<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
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
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
Author

Thank you for pointed.

@Naoki-Kameyama
Copy link
Author

Naoki-Kameyama commented Sep 13, 2016

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

@yoneken
Copy link

yoneken commented Sep 21, 2016

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

@Karsten1987
Copy link
Contributor

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

@Karsten1987
Copy link
Contributor

any updates on this?

@suryaambrose
Copy link
Member

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

Otherwise, lgtm

@mikaelarguedas
Copy link
Member

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

@nlyubova
Copy link
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