Simplify PrintNumber to use less memory with no visible impact to performance.

[GoogleCodeExporter]

What change would like to see?
Modify PrintNumber to remove the 'buf'.

Why?
'buf' is allocated as:
   unsigned char buf[8 * sizeof(long)];
That means if a 'long' is only 16 bits, a buffer of 128 bytes is allocated.  Of 
course a 32 bit 'long' allocates a 256 byte buffer.  This is a lot of memory on 
a small embedded processor.

My proposed function is:
void Print::printNumber(unsigned long n, uint8_t base)
{
  unsigned int i = 0;

  if (n == 0) {
    print('0');
    return;
  } 

  while (n > 0) {
    i = n % base;
    n /= base;
  }

  print((char) (i < 10 ? 
                '0' + i :
                'A' + (i - 10);
}

Would this cause any incompatibilities with previous versions?  If so, how can 
these be mitigated?

I do not see how.  It uses less stack/memory not more.  It produces the same 
results.  There are the same number of calls to 'Print'.

Original issue reported on code.google.com by Randall....@gmail.com on 14 Nov 2011 at 8:00

[GoogleCodeExporter]

sizeof() retuns the number of bytes, not bits.  The buffer in printNumber is 32 
bytes, not 256 bytes, because sizeof(long) is 4.

Did you test your proposed implementation?  I gave it a try.  It compliled 
after I adding the missing parens on the last line.  But it does not seem to 
work for numbers larger than 9.

Original comment by paul.sto...@gmail.com on 15 Nov 2011 at 2:33

[GoogleCodeExporter]

You are right, sizeof() returns size in bytes not bits.  That was an error on 
my part.

You are also correct that I did not test it.  I am new to the Arduino.  I have 
had a board for a couple week.  I have only recently attempted to do much 
programming on it.

I was working with the LCD library and did not see the 'Print' method.  Of 
course I found it and while looking through the library I came across the 
PrintNumber function.  I didn't like the looks of it.  I got the feeling that 
the buffer was not necessary.

I want to contribute back to the community.  That is why I made the suggestion 
that I did.  I believe the problem is that the print is not in the loop.  The 
following should correct that:
  while (n > 0) {
    i = n % base;
    n /= base;
    print((char) (i < 10 ? 
                  '0' + i :
                  'A' + (i - 10);  
  }

Again, I have not tested this.  I believe it should be correct but I am not 
familiar enough with the development environment yet.  I hope to contribute 
more in the future.

I apologize for my programming errors.

Original comment by Randall....@gmail.com on 15 Nov 2011 at 4:30

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Please test your code.  

When you do test this latest attempt, I'm confident you'll discover it does 
print all the digits, but in reverse order.

I realize you're only trying to be helpful.  But posting untested code just 
isn't a useful way to contribute.  This only creates more work for the 
developers who maintain this issue list, to read and categorize this bug 
(likely marking it as invalid).  Please, test your code.  By only contributing 
code that's been tested to the best of your ability, you'll save everyone time.

Original comment by paul.sto...@gmail.com on 15 Nov 2011 at 6:35

[GoogleCodeExporter]

I will learn the development environment and hold off on offering enhancement 
until I can test the code I recommend.  Again, I apologize for wasting your 
time.

Original comment by Randall....@gmail.com on 15 Nov 2011 at 7:51

[GoogleCodeExporter]

Thanks for taking the time to look into this, Randall, and to provide a 
suggestion.  As Paul says, it is good to test code before posting here... but 
please don't let this experience discourage you.  It's always great to have 
contributions.  Check out the developers mailing list for more.

Also, the reason the buffer was added was to make the function more efficient 
by potentially using versions of write() that were optimized for buffers.  For 
example, the Ethernet library is more efficient when printing a whole buffer 
instead of a single character at a time.

Original comment by dmel...@gmail.com on 15 Nov 2011 at 10:08

• Changed state: WontFix

[GoogleCodeExporter]

Thank you for your kind comments.  I appreciate the time you took to review my 
fumbling attempts to help.  I also appreciate you perspective on your reasoning 
for using a buffer.  I have experience with C but very limited experience with 
C++ and the full use of inheritance and overriding of methods.

However, if the reason for using a buffer is to optimize the write process I 
would think that you would not want to call print for every character as is 
done in the current code:
  for (; i > 0; i--)
    print((char) (buf[i - 1] < 10 ?
      '0' + buf[i - 1] :
      'A' + buf[i - 1] - 10));

I would think you would want to call print with a full buffer, more like:

void Print::printNumber(unsigned long n, uint8_t base)
{
  unsigned char buf[(8 * sizeof(long))+1]; // Assumes 8-bit chars. 
  unsigned char c;
  unsigned char *ptr;

  if (n == 0) {
    print('0');
    return;
  } 

  ptr = buf[ sizeof(buf) ]; // Point to last character
  *ptr-- = '\0';            // Null terminated string;

  while (n > 0) {
    c = n % base;
    n /= base;
    *ptr-- = c < 10 ? 
             '0' + c : 
             'A' + (c-10);
  }

  ptr++; // Point to the first character of the string
  print(ptr);
}

This is NOT tested code and is ONLY presented as an example of what I was 
thinking.  In this code the number is built into a null terminated string and 
after the string is built the print function is call once with the sting 
instead of for every character.  

Due to my limited knowledge there may be reasons that this APPROACH will not 
work and since the code is untested it is likely that the current code will not 
work.  But again this is not an enhancement request but is simply an attempt to 
better understand.  

If this is better a subject for a mailing list of forum then I understand.  

Original comment by Randall....@gmail.com on 16 Nov 2011 at 5:12

[GoogleCodeExporter]

I have been able to test the code.  It is as follows:

void Print::printNumber(unsigned long n, uint8_t base)
{
  unsigned char buf[(8 * sizeof(long))+1]; // Assumes 8-bit chars.
  unsigned char c, *ptr;

  if (n == 0) {
    print('0');
    return;
  }

  ptr = buf+(sizeof(buf)-1); // Point to last character
  *ptr-- = '\0';             // Null terminated string;

  while (n > 0) {
    c = n % base;
    n /= base;
    *ptr-- = c < 10 ? '0' + c : 'A' + (c-10);
  }

  ptr++; // Point to the first character of the string
  print((const char *)ptr);
}

I'm sure someone can find errors in the code but any remaining errors allude me.

Randall

Original comment by Randall....@gmail.com on 17 Nov 2011 at 4:56

[GoogleCodeExporter]

I should have mentioned this earlier... in addition to testing your code first, 
you should also be working with 1.0-rc2, or even better, a fresh pull from 
github.  By working with 0022 or 0023, you're starting with code that's well 
over a year old.  Much has happened since.  Useful contributions really need to 
start with the latest code, even though what's "released" on the public website 
is older.

For example, here's the printNumber in 1.0-rc2:

size_t Print::printNumber(unsigned long n, uint8_t base) {
  char buf[8 * sizeof(long) + 1]; // Assumes 8-bit chars plus zero byte.
  char *str = &buf[sizeof(buf) - 1];

  *str = '\0';

  // prevent crash if called with base == 1
  if (base < 2) base = 10;

  do {
    unsigned long m = n;
    n /= base;
    char c = m - base * n;
    *--str = c < 10 ? c + '0' : c + 'A' - 10;
  } while(n);

  return write(str);
}

If this sort of thing really intersts you, it might be worth taking a look at 
Teensyduino, which is the add-on for an Arduino-compatible board my company 
makes.  Here's the link.

http://www.pjrc.com/teensy/td_download.html

After you run the installer, you Arduino folder will contain a 
hardware/teensy/cores/teensy folder.  Inside you'll find a different 
implementation of nearly all Arduino functions.  Almost all have been heavily 
optimized.  For example, here's the printNumber:

#if ARDUINO >= 100
size_t Print::printNumber(unsigned long n, uint8_t base, uint8_t sign)
#else
void Print::printNumber(unsigned long n, uint8_t base, uint8_t sign)
#endif
{
        uint8_t buf[8 * sizeof(long) + 1];
        uint8_t digit, i;

        // TODO: make these checks as inline, since base is
        // almost always a constant.  base = 0 (BYTE) should
        // inline as a call directly to write()
        if (base == 0) {
#if ARDUINO >= 100
                return write((uint8_t)n);
#else
                write((uint8_t)n);
                return;
#endif
        } else if (base == 1) {
                base = 10;
        }

        if (n == 0) {
                buf[sizeof(buf) - 1] = '0';
                i = sizeof(buf) - 1;
        } else {
                i = sizeof(buf) - 1;
                while (1) {
                        digit = n % base;
                        buf[i] = ((digit < 10) ? '0' + digit : 'A' + digit - 10);
                        n /= base;
                        if (n == 0) break;
                        i--;
                }
        }
        if (sign) {
                i--;
                buf[i] = '-';
        }
#if ARDUINO >= 100
        return write(buf + i, sizeof(buf) - i);
#else
        write(buf + i, sizeof(buf) - i);
#endif
}

David has said in the past he might be interested to incorporate some of these 
improvements into Arduino.  However, he requires each individual change 
submitted as a separate patch.  Separating every little change, testing 
carefully, submitting each as a patch, and answering questions on each one is a 
time-consuming chore.  I simply do not have time to do that.

Maybe you would?  Or maybe you might discover even more efficient optimizations 
than currently exist in Teensyduino?  If so, I would be very interested to 
incopropate them (Teensyduino is maintained separately and is not directly 
affiliated with Arduino).  Maybe they can get into Arduino too, if you submit 
patches here in a format and manner David will accept?

Since this issue is closed, I won't any more comments here.  The Arduino 
developer mail list is the best place to talk about this sort of thing.  Again, 
testing ideas/code before delivering a message to hundreds of inboxes saves 
everyone a lot of time.

If you play with the Teensyduino code and make improvements, email me directly, 
paul at pjrc dot com.

Happy hacking!  :-)

Original comment by paul.sto...@gmail.com on 17 Nov 2011 at 8:51