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.

Sign up for GitHub

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

Jump to bottom

Stream sound in chunks of 1 sec instead of all at once. fixes #21 #65

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

Stream sound in chunks of 1 sec instead of all at once. fixes #21 #65

wants to merge 5 commits into from

Conversation

anandgokul18
Copy link

Sending the audio file in chunks of 1 sec each instead of at once so that it can be interrupted immediately. Also, setting sleep on the listener to 20 Hertz

audrow
audrow suggested changes Nov 21, 2020
View reviewed changes
Copy link
Contributor

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Thanks for the interest and contribution. Looks pretty good.

I've merged in #63, you should be able to rebase on master so that we can see if this passes CI. Also, you should put the "fixes issue #" in the description (not in the PR title) so that it will automatically link the associated issue. Also, make sure to sign-off so that you pass the DCO test.

It would also be great to add some tests, if you can. There are some examples in cordial_manager.

src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
@audrow audrow added the enhancement New feature or request label Nov 21, 2020
anandgokul18 added a commit to anandgokul18/cordial that referenced this pull request Dec 9, 2020
@anandgokul18
Addressing comments of pull request robotpt#65
f0228a1
anandgokul18 added a commit to anandgokul18/cordial that referenced this pull request Dec 9, 2020
@anandgokul18
Addressing comments of pull request robotpt#65
22d7d4a
@audrow audrow mentioned this pull request Dec 9, 2020
Closed
@audrow
Copy link
Contributor

audrow commented Dec 13, 2020

@anandgokul18, when you've addressed my comments and made the DCO and CodeFactor pass, rerequest a review from me. As for the CI, it seems that our current setup doesn't work for external contributors. I'll look into this, but if you're ready first, we'll just add you as a contributor to the project so that you can run CI.

anandgokul18 added a commit to anandgokul18/cordial that referenced this pull request Dec 16, 2020
@anandgokul18
Addressing comments of pull request robotpt#65
2e28e49 
Signed-off-by: Anand Gokul <anandgokul18@gmail.com>
@anandgokul18 anandgokul18 force-pushed the anandgokul18 branch 2 times, most recently from ab50112 to 695a130 Compare December 16, 2020 12:55
@audrow
Copy link
Contributor

audrow commented Dec 22, 2020

Hey, CI should be fixed by #69. Please try rebasing your commits on the master branch to see if we pass CI. For the full CI to pass on your repository, you'll have to add the Github secrets mentioned in .github/workflows/docker-ci-full.yaml to your Github repository in settings.

audrow
audrow reviewed Dec 24, 2020
View reviewed changes
Copy link
Contributor

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Try rebasing on master so that #69 can be applied and then I'll take another look.

anandgokul18 added a commit to anandgokul18/cordial that referenced this pull request Dec 26, 2020
@anandgokul18
Addressing comments of pull request robotpt#65
284639c 
Signed-off-by: Anand Gokul <anandgokul18@gmail.com>
anandgokul18 added a commit to anandgokul18/cordial that referenced this pull request Dec 26, 2020
@anandgokul18
Addressing comments of pull request robotpt#65
346efcb 
Signed-off-by: Anand Gokul <anandgokul18@gmail.com>
audrow
audrow suggested changes Dec 27, 2020
View reviewed changes
Copy link
Contributor

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Your commit history has one of my commits in it, which makes the diff weird, as a bunch of files from that commit are included in this PR. See if you can figure out how to remove my commit. It probably involves reverting my commit, pulling from this remote's master branch (i.e., https://github.com/robotpt/cordial.git), and then rebasing your work on this this remote's master branch.

If you have questions, feel free to use the same link as before to schedule a meeting together.

@anandgokul18 anandgokul18 reopened this Dec 28, 2020
@anandgokul18
Reverting Commit and Rebasing to make changes
88faa4f 
Signed-off-by: Anand Gokul <anandgokul18@gmail.com>
audrow
audrow suggested changes Dec 28, 2020
View reviewed changes
Copy link
Contributor

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks better. A few more parameter related changes. You should also make a parameter yaml file with good defaults and load it into the launch file. This will help us use this change in the rest of the system.

If you can figure out how, it would be great to include tests or a video on the PR chat of this working.

src/cordial_sound/scripts/sound_listener.py
@@ -29,10 +29,8 @@ def play_sound(data):

p = pyaudio.PyAudio()

"""
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change is necessary - it's good to keep the diff small so it's easy to see what has happened.

Copy link
Author

Choose a reason for hiding this comment

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

This was showing up as an issue in Codefactor. That's why I had to change it

src/cordial_sound/scripts/sound_listener.py Outdated Show resolved Hide resolved
src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
src/cordial_sound/scripts/wav_file_publisher.py Outdated Show resolved Hide resolved
@audrow
Copy link
Contributor

audrow commented Dec 28, 2020

@emilyxzhou, I've asked for a few small changes. Would you take a look and perhaps review and test it out for yourself?

@anandgokul18
changing queue size as 1
44bd29b 
Signed-off-by: Anand Gokul <anandgokul18@gmail.com>
@anandgokul18
Making sleep time and seconds per chunk as paramaters
cd406b0 
Signed-off-by: Anand Gokul <anandgokul18@gmail.com>
@emilyxzhou
Copy link
Contributor

I tested this out on my QT, looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@audrow audrow audrow requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anandgokul18 @audrow @emilyxzhou