Is this C function written in poor form?

Question

char byte_to_ascii(char value_to_convert, volatile char *converted_value) {

 if (value_to_convert < 10) {
  return (value_to_convert + 48);
 } else {
  char a = value_to_convert / 10;
  double x = fmod((double)value_to_convert, 10.0);
  char b = (char)x;
  a = a + 48;
  b = b + 48;
  *converted_value = a;
  *(converted_value+1) = b;
  return 0;
 }
}

The purpose of this function is to take an unsigned char value of 0 through 99 and return either it's ascii equivalent in the case it is 0-9 or manipulate a small global character array that can be referenced from the calling code following function completion.

I ask this question because two compilers from the same vendor interpret this code in different ways.

This code was written as a way to parse address bytes sent via RS485 into strings that can easily be passed to a send-lcd-string function.

This code is written for the PIC18 architecture (8 bit uC).

The problem is that the free/evaluation version of a particular compiler generates perfect assembly code that works while suffering a performance hit, but the paid and supposedly superior compiler generates code more efficiently at the expense of being able reference the addresses of all my byte arrays used to drive the graphics on my lcd display.

I know I'm putting lots of mud in the water by using a proprietary compiler for a less than typical architecture, but I hope someone out there has some suggestions.

Thanks.

Answer 1

It is typical for optimizers to do unwanted thingies from time to time if you poke around in the internals.

Is your converted_value a global value or otherwise assigned in such a fashion that the compiler knows not to touch it?

Answer 2

Is it poor form to convert to floating, call fmod, and convert to integer, instead of just using the % operator? I would say yes. There are more readable ways to slow down a program to meet some timing requirement, for example sleeping in a for loop. No matter what compiler or what tweaking of assembly code or whatever else, this is a highly obfuscated way to control the execution speed of your program, and I call it poor form.

If perfect assembly code means that it works right but it's even slower than the conversions to floating point and back, then use integers and sleep in a for loop.

As for the imperfect assembly code, what's the problem? "at the expense of being able reference the addresses of all my byte arrays"? It looks like type char* is working in your code, so it seems that you can address all your byte arrays the way the C standard says you can. What's the problem?

Answer 3

PIC's don't like doing pointer arithmetic.

As Windows programmer points out, use the mod operator (see below.)

char byte_to_ascii(char value_to_convert, volatile char *converted_value) {

 if (value_to_convert < 10) {
  return (value_to_convert + 48);
 } else {
  char a = value_to_convert / 10;  
  char b = value_TO_convert%10;
  a = a + 48;
  b = b + 48;
  *converted_value = a;
  *(converted_value+1) = b;
  return 0;
 }
}

Answer 4

The code seems to be doing two very different things, depending on whether it's given a number in the range 0-9 or 10-99. For that reason, I would say that this function is written in poor form: I would split your function into two functions.

Answer 5

Frankly, I would say yes..

If you wanted b to be the remainder, either use MOD or roll-your-own:

char a = value_to_convert / 10;
char b = value_to_convert - (10 * a);

Conversion to/from floats is never the way to do things, unless your values really are floats.

Furthermore, I would strongly recommend to stick to the convention of explicitly referring to your datatypes as 'signed' or 'unsigned', and leave the bare 'char' for when it actually is a character (part of a string). You are passing in raw data, which I feel should be an unsigned char (assuming of course, that the source is unsigned!). It is easy to forget if something should be signed/unsigned, and with a bare char, you'll get all sorts of roll-over errors.

Most 8-bit micros take forever for a multiply (and more than forever for a divide), so try and minimise these.

Hope this helps..

Answer 6

I'd probably write that as:

char byte_to_ascii(char value_to_convert, volatile char *converted_value)
{
 if (value_to_convert < 10) {
  return value_to_convert + '0';
 } else {
  converted_value[0] = (value_to_convert / 10) + '0';
  converted_value[1] = (value_to_convert % 10) + '0';
  return 0;
 }
}

Answer 7

modulo and integer division can be very very expensive. I have do not know about your particular architecture, but my guess it is expensive there as well.

If you need both, division and modulo, do one of them and get the other one by multiplication/difference.

q =p/10;
r = p - q*10;

Answer 8

I would definitely avoid using floating point anything on a PIC. And I would -try not to- use any divisions. How many times do you see sending a non-ascii char to the LCD? Can you save it to the LCD's memory and then call it by it's memory position?

Here's what a divide by 10 looks like in my code, note the 17 cycles it needs to complete. Think about how long that will take, and make sure there is nothing else waiting on this.

61:                         q = d2 / 10;
 01520  90482E     mov.b [0x001c+10],0x0000
 01522  FB8000     ze 0x0000,0x0000
 01524  2000A2     mov.w #0xa,0x0004
 01526  090011     repeat #17
 01528  D88002     div.uw 0x0000,0x0004
 0152A  984F00     mov.b 0x0000,[0x001c+8]

If you do a floating point anything in your code, look in the program memory after you've compiled it, on the Symbolic tab (so you can actually read it) and look for the floating point code that will need to be included. You'll find it up near the top (depending on your code), soon(ish) after the _reset label.

Mine starts at line number 223 and memory address of 001BC with _ floatsisf, continues through several additional labels (_fpack, _divsf3, etc) and ends in _funpack, last line at 535 and memory address 0042C. If you can handle (42C-1BC = 0x270 =) 624 bytes of lost program space, great, but some chips have just 2k of space and that's not an option.

Instead of floating point, if it's possible, try to use fixed point arithmetic, in base 2.

As far as not being able to reference all the byte arrays in your LCD, have you checked to make sure that you're not trying to send a null (which is a fine address) but it get's stopped by code checking for the end of an ascii string? (it's happened to me before).

Answer 9

Since we're discussing divisions by 10 here..

This is my take. It only simple operations and does not even need wide registers.

unsigned char divide_by_10 (unsigned char value)
{
  unsigned char q;
  q = (value>>1) + (value>>2);
  q += (q>>4);
  q >>= 3;
  value -= (q<<3)+q+q;
  return q+((value+6)>>4);
}

Cheers, Nils

Answer 10

Just to be a nitwitt, but multiple return statements from the same function can be considered bad form (MISRA).

Also, some of the discussions above are on the limit of permature optimizations. Some tasks must be left to the compiler. However, in such a minimalistic embedded environment, these tricks may be valid still.