Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
microbit-carlos opened this issue Nov 25, 2020 · 11 comments
Milestone

Comments

@microbit-carlos
Copy link
Contributor

microbit-carlos commented Nov 25, 2020

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:

  • Right now we cannot have any two pins.
    • So in V2 pin=(pin0, pin1) would not work
  • In v1 pin=(pin0, pin1) will throw an error, even though both pins are available there
    • music.play(music.PYTHON, pin=(pin_0,)) will also throw an error in v1 and work on v2
  • This could be confused with the return pin feature in audio and speech for v1
  • In a less important note, is harder to document methods that have different signatures in V1 vs V2
  • The audio and music stop functions also take the pin argument, this should probably mirror the same format as play, which it currently doesn't do
    • This could be updated in MicroPython, is CODAL ready for disconnecting pins from the mixer at any point?

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
Copy link
Contributor Author

@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:

  • Just like the display has pins or the sensors have i2c addresses, these things are not directly configurable, the features are simply part of the board.
  • The speaker should just work when playing anything without users needing to have the notion that a speaker-specific pin is used, or that the play() methods need to configure the speaker pin.
  • Just like the user can still read/write to the sensors via i2c methods (in V1) and use the display pins (if the display is disabled), we should keep possibility to drive the pin_speaker.

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

  • We have to plan for the majority of the users to not read the docs, or not in enough detail
  • We expect the API to be exposed via (all?) editors autocompletion
    • Also a reason why having the same API can be beneficial, as some editors won't know if the user has a V1 or V2
  • So it is important to have an API that that is obvious to use without documentation

Current implementation of the pin argument:

  • The concept of the speaker that "just works" is partially achieved by the default value for the pin arguments
    • A tuple including pin_speaker: play(Sound.HAPPY) == play(Sound.HAPPY, pin=(pin_speaker, pin0))
    • But can also take a single pin for v1 compatibility: play(Sound.HAPPY, pin=pin0)
  • The concept of the speaker being driven by a specific pin is needed if we want to switch pin0 with a different pin
    • This outputs sounds via speaker and pin0: play(music.PYTHON)
    • This outputs sound via pin2 and not the speaker: play(music.PYTHON, pin=pin2)
    • This outputs sound via pin2 and the speaker: play(music.PYTHON, pin=(pin_speaker, pin2))
  • Once we introduce the concept of the pin argument using a tuple there are several questions that are not immediately obvious from the API:
    • How many pins can we use?
    • Why (pin0, pin1) doesn't work?
    • What do I do if I want to use the speaker only?
  • The relationship of when to use speaker.disable() or change the pin argument value is not as clear

Suggested change to the pin argument:

  • The speaker just works when any sound is played
  • The pin argument goes back to taking a single pin
    • This is because the argument in this case means "what pin to output the sound as well"
  • The default value is the same as V1: play(Sound.HAPPY) == play(Sound.HAPPY, pin=pin0)
  • To not use any pins: play(Sound.HAPPY, pin=None)

Common uses cases:

  • User wants to use headphones:
    microbit.speaker.disable()
    music.play(music.PYTHON)
  • Accessory or user wants to use pin0 for something else:
    music.play(music.PYTHON, pin=None)
    • In V1 if an accessory uses pin0 the users would need to route the sound to a different pin as well.

@microbit-mark
Copy link

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
Copy link
Collaborator

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

@microbit-carlos
Copy link
Contributor Author

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
Copy link

microbit-mark commented Dec 16, 2020

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
Copy link

microbit-mark commented Dec 16, 2020

Discussed #30 (comment) 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
Copy link
Contributor Author

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
Copy link

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
Copy link
Collaborator

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

@dpgeorge
Copy link
Collaborator

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

@microbit-carlos
Copy link
Contributor Author

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 bbcmicrobit/micropython#696.

The stop() functionality still to be covered in #31 and how it interfaces with the pin modes and sound coming from other modules in issue #50.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants