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

[respeaker_ros] Add publish_multichannel option for publishing multi channel audio #350

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

iory
Copy link
Member

@iory iory commented Jun 11, 2022

What is this?

This PR adds features publishing ~audio_raw and ~speech_audio_raw for publishing multi-channel audio.

Along with that, there is a change to publish audio_common_msgs/AudioInfo.
This change does not affect those who were originally using it on channel 1 because ~audio output does not change.

@708yamaguchi
Copy link
Member

708yamaguchi commented Jun 13, 2022

Thank you for your update!

I think it is desirable to use this feature on real robot (e.g. Fetch) before merging.

Copy link
Member

@tkmtnt7000 tkmtnt7000 left a comment

Choose a reason for hiding this comment

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

I think so, too. We have to confirm that this node works well on real robots before merging.
Anyway, that's a wonderful node. I hope this node to solve the problem around voice recognition. e.g: We have to speak voice commands extremely near the robot.

@iory
Copy link
Member Author

iory commented Jun 13, 2022

Removed ~publish_multichannel option and I modified the node to make publish ~audio_raw and ~speech_audio_raw for publishing multi-channel audio.

This change does not affect those who were originally using it on channel 1 because ~audio output does not change.

@708yamaguchi
Copy link
Member

Thanks.

This PR has many good features, so I would appreciate it

  • to add README about pub/sub topics and rosparams. (I am very sorry that I did not add these information before...)
  • to split the pull request. (Not necessary, but it is a bit difficult for me to check big diffs...)
    For example,
    • PR1: add audio info
    • PR2: add multichannel
    • PR3: add merged playback

@iory
Copy link
Member Author

iory commented Jun 13, 2022

@708yamaguchi
Thanks for your comments.

to add README about pub/sub topics and rosparams. (I am very sorry that I did not add these information before...)

I added the commits to reflect your comments.

to split the pull request. (Not necessary, but it is a bit difficult for me to check big diffs...)
For example,
PR1: add audio info
PR2: add multichannel
PR3: add merged playback

This PR is for multichannel output.
When using multichannel, user does not know the number of channels from the information only for audio. Therefore audio_info is required. (i.e. PR2 depends on PR1).
As the merged playback information has already been obtained when outputting the multichannel, it's clear to publish merged playback in this PR.

For you, I wrote the document, 92477a5 8caae81 383670f
 and wrote the comment in codes.
I'm looking forward to your feedback.

@708yamaguchi
Copy link
Member

708yamaguchi commented Jun 14, 2022

@iory

Thank you for your quick update!
I check this code as much as I can.

In addition, please check the behavior of respeaker_node.py with the following branches.
For fetch, knorth55#23
For PR2, knorth55#24

Then, please check multi channel version.

@iory
Copy link
Member Author

iory commented Jun 14, 2022

Thank you.
I'm testing on pr1040 and fetch15.
708yamaguchi#4
708yamaguchi#3

@708yamaguchi
Copy link
Member

I am very sorry, but I had tested this pull request in fetch, but not in pr2.
This is because I left the following pull request unmerged for a long time.
708yamaguchi#4

At least this PR is working well with fetch, so I will look at PR2 for a few days and if it looks OK, I will approve it.

Copy link
Member

@708yamaguchi 708yamaguchi left a comment

Choose a reason for hiding this comment

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

PR2 has not output any respeaker-related errors for the past three days.
I check that /audio and /speech_to_text seems good.

@iory
Copy link
Member Author

iory commented Aug 11, 2022

Thanks!

Copy link
Member

@k-okada k-okada left a comment

Choose a reason for hiding this comment

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

please proved linke to demo that requres multichannel audio

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants