-
Notifications
You must be signed in to change notification settings - Fork 96
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
Conversation
@MLXXXp Does this properly deal with your issue? |
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. |
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.
The hardware will boot with them low, this isn't necessary.
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. :) |
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). |
Not necessarily. The instructions might just be to start your 3rd party audio init first THEN call |
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. |
I think we should encourage the use of |
But now we're back to a situation like what issue #108 was originally concerned about:
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. |
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. |
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. |
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. |
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 |
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". |
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? |
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. |
Go ahead and implement it your way. |
It's too bad we can't say "hold down A for settings"... but I"m not sure it's worth the space... |
audio on/off sets up pins appropriately for each mode
No description provided.