Pin disconnection from the mixer

microbit-carlos commented 3 years ago

Current implementation

Executing stop() from audio/music/speech disconnects the pin from the mixer
When audio/music/speech finish playing their sound they automatically disconnect their pin from the mixer
Issue: This automation will stop other sound-making modules that were playing sound asynchronously
- So a short pitch will stop a long sound expression from finishing

Current proposition

When audio/music/speech finish playing their sound they do not disconnect their pin from the mixer
This allows other sound-making modules to continue playing sound asynchronously
Issue: This means the pins stay locked into "sound mode"
- So if the user want's to use the pin for something else they'll see an exception
  - This won't happen very often, as that means the user has to physically disconnect a speaker from the pin and plug something else
- To unlock the pin for something else they need to call the stop() function from audio/music/speech

Ideal implementation

When audio/music/speech finish playing their sound they let other sound-producing module continue
The pin is automatically unlocked when all sound is finished
- In the current MicroPython implementation this might require:
  - Some additional state to be stored and checked when switching the pin mode
    - Or the state to be cleared via CODAL "sound finished" events
  - Some kind of reference-count-like implementation for sound on the pin objects, to check when switching the pin mode
- @finneyj is this something that MicroPython could check directly from CODAL?

jaustin commented 3 years ago

@finneyj any thoughts on the third block above on CODAL options?

jaustin commented 3 years ago

@finneyj just a quick prod - is there any kind of 'is the mixer currently supposed to be making a sound?' check that we could call after each audio operation to decide whether MicroPython should 'free' the pin? Or an event to hook for 'silence started' on the mixer (where I'm assuming silence means no data from any source, not a source playing a quiet or no sound).

If there isn't, should we build a getter for this?

jaustin commented 3 years ago

...looking at the implementation of mixer, it seems like there's not a ready flag and 'silence' is a candidate for elevating to class level and exposing with a getter, but right now there's nothing. @finneyj is that right? Even just a 'yes' or 'no' so we can decide to do option 2 above for now would be good.

(If you think the general 'is it silent' API is useful and want to implement it fast, though, please shout.)

finneyj commented 3 years ago

Hi @jaustin

yes, we could expose that if it's useful. I'm slightly curious why we're doing it though? e.g. why are we disconnecting and reconnecting from the mixer at all? Is there a reason why we don't just connect each sound generating module to its own mixer channel?

finneyj commented 3 years ago

(p.s. its perfectly legal to just return an empty buffer to the mixer if there's no sound to generate)

finneyj commented 3 years ago

heh - sorry guys, I just realized you're talking about the mixer OUTPUT pin here. For some reason I thought you were talking about the virtual sound input pin. Your suggestions make much more sense now :)

I think @jaustin's suggestions make perfect sense. We're already doing silence detection in the mixer, so it would be trivial to add some new events and APIs for:

An event when the mixer starts generating silence
An event when the mixer starts generating sound
an isSilent() method that returns a boolean value.

It sounds like that would be enough to build what you need for option 3? just let me know if you agree, and i'll add this in on the CODAL side.

dpgeorge commented 3 years ago

We're already doing silence detection in the mixer, so it would be trivial to add some new events and APIs for:
* An event when the mixer starts generating silence

As @jaustin mentioned above, it's not technically silence that we want an event for but rather "no source is generating data anymore", as opposed to "sources may be generating but they are all generating silence".

microbit-carlos commented 3 years ago

@finneyj is silence in the mixer the same as "all the sources are done generating data" or can silence be true if a source is still running but producing "no sound"?

finneyj commented 3 years ago

@microbit-carlos The current semantics are "all inputs are sending me empty buffers"

So technically we would not be classed as silent if a source was actively producing a buffer containing samples (even if those samples were all zeroes). I think this is generally OK though, as those are probably the semantics we want?

microbit-carlos commented 3 years ago

Yeah, that sounds like what we want. While inputs to the mixer generate samples (no matter if they are all 0), then isSilent() is false, and when all inputs are done generating samples then isSilent() returns true.

We're already doing silence detection in the mixer, so it would be trivial to add some new events and APIs for:

An event when the mixer starts generating silence

An event when the mixer starts generating sound

an isSilent() method that returns a boolean value.

So, after each play method in music/audio/speech are done, they can check the mixer isSilent() and only disconnect the pin from the mixer when it returns true.

@dpgeorge does that sound good? @finneyj as you might have to concentrate on other CODAL tasks this is probably something @martinwork can do?

dpgeorge commented 3 years ago

does that sound good?

Yes that sounds good!

finneyj commented 3 years ago

That sounds easy and low risk. I'm afriad this got a bit lost though, so didn't make it into codal-microbit-v2#v0.2.25... sorry

I've raised an issue over there so we catch it for next time: https://github.com/lancaster-university/codal-microbit-v2/issues/90

finneyj commented 3 years ago

p.s. Would it be useful to raise events when the mixer enters/leaves silence? or is the isSilent() method all that's needed?

dpgeorge commented 3 years ago

Would it be useful to raise events when the mixer enters/leaves silence?

Events would be best because then we can act on them as soon as they arrive, instead of polling isSilent().

martinwork commented 3 years ago

PR https://github.com/lancaster-university/codal-microbit-v2/pull/91 has been merged.

dpgeorge commented 3 years ago

Waiting on a new version of codal-microbit-v2 that includes this addition.

microbit-carlos commented 3 years ago

Looks like we can continue work on this and https://github.com/microbit-foundation/micropython-microbit-v2/issues/31 now that the latest tag has been integrated with MicroPython.

dpgeorge commented 3 years ago

Looking into this issue I noticed some things which may change we resolve this issue.

When audio/music/speech finish playing their sound they automatically disconnect their pin from the mixer

That's not actually true. The current code (as of f0e630bb0b03d8c22ec98d131ee63cc0a2524bb9) gives the following behaviour:

>>> import speech, music, microbit
>>> microbit.pin0.get_mode()
'unused'              # pin starts out unused
>>> music.play(music.JUMP_UP)
>>> microbit.pin0.get_mode()
'unused'              # pin is disconnected after the tune finishes
>>> speech.say('hello')
>>> microbit.pin0.get_mode()
'music'                # pin is NOT disconnected after speech finishes
>>> music.play(music.JUMP_UP)
>>> microbit.pin0.get_mode()
'unused'              # tune can play and then disconnects the pin
>>> microbit.audio.play(microbit.Sound.HAPPY)
>>> microbit.pin0.get_mode()
'music'                # pin is NOT disconnected after sound expression finishes
>>> microbit.pin0.write_digital(0)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Pin 0 in music mode

Actually, the important thing is that, after speech/audio run in the above example session, pin0 continues to output a PWM signal with 50% duty cycle. This is making sure the attached speaker is at its "idle" position and ensures there is no clicking on the speaker when the next sound starts.

If the code disconnected pin0 when audio/speech/music finished then the pin would got high or low and possibly lead to clicks for subsequent sounds. IMO the existing behaviour of outputting an "idle" PWM signal of 50% is good behaviour that should remain.

As stated at the top of this issue, the main problem to solve is that:

pins stay locked into "sound mode". So if the user want's to use the pin for something else they'll see an exception

My proposal to fix this would be:

keep pins in "music" mode after audio/music/speech finishes, ie don't automatically disconnect
if the user tries to grab the pin for something else then allow it if the mixer is "silent", otherwise raise an exception

A slight modification on this proposal would be to add a new pin mode called "music silent" which is the state of a pin in music mode but when the output is "idle" (ie PWM of 50% an nothing playing).

dpgeorge commented 3 years ago

@microbit-carlos any thoughts on the above comment?

dpgeorge commented 3 years ago

Decision made during meeting: implement proposal above, ie:

keep pins in "music" mode after audio/music/speech finishes, ie don't automatically disconnect
implement logic at the MicroPython-level, to know when audio/music/speech is not active
if the user tries to grab the pin for something else then allow it if the mixer is "silent", otherwise raise an exception

dpgeorge commented 3 years ago

The above behaviour is implemented by 6790bb511b7966e727c147ff27269121e340435f and bca4dc5690998e8e5de07019a44185fc9b9ea080

Here is a test I used:

# test audio/music/speech in conjunction with pin mode

import microbit, music, speech, time

def try_pin(p):
    print("Waiting for pin, currently in mode {}".format(p.get_mode()))
    attempts = 0
    while True:
        try:
            p.write_digital(0)
            break
        except Exception as er:
            attempts += 1
    print("Finished waiting, took {} attempts, pin now in mode {}".format(attempts, p.get_mode()))
    time.sleep(2)

print("=== Playing audio ===")
microbit.audio.play(microbit.Sound.HELLO)
try_pin(microbit.pin0)

print("=== Playing music ===")
music.play(music.BA_DING)
try_pin(microbit.pin0)

print("=== Playing speech ===")
speech.say('hello')
try_pin(microbit.pin0)

print("=== Playing audio in background ===")
microbit.audio.play(microbit.Sound.HAPPY, wait=0)
try_pin(microbit.pin0)

print("=== Playing music in background ===")
music.play(music.BA_DING, wait=0)
try_pin(microbit.pin0)

print("=== Playing long audio and short music, in background ===")
microbit.audio.play(microbit.Sound.YAWN, wait=0)
music.play(music.BA_DING, wait=0)
try_pin(microbit.pin0)

print("=== Playing short audio and long music, in background ===")
microbit.audio.play(microbit.Sound.HELLO, wait=0)
music.play(music.BADDY, wait=0)
try_pin(microbit.pin0)

microbit-carlos commented 3 years ago

Thanks Damien!

microbit-foundation / micropython-microbit-v2