 |
 |
| View previous topic :: View next topic |
| Author |
Message |
The Puma
Joined: 23 Apr 2004
Posts: 227
Location: The Netherlands
|
| strange thing with reversing a string |
 Posted: Thu Oct 11, 2007 6:14 am |
 |
|
I have this function to reverse a string
The string can hold max 10 chars
Let say: "1234567890"
After the reversing he outputs "=098765432"
Whats is wrong here??
| Code: |
void max7221_fill_char_buffer(char *buffer) {
int i,j;
char currentchar;
char *msgptr,*rev_buffer;
j=strlen(buffer);
rev_buffer[j--]=0;
for(i=0;i<strlen(buffer);i++,j--)
rev_buffer[j]=buffer[i];
memset(message,0x00,50);
msgptr=&message[0];
currentchar=*rev_buffer;
while(currentchar) {
memcpy(msgptr,font5x7[currentchar-0x20],5);
msgptr+=5;
currentchar=*(++rev_buffer);
}
}
|
But if i only reverse a string that is lower than the 10 chars is going well |
|
 |
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Thu Oct 11, 2007 6:32 am |
|
|
| Where do you initialise rev_buffer? My guess is it is pointing to a random location in RAM and conflicting with other variables. |
|
|
The Puma
Joined: 23 Apr 2004
Posts: 227
Location: The Netherlands
|
|
| Posted: Thu Oct 11, 2007 7:36 am |
|
|
| Can you give me a hint how i can change the code, so that it works? |
|
|
rberek
Joined: 10 Jan 2005
Posts: 207
Location: Ottawa, Canada
|
|
| Posted: Thu Oct 11, 2007 7:42 am |
|
|
| ckielstra did give you a hint. Initialize rev_buffer. |
|
|
The Puma
Joined: 23 Apr 2004
Posts: 227
Location: The Netherlands
|
|
| Posted: Thu Oct 11, 2007 7:54 am |
|
|
But how??
Please help me |
|
|
Wayne_
Joined: 10 Oct 2007
Posts: 681
|
|
| Posted: Thu Oct 11, 2007 8:30 am |
|
|
OK, your first problem is that you think strlen returns the length of the string including the null termination char '\0' it does NOT so if you changed the code
| Code: |
j=strlen(buffer);
rev_buffer[j--]=0;
to
j=strlen(buffer);
rev_buffer[j+1] = 0;
That would fix the first problem.
Secondly and more imprtantly.
char *rev_buffer; needs to be changed to
char rev_buffer[MAX_SIZE];
where MAX_SIZE = strlen(buffer) + 1;
Hope that helps.
|
|
|
|
The Puma
Joined: 23 Apr 2004
Posts: 227
Location: The Netherlands
|
|
| Posted: Tue Oct 23, 2007 6:04 am |
|
|
I can't get it to work
| Code: | void max7221_fill_dot_matrix_buffer(char *buffer) {
int i,j;
char reverse_buffer[10],*msgptr,currentchar;
j=strlen(buffer);
reverse_buffer[j--]=0;
for(i=0;i<strlen(buffer);i++,j--)
reverse_buffer[j]=buffer[i];
memset(dot_matrix_buffer,0x00,50);
msgptr=&dot_matrix_buffer[0];
currentchar=*reverse_buffer;
while(currentchar) {
memcpy(msgptr,font5x7[currentchar-0x20],5);
msgptr+=5;
currentchar=*(++reverse_buffer);
}
}
|
get the follwoing error:
Expecting LVALUE such as a variable name or * expression
(in line: currentchar=*(++reverse_buffer);)
How can i made this that the 4 four lines reverse the string (in buffer) to reverse_buffer? |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Tue Oct 23, 2007 8:25 am |
|
|
[spam],
You did not react to Wayne's suggestions. This is impolite and it might have helped you.
| Wayne_ wrote: | OK, your first problem is that you think strlen returns the length of the string including the null termination char '\0' it does NOT so if you changed the code
| Code: |
j=strlen(buffer);
rev_buffer[j--]=0;
to
j=strlen(buffer);
rev_buffer[j+1] = 0; |
That would fix the first problem. |
Sorry Wayne, the original code as posted is correct. You made a similar thinking error because an array starts counting at position 0, not at 1. For example, a string with length 10 would occupy 11 bytes in memory including the terminating zero. Strlen would return 10, writing a terminating 0 at rev_buffer[10] is then correct.
| Wayne_ wrote: | Secondly and more imprtantly.
char *rev_buffer; needs to be changed to
char rev_buffer[MAX_SIZE];
where MAX_SIZE = strlen(buffer) + 1; |
This is an important bug and [spam] did not fix it, the reverse_buffer size is still 10 instead of 11.
I want to extend on this a bit more. It is good coding practice to avoid the use of 'magic numbers', i.e. don't use hard coded numbers. In your small function here you have the hard coded numbers 10, 50 and 5. The big risk you are running here is that you change the size of one such number in one place in your program but forget to change it in another place. Replace the hard coded numbers by a meaningful #defined name.
Another potential problem: You are using the strlen function to get the length of the input buffer, but there is no test on the input buffer being too long. This could cause your program to overwrite other data in RAM.
| The [spam] wrote: | get the follwoing error:
Expecting LVALUE such as a variable name or * expression
(in line: currentchar=*(++reverse_buffer);) |
This one is strange, it looks like you have found a limitation (bug) in the CCS compiler. The compiler complains about the '++reverse_buffer' part. I tried it both in v3.249 and 4.057 and both compiler versions don't allow calculations on the buffer address. This is not good, but I have to say it is an ugly coding style to want to change a buffer's base address. A more elegant solution is to change currentchar into a pointer and then change this pointer instead.
A final remark: Even in a small program it is good practice to add a little comment about what is going on. And when you copy code from other people it is polite to give them credits (add a link to the original code in your program).
| Code: | #define DOT_MATRIX_BUFFER_SIZE 50
#define FONT_SIZE 5
#define MAX_REV_BUF_SIZE (DOT_MATRIX_BUFFER_SIZE / FONT_SIZE) // 50 / 5 = max 10 characters
void max7221_fill_dot_matrix_buffer(char *buffer)
{
int i,j;
char reverse_buffer[MAX_REV_BUF_SIZE+1],*msgptr;
char *currentchar; // Changed into a pointer
// Reverse the string sequence.
// Code supplied by asmallri in http://www.ccsinfo.com/forum/viewtopic.php?t=23498
j=strlen(buffer);
if (j > MAX_REV_BUF_SIZE) // Added a test for buffer overflow
j = MAX_REV_BUF_SIZE;
reverse_buffer[j--]=0;
for(i=0;i<strlen(buffer);i++,j--)
reverse_buffer[j]=buffer[i];
// Clear the matrix buffer.
memset(dot_matrix_buffer, 0x00, DOT_MATRIX_BUFFER_SIZE);
//Copy the reversed string to the matrix buffer.
msgptr = &dot_matrix_buffer[0]; // Here you have msgptr for same reason we
// had to make currentchar into a pointer.
// Looks like you had this problem before...?
currentchar = &reverse_buffer[0];
while(*currentchar)
{
memcpy(msgptr, font5x7[*currentchar-0x20], FONT_SIZE);
msgptr+=5;
currentchar++;
}
} |
Note: You can optimize this function by removing the reverse and walking the input buffer from back to front. |
|
|
Ttelmah
Guest
|
|
| Posted: Tue Oct 23, 2007 9:00 am |
|
|
Not allowing the base address to be changed, is correct 'C',not a limitation in CCS.
Key to remember, is that 'rev_buffer', is _not_ a variable holding a pointer, but is a shortcut (the name of an array is it's address). Since it is not a variable, it can't itself be incremented.
In K&R, the paragraph about this says:
"There is one difference between an array name and a pointer that must be kept in mind. A pointer is a variable, so pa=a, and pa++ are sensible operations. But an array name is a _constant_, not a variable: constructions like a=pa or a++ or pa=&a are illegal."
If you want to increment the address, then you need to declare:
char rev_buffer[MAX_SIZE];
char *buff_address;
Then add:
buff_address=rev_buffer;
and increment 'buff_address', not 'rev_buffer.
Best Wishes |
|
|
The Puma
Joined: 23 Apr 2004
Posts: 227
Location: The Netherlands
|
|
| Posted: Tue Oct 23, 2007 9:01 am |
|
|
Ok, thz so far,
but how can i rewrite that code so that the fontdata (5 bytes) are not placed on position 0 of the string, but on position 49
An example:
0x36, 0x49, 0x49, 0x49, 0x7F // 0x42, B
The data for the letter B must be place on postion 49
| Code: | msgptr=&dot_matrix_buffer[0];
currentchar=*buffer;
while(currentchar) {
memcpy(msgptr,font5x7[currentchar-0x20],MAX_FONT_SIZE);
msgptr+=MAX_FONT_SIZE;
currentchar=*(++buffer);
}
} |
|
|
|
The Puma
Joined: 23 Apr 2004
Posts: 227
Location: The Netherlands
|
|
| Posted: Tue Oct 23, 2007 9:56 am |
|
|
| Ttelmah wrote: | Not allowing the base address to be changed, is correct 'C',not a limitation in CCS.
Key to remember, is that 'rev_buffer', is _not_ a variable holding a pointer, but is a shortcut (the name of an array is it's address). Since it is not a variable, it can't itself be incremented.
In K&R, the paragraph about this says:
"There is one difference between an array name and a pointer that must be kept in mind. A pointer is a variable, so pa=a, and pa++ are sensible operations. But an array name is a _constant_, not a variable: constructions like a=pa or a++ or pa=&a are illegal."
If you want to increment the address, then you need to declare:
char rev_buffer[MAX_SIZE];
char *buff_address;
Then add:
buff_address=rev_buffer;
and increment 'buff_address', not 'rev_buffer.
Best Wishes |
I did't understand it anymore
Can you me a working reserve code for above procedure |
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Tue Oct 23, 2007 10:12 am |
|
|
| I don't understand anymore either. I gave you a compilable version of your function. I don't know the input or output requirements of your function, but have you tested my code and what did you see going wrong? |
|
|
The Puma
Joined: 23 Apr 2004
Posts: 227
Location: The Netherlands
|
|
| Posted: Tue Oct 23, 2007 10:17 am |
|
|
| ckielstra wrote: | | I don't understand anymore either. I gave you a compilable version of your function. I don't know the input or output requirements of your function, but have you tested my code and what did you see going wrong? |
It is very simple:
I pass a string. lets say DEMO
now the passing string (in char buffer) must reversed
| Code: | void max7221_fill_char_buffer(char *buffer) {
char currentchar;
char *msgptr;
HERE MUST THE REVERSE CODE
memset(message,0x00,MAX_DOT_MATRIX_BUFFER_SIZE);
msgptr=&message[0];
currentchar=*buffer;
while(currentchar) {
memcpy(msgptr,font5x7[currentchar-0x20],MAX_FONT_SIZE);
msgptr+=MAX_FONT_SIZE;
currentchar=*(++buffer);
}
}
|
|
|
|
The Puma
Joined: 23 Apr 2004
Posts: 227
Location: The Netherlands
|
|
| Posted: Tue Oct 23, 2007 11:40 am |
|
|
I have now this and it works
Can i optimise this ?
Maybe some advise
| Code: | void max7221_fill_dot_matrix_buffer(char *buffer) {
int i,j;
char temp,*msgptr,currentchar;
for(i=0,j=strlen(buffer)-1;i<j;i++,j--) {
temp=buffer[i];
buffer[i]=buffer[j];
buffer[j]=temp;
}
memset(dot_matrix_buffer,0x00,MAX_DOT_MATRIX_BUFFER_SIZE);
msgptr=&dot_matrix_buffer[0];
currentchar=*buffer;
while(currentchar) {
memcpy(msgptr,font5x7[currentchar-0x20],MAX_FONT_SIZE);
msgptr+=MAX_FONT_SIZE;
currentchar=*(++buffer);
}
}
|
|
|
|
ckielstra
Joined: 18 Mar 2004
Posts: 3680
Location: The Netherlands
|
|
| Posted: Tue Oct 23, 2007 12:29 pm |
|
|
| The [spam] wrote: | I have now this and it works
Can i optimise this ?
|
Get rid of the lines reversing the buffer, you can do this in the copy-loop by reading the input buffer backwards (from the end to start).
| Code: | void max7221_fill_dot_matrix_buffer(char *buffer) {
int8 i;
char *msgptr, currentchar;
int8 MsgLength;
memset(dot_matrix_buffer,0x00,MAX_DOT_MATRIX_BUFFER_SIZE);
MsgLength = strlen(buffer);
msgptr=&dot_matrix_buffer[0];
for (i=0; i<MsgLength; i++)
{
currentchar=buffer[MsgLength - i - 1];
memcpy(msgptr,font5x7[currentchar-0x20],MAX_FONT_SIZE);
msgptr+=MAX_FONT_SIZE;
}
} |
|
|
|
|
|
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB © 2001, 2005 phpBB Group
|
FAQ 
Search
Register
Profile 
Log in to check your private messages 
Log in
