microbit-foundation / micropython-microbit-v2

Temporary home for MicroPython for micro:bit v2 as we stablise it before pushing upstream
MIT License
43 stars 24 forks source link

Should we remove the addition of tuples in music/audio play() pin argument? #30

Closed microbit-carlos closed 3 years ago

microbit-carlos commented 3 years ago

Right now on V2 these functions can take a tuple for the pin argument:

audio.play(..., pin=(pin_speaker, pin0))
music.play(..., pin=(pin_speaker, pin0))
speech.say(..., pin=(pin_speaker, pin0))

This can be a bit confusing because:

Since we can control sound output on the speaker via pin_speaker.enable() and pin_speaker.disable(), we could revert the pin argument to be the same as V1, and then document that music, speech and audio always route the sound to the built-in speaker as well and how that can be enabled/disabled.

If the user only wanted sound via the speaker, and not any other pin, they can still do audio.play(..., pin=pin_speaker).

Alternatively, microbit.pin_speaker.disable() could become microbit.speaker.disable(), and to only use the speaker, the user could do audio.play(..., pin=None). In this case we would be removing the idea of the speaker being a pin, and becomes something that plays sounds, and then the pin argument is the "additional place to route the sound output into".

Thoughts?

microbit-carlos commented 3 years ago

@jaustin , @microbit-giles, @microbit-mark and I had a chat about this today and one thing that became clear is that it makes sense to move the enable()/disable() methods out of microbit.pin_speaker into a new object instance called microbit.speaker.

An important idea we discussed was to change the concept of the speaker being controlled by a pin and think of it as being a built-in feature:

Another thing to take in consideration is that any issues with the pin argument can be addressed in the documentation, but:

Current implementation of the pin argument:

Suggested change to the pin argument:

Common uses cases:

microbit-mark commented 3 years ago

Does pin_speaker still exist within MicroBitPin in this scenario? And if not, does speaker move into the microbit module? Or do both exist?

martinwork commented 3 years ago

Is there a function to configure the default pin when no pin is specified?

microbit-carlos commented 3 years ago

Does pin_speaker still exist within MicroBitPin in this scenario? And if not, does speaker move into the microbit module? Or do both exist?

Yes, pin_speaker still exists, but without the enable/disable, and now there would be a speaker.enable() and speaker.disable().

Is there a function to configure the default pin when no pin is specified?

No, the default is for v1 compatibility, so if the users want to use a different pin they need to use the pin argument, as with V1.

microbit-mark commented 3 years ago

Should speaker.enable/disable be speaker.on/off in line with MakeCode? We switched to on/off here to be more child friendly.

image

This is also in line with MicroPython radio.on/off and display.on/off. I don't think we have precedent for enable/disable

microbit-mark commented 3 years ago

Discussed https://github.com/microbit-foundation/micropython-microbit-v2/issues/30#issuecomment-745980525 in standup and we should go with on/off for consistency and for translation. For note, MakeCode also uses true/false for the display image so should we also be consistent there?

microbit-carlos commented 3 years ago

I probably wouldn't change a MakeCode block that's been available for years.

For the MicroPython speaker API, we should mirror display with on, off, and is_on.

microbit-giles commented 3 years ago

I think the whole 'led enable' block name would need changing to make sense, which feels like a breaking change to me. It would need to be something like 'set display on/off'. I think we should leave the MakeCode block as it is.

dpgeorge commented 3 years ago

The microbit.speaker object with off and on methods was added in cac1c0cf793e200f3a61ba5b33c7be52aea6c3ad

dpgeorge commented 3 years ago

Audio, music and speech output are now changed to accept only a single pin (or None) as the argument to pin output selection. See 0c4287ca33702f85917bd706067a6ca41a21510f

microbit-carlos commented 3 years ago

We will not implement is_on() as there is little benefit and it's better to keep things lean and simple. I've update the docs in https://github.com/bbcmicrobit/micropython/pull/696.

The stop() functionality still to be covered in https://github.com/microbit-foundation/micropython-microbit-v2/issues/31 and how it interfaces with the pin modes and sound coming from other modules in issue https://github.com/microbit-foundation/micropython-microbit-v2/issues/50.