Skip to content

Add pause/resume control to AudioOut and I2SOut#820

Merged
dhalbert merged 3 commits into
adafruit:masterfrom
tannewt:pause_audio
May 8, 2018
Merged

Add pause/resume control to AudioOut and I2SOut#820
dhalbert merged 3 commits into
adafruit:masterfrom
tannewt:pause_audio

Conversation

@tannewt
Copy link
Copy Markdown
Member

@tannewt tannewt commented May 8, 2018

Fixes #808

Copy link
Copy Markdown
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Tested on CPX with a 13-second wav file. Works great!

There is inconsistency about whether you can call pause() twice in a row vs resume() twice in a row:

>>> a.pause()
>>> a.pause()
>>> a.resume()
>>> a.resume()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: No paused sample

Either both should give an error or it should be ok to call resume() while already playing. Having both be idempotent seems more generous to me: one less state check the user program has to make.

Comment thread shared-bindings/audiobusio/I2SOut.c Outdated
raise_error_if_deinited(common_hal_audiobusio_i2sout_deinited(self));

if (!common_hal_audiobusio_i2sout_get_playing(self)) {
mp_raise_RuntimeError("No sample playing cannot pause");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was going to suggest adding a ":" to make "No sample playing: cannot pause", but how about just "Not playing"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

Comment thread shared-bindings/audioio/AudioOut.c Outdated
raise_error_if_deinited(common_hal_audioio_audioout_deinited(self));

if (!common_hal_audioio_audioout_get_playing(self)) {
mp_raise_RuntimeError("No sample playing cannot pause");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was going to suggest adding a ":" to make "No sample playing: cannot pause", but how about just "Not playing"?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

@dhalbert
Copy link
Copy Markdown
Collaborator

dhalbert commented May 8, 2018

Also if a sample has never started playing, could resume() be the equivalent of play()?

@deshipu
Copy link
Copy Markdown

deshipu commented May 8, 2018

Or even better, only have play().

@dhalbert
Copy link
Copy Markdown
Collaborator

dhalbert commented May 8, 2018

If you just have play(), then you need some way to rewind or seek to restart playing from the beginning.

@dhalbert
Copy link
Copy Markdown
Collaborator

dhalbert commented May 8, 2018

maybe play-stop does a rewind. play-pause-play is like resume. This is the way music player buttons work, right?

@tannewt
Copy link
Copy Markdown
Member Author

tannewt commented May 8, 2018

The new API also has the sample itself provided to play(sample). (Unlike 2.x which had the sample provided in the constructor.) I like resume separate because it doesn't require the sample again and makes it clear that its simply resuming whatever was already going.

@dhalbert
Copy link
Copy Markdown
Collaborator

dhalbert commented May 8, 2018

do you want to regularize pause-pause vs resume-resume?

@tannewt
Copy link
Copy Markdown
Member Author

tannewt commented May 8, 2018

Yup! Just saw that. Added in the latest commit.

Copy link
Copy Markdown
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Retested! Great!

@dhalbert dhalbert merged commit 734596c into adafruit:master May 8, 2018
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.

3 participants