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

[WIP] add subscribers/speech.cpp #43

Merged
merged 1 commit into from
Oct 25, 2015
Merged

Conversation

k-okada
Copy link
Member

@k-okada k-okada commented Oct 2, 2015

This is an attempt to re-enable text-to-speech and speech recognition feature, which exists in old naoqi bridge (
https://github.com/ros-naoqi/nao_robot/blob/d88b154d91e785ed7071efb0d7f00c0096cee28e/nao_apps/nodes/nao_speech.py)

This PR only support tts feature (configuration (https://github.com/ros-naoqi/nao_robot/blob/d88b154d91e785ed7071efb0d7f00c0096cee28e/nao_apps/nodes/nao_speech.py#L256) are not included )
Since I'm very new to naoqi_driver package and just want to confirm that I'm on the right track.

@kochigami
Copy link
Contributor

I have a question about the purpose of pepper ROS packages change.
I am a beginner, and I am very sorry for troubling you.

I understood that previous pepper.launch in pepper_bringup was separated into two launch files: one is pepper_full.launch including C++ code, and the other is pepper_full_py.launch including python code.
The final goal is to unify all the codes into C++?, or keep C++ and python codes co-existing and just make packages easy to see?

I also found the following issue.
Issue in nao_robot: moving diagnostic updater for other robot

In addition, I found naoqi_apps package in naoqi_bridge package.
naoqi_apps doesn't contain any launch files including ones in nao_apps package.
I thought naoqi_apps is expected to replace for nao_apps in the case of Pepper, but I don't know if it's right.

I also found the following discussion.
Pull-request discussion in nao_robot: add launch/nao_wheel.launch nodes/nao_wheel.py

@k-okada
Copy link
Member Author

k-okada commented Oct 11, 2015

The final goal is to unify all the codes into C++?, or keep C++ and python codes co-existing and just make packages easy to see

I believe so.

In addition, I found naoqi_apps package in naoqi_bridge package.
naoqi_apps doesn't contain any launch files including ones in nao_apps package.
I thought naoqi_apps is expected to replace for nao_apps in the case of Pepper, but I don't know if it's right.

That's correct. Pepper should not depend on Nao, but NaoQi.

In general, people who develop basic/system stuff did not see all use cases, so what they said 'Done, pleas use this one' will not work. In this case, developer of this package thinks it's finished, but it actually does not work in your application and we need more improvements.
Of course If you think you have developed good demo program and show off in open-campus at somewhere but users may require more features or fixed and you have to improve your code.

@Karsten1987
Copy link
Contributor

@kochigami you are absolutely right, that the current structure might be a bit cumbersome. We initially planned to drop the python support, but keep it up right for the moment. That's why you can either launch the bridge with c++ or python.

The package naoqi_apps was left empty for the moment, since all existing apps (for the time being) were rather hardcoded to NAO robot such as walking or body poses.

In general, we need feedback from the community and that's why all PRs are welcomed here to discuss and steadily improve the functionality.

@kochigami
Copy link
Contributor

Thank you very much for teaching me. I could understand the current structure more.

The current flow is to add all functions (for example speech.cpp) to naoqi_driver.cpp for the efficiency of running Pepper.
naoqi_apps is remained empty because it is expected to contain python codes from nao_apps in the future when they are modified to be executable for Pepper.

Thank you very much again. I am very sorry for my disturbance.

@Karsten1987
Copy link
Contributor

@k-okada What's the status of this initial PR? Roughly looking at the code, this looks good to me.

However, as I mentioned in #46 we could think if this should be part of the naoqi_driver core functionality or can be excluded in any kind of naoqi_apps repo.

@vrabaud
Copy link
Contributor

vrabaud commented Oct 21, 2015

@Karsten1987 , this is C++, so how would you do that ? Create a different node ? This is still basic stuff offered by NAOqi so it could belong here I believe no ?

@k-okada
Copy link
Member Author

k-okada commented Oct 22, 2015

@k-okada What's the status of this initial PR?

Sorry, there're no update on this feature, since I spent most of time on #49

This is still basic stuff offered by NAOqi so it could belong here I believe no ?

This is very interesting question, I thought naoqi_driver should provide all NAOQi API and I think we still missing face recognition, behavior control etc.. But if you think we should put some API to naoqi_driver and some to naoqi_app, I'd like to know criteria for choosing as core API and app API.

@Karsten1987
Copy link
Contributor

In the initial idea, the naoqi driver package was meant to provide 'basic' driver functionality such as TF, joint_states, move(To) etc. Hence the name, all components which drive the robot.

For everything else which could be classified as an app, i'd prefer a separate package. This would also allow a more failsafe execution, saying that if face recognition takes a lot of CPU time or even crashes, the naoqi driver stays unaffected in its scheduler.
But I am open to any discussion here.

@k-okada
Copy link
Member Author

k-okada commented Oct 22, 2015

Oh, ok, I personally likes to put everything into naoqi driver because

  • it's hard to define 'basic' functionality
  • it seems naoqi_driver is designed to read data from the robot only when
    it is required (I look into audio event), so hope face recognition is also
    works in this manner, and i thought naoqi driver was designed to run
    efficiency.
  • I like pepper robot because it provides all high level stuff like face
    and speech recognition (which we have been thought as a high level...) , so
    all "non-basic" (again , non-basic for previous robotics researchers)
    features should be provided as basic function.

But, I'll respect your basic idea and if so, should we put C++ version of
naoqi_app and running with naoqi_driver or are you intended to run
naoqi_driver and python version of naoqi_app nodes?

◉ Kei Okada

On Thu, Oct 22, 2015 at 3:29 PM, Karsten Knese notifications@github.com
wrote:

In the initial idea, the naoqi driver package was meant to provide 'basic'
driver functionality such as TF, joint_states, move(To) etc. Hence the
name, all components which drive the robot.

For everything else which could be classified as an app, i'd prefer a
separate package. This would also allow a more failsafe execution, saying
that if face recognition takes a lot of CPU time or even crashes, the naoqi
driver stays unaffected in its scheduler.
But I am open to any discussion here.


Reply to this email directly or view it on GitHub
#43 (comment)
.

Karsten1987 added a commit that referenced this pull request Oct 25, 2015
[WIP] add subscribers/speech.cpp
@Karsten1987 Karsten1987 merged commit 7107140 into ros-naoqi:master Oct 25, 2015
@k-okada k-okada deleted the add_speech branch November 9, 2015 04:10
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