Rip tunes out of Arduboy lib

[joshgoebel]

Reference: http://community.arduboy.com/t/atomic-puzzle-pack-beta-version/459/22

I think this needs to happen. People want to do different things with audio (and I don't blame them, tunes kind of sucks) and it's impossible because we use ISRs for timer 1 and 3, which a lot of audio stuff also uses. As long as audio.cpp is getting compiled it holds those vectors and no other compiled software can use them... so it's impossible to replace tunes with something else... so that causes people to hack the library and create custom versions of it that then make upgrading to later versions of the library hard (or impossible).

Ripping out tunes would solve the need for this hacking. I propose we split what we've already done into ArduboyTunes and then a program would look something like:

Arduboy arduboy;
ArduboyTunes tunes;

// code
arduboy.audio.begin(); 
tunes.begin(); 
if (arduboy.audio.enabled()) {
  tunes.playSong(...)
}

[joshgoebel]

This single issue would also close 2-3 related issues.

[joshgoebel]

Doing this should allow me to then pretty easily fix Puzzle Pack by replacing tunes with it's own custom SFX library.

[joshgoebel]

@davidperrenoud To do this you'd need to create a new "ArduboyTunes" repository and give me access to that also. Then I'd have everything I needed to split this up. Tunes example would probably move to that repository.

[MLXXXp]

and it's impossible because we use ISRs for timer 1 and 3, which a lot of audio stuff also uses.

Not to mention that it breaks PWM for the RGB LED.

[shogerr]

I think this is a great idea. There is a huge community of people interested in using the Arduboy for making music. Separating out the audio portion from our libs would make it easier to hack on. As long as we provide some solution with the library that is easy to use for new programmers, I think it would be a huge benefit to pull the audio portion out into its own module. We can get a new repo open for this project whenever it is needed.

[joshgoebel]

As long as we provide some solution with the library

The solution would be "pick a library of your choice" and ArduboyTunes would be the crappy choice... with the nicer tracker/4 channels choices being nicer if they can spare the room.

The Arduboy library itself would just manage audio on/off settings, nothing more.

[joshgoebel]

Please make an ArduboyPlaytune repository and give me access.

[joshgoebel]

Ping.

[shogerr]

Is ArduboyAudio a candidate as name? We might as well let the name help explain the library use and keep it simple for users new to the Arduboy library.

[joshgoebel]

It's NOT our library. It's the Playtune library with support for Arduboy. It already has a name. I'm not comfortable stealing their hard work and then just renaming it.

[shogerr]

Sounds good

[joshgoebel]

This is essentially done. How do we want to get it released?

[shogerr]

We do have to think about this a bit. We want to keep it as simple for the user as is possible. All of the solutions I can think of now involve the user having to do multiple steps. It's not totally unreasonable to push it as a separate library, but it adds a step to a process that is already confusing for a decent percentage of the users.

Can we utilize define blocks elegantly to include the source for the ArduboyTunePlayer in the master branch, but not have it excluded unless a pre-processor directive is defined? It would be nice if everyone could get up and running with the Arduboy Library in the one step, and that is, by including the Arduboy library from the arduino index everything just works.

"Power users" can be expected to consult the documentation, but otherwise it's a fair assumption to say a normal user might read one paragraph and start tinkering, and quickly find themselves without audio, and then find themselves frustrated.

[joshgoebel]

No this isn't a define type problem, defines can't help someone using libraries because you can't set defines that affect the library. ArduboyLib compiles however it wants and your sketch can't make it compile differently. To set a define you have to hack the library directly - which is exactly what we are trying to get people to stop doing.

This is going to break things, no way around it - the question is how to best approach that - not how to avoid it.

[joshgoebel]

It seems like the best time to do this would be NOW while as few games as possible are production ready and then get authors to move those games to the new API... then we're golden.

[joshgoebel]

One can't easily use multiple library versions with the IDE, or can you?

[joshgoebel]

Actually the IDE makes it pretty easy to swap versions... so I think you could say these gamea compile with 1.1 and these games compiled with 1.2+. Do this big break now (for print and tunes) and then I think that we're on a decent foundation for the future.

Doing it now the list of game that compile with 1.1 would be short and we'd slowly erradicate them in favor of 1.2.

[joshgoebel]

We do have to think about this a bit. We want to keep it as simple for the user as is possible. All of the solutions I can think of now involve the user having to do multiple steps. It's not totally unreasonable to push it as a separate library, but it adds a step to a process that is already confusing for a decent percentage of the users.

It's not an added step, it's a step. Tunes sucks and a lot of sketches are going to use better stuff - spec once the new stuff JO3Ri it working on is done. So they will have to install a SFX library one way or another - it's just a matter of which one it is. And diff sketches will require diff libraries - that's just how libraries work.

So users wanting to compile from source would have to read the READMEs, make sure they have all the pre-requisite libraries, then compile. The way around this is binary distributables.

[MLXXXp]

I'm sure y'all are expecting another "long winded" response from me, as you've come to know and love, so here it is. :smile:

We want to make things as easy as possible for beginning developers but also separate tunes into it's own library, which makes using it slightly more complicated than before. I think we can make these issues mutually exclusive. Let's separate tunes into it's own library, using PR #106 as a base, but...

A beginner is going to want to add audio to their game but most likely, at first, this would just be "beeps" and "boops", when the ball bounces off the paddle or a projectile annihilates the monster. Or maybe for a three or four note melody at the start or end, or when entering a new level. For this, just having the equivalent of Arduino tone() would be sufficient. (Indeed, it's also sufficient for a good many advanced games, as is the case for some existing ones, ArduBreakout being but one.) So let's build just the equivalent of tone() and noTone() directly into the Arduboy library, with the only difference from the Arduino versions being that a pin doesn't have to be specified, and they will honour the global, non-volatile, EEPROM based mute capability that we've already devised.

For more advanced developers, or beginners who want to learn how to use it, the ArduboyPlaytune library will be available. We don't need any _#define_s or such to try to make it easier. We just document it well enough so developers can use it, just like any other Arduino library, such as those for stepper-motors, temperature sensors, etc.

We would probably want to refrain from expanding the Arduboy library itself beyond providing only tone() and noTone() for audio generation. Anything above that would be the work of external libraries, such as ArduboyPlaytune.

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.

Finally, we add tone() and noTone() functions, which could actually call the Arduino versions:

void Arduboy::tone(unsigned int frequency)
{
  if (audioEnabled) ::tone(PIN_SPEAKER_1, frequency);
}

void Arduboy::tone(unsigned int frequency, unsigned long duration)
{
  if (audioEnabled) ::tone(PIN_SPEAKER_1, frequency, duration);
}

void Arduboy::noTone()
{
  ::noTone(PIN_SPEAKER_1);
}

Of course, we could use our own code, instead of the Arduino tone() and noTone() if it turns out to be necessary or better.

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;
}

For the ArduboyPlaytune library:

Nitpick: Decide whether "tune" is capitalised in camelCase or not (I think not). If the actual class and file names use ArduboyPlaytune, then the repository should be the same, not ArduboyPlayTune

Keep the library as is, except:

It should honour the audio on/off state and remain silent if audio is off. It doesn't have to actually be able to change the state. This can, and should, still be done using Arduboy::audioOn() and Arduboy::audioOff().

The problem is how to make ArduboyPlaytune aware of the current audio state. I'd suggest we just pass Arduboy::audioEnabled, as a pointer, as an ArduboyPlaytune constructor parameter: ArduboyPlaytune tunes(&arduboy.audioEnabled); Yes, instantiation is a bit more complicated for the sketch developer, but as has been said before, that's what documentation is for. And remember, we assume a more advanced developer at this point, because we have easy to use tone functions in the Arduboy library for beginners.

The constructor would save the Arduboy::audioEnabled pointer to its own private pointer variable, for use by all other functions to decide whether to mute or not.

Alternatively we could pass a function pointer, or a pointer to the entire Arduboy class, but I think either of these would just make for more and slower code. ArduboyPlaytune tunes(&arduboy.audioEnabled()); ArduboyPlaytune tunes(&arduboy);

The ArduboyPlaytune library should retain its own version of tone(), in case the one built into Arduboy causes conflicts, now or in the future.

End of sermon. Time for objections.

[joshgoebel]

So let's build just the equivalent of tone() and noTone() directly into the Arduboy library, with the only difference from the Arduino versions being that a pin doesn't have to be specified, and they will honour the global, non-volatile, EEPROM based mute capability that we've already devised.

No, because this requires tone() (or us to rebuild tone with the same interrupts) and then we have a sound library and no way for others to choose a different sound library. Someone who wants to use tone can use the official tone library and pass a speaker pin - or write a wrapper if that's too hard.

We could include a sample wrapper somewhere (no to be compiled) that showed how to use tone with the audio API, but it can't be part of the core library.

The problem is how to make ArduboyPlaytune aware of the current audio state. I'd suggest we just pass Arduboy::audioEnabled, as a pointer, as an ArduboyPlaytune constructor parameter:

Interesting, I've been giving some thought to this... this doesn't allow audio to hook functions though.. I had imagined passing on and off function pointers, but I was going to review this when I reworked the Puzzle game to work with Audio and see what worked best.

The ArduboyPlaytune library should retain its own version of tone(), in case the one built into Arduboy causes conflicts, now or in the future.

Again, it can't. The problem tone causes conflicts is because it uses interrupts for the tones, and we'd cause the same conflicts if we did it ourself.

. 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()

It's not that strange, plus this is really a separate discussion. I'd like to perhaps namespace a few other internals in the future (frame management, etc) so I'm not sure we want to start walking the opposite direction,

[joshgoebel]

We could include a sample wrapper somewhere (no to be compiled) that showed how to use tone with the audio API, but it can't be part of the core library.

Great use for a example perhaps? The nice thing about a tone only example is it doesn't require installing more libraries and it's a part of Arduino proper.

[MLXXXp]

No, because this requires tone() (or us to rebuild tone with the same interrupts) and then we have a sound library and no way for others to choose a different sound library.

If the sketch developer doesn't use Arduboy::tone() or Arduboy::noTone() then they won't be compiled in. The developer will be free to use a different sound library without conflict. It's no different than wrapping Arduino tone() within the sketch itself or not.

The user could still use the rest of the Arduboy library audio functions to control audio on/off.

The ArduboyPlaytune library should retain its own version of tone(), in case the one built into Arduboy causes conflicts, now or in the future.

Again, it can't. The problem tone causes conflicts is because it uses interrupts for the tones, and we'd cause the same conflicts if we did it ourself.

So then are you going to remove tone() from ArduboyPlaytune?

[joshgoebel]

If the sketch developer doesn't use Arduboy::tone() or Arduboy::noTone() then they won't be compiled in. The developer will be free to use a different sound library without conflict.

We have to require Tone to reference those functions, that will result in the ISR getting compiled, ISRs are always compiled and linked and that is what causes the conflicts with other libraries.

It's no different than wrapping Arduino tone() within the sketch itself or not.

It's very different because in this case the sketch calls require, if the sketch doesn't call require Tone is not required and the ISR is never compiled.

So then are you going to remove tone() from ArduboyPlaytune?

I don't plan on doing much with playtune after it's removed. We can't remove tone though because if you use Playtune and want to generate a tone the only way to do so is to have a built in method for it - since the Tone library can't be linked because of the ISR conflict. I think "it can't" we referring to the fact that we can't bundle our own tone with ArduboyLib.

[joshgoebel]

ISRs are the exception to every rule since they are called by the CPU itself, not your compiled code.

[MLXXXp]

We have to require Tone to reference those functions, that will result in the ISR getting compiled, ISRs are always compiled and linked and that is what causes the conflicts with other libraries.

OK, understood. That's why I wrote:

Of course, we could use our own code, instead of the Arduino tone() and noTone() if it turns out to be necessary or better.

Necessary being the key word here.

So we write our own code for Arduboy::tone() and Arduboy::noTone(), and add another function Arduboy::tonesBegin(), which sets up the ISR. If the sketch doesn't call tonesBegin() then the ISR isn't set up (and obviously Arduboy::tone() and Arduboy::noTone() can't be used). Other audio libraries can then be used without conflict. If desired, we could add Arduboy::tonesEnd() to disable the ISR. Is this viable?

The original tone library could possibly be used as a base, or what's in ArduboyPlaytune, or the actual Arduino tone source code.

[joshgoebel]

Of course, we could use our own code, instead of the Arduino tone() and noTone()

Unless tone() is to be a BLOCKING function (i.e. nothing else happens while the tone run), then you have to do it with ISRs, and that means you're right back to the same way tone does it so it's the same as using tone (with all the same disadvantages). One could easily write a blocking tone (provided that microseconds were a good enough measure of time to do frequencies). That's essentially all the boot noises I wrote before were - blocking tones.

Arduboy::tonesBegin(), which sets up the ISR.

You can't do this, the ISR has to be in the vector table and that's setup at compile time (because the vector table exists statically at the beginning of flash memory). The Arduino code that makes it appear like you can do stuff like this really just sets up a static ISR and then uses C glue to build a jump table ofh function pointers... there is no magic way to swap ISRs wholesale across multiple libraries. Inside a library you could build your own proprietary system (like Arduino does), but with multiple libraries trying to latch onto the same ISR vectors C is going to yell loudly and shut you down.

[MLXXXp]

You can't do this, the ISR has to be in the vector table and that's setup at compile time (because the vector table exists statically at the beginning of flash memory).

OK, got it. So I guess the Arduboy library audio support will just be functions for enabling and disabling sound, which is only of use if code that actually generates the audio uses these functions.

We could include a sample wrapper somewhere (no to be compiled) that showed how to use tone with the audio API, but it can't be part of the core library.

For just simple tone() and noTone() functions, this might be the best approach for beginners, or sketches that don't require anything more.

A file containing the sample code could be put somewhere under the extras folder for savekeeping, but for a sketch developer, it would probably be easiest to have them just copy and paste the code directly from the documentation into their sketch. If our online documentation could implement a "get code" feature, like is done with Arduino tutorials, such as near the bottom of this one, it would make it a bit easier. We would just say:

• Click on [Get Code]
• Copy the code to the bottom of your sketch.
• If <name> is something other than arduboy in the Arduboy <name>; line near the top of your sketch, then change the two instances of arduboy.audioEnabled to <name>.audioEnabled
• Functions aboyTone(<frequency>), aboyTone(<frequency>, <duration>) and aboyNoTone() work the same as the Arduino tone() and noTone() functions except you don't have to provide a pin number.

The code would be:

void aboyTone(unsigned int frequency) {
  if (arduboy.audioEnabled) {
    tone(PIN_SPEAKER_1, frequency);
  }
}

void aboyTone(unsigned int frequency, unsigned long duration) {
  if (arduboy.audioEnabled) {
    tone(PIN_SPEAKER_1, frequency, duration);
  }
}

void aboyNoTone() {
  noTone(PIN_SPEAKER_1);
}

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

Regarding thoughts on passing audioOn(), audioOff, etc. as function pointers to an audio library such as ArduboyPlaytunes, I think it would be wiser to have the Arduboy library itself be the only thing controlling on/off. The audio library would just act on the state dynamically, by reading the value at the Arduboy::audioEnabled pointer, that it was passed.

Handling volume control is a bit trickier. It would likely be best if the Arduboy library handled this as well, the same as on/off. However, not knowing the levels supported by the particular audio library, it would have to blindly allow setting values from 0 to 255.

Maybe volume isn't something we want to save in system EEPROM. Just have volume control be volatile and handled by the sketch itself, based on the capabilities of the audio library used. This is where EEPROM management for sketches would come in handy, so each game could individually save it's last volume level, if desired.

[joshgoebel]

could be put somewhere under the extras folder for savekeeping,

Why not just a demo?

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

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?

The audio library would just act on the state dynamically, by reading the value at the Arduboy::audioEnabled pointer, that it was passed.

Exception reading is expensive depending on the audio library... does it read it every time it flips a pin for a frequence and use 50% of the CPU? Does it do it ever 1ms? Every 10ms? Might be easier to have it NEVER poll and just tell it when the audio state changes. I'll know more when I actually TRY this.

Handling volume control is a bit trickier. It would likely be best if the Arduboy library handled this as well, the same as on/off. However, not knowing the levels supported by the particular audio library, it would have to blindly allow setting values from 0 to 255.

I'd like to consider saving some bits in the storage for data checksums (so in the future we could know if there data there was valid or just random junk), but otherwise I agree with this. I'm not sure we need two set bytes for on/off vs volume though.

[on/off] [reserved 3 bits] [volume level 4 bits]...

That's 16 volume levels... 5 bits would give you 32 volume levels... I'm not sure we need to adjust it anymore than that. So volume and on/off are individual settings, but single byte of storage.

[joshgoebel]

The audio library would just act on the state dynamically, by reading the value at the Arduboy::audioEnabled pointer, that it was passed.

You have to realize we're also looking at working with EXISTING audio libraries... that likely don't support passing in a flag. But they may support stop/start audio... or something similar... this is why I say it needs more research. The more stuff we can make easily work with audio the better.

[joshgoebel]

Maybe volume isn't something we want to save in system EEPROM. Just have volume control be volatile and handled by the sketch itself, based on the capabilities of the audio library used.

I think saving it one place is better than many. :-)

[MLXXXp]

could be put somewhere under the extras folder for savekeeping,

Why not just a demo?

In my opinion, anything in the examples folder should compile and load and do something. If it was a working sketch that used the functions, and the section of code that needed to be copied and pasted into your sketch was well delimited and commented, then that would be fine.

[joshgoebel]

Right, it would be a nice example of using abstracted Tone methods with Arduboy. :-) Just a thought.