Serial Rx problem

Jason,

Regardless of the cross conversation going on in this topic, I would suggest that you re-visit the ISR and consider re-structuring it.

At this point in your learning, you might find that it makes it easier for you to follow the problem through.

Regards.

Jason,

Regardless of the cross conversation going on in this topic, I would suggest that you re-visit the ISR and consider re-structuring it.

At this point in your learning, you might find that it makes it easier for you to follow the problem through.

Regards.

The original code did use getchar().

The Keil documentation says:

The getchar function reads a single character from the
input stream using the _getkey function. The character
read is then passed to the putchar function to be
echoed.

 Note

This function is implementation-specific and is based
on the operation of the _getkey and putchar functions.
These functions, as provided in the standard library,
read and write characters using the microcontroller's
serial port. Custom functions may use other I/O
devices.

A customer-supplied _getkey() that has a timeout would solve the deadlock problem. The bad thing is that the code just waits for four characters without checking if valid characters are found or not. It doesn't contain any code to verify if a custom _getkey() function has had a timeout. And it doesn't contain code to synchronize after spurious noise.

All protocols should have some form of synchronization. Either unique timing requirements, where characters in a packet _always_ arrives with a short time delay compared to the delay between packets. Or the protocol should make use of unique characters to synchronize the transfer.

And if applicable, transfer protocols should consider two-way transfers with acknowledge, and the use of checksumming or similar to make sure that a correct message was received.

A protocol with resend should also consider sequence numbers or similar to make sure that a resend after a lost ack isn't treated as a new message.

And as already noted, atoi() does work with C strings. A four-digit number requires (at least) five characters in the array, since the four digits must be followed by a terminating zero. White space may be added before the first digit, or after the last digit. An optional sign may be added before the first digit. Your code should very explicitly make sure that the received array gets terminated. Source code is easier to maintain if you think about locality of reference. Place the code that terminates the string close to the lines that assigns the four characters. Even if the array is global and initially zero-filled by the C startup code, your code should explicitly assign a termination. Both to make the code easier to read, and to make the code self-repairing. If your code expects the fifth character position to have been zero-initialized by the startup code, and this byte once every third year gets overwritten, your code will then spend the next years with whatever termination there may be somewhere later in the memory. An embedded application should detect corruption and report/abort, and/or auto-repair.

What would happen if the fifth position in the array happens to be overwritten with the value '7', and the six position (or the variable directly after the 5-element array) contains the value zero? You send the value 1234 to your device, and atoi() finds 12347 in the array.

Per,

Thank you for your (relevant) input.

The OP now surely has enough to aid his understanding ;)

'my code monkey manual states that...'

Yeah, I thought so... so I wrote it down... just to be CLEAR about it.

--Cpt. Vince Foster
2nd Cannon Place
Fort Marcy Park, VA

Just one more note about atoi().

The function is intended for use with C strings, but all atoi() implementations I have seen don't care about the existence of a terminating zero as long as there are any other non-digit character terminating the number.

But good-quality code should very explicitly make sure that there always are any form of termination of the number. And relevant code should be grouped together, so the termination character should either be transmitted on the serial link, and used to flag the potential start of an atoi() call, or the termination should be explicitly performed very close to the source lines that receivers the four digit characters and assigns them to the array. The goal is to allow the developer to see a block of code, and without scrolling showing all relevant code to understand what is happening. When too much source lines are needed to get a clear overview, it is time to factor the code into multiple, simpler, functions.

Performing some assigns in an interrupt handler, and some assigns to the same array in the main loop is not a good solution, since that results in a loss of overview. Without the overview, a code change in one section of the code is then likely to introduce bugs because the other section was not updated correspondingly.

The use of "magic numbers" - the value 4 for number of characters - makes this problem even bigger. If the serial transmission is changed to send 5 characters instead, then the loop waiting for all characters has to be changed. And the code line that assigns the termination character has to be changed, unless the code is rewritten to instead increment a character pointer.

Thanks Per and Stephen.

I will re-structure the interrupt to collect chars one at a time. Its clear this is not optimal

For the atoi() problem, I can easily add an alpha char to the front of the string. I always terminate these command strings with a '\n' so this should sort that issue out if I check this and then do the atoi() afterwards.

Anyway, I'll be off line for a few days - other problems to deal with right now.

Again, appreciate the help.

Jason.