Simplify the audio functions

[MLXXXp]

As per the discussion here in issue #102.

I wrote:

Are removing the ArduboyAudio subclass, and having audioOn() and audioOff() write EEPROM immediately, still good ideas?

@yyyc514 replied:

I'm still thinking about that, sep issue - lets try not to confuse all sorts of things. That makes more sense if there is a mute. I'm not sure someone turning on/off audio in a game should override the system default that they set using the boot tool. Can you open a separate issue to discuss this?

Copied from my proposal here:

---

For the Arduboy library:

For starters, the ArduboyAudio class is quite small and simple now. I don't think it needs to be a separate sub-class. Let's just move its functions directly into the Arduboy class, adding audio to the beginning of each function. After all, we don't have display or buttons sub-classes. This makes it easier for beginners. They don't have to wonder why they have to use the strange syntax arduboy.audio.on() instead of arduboy.audioOn(). If desired, the function prototypes and code, etc. could still remain in separate audio.h and audio.cpp files (as they are with core), but not as a sub-class.

Next, I'd make audioOn() and audioOff() always set the state in EEPROM, instead of requiring the extra step of calling audioSaveOnOff() to do it. This is another simplification for beginners, and I don't think it causes any problems. There's a chance that a more advanced developer may wish to only temporarily set the on/off state without saving it to EEPROM, but this could be accomplished by making the audioEnabled (formerly _audioenabled) variable public and allow them to directly set it true or false. Also with audioEnabled exposed, there isn't a need for the audioEnabled() function but it could be kept if desired.

I don't think we need audioBegin() (at least as a public user function). Just do what's necessary in the Arduboy constructor or in boot(). We just need to read _EEPROM_AUDIO_ONOFF and set audioEnabled accordingly, and set speaker pin 2 to output low.

[Section about adding tone functions removed]

The new versions of the remaining audio functions:

// Private function, called from the constructor or boot()
void Arduboy::audioBegin()
{
  audioEnabled = EEPROM.read(EEPROM_AUDIO_ON_OFF);
  pinMode(PIN_SPEAKER_2, OUTPUT); // I still think this could just be added to pinBootProgram[]
  digitalWrite(PIN_SPEAKER_2, LOW);
}

void Arduboy::audioOn()
{
  audioEnabled = true;
  EEPROM.update(EEPROM_AUDIO_ON_OFF, true);
}

void Arduboy::audioOff()
{
  audioEnabled = false;
  EEPROM.update(EEPROM_AUDIO_ON_OFF, false);
}

// Optional. User can just use the public audioEnabled variable.
bool Arduboy::audioEnabled() 
{
  return audioEnabled;
}

---

[joshgoebel]

For starters, the ArduboyAudio class is quite small and simple now.

As a class should be, this is NOT a bad thing. Arduboy is honestly too large and complex.

After all, we don't have display or buttons sub-classes.

Perhaps we should. Gamebuino does this and it makes a lot of sense. It's a nice way to divide everything. I think we just got to where we are because of legacy, not sure it's even been given much though.

In the future I'd like to look at making the library more modular, not more monolithic, so I don't think this is a step in the right direction right now.

[joshgoebel]

We've mixed too much into the main library class already, control of hardware, graphics drawing, printing, frame management, etc... and we've taken steps to fix some of that already: ripping out printing.

I'd like to see some alternative modes (text mode, tiled based) developed and look at how those fit in before we decide what belongs or doesn't in Arduboy. A lot of "common" stuff in Arduboy points to either a need for a parent class or breaking out some of that functionality.

Frame stuff is first on my list of potential candidates to break out. It would be applicable to any graphics mode and actually even any game/app across the entire Arduino platform - ie, it's not Arduboy specific in the least.

[MLXXXp]

OK, keeping audio as a subclass is fine by me.

What about my suggestion for having on()/off() immediately modify EEPROM, and other related changes? Few, if any, sketches are using this yet, so if we change it quickly it will probably have little impact in terms of breaking things.

[joshgoebel]

Well, I'm not actually sure that is the desired behavior. If I have a global panel where I change a setting I'm not sure app settings should just overwrite that global setting. Perhaps apps can only mute/unmute.

The behavior can always change later if we declare the "new" behavior is "correct". What's hard is changing the API, not behavior.

[joshgoebel]

The music demo always turns sound on (cause it's a music demo) but that doesn't mean I want my global setting of "off" to be mucked with. Yet it also really doesn't make sense to have a mute music demo. I think this needs some time to see how it will be used.

[MLXXXp]

The new way mainly just eliminates the need for the saveOnOff() function, due to saving being the default action.

Like I've said, if you want a non-saved change, you can write directly to the audioEnabled (or just called enabled if we keep the subclass) variable (which I'm saying make public). This is what the demo would do. It would just set audio.enabled = true to always have sound for the demo but not affect the global EEPROM setting for other sketches.

Another way is to still eliminate saveOnOff(), and keep on() and off() the way they are now, but add onSaved() and offSaved().

I still think begin() should be private and called by the constructor. It's only purpose is to set the proper on()/off() state from EEPROM (and possibly volume in the future), and to set the speaker pins to a safe, usable state. I can't see a reason for the need to delay the begin() to a point further on than when the class is instantiated.

Perhaps apps can only mute/unmute.

I though that was all we were discussing here. If by "only" you mean not including volume, I've already said mute should be separate from volume. Just like it is on PCs and most other things. You don't just set the volume to 0 for a mute. You have a separate mute so that volume is restored directly to the previous level when you unmute. How volume is set, and who can do it, can be discussed separately.

[joshgoebel]

Setting and saving should be discrete options (as they are now), and it's better to turn the audio on/off with the API... not a variable... We may want to turn pins on off, power up/down timers, etc... by having an API we give ourselves that ability... just telling someone to flip a variable you can't do anything in response.

We should probably make the variable private/protected.

I can't see a reason for the need to delay the begin() to a point further on than when the class is instantiated.

This is a good point, although it's forcing the use of more space for sketches that don't need audio and do a custom boot - you're removing the ability to not compile in "on" and "begin". That's minor... but... are you just trying to simplify the init sequence or?

[joshgoebel]

It's already protected, good.

[joshgoebel]

Closing. Audio is not complex as it stands now. On and Off should not have additional side effects other than turning audio on and off. Changing the system level audio state requires two operations on purpose: set, save.

[joshgoebel]

Another example I just thought of - I did the audio stuff while working on my own audio settings panel... you could toggle the audio on/off several times in settings but only save it ONCE (when confirming the settings). Reducing writes to EEPROM is good. Sure I suppose you could use a temporary variable to do the same thing, but if you actually had a sample tune or something playing (could imagine this in a game or if we had a bit more space) then a temporary variable isn't of any help - you really do want to turn the audio on/off but NOT persist the setting. And I've already explain earlier why using API methods are better than direct variable manipulation. It might require two lines of code, but it's still simpler that the operations are discrete.

Believe it or not a lot of thought has gone into some of this stuff with real world use cases. I'd suggest writing a game or app or something and focus on the API indirectly, not directly. That's how I've done most of my API work. APIs don't exist in a vacuum - they only exist to serve the applications being developed. Build an API as you build an app and you're much more likely to end up with a great API that's useful than if you set out to design a "super API" and then later try to build apps with it. To me it feels like a lot of your suggestions are efforts to "make it simpler" in a vacuum rather than borne out of experience (or problems) you've had using the API to get actual work done.

"I've written 5 games/apps now and in every single one I include my own audioOnSave and audioOffSave BECAUSE..." might be a lot more compelling than "what if we had just one line of code instead of two, I think it might be easier".

[MLXXXp]

I'd suggest writing a game or app or something and focus on the API indirectly, not directly.

Believe it or not, that's the approach I took; using my ArduboyLife sketch with direct button control, or games that use a menu for selection such as Team Arg's Shadow Runner, as possible scenarios. For ArduboyLife, I'd want audio to always be disabled on start up and the state never saved to EEPROM. For Shadow Runner, you would probably want the setting saved to EEPROM at the time it was selected from the menu.

Anyway either the exiting API or my proposal both let you accomplish the same things, one way or another, so keeping the existing API is fine.