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

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

Closed
MLXXXp opened this issue Feb 19, 2016 · 17 comments
Closed

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

MLXXXp opened this issue Feb 19, 2016 · 17 comments

Comments

@MLXXXp
Copy link
Collaborator

MLXXXp commented Feb 19, 2016

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.

@MLXXXp MLXXXp changed the title Rename beginNoLogo and have it free up as much a possible Rename beginNoLogo() and have it free up as much a possible Feb 19, 2016
@MLXXXp MLXXXp changed the title Rename beginNoLogo() and have it free up as much a possible Rename beginNoLogo() and have it free up as much as possible Feb 19, 2016
@joshgoebel
Copy link
Collaborator

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
Copy link
Collaborator Author

MLXXXp commented Feb 19, 2016

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
Copy link
Collaborator Author

MLXXXp commented Feb 19, 2016

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
Copy link
Collaborator Author

MLXXXp commented Feb 20, 2016

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
Copy link
Collaborator Author

MLXXXp commented Feb 20, 2016

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
Copy link
Collaborator Author

MLXXXp commented Feb 20, 2016

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

MLXXXp commented Feb 20, 2016

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:

  • You can just overload begin() instead of needing a separate beginNoFrills() function.
  • By passing the "frills" encapsulating function as an argument to begin(), the library has control of exactly where the group of "frills" functions gets executed, whether at the start, middle or end of begin(). (This was the initial goal.)
  • The list of "frills" calls (or a single function, frillsFunctions, that in turn calls them all) doesn't have to be added to the sketch after the begin() call, where they/(it) could be inadvertently separated by other code in between. Plus, it requires one less step for the sketch developer.
    • Instead of:
      • Use arduboy.beginNoFrills(); instead of arduboy.begin();.
      • Copy in and edit the FrillsFunctions.c file.
      • Add a call to frillsFunctions() immediately after the beginNoFrills() call, and make sure they never get separated or called in the wrong order.
    • It would be just:
      • Add an argument to begin(): arduboy.begin(frillsFunctions);.
      • Copy in and edit the FrillsFunctions.c file.

@MLXXXp
Copy link
Collaborator Author

MLXXXp commented Feb 20, 2016

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
Copy link
Collaborator Author

MLXXXp commented Feb 20, 2016

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

@shogerr
Copy link
Collaborator

shogerr commented Feb 20, 2016

startupFeatures 👍

@joshgoebel
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

Along those lines:

#101

@MLXXXp
Copy link
Collaborator Author

MLXXXp commented Feb 20, 2016

Actually the original suggestion was honestly the best i think

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

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
Copy link
Collaborator

Closing this as we have a PR:
#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants