microbit-foundation / micropython-microbit-v2

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

In music.play(), pin and speaker selection doesn't work as expected #63

Closed martinwork closed 3 years ago

martinwork commented 3 years ago

I would expect each music.play() to be able to choose pin or speaker or both.

music.play(...,pin=pin0) plays on both pin and speaker.

After music.play(..., pin=pin_speaker), all variations play on the speaker only.

import microbit

from microbit import *
import music

music.play(music.JUMP_UP)
sleep(1000)

music.play(music.JUMP_UP, pin=pin0)
sleep(1000)

music.play(music.JUMP_UP, pin=pin_speaker)
sleep(1000)

music.play(music.JUMP_UP, pin=pin0)
sleep(1000)

music.play(music.JUMP_UP)
sleep(1000)

while True:
    sleep(1000)
jaustin commented 3 years ago

I think this is definitely a bug, but it's a bug caused by using a 'feature' left over from development stages when pin_speaker was going to be more prevalent.

The current design is that the speaker is not a pin, so the pin= argument to play is for EC pins, and the pin_speaker is still there so that you can do other interesting things like driving the speaker with PWM or making your own DAC etc.

The 'planned' way to make sound play on the speaker only is to use pin=None.

import microbit

from microbit import *
import music

#Plays on both (✅)
music.play(music.JUMP_UP)
sleep(1000)

#Plays on both (✅)
music.play(music.JUMP_UP, pin=pin0)
sleep(1000)

#Plays on speaker only (✅)
music.play(music.JUMP_UP, pin=None)
sleep(1000)

#plays on speaker only (✅)
music.play(music.JUMP_UP, pin=pin_speaker) # If you remove this line then all commands below work correctly
sleep(1000)

#plays on speaker only (🐞BUG, should play on both)
music.play(music.JUMP_UP, pin=pin0)
sleep(1000)

#Is silent (🐞BUG, should play on speaker only) 
music.play(music.JUMP_UP, pin=None)
sleep(1000)

#plays on speaker only (🐞BUG, should play on both)
music.play(music.JUMP_UP)
sleep(1000)
martinwork commented 3 years ago

music.play(...,pin=pin0) plays on both pin and speaker.

Is there a separate speaker on/off API?

dpgeorge commented 3 years ago

I don't think it's worth fixing this, ie making music.play(music.JUMP_UP, pin=pin_speaker) work. Instead I'd suggest just making it raise an exception if pin_speaker is passed as a pin argument to play.

jaustin commented 3 years ago

Yes, I think raising the exception is fine. @microbit-mark we should definitely explain somewhere why you can't use pin_speaker.

(We do still and people to be able to do PWM or other 'manual sound-like things' on pin_speaker, which is why we left it exposed.... Is it possible to make music.play() throw an exception but not other PWMy writes?)

microbit-carlos commented 3 years ago

Okay, @dpgeorge in that case let's raise the exception for music/audio/speech 👍

@microbit-mark can we update the docs to indicate that the pin is only mean for pwm stuff? @martinwork lets update the microbit-tests as well.

martinwork commented 3 years ago

Will do @microbit-carlos . I can see the new API for the speaker is microbit.speaker.on / off()

dpgeorge commented 3 years ago

pin_speaker is disallowed in d60c75b646bf4f96f685506c6094114bad9be68f

You can still do pin_speaker.write_analog(128) to get a manual PWM tone.