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

audio on/off sets up pins appropriately for each mode #124

Merged
merged 1 commit into from
Mar 6, 2016

Conversation

joshgoebel
Copy link
Collaborator

No description provided.

@joshgoebel
Copy link
Collaborator Author

@MLXXXp Does this properly deal with your issue?

@MLXXXp
Copy link
Collaborator

MLXXXp commented Mar 3, 2016

It's better to just initialise the pin(s) to outputs and leave them that way. It might result in less power draw than having them be inputs at any time, as discussed in #108 (comment). Also, although the outputs will likely default to LOW, it would be safer to set them that way, just in case. The sooner all this happens the better.

Ideally it should be done in the bootloader, but my second choice would still be in the pinBootProgram[] array. If you still object to that, then the ArduboyAudio class constructor is next best, then finally in ArduboyAudio::begin().

So, move setting the pins to outputs from on() to somewhere earlier (you only have to do it once), and remove setting them back to inputs from off() so that they remain as outputs.

The removal of the SPEAKER_2 defines for the DevKit is good.

@joshgoebel
Copy link
Collaborator Author

It's better to just initialise the pin(s) to outputs and leave them that way.

Currently off() should "quiet" any library just by turning off the pins (which really makes it technically more of a mute) - whether a library knows about us officially or not - off is off. That's why I did this. Turning the pins on multiple times does no harm if someone cycles audio.

Also, although the outputs will likely default to LOW

The hardware will boot with them low, this isn't necessary.

The sooner all this happens the better.

Except it doesn't need to happen at all if audio is not used. I don't care about "might" regarding power savings unless we actually know for sure and put a # on it. :-)

I want to see how the multiple audio stuff pans out and how more sketches use audio and libraries before we'd decide to make this part of the bootPins. Right now I see benefit in on/off working despite the audio library in use. Ie, I'm not sure (yet) that they should default to OUTPUT and I'm not sure they should be left OUTPUT forever. If long-term both those two things turn out to be true then bootPins would make sense.

Sometimes I like your changes I just think they are premature. :)

@MLXXXp
Copy link
Collaborator

MLXXXp commented Mar 3, 2016

Currently off() should "quiet" any library just by turning off the pins (which really makes it technically more of a mute) - whether a library knows about us officially or not - off is off.

OK, I now understand your intent. There may be some issues though.

On a production Arduboy, an Arduboy ignorant audio library that is designed to control two pins for higher volume or can play two tones simultaneously which the Arduboy could mix, would set both pins to outputs, so a "mute on startup, based on EEPROM" wouldn't work.

On a DevKit, any Arudboy ignorant library would set the single pin to an output, so a "mute on startup, based on EEPROM" again wouldn't work.

If you expect the sketch itself to use "off()* as a mute, it would be better off getting the state using enabled() and performing the mute by not calling the audio generation functions and/or using a mute feature (if available) in the audio library itself. This way the pin doesn't have to be changed to an input, which again, might draw less power (better safe than sorry).

@joshgoebel
Copy link
Collaborator Author

so a "mute on startup, based on EEPROM" wouldn't work.

Not necessarily. The instructions might just be to start your 3rd party audio init first THEN call audio.begin() to let the audio system manage the audio preferences.

@MLXXXp
Copy link
Collaborator

MLXXXp commented Mar 3, 2016

So you're saying that all sketches that want to use audio, must use the ArduboyAudio class functions to control it, otherwise they might not get any sound. If that's the way you want to go, then this PR is fine.

@joshgoebel
Copy link
Collaborator Author

So you're saying that all sketches that want to use audio, must use the ArduboyAudio class functions to control it, otherwise they might not get any sound.

I think we should encourage the use of audio as long as that seems possible. :-) Of course any sketch can do what they want, but I think there is value in the platform having centralized and persisted global audio settings.

@MLXXXp
Copy link
Collaborator

MLXXXp commented Mar 3, 2016

Of course any sketch can do what they want

But now we're back to a situation like what issue #108 was originally concerned about:

  • A sketch (possibly through ignorance) doesn't use any ArduboyAudio functions and just uses Arduino tone(). This is working well.
  • A different sketch is loaded which uses audio.off() and audio.saveOnOff() to mute.
  • The first sketch is reloaded.
  • When this sketch starts, arduboy.begin() calls audio.begin() and because the EEPROM setting is "off", both speaker pins are left as inputs.
  • The user is confused as to why they aren't getting any sound.

So even though a sketch can do what it wants, it either has to set the unused speaker pin to an output itself, or call audio.On() to do it.

I think it would be better to take the attitude that if a sketch, or the audio libraries it uses, doesn't make use of the ArduboyAudio API, then it should always make sounds as designed, regardless of the API and EEPROM settings, rather than trying to mute it "under the covers". This means setting the speaker pins both to outputs as soon as possible and leaving them that way. We shouldn't be setting the pins to inputs as a way to mute, unless we add a non-persistent mute via setting to input function to the API, that sketches could use.

If people start complaining that they can't mute this sketch, even using whatever becomes of issue #81, then the original author, or someone else, should be encouraged to update this sketch (possibly by using a different "Arduboy aware" audio library) to be ArduboyAudio API compliant. We could have a tag in the "Official" Game List indicating ArduboyAudio compliant: OK/NO.

@joshgoebel
Copy link
Collaborator Author

So even though a sketch can do what it wants, it either has to set the unused speaker pin to an output itself, or call audio.On() to do it.

Yes, this seems entirely reasonable to me. You either SUPPORT our platform audio (and it's on/off saved state), or you have to do your own thing manually - and the pins defaulting to off encourage simply supporting our audio platform code.

Someone brought up the point of sound in a quiet environment earlier... if we have the ability to set "mute" at the platform level, then we need to do our best (initially) to get sketches to honor that. Once we have on/off toggle code we honestly should also bring the boot noise under the same umbrella for this exact reason - wanting to use the Arduboy in quiet environments.

I think we'll see this works really well in practice... if it doesn't then we can re-review it and change the policy. The exclusive point of audio is to have this platform setting - which I think has value.

If no one wants to honor care about the platform setting then audio long-term could just be a deprecated stub that did nothing... but I think that would be a poor turn of events.

@joshgoebel
Copy link
Collaborator Author

Once there is a system level boot toggle I think all of this will be a lot clearer and the value will be readily apparent.

@MLXXXp
Copy link
Collaborator

MLXXXp commented Mar 4, 2016

I'm perfectly fine with your goal of trying to enforce a global mute, even for sketches that aren't compliant, as long as you recognise, and are comfortable with, the fact that we may have to be ready for, and be willing to assist, developers and users who are wondering why their sketch is making no sound, due to this.

Of course the better we can document this, the less of an issue it will be.

I would hope, also, that whenever this issue arises, we show the developer how to modify their sketch to be Arduboy audio compatible. Or, if it's from using a non-compliant audio library, we look into the feasibility of forking to create an Arduboy compliant version.

@MLXXXp
Copy link
Collaborator

MLXXXp commented Mar 4, 2016

Once there is a system level boot toggle I think all of this will be a lot clearer and the value will be readily apparent.

And so, implementing this should be a relatively high priority, even if, for starters, it's just the "blind" simple method I proposed in issue #81

@joshgoebel
Copy link
Collaborator Author

I would hope, also, that whenever this issue arises,

This is also why begin calls audio.on(). If a developer has turned audio off manually then they should kind of be expecting it to be off. :-) We just need to find out what the EEPROM ships with and make sure that's considered to be "on".

@MLXXXp
Copy link
Collaborator

MLXXXp commented Mar 4, 2016

If it's been left uninitialised, EEPROM will be 0xFF, which would be considered on.

@joshgoebel
Copy link
Collaborator Author

If it's been left uninitialised, EEPROM will be 0xFF, which would be considered on.

Right, but can we trust that shipping production Arduboys are uninitialized?

@joshgoebel
Copy link
Collaborator Author

And so, implementing this should be a relatively high priority, even if, for starters, it's just the "blind" simple method I proposed in issue #81

I was working on this LONG before you were, so if you don't have the time just say so and I'll pull up my work over the weekend and bang out something simple and usable along the lines of what I proposed.

@MLXXXp
Copy link
Collaborator

MLXXXp commented Mar 4, 2016

Go ahead and implement it your way.

@joshgoebel
Copy link
Collaborator Author

It's too bad we can't say "hold down A for settings"... but I"m not sure it's worth the space...

joshgoebel added a commit that referenced this pull request Mar 6, 2016
audio on/off sets up pins appropriately for each mode
@joshgoebel joshgoebel merged commit c2dbb10 into Ar-zz-duboy:master Mar 6, 2016
@joshgoebel joshgoebel deleted the audio_boot_pins branch March 6, 2016 01:10
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.

2 participants