memory improvement of the tone.cpp lib #1220

Open
RobTillaart opened this Issue Jan 12, 2013 · 3 comments

6 participants

@RobTillaart

To find the optimal prescaler a formula is repeatedly calculated (9 times). By replacing this expensive formula (long divisions) partly by a precalculated value the size of a sample sketch, and thus the tone lib was reduced by 88 bytes.
Did not test the code but I assume it executes a tiny bit faster.

improved code snippet

void tone(uint8_t _pin, unsigned int frequency, unsigned long duration)
{
  uint8_t prescalarbits = 0b001;
  long toggle_count = 0;
  uint32_t ocr = 0;
  int8_t _timer;

  _timer = toneBegin(_pin);

  if (_timer >= 0)
  {
    uint32_t ocrRaw = F_CPU / frequency / 2;
   ....

and replace F_CPU / frequency / 2 in the rest of the function with ocrRaw.

See forum - http://arduino.cc/forum/index.php/topic,142370.msg1069191.html#msg1069191 -

@ffissore ffissore added the New label Feb 27, 2014
@matthijskooijman matthijskooijman removed the New label Sep 11, 2014
@cmaglie cmaglie was assigned by ffissore Jul 1, 2015
@matthijskooijman

Somewhat related, I just noticed that Tone.cpp defines a lot of variables that are not actually used. For each timer available, it defines timerx_toggle_count and related variables. For timer 1 and 2, these are even defined unconditionally, even when they don't exist (like timer2 on 32u4).

Then, depending on the MCU used, the code below selects just one timer to use, and defines the USE_TIMERx macro accordingly. It seems like previously multiple timers were supported, but I suspect that this was reduced to 1 to allow other timer (interrupts) to be used for other purposes.

The tone() function itself then again contains code again for modifying all supported timers, even the ones that are not being used.

This leads to additional flash usage, but also a bit of memory overhead, since the compiler doesn't seem to always optimize away the unused variables (I noticed this happens when enabling lto).

To fix this, both the declarations of variables as well as the code in tone() should simply adhere to whatever USE_TIMERx macros are defined. This should ensure that only code that is potentially needed will be included.

I've got no time right now to prepare a PR, but still wanted to note this down somewhere in case someone else wants to pick up improving the tone function.

@RobTillaart
@q2dg

Why don't try to replace tone()/notone() functions with similar ones obtained from (very) improved libraries like https://code.google.com/p/arduino-new-tone/ or even https://code.google.com/p/arduino-tone-ac/ ??

On the other hand, I didn't understand why official tone() was stripped from https://code.google.com/p/rogue-code/wiki/ToneLibraryDocumentation ...the ability of using more than one timer or having the list of musical notes already preconfigured are very pleasant things which aren't present in official implementation. It's a pity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment