2
\$\begingroup\$

I write a code (for Atmega168P) for receive data from USART interrupt as below:

ISR     (USART_RX_vect)
{
    unsigned char count=0;      
    unsigned char coder[13];    
    int over_uart=0;

    for (count=0;count<13;count++)
    {
        while (!recive_485)
        {
            over_uart++;                
            wdt;
            if (over_uart>=20000)   {   
                coder[0]=0;     
                count=14;       
                over_uart=0;    
                break;  }
        }
        coder[count]=UDR0;
    }
}

Each time that i receive 13 byte of data and put in coder array.

I use a variable over_uart for calculate time between each of received byte. if over_uart>=20000, means that:

The receive progress is start but we have problem in lines that do not get next byte in 20000ms And terminate the last command and look for new 13byte that will receive from line.

Any body know a optimized way that find delay between receive data from USART (without help of over_uart variable)?

EDIT:

I correct the problem in my code and remove put delay in ISI as blow:

ISR     (USART_RX_vect)
{
    coder[count]=UDR0;
    if (coder[count] == 20)
        UartFlag=True;
    else
        count++;
}

And in Main function:

while (1)
    {
        if (UartFlag)
        {
            DoSomthing();
            count=0;
            UartFlag=Fulse;
        }
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ putting a delay in the interrupt handler is a bad idea. Just read all of the available data in the handler, take a time (from some system timer) and check it after the next interrupt is occurring. \$\endgroup\$ – Eugene Sh. Jun 29 '17 at 14:02
  • 3
    \$\begingroup\$ You've written this as if an ISR is like a task or a thread. It's not. When hardware triggers the ISR you do the bare minimum necessary to service the hardware event, then exit the ISR and do the rest of your processing in your 'main' code (which may or may not be a task depending on your system). \$\endgroup\$ – brhans Jun 29 '17 at 14:13
  • 3
    \$\begingroup\$ @combo_ci Even if you must gettting 13 bytes, it doesn't mean you have to wait for them in the interrupt. \$\endgroup\$ – Eugene Sh. Jun 29 '17 at 14:37
  • 4
    \$\begingroup\$ The interrupt fires for 1 byte. Put that 1 byte into coder, then exit the ISR. Wait for the next interrupt, repeat until you've put 'enough' bytes into coder or whatever other condition you need (like a timer expiring). Don't sit waiting inside the ISR for more stuff to happen. \$\endgroup\$ – brhans Jun 29 '17 at 15:21
  • 2
    \$\begingroup\$ In each ISR you should collect the single received byte (or error condition) and if you like store the timestamp. If you can act on the interarrival time efficiently you can do it in the ISR, otherwise you should handle that elsewhere, perhaps setting a "took too long" flag or something. Remember that any variable or data structure shared between the ISR and main program must be volatile and that access to multi-byte items from the main thread is not safe without disabling interrupts to ensure atomicity. \$\endgroup\$ – Chris Stratton Jun 29 '17 at 19:02
3
+25
\$\begingroup\$

Realize that interrupt routine is only the code to be jumped into, nothing more. So, your code totally a misunderstatement:

  • Your most variables there must be static. In your code they are stacked locals which are recreated at every interrupt. Also because of undetermined code flow caused by interrupts, you have to disable possible optimizations on vulnerable variables by declaring them as "volatile".

  • Don't make that kind of loops there. Realize that every interrupt etrance is the part of data collecting loop.

To solve the problem, your best shot is using a hardware timer. At every interrupt etrance, take a reading from a timer which is reset by the previous interrupt's exit code. So, you can easily determine if there is a overflow and take care of it.

\$\endgroup\$
3
\$\begingroup\$

You seem to be designing some sort of communication protocol which guarantees data integrity. Your criterion for integrity consists in receiving an expected amount of data in a limited time.

While this approach is technically possible, I advise you to look at more standard approaches (e.g. sending packets with a start byte and a checksum). Such approaches don't have any timing constraints (instead, you simply parse a continuous stream of bytes) and can be made arbitrarily robust (with a long enough checksum, you are practically guaranteed to never receive a damaged packet).

Relying on timing for data integrity checks scales very poorly. Your approach will inevitably fail you once your system needs to receive more data or your code grows complex enough to introduce internal delays you won't be able to predict.

Also, if you stick to a standard protocol such as HDLC, XMODEM or MIN, you'll likely find libraries which implement it already, saving you a good deal of time.

\$\endgroup\$
  • 1
    \$\begingroup\$ I read Min Document, it seem a good repository..do you use that for Atmega series until now? \$\endgroup\$ – Ali Jul 3 '17 at 11:51
  • \$\begingroup\$ @combo_ci They claim their RAM requirements are in tens of bytes, so it should work. Try building their sample program for an Atmega-based board. \$\endgroup\$ – Dmitry Grigoryev Jul 4 '17 at 9:32
3
\$\begingroup\$

Here is a pseudo code version for your ISR that does not enter into a wait state in the ISR and requires no changes to any other part of your program.

The basic premise is that the ISR maintains a local copy of the receive buffer. Once this buffer has a full 13 characters within the time limit, it then passes the buffer to the main code through a global buffer called rcv_buff and raises a semaphor named data_ready

To determine if it has received 13 characters within the time limit, it uses the time() function which returns the time in seconds since the epoch. If time() is not available in your library, then you can create a like function from a system clock.

ISR:
     // declare local_count,local_buff, last_time as local static variables
     // rcv_buff and data_ready are globals for main()

     local_count++;                   // bump receive counter
     local_buff[local_count-1]=UDR0;  // save in local buffer
                                      // time() returns time in secs (since epoch)
     if time()-last_time>20 then {    // over time limit, empty local buffer
          local_count=0;              // reset the local character counter
          local_buff=''; }            // flush the local buffer
     last_time = time();              // save current time in secs for next pass
     data_ready=False;                // squelch the semaphore
     if (local_count==13) then {      // we have 13 characters, pass to main code
          local_buff[local_count)=\0  // ensure string ends with null
          rcv_buff=local_buff;        // pass the local buffer to main
          data_ready = True;          // raise the semaphore for main
          local_count=0;              // reset local buffer for next packet
          local_buff='';  }           // flush the local buffer

Edit: Cleaned up the buffer indexing for C compatibility. Enhanced comments

\$\endgroup\$
1
\$\begingroup\$

you probably want to understand how interrupts work in general before diving into it. Once you do that, you likely will have to rewrite your isr completely.

with that said, one way to do what you want to do is something like thhis:

  current_time = time_now(); //stamp current time
  time_elapsed = current_time - previous_time; //calculate the time difference
  previous_time = current_time;  //update previous time

it requires a framework totally different from what you have now, btw.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.