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

Adds capability for blocking calls to SoundClient's play. #64

Merged
merged 10 commits into from
Nov 13, 2015
Merged

Adds capability for blocking calls to SoundClient's play. #64

merged 10 commits into from
Nov 13, 2015

Conversation

felixduvallet
Copy link
Contributor

The SoundClient can now be asked to wait until the requested sound has finished playing before returning, by making use of @aginika 's actionlib implementation. This removes the need for manual sleeping after a sound request is published. The default (out-of-the-box) behavior is unchanged (SoundClient returns immediately) so this should be transparent to everyone.

I also added an example (soundclient_example.py) that shows how to use the SoundClient in both blocking and non-blocking modes.

Lastly I snuck in some linting (trailing whitespace, new-style python classes), let me know if you object.

@trainman419
Copy link
Contributor

Beautiful, clean and well implemented!

I have one thought about the overall API. As implemented, the user/developer has to choose between all requests being non-blocking, or all requests being blocking when they create their SoundClient.

What do you think about having blocking as an argument to play() and playWave() instead?

@felixduvallet
Copy link
Contributor Author

I think that would also be a very reasonable way to do it, I went for the class-wide setting because it would be:

  1. fewer LOC to change & review: each method of libsoundplay would have to change.
  2. fewer things to remember for the user (changing each call to play()/say()/etc.. vs adding one flag in the constructor): personally this is more important to my use case

One compromise would be to provide the option in both places (SoundClient(blocking=true) and play(blocking=true)/etc, but I am afraid this could get confusing (presumably the play() call would take precedence, but only if the argument is specified explicitly??). The logic would definitely get a little weird.

There would still exist a workaround in this implementation: the user could have two SoundClient() instances: one blocking and one not.

Perhaps someone else can weigh in on this with their use case.

In any case I'd be happy to update this pull request if you feel the blocking argument should belong in the class to play/playWave/say/etc...

@trainman419
Copy link
Contributor

Adding a per-method flag for blocking or non-blocking is definitely more work, but I also think it's clearer.

There are also a number of methods for which a blocking call to sound_play does not make sense; for example when repeating a sound, or when stopping a sound that is already playing. I think this is the most compelling reason to have a blocking flag on each method.

… playing the sound.

Each play/repeat/say/... method can take an option blocking=True|False argument (using **kwargs), which over-rides the class-wide setting.
@felixduvallet
Copy link
Contributor Author

@trainman419 I agree that calling play/repeat with an explicit blocking argument is a good use-case.

I updated the SoundClient to take in an explicit (but optional) blocking=True|False argument for each play call (say/play*/repeat*). This over-rides the class-wide blocking setting (NOTE: I still think it's good to have the option to choose the standard behavior).

I also updated the example to show this behavior in action.

I did not implement the blocking behavior this for stop() since I didn't think it made sense there, but since personally I don't really use the start/stop behavior I may be mistaken. Please advise.

@felixduvallet
Copy link
Contributor Author

@trainman419 friendly ping on this PR.

@trainman419
Copy link
Contributor

Sorry; things have been very busy lately.

I agree that a blocking stop() doesn't make much sense.

This looks great! Thanks for the update and the blocking flag on individual methods, and thanks for updating the docstrings!

trainman419 added a commit that referenced this pull request Nov 13, 2015
Adds capability for blocking calls to SoundClient's play.
@trainman419 trainman419 merged commit 67d4081 into ros-drivers:master Nov 13, 2015
@trainman419
Copy link
Contributor

I've cherry-picked these changes onto the Indigo branch as well.

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.

None yet

2 participants