Ar-zz-duboy / Arduboy

Core library for the Arduboy.
https://www.arduboy.com
Other
330 stars 96 forks source link

Rename beginNoLogo() and have it free up as much as possible #99

Closed MLXXXp closed 8 years ago

MLXXXp commented 8 years ago

I propose that we rename function beginNoLogo() to something like beginNoFrills() and have it free up even more memory for the sketch, while making things more "future proof".

In addition to the boot logo, we've added a "flashlight" feature within the begin() function. Like the boot logo, the flashlight will use some code space that a large sketch may wish to recover.

I've recently proposed a "system control" feature as issue #81 that, if implemented, would also use up some code. I tried to minimise code impact but @yyyc514 has suggested it include basic visual feedback, which could use a fair bit more code space than my original technique. In the future, there could also be other "frills" added that a developer may wish to suppress, if absolutely necessary, to free up code space for use by the sketch.

We could create a whole bunch of begin...() functions to suppress various features E.g. beginNoLogo(), beginNoFlashlight(), beginNoSysControl(), beginNoLogoAndFlashlight(), beginNoLogoAndSysControl() but the number of functions required to support all combinations is unwieldy and could become more so as features are added.

Instead, why not have just beginNoFrills() which leaves out all of these types of features, both current and future ones if possible. We would then strive to make functions for these features public and callable immediately after beginNoFrills().

So, a sketch using beginNoFrills() would start out with maximum memory available and then could pick and chose which "frills" to add back in, space permitting.

We would strongly recommend that a sketch use begin() unless space became tight. We could provide a commented template, and/or an example sketch, that could be cut and pasted in place of begin():

  beginNoFrills(); // use in place of begin to free up maximum memory

  // Each of the following can be commented out or removed to suppress the
  // feature and free up some memory.
  bootLogo(); // Displays the animated Arduboy logo on start up
  bootFlashlight(); // Turns on the RGB LED bright white if the UP button is held during start up
  bootSysControl(); // Allows control of audio mute and screen brightness if the B button is held during start up  

There could be a problem with features that require they be called before some other required internal code in the begin sequence, but I don't think this is currently the case. I don't believe there's a problem with doing the audio set up before doing the above features.

joshgoebel commented 8 years ago

I don't believe there's a problem with doing the audio set up before doing the above features.

There actually might be, need to figure out why the boot sound never worked on devkits the other day and what that has to do with pinouts. The environment is different after audio is inited, you now have timers running (though they might not be doing much), etc.

I do think you're onto something here though, let me give it a little thought and see if there is any better way to approach it.

MLXXXp commented 8 years ago

There actually might be, need to figure out why the boot sound never worked on devkits the other day and what that has to do with pinouts.

If the audio gets initialised before the boot logo, it may actually help. You can then just use the standard audio functions like a sketch would. A bonus would be that the boot logo would automatically honour the EEPROM_AUDIO_ON_OFF setting.

MLXXXp commented 8 years ago

let me give it a little thought and see if there is any better way to approach it.

I thought of a different way but I don't know if you'd consider it better or worse. It would, however, allow you to run the "frills" functions between two blocks of internal "begin" code, like bootLogo() does now.

We can likely come up with better names for the functions, but: The sketch contains a function that calls the "frills" functions. This function is passed as a pointer to BeginNoFrills(), which can then call it anywhere within its code.

The user sketch:

#include Arduboy.h

Arduboy arduboy;

void setup() {
  arduboy.beginNoFrills(frillsFunctions);
}

void loop() {
}

void frillsFunctions() {
  // Any or all of the following can be commented out or removed to suppress the
  // feature and free up some memory.
  bootLogo(); // Displays the animated Arduboy logo on start up
  bootFlashlight(); // Turns on the RGB LED bright white if the UP button is held during start up
  bootSysControl(); // Allows control of audio mute and screen brightness if the B button is held during start up
}

beginNoFrills() function in Arduboy.cpp:

void Arduboy::beginNoFrills(void (*frillsFuncs)()) {
  boot(); // required

  frillsFuncs();

  // Audio
  tunes.initChannel(PIN_SPEAKER_1);
  tunes.initChannel(PIN_SPEAKER_2);
  audio.begin();
}
MLXXXp commented 8 years ago

A further thought: We wouldn't need a separate beginNoFrills() function. We could just overload begin().

void begin(); // Calls all "frills" functions itself, as it currently does
void begin(void (*frillsFuncs)()); // Does the same as beginNoFrills() did in my previous comment
MLXXXp commented 8 years ago

To make the above more "idiot proof", the frillsFunctions() function could be provided by us as a standalone FrillsFunctions.c file. We would put it in the library extras folder and the developer would be instructed to copy it into their sketch's folder and edit it as desired. The only other thing the developer would have to do would be to use arduboy.begin(frillsFunctions); instead of arduboy.begin();.

MLXXXp commented 8 years ago

Note that in all of the above, I forgot to add the class name in front of the member functions, so the "frills" function calls from the sketch should be:

  // Any or all of the following can be commented out or removed to suppress the
  // feature and free up some memory.
  arduboy.bootLogo(); // Displays the animated Arduboy logo on start up
  arduboy.bootFlashlight(); // Turns on the RGB LED bright white if the UP button is held during start up
  arduboy.bootSysControl(); // Allows control of audio mute and screen brightness if the B button is held during start up

That is, unless we wanted to make all the "frills" functions standalone C functions, outside of the Arduboy class, which could also be done.

joshgoebel commented 8 years ago

We would put it in the library extras folder and the developer would be instructed to copy it into their sketch's folder and edit it as desired.

Is that even a possibility? I'd think both would be found and compiled resulting in dup symbol errors - no?

joshgoebel commented 8 years ago

The only reason you proposed passing the function was the worry that thing may need to be run in the middle of the function, yes?

MLXXXp commented 8 years ago

Is that even a possibility? I'd think both would be found and compiled resulting in dup symbol errors - no?

No, only code residing in or under the src directory is compiled. The content of the extras folder is totally ignored by the IDE.

The only reason you proposed passing the function was the worry that thing may need to be run in the middle of the function, yes?

Well, initially that was the goal, and it does accomplish that. But then I saw the other possible benefits that I mentioned above:

MLXXXp commented 8 years ago

Something else that passing the function as an argument would allow:

If I turned out that different groups of "frills" had to be executed at different spots within begin() we could further overload begin() to accept more than one function pointer

// Include all frills
arduboy.begin();

// Frills that are executed in begin() at one spot
arduboy.begin(frillsFunctions);

// frillsFunctions() is executed as before
// frillsFunctions2() is executed at a different spot in begin()
arduboy.begin(frillsFunctions, frillsFunctions2);
MLXXXp commented 8 years ago

I think a better name for frillsFunctions would be something like startupFeatures.

shogerr commented 8 years ago

startupFeatures :+1:

joshgoebel commented 8 years ago

While we're copying and pasting it seems we could just copy and paste an entire "startup" snippet if the default begin() wasn't acceptable. It feels like we're over-engineering this a bit.

So I'd have begin do most everything by default and then the comments would say "see customized_startup.md to learn how to customize your startup or save flash storage space". Or you could inline it all in the comments.

And then that file would have some short docs and a full boot method:

Arduboy arduboy;

// you will call this instead of arduboy.begin()
void beginCustom() {
  // the minimum
  arduboy.boot();
  // whatever else you want
  arduboy.bootFlashlight();
  arduboy.bootLogo();
  arduboy.bootConfiguration();
  arduboy.bootTunes();
  arduboy.bootAudio();
}

I also find the overloading confusing as the behavior changes COMPLETELY whether you pass a function or not. I think that's counter intuitive so even if we went the overloading route I think there should be two function calls to make it clear what is happening.

But again, I don't think that's necessary.

joshgoebel commented 8 years ago

Actually the original suggestion was honestly the best i think:

beginNoFrills(); // use in place of begin to free up maximum memory
  bootLogo(); // Displays the animated Arduboy logo on start up
  bootFlashlight(); // Turns on the RGB LED bright white if the UP button is held during start up
  bootSysControl(); // Allows control of audio mute and screen brightness if the B button is held during start up  

One call that is required (called begin*, to do init stuff), and then a bunch of optional feature calls. I don't think any more complex of a solution is required here.

I don't think the boot* naming is a requirement though, as long as boot time utils were grouped in the .h file and named reasonably. You might also want to use the flashlight or system controls NOT at boot time, but include them in your sketches, etc.

joshgoebel commented 8 years ago

Along those lines:

https://github.com/Arduboy/Arduboy/pull/101

MLXXXp commented 8 years ago

Actually the original suggestion was honestly the best i think

That's fine with me, since I proposed it. :wink:

As long as we don't foresee ever having any future "optional features" needing to execute somewhere in the middle of beginMinimal(). That's really the main thing that anything past my initial comment was intended to solve.

joshgoebel commented 8 years ago

Closing this as we have a PR: https://github.com/Arduboy/Arduboy/pull/101

Also this function isn't needed since core already exposes boot(). If someone wants to do a frills free boot they should start from just boot(). The PR in question cleans up a few boot-time things and mentions this in the comments.