Good afternoon....
I am a hardware engineer who has been dabbling in firmware so my apologies if the code isn't tight. I have code where I want to send out a command over UART, receive the echo back and then transmit 'ACK'. My code continually seems to fail on the strcmp line in the rx ISR section. For instance my pTx points to "ER_CMD#a01". I can see on the logic analyzer this command. I can also see on the logic analyzer rx line this command echoed back to me, however pRx does NOT seem to update to this value. All problems seem to point to the strcmp. If I remove and put a simple for loop in there pRx updates fine. Any comments would be greatly appreciated as I have been struggling with this???
Thanks!
As others have pointed out, strcmp() requires NUL terminated strings.
You also have the option of using strncmp() if you know your string length. A NUL termination is not required, strncmp() will test only the specified number of characters.
Good suggestion, but beware of the "gotcha" concerning the length passed to strncmp: which length do you specify, the length of the string you expect to receive, or the length of the string you actually received (possibly after dropping a character or two)?
If you need to compare against a set of expected strings that have different lengths, then this "gotcha" could bite you, because you would (likely) want to specify the length of the string you actually received. Problem is you could get a match to a subset of the expected string, which may or may not be what you want.
If you accept characters from the UART until you receive as many characters as are in the (unique) string you expect to receive, no "gotcha" because the two possible lengths will be same. In my experience, this situation is rare, so I mention the "gotcha."
Yes, absolutely correct, you have to pay attention to that. Using string lengths to trigger the comparison makes sense whether you use strncmp() or strcmp() because you don't really want to be running string compares in the ISR with each new RX character. I'm not saying the original code can't be made to work, but I think there's a much better way to go here. The approach suggested by dnj below is exactly what I have in mind.
In your interrupt service code you are not transmitting '\0' and hence in your strcmp() which looks for '\0' to terminate is failing to return 0.
You modify the code as follows:
#pragma vector=USCI_A1_VECTOR
__interrupt void Payload_RF(void)
{
static char c = 0;
switch(__even_in_range(UCA1IV, USCI_UART_UCTXCPTIFG))
{
case USCI_NONE: break;
case USCI_UART_UCRXIFG:
*pRx = UCA1RXBUF;
pRx++;
if (strcmp(pTx, pRx) == 0) {
__bic_SR_register_on_exit(LPM3_bits);
}
break;
case USCI_UART_UCTXIFG:
if (c == 1) {
UCA1IE &= ~UCTXIE;
__bic_SR_register_on_exit(LPM3_bits);
c = 0;
}
if (*pTx == '\0') {
c = 1;
}
UCA1TXBUF = *pTx;
pTx++;
break;
case USCI_UART_UCSTTIFG: break;
case USCI_UART_UCTXCPTIFG: break;
}
}
This code ensures '\0' is transmitted and hence it will be echoed and the strcmp(); will work properly.
Regards,
BV Ramesh.
Hi, @ridgerunnersjw !
I use the online C compiler ( https://www.onlinegdb.com/online_c_compiler ) to test a C code snippet when I'm not sure how it behaves.
It's very good, as long as the code is not related to hardware.
Perhaps it's worthwhile to give it a try.
Cheers!
I took at quick glance at your code and I noticed something missing.
strcmp will compare characters to the end of the string which is marked with a nul 0x00 character. As you are assembling the string, I don't see where you terminate that string which may be the problem. So, where you have
*pRx = UCA1RXBUF;
pRx++;
Add:
*pRx = 0;
Don't increment after using pRx when you add the nul so that the next received character will overwrite it.
I'm sorry I'm not sure I understand: I did what I think you were saying but it doesn't look correct and doesn't work
*pRx = UCA1RXBUF;
pRx++;
*pRx = 0;
if (strcmp(pTx, pRx) == 0) {
__bic_SR_register_on_exit(LPM3_bits);
}
At the top of the file where pRx points to incoming, incoming is filled with 0x00. So I am expecting the UCA1RXBUF to fill up 10 bins of incoming with the value and the remaining bins will be 0x00....Doesn't that constitute the null char? Also does it matter that the pRx is 20 deep and the pTx is only 10 deep in this instance?
I see what you mean. I also notice that you are incrementing the pTx pointer as you send.
Here is how I handle serial interrupts.
I build a circular buffer with an index at the place where i put incoming characters into the buffer; and an index at the rear of the buffer where i take out the inserted characters. You want to spend as little time in the interrupt routine as possible. The idea is to just save the character at the time of the interrupt and do something with it in the main loop.
#define BUFFER_SIZE 64
#define BUFFER_MASK 63
// using powers of 2 because the wrapping around at the end of the buffer is easier
char buffer[BUFFER_SIZE];
int front = 0;
int rear = 0;
uint8_t charAvailable(){return (front != rear);}
uint8_t putChar(char c)
{
if (((front+1) & BUFFER_MASK) == rear) return 0; // no room in the buffer
buffer[front++] = c;
front &= BUFFER_MASK; // wrap back to index 0 when index runs past 63
}
// two ways to do this
char getChar()
{
char c = buffer[rear++];
rear &= BUFFER_MASK;
return c;
}
Then your code in the main loop is:
while (charAvailable())
{
char c = getChar();
// build your string and process it when you get \r\n
}
OR
int getChar(char *c)
{
if (charAvailable() == 0) return 0; // no char available
*c = buffer[rear++];
rear &= BUFFER_MASK;
return 1;
}
Then your main loop looks like this:
char c;
while (getChar(&c))
{
// this only executes when there is a character in the buffer
// build your string from these characters and process
}
I am not following how or why you want to compare the receiving and the transmitting buffers when the two pointers are so dynamic. It looks like you are comparing them as one shrinks and the other grows. If you can use printf to see these strings on a screen, you may see exactly what is being compared.
Yes! I couldn't agree more emphatically with this. Spend as little time in the ISRs as possible, keep them dead simple. Perform all of your string or message handling in the application. This is an approach that has served me well, and is advice that should be heeded! I've always used separate transmit and receive circular buffers and head/tail indices so that TX and RX operations can run concurrently, but this is otherwise the same approach.
There are two reasons for doing it this way. One is that it is simple, robust and reliable. The other is that it is very portable between projects, even on different target hardware. Typically all you have to touch is the ISR code. You can easily build console I/O handling or CPU-CPU message handling on top of this framework.
One possible exception where you might deviate from this approach is the case where you are up against a wall wrt. performance or memory and you have to find a way to eliminate data copies between circular I/O buffers and string or message buffers. This is not typically necessary, and these changes come at the expense of making the code more complicated (more places for bugs to hide), more application specific and less portable.
OK, I just took a closer look at this. I've never tried to make this work with one buffer. I've typically used two buffers, a receive buffer where an RX interrupt adds to rxfront and getc() takes from rxrear, and a transmit buffer where putc() adds to txfront and a TX interrupt takes from txrear.
You can't have TX interrupt, RX interrupt, getc() and putc() all beating on the same buffer and indices can you? I guess you're assuming that transmit and receive operations always run in mutual exclusion and never run concurrently?
I didn't mean to confuse, but, yes, you need two buffers with front and rear indices for both. I would have written more than anyone would want to read (or I wanted to write). It is a simple matter of duplicating the code and changing names. OR Use C++ and define a class with buffer, get and put. Instantiate the class twice for Rx and Tx. That is really the way to go if you have a processor with more than 1 uart, too.
Yes, agree with all that! C++ brings its own baggage to the party, but class instantiation, encapsulation and consistency checks are worth the cost. With C you define a structure with buffer, pointers and whatever else you need like overflow counts. Instantiate a copy of the structure for each UART, the C functions get a parameter defining which UART/structure to pick up.
However you get there, IMO the really important takeaway from this discussion is that you want to keep the hardware dependent ISRs as simple and short as possible, and create stdio-like low level I/O routines that are portable with little or no change, providing easy support for interactive console and/or CPU-CPU message functionality.
I've done this frequently with two UARTs, one for a debug console, the other for a messaging interface to another processor, same low level stdio-like code supporting both.
I have rules similar to dnj and matthewbarr's replies. I like to keep the ISRs as short and minimal as possible, part of that is to avoid calling external functions in general and if you do make one be sure the ones you call are re-entrant and have tight code. For a serial ISR I would typically write my own buffer routines to create a nice ring buffer where I can put and get a byte or buffer of bytes. Definitely never use string routines in a device driver (mem routines are usually okay).
In the case of a BFL (big F'n loop) system like dnj is proposing there are two issues to watch out for; 1) you need to make sure the access of the registers is atomic, meaning the ISR can't interrupt the background loop and wreck you index or pointer (you may need to add a lock around code that accesses the ring buffer to ensure this, depends on the processor as some can do the whole operation in one cycle), and 2) don't use indexing use a pointer system. Indexing in C when the code gets generated will typically generate a function call to calculate the address, so just use a pointer and it will be much faster. Check your list file and you'll see what I mean. Or being a hardware guy connect a scope and measure the interrupt time using some digital IO pins (hint: my most common beef with hardware guys is they don't give me enough test points to prob and profile performance)
Sizing of the ring buffer is related to your application and the processor you're using, but typically 256 or 512 bytes should be enough. This allows the background loop to take its time on a calculation and not lose any characters, just make sure your background loop processes fast enough to catch up when its not busy.
Lastly, remember the KISS rule. I often see serial drivers that have parts of the protocol mixed in which complicates and probably makes it fail when but under stress. The ISRs should just get bytes into and out of the UARTs and the associated ring buffer(s). Then you handle the link or protocol in a different context more appropriate for the task.
Bit of a ramble, but I recently ran into this exact issue on what was a very mature project. Similar issue with a timer ISR but I'll save that for another time.
Cheers.
Hello, Ridge.
Note that strcmp() is string compare, not memory compare. If either strings in memory at pTx or pRx do not end in a zero (sometimes called an asciiz terminator), then strcmp() will not know when to stop comparing, and most likely will fail the comparison. (I'm not sure how you are pre- or post-processing the buffers, but you would need to add that asciiz terminator.)
If you don't want to bother adding terminators, and you know the length of the strings (the string length--not including the asciiz terminator), then you could use memcmp(), which tasks two pointers to memory, and the expected length of the comparison (you only need to compare "significant" characters).
For instance, given pTx = "ACK", pRx[3] = { 'A', 'C', 'K' }, and n = 3,
strcmp(pTx, pRx, n) would return 0, or "equal".
To be safe, always use a safe compare length so memcmp() doesn't go "off in the weeds". That is the biggest problem with "unterminated" string compares.
Good luck,
Chuck
Chuck...
My command on the bus (logic analyzer) appears as ER_CMD#a010...In code: #define r_RSSIon "ER_CMD#a01"...so am I not sure if the added 0 is because of the double quotes and it automatically gets added? On the rx side of the bus I do see the ER_CMD#a010 on the bus but yet the strcmp fails...
Thanks
In C, a string literal has an implied terminator. char s[] = "abc" has a string length of 3, a memory buffer length of 4, and appears in memory as
'a', 'b', 'c', 0
Are you seeing "ER_CMD#a01" + 0, or "ER_CMD#a010"? The latter includes the zero character at the end, not an asciiz (aka NUL, aka terminator, aka '\0')!
The terminator is literally an 8-bit character with value 0 (hex 0x00), not value '0' (which is hex 0x30)
The latter....0x00
I realize I am coming late to the discussion.
One trick I use in writing drivers is use an Arduino on the other end. It is easy to write a little "sketch" that does RX and TX on the UART or I2C. This way, you control both sides. I use the ATXMEGA because it has multiple UART channels and I2C. It also has plenty of digital pins to trigger scopes.
strlen() will return the number of characters in a string, but NOT the \0 delimiter.
strlen("123") returns 3
And these comments are spot-on: (a) using two buffers (TX and RX), using a ring buffer are the accepted method; and (2) spend as little inside an ISR as you can.
It is OK to loop in the ISR:
while( status_says_RX_has_more() )
{
*Rx_ptr++ = rx_register;
}
The reason is: if you read a byte where there are several in the UART RX FIFO, you will spend more time exiting the ISR, then immediately coming back in. Better to drain the UART FIFO, then exit. Same logic if you have a TX FIFO.
