ev3dev / ev3dev-lang-python

Pure python bindings for ev3dev
MIT License
422 stars 146 forks source link

PLAY_NO_WAIT_FOR_COMPLETE Documentation Ambiguous #740

Open attilavago opened 4 years ago

attilavago commented 4 years ago

I am not very new to Python, but fairly new to this library. Reading through the docs and trying things out, I stumbled upon something that doesn't seem to either make much sense, or work the way I expected. This is part of my code:

if guitarSensor.color_name == 'Blue':
        spkr.play_file('/home/robot/guitar/a.wav', 100, play_type=Sound.PLAY_NO_WAIT_FOR_COMPLETE)
        time = time - 1
    elif guitarSensor.color_name == 'Green':
        spkr.play_file('/home/robot/guitar/b.wav', 100, play_type=Sound.PLAY_NO_WAIT_FOR_COMPLETE)
        time = time - 1

The expectation was that as soon as I select another colour, the previous sound would stop playing and the new one would start. That however, is not the case, and the new sound is not played until the previous one finished. What am I missing? The example in the docs does not illustrate a use-case with PLAY_NO_WAIT_FOR_COMPLETE, which is why I am submitting the issue.

WasabiFan commented 4 years ago

PLAY_NO_WAIT_FOR_COMPLETE specifies that the function should return immediately and leave the sound to play in the background. In other words, it means that the function is "non-blocking" — your code can continue doing other things while the sound is finishing.

If you'd like to immediately force it to stop playing the other sound, you can save the returned value from play_file. This returned value (a Python Popen object) has a terminate method which should cause it to stop playing the sound. So you'd save the return value in some long-lived variable and stop the previous sound before you start a new one.

If that works, how do you think we should improve the docs to make it clearer? Was it mainly the definition of PLAY_NO_WAIT_FOR_COMPLETE that was confusing?

attilavago commented 4 years ago

PLAY_NO_WAIT_FOR_COMPLETE specifies that the function should return immediately and leave the sound to play in the background. In other words, it means that the function is "non-blocking" — your code can continue doing other things while the sound is finishing.

If you'd like to immediately force it to stop playing the other sound, you can save the returned value from play_file. This returned value (a Python Popen object) has a terminate method which should cause it to stop playing the sound. So you'd save the return value in some long-lived variable and stop the previous sound before you start a new one.

If that works, how do you think we should improve the docs to make it clearer? Was it mainly the definition of PLAY_NO_WAIT_FOR_COMPLETE that was confusing?

OK, that's a very good explanation. Thanks a million @WasabiFan! Hugely appreciate it. Will give that a try over the weekend. 👍 🙏

In terms of documentation update, I think what confused me was the fact that the parameter being so close to the audio file, as in literally in the same function made me assume it referred to stopping playing the file as soon as that other function is triggered somewhere else, which is my case. I think just adding that comment you gave me as a Note in the docs, and maybe an example or two (after all this library is being advertised as suitable for kids 10 years+ too by Lego itself) would be incredibly helpful. It's a great library otherwise. Very pleased it exists and is being often updated especially now that Lego launched their awful new version of their visual coding app, which provides only like 20% of the functionality compared to the old one on MacOS 10.15. and this is the only "advanced" alternative out there.

WasabiFan commented 4 years ago

Hey @attilavago! Did you get a chance to test this?

If not I can+will still update the docs, but I wanted to confirm this solved your issue.

attilavago commented 4 years ago

Hey @attilavago! Did you get a chance to test this?

If not I can+will still update the docs, but I wanted to confirm this solved your issue.

Hi @WasabiFan. Apologies for taking my sweet time on getting back to you on this. So, I managed to finally sit down and fiddle around testing your proposed solution. It works. Basically the code in its simplest form looks something like this:

currentlyPlaying = spkr.play_file('/home/robot/a.wav', 100, play_type=Sound.PLAY_NO_WAIT_FOR_COMPLETE)

if colorSensor.color_name == 'Blue':
        currentlyPlaying.terminate()
        currentlyPlaying = spkr.play_file('/home/robot/b.wav', 100, play_type=Sound.PLAY_NO_WAIT_FOR_COMPLETE)

So, summa summarum, good idea, and I think this is also worth putting into the docs. The Popen methods don't seem to be mentioned anywhere and I think for some use-cases this might be very beneficial. Of course, I am also somewhat hindered by Python itself given my JavaScript background, so am less familiar with what's available to me from the language itself.