wkteoh:
a) No bitshift operator: I found out that in MODE2ISR(), instead of using bitshift operator, temp variable was formatted by dividing it with 64. Why isn’t a bitshift used instead? First of all, it would be more straightforward to use bitshift; secondly, isn’t division a more expensive operation compared to bitshift. Is bitshift not supported in ARM compilers?
Strength reduction (replace divide by power of two with a right shift) is a standard feature of all modern compilers. This may depend on compiler optimization modes which are set to -Os (size) in the Makefile
But since a bit shift is clearly what is desired, writing it as a divide is confusing and bad practice.
Weird arrangement of ports: Why is data logged in the sequence of port 8 → 1 → 2 → 3 → 7 → 4 → 5 → 6 instead of 1 to 8? Is this some kind of optimal configuration?
The order I see in the code is:
1.3
0.3
0.2
0.1
1.2
0.4
1.7
1.6
(ADC.channel)
So it is actually spread out over the inputs of two ADC’s in no particular order. I suppose it made sense at the time.
What makes no sense at all is to repeat the same code for each conversion. It is like they never heard of subroutines. I hated the code so much that I [rewrote most of it. I did this for the version 1 hardware but a lot of what I did can be easily transferred.
“NULL” as delimiter: according to the datasheet, it is supposed to use tabs as delimiter, but from MODE2ISR() I see that “NULL” is used instead (ASCII 0). I suspect this is what is causing Excel 2007 not being able to recognize the delimiter (shows as space in Notepad, and as unrecognizable character in Wordpad).
A major brain freeze there. You can either edit, recompile, and download the firmware or write a short and very simple filter program to replace the nulls with tabs.
Another critique of the code. The code to convert the ADC reading to an ASCII string is insane.
itoa(temp2, 10, temp_buff);
if(temp_buff[0] >= 48 && temp_buff[0] <= 57)
{
q[ind] = temp_buff[0];
ind++;
}
if(temp_buff[1] >= 48 && temp_buff[1] <= 57)
{
q[ind] = temp_buff[1];
ind++;
}
if(temp_buff[2] >= 48 && temp_buff[2] <= 57)
{
q[ind] = temp_buff[2];
ind++;
}
if(temp_buff[3] >= 48 && temp_buff[3] <= 57)
{
q[ind] = temp_buff[3];
ind++;
}
q[ind] = 0;
ind++;
temp = 0;
temp2 = 0;
temp_buff[0] = 0;
temp_buff[1] = 0;
temp_buff[2] = 0;
temp_buff[3] = 0;
Lots of assorted foolishness here. Four characters are tested to see if they are in the range of ‘0’ to ‘9’. Instead of using character constants, the decimal equivalent is used which hides what is happening. For some reason even after a bad character is found, it keeps checking. All of which is useless because itoa() returns a null terminated string of characters in the required range. You could just copy until you find a null. Or you could take advantage of the fact that the version of itoa() used (the source is in another file) returns a count of characters converted to write simply:
ind += itoa(temp2, 10, &q[ind]);
q[ind++] = ‘\t’;](http://home.earthlink.net/~schultdw/logOmatic/)
