Sluggish UART output

Go To Last Post
8 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi all,

 

I am trying to add UART output and (eventually) input to my Atmega328P project (see code below).  I had hoped to have the loop write out status every second via the serial connection, but sometimes it skips multiple seconds, sometimes it outputs once a second no problem, but it is not consistent (see screenshot of Bray's terminal, noting that the numbers refer to how many 10ms ticks have passed since reset).  I am using a slightly modernized version of Fleury's interrupt UART library from here:  http://beaststwo.org/avr-uart/.  I am using 4800 baud, no external crystal.  In the code below, you will see stuff to poll the ADC and write LCD; I commented that out in case it was hogging resources, but it doesn't seem to make much of a difference.

 

Does anybody have any ideas on how to make the output consistent, preferably deterministic?  Should I ditch the library, handroll it?  Is there a better library that doesn't use interrupts?  Am I a coming at the issue all wrong?

 

Just for broader context, I am playing make believe that the program is an industrial controller with various inputs on the ADC (right now just pots), an LCD, a special high tech button that does something very high tech (haha), calibration data stored in the EEPROM, and now I want to have it talk to a computer once in a while to send diagnostic output and eventually read parameter changing input.  I don't want to simplify the ADC and button and LCD away to make it fast, I want to figure out how to make it all work together.

 

Thanks!

 

Code: 

/*
    lcd_adc.c
    Synopsis:  Using a 10ms tick, handle long and short button inputs 
        and do something with ADC input
    Created: 2018-03-20 7:38:09 PM
    Author : wsprague
 
    HARDWARE DESCRIPTION (lab notes might be more up to date)
        PD2      (4) button input 
        PC5     (28) power on LED
        PC2     (26) multiplex ADC hyst pot
        PC1     (25) multiplex ADC setpoint pot 
        PC0     (24) multiplex ADC input
        PC4     (27)    output LED 
        PBx      (*) lcd control pins   

        
    Notes:
    
        2018-05-02 : original design was deeply flawed -- stomped all over the time it takes for a capture, misused 
            ISRs, probably screwed up the LCD writing too.
            
            Next version will have the ADC read and multiplex the three inputs during a time slice.  As I calculate it,
            I have 105 us for each capture, so I can do them all one after the other in a single 10ms slice.  
            (1Mhz clock / 8 prescale div = 125kHz.  => 0.000008 s per ADC cycle.  *13 => 105us   )
            
            https://www.avrfreaks.net/forum/adc-design-advice

        2018-04-19 wws:  modifying with ADC; pot input for hyst and setpoint; function for LCD display at x, y, buffer; 
        
            I don't know why I did this earlier, deleting:  PORTC |= ( 1 << PC5);  // pull up 

            TODO make sure button working and counting up.  I am going to use it for something....
            
        2018-03-20 wws:  remember && for logical AND  
        
 */

#define F_CPU 1000000UL  // delay.h supplies reasonable default, but this fixes warning

#include <string.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <avr/eeprom.h>
#include <avr/interrupt.h>
#include <avr/io.h>
#include <avr/pgmspace.h>
#include <avr/sleep.h>
#include <util/atomic.h>

// include header file for lcd library 
#include "lcd.h"
#include "uart.h"

// prototypes 
void wws_write_lcd (int x, int y, int w, char* str);
uint16_t adc_read(uint8_t channel);

// define event names and numbers
#define _0NULL     0
#define _1TICK     1

// define state names and numbers
#define _0NULL     0

// useful constants
#define RADIX        10    // radix for lcd numeric output, want hex but decimal much easier still
#define HOLD_TICKS 1000    // how many ticks to wait for hold before reset
#define TICK_TOP    155    // how many ticks before heartbeat/ cld increment. 155 with 64 prescale gives about  10.0 msec, 100 Hz
#define UART_BAUD_RATE      4800      

// globals
volatile uint32_t  tick       = 0;        // ticks (~10ms) since reset, incremented each wake up.  Overflows on 1.36 years
volatile uint16_t  adc_val    = 0;        // ADC value set by ISR at capture
uint16_t EEMEM eeprom_pcount  = 0;        // eeprom memory allocation 

// progmem something with name of project
const char pname[] PROGMEM = "exp#15";

// define short nop macro
#define _NOP() do { __asm__ __volatile__ ("nop"); } while (0)

// 
int main () {    
    char buffer[32];       // to store lcd stuff or whatever
    uint8_t ind;           // to store PD input 
    uint8_t holdt   = 0;   // number of held down ticks
    uint8_t p1      = 0;   // boolean for whether or not button has been pushed and not yet released
    uint16_t pcount = 0;   // count of button pushes, 
    uint16_t hbeat  = 0;   // 1 sec heartbeat counter
    uint16_t setp   = 0;   // setpoint 10 bit like ADC
    uint16_t hyst   = 0;   // hyst value 10 bit like ADC
    uint16_t lumen  = 0;   // light value 10 bit like ADC

    // initialize uart
    uart_init( UART_BAUD_SELECT(UART_BAUD_RATE, F_CPU) ); 

    // initialize lcd, put pname in lower right corner
    lcd_init(LCD_DISP_ON);
    lcd_clrscr();
    // lcd_gotoxy(10,1);     // x,y zero indexed
    // lcd_puts_p(pname);
    
    // update lcd with pcount and progmem info on program.  Relies on consistent EEPROM behavior after being programmed
    pcount = eeprom_read_word (& eeprom_pcount );
    if (pcount == 0xFFFF) {
        pcount = 0;
        eeprom_update_word (& eeprom_pcount, pcount );
    }
    // lcd_gotoxy(0,1);       // x,y zero indexed
    // itoa(pcount, buffer, RADIX);
    // lcd_puts(buffer);
    
    // enable output power-on-indicator light on PC5 (28), nightlight LED PC4.   
    DDRC  |= ( 1 << PC5) | ( 1 << PC4) ;
    PORTC |= ( 1 << PC5);

    // input on PD2 (button, pin 4)
    DDRD  &= ~(1 << PD2 );    // ensure input pin 
    PORTD = 0xFF;             // set pull up on all pins for well defined behavior, assumes all zero and input pins
    holdt = 0; // initialize for how long the button pin is held
    
    // config TC0 ticking at 10ms CTC. 
    TCCR0A |= (1 << WGM01);                // mode 2 CTC 
    OCR0A   = TICK_TOP;                    // turn over when hit OCROA,
    TCCR0B |= (1 << CS01) | (1 << CS00);   // turn on, 64 prescale
    TIMSK0 |= (1 << OCIE0A);               // fire off interrupt on match
    
    // config ADC
    ADMUX |= (1 << REFS0);  // simply use VCC of chip as reference
    ADCSRA |= (1 << ADEN) | (1 << ADIE) | (1<<ADPS1) | (1 << ADPS0);  // ADC enable, ISR enable, 8 prescale (1M / 8 ) == 125k
        
    // go to sleep and wait for 10ms ISR or other interrupt 
    set_sleep_mode(SLEEP_MODE_IDLE);
    sei();
    while (1) {
        // sleep, waking on interrupts
        sleep_enable();
        sleep_cpu();
        sleep_disable();

        // read input pin, deal with later
        ind = PIND;  

        // handle tick things.
        // .. 
        if ( tick % 10 == 0 ) {   
            // ..update heartbeat thing, hope modulus not too slow       
            // ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
            //     hbeat++;
            // }
            // itoa(hbeat, buffer, RADIX);
            // lcd_gotoxy(0,0);         // x,y zero indexed
            // lcd_puts("        ");
            // lcd_gotoxy(0,0);
            // lcd_puts(buffer);
            // PINC |= (1 << PC3); 
            
            
            //         PC2     (26) multiplex ADC hyst pot
            //         PC1     (25) multiplex ADC setpoint pot 
            //         PC0     (24) multiplex ADC input
        } else if ( tick % 100 == 0 ) {
            
            uart_puts_P("String once a second\r\n");
            snprintf(buffer, 24, "%d\r\n", tick);
            uart_puts(buffer);
            
            
            // lumen  = adc_read(2); // read ADC2        
            // setp   = adc_read(1); // read ADC1 
            // hyst   = adc_read(0); // read ADC0 
            // 
            // itoa(lumen, buffer, RADIX);
            // wws_write_lcd(10, 0, 5, buffer);           
            // itoa(setp, buffer, RADIX);
            // wws_write_lcd(5, 0, 5, buffer);
            // itoa(hyst, buffer, RADIX);
            // wws_write_lcd(0, 0, 5, buffer);
            // 
            // // (un)light LED depending on set point and hyst values use PCn pin config'ed  above
            // //  should be atomic for math?
            // if  (lumen >= setp + hyst) {                                              // (lumen >= setp + hyst) {
            //     PORTC  &= ~( 1 << PC4);        // clear LED
            // } else if (lumen < setp - hyst) {                                         // (lumen <= setp - hyst){
            //     PORTC  |= ( 1 << PC4);         // light LED
            // } else {
            //     // no op just keep current state at PC4
            // }

        
        }   
       
        // // handle button input        
        // ..  count number of ticks with button held/ pushed.  (ind & (1<<PD2))  syntax to check single bit at PD2.  .  
        // if (!(ind & (1<<PD2)) && p1 == 0){                // bit low and not started a press,  so start incrementing
        //     holdt ++;
        //     p1 = 1;
        // } else if (!(ind & (1<<PD2)) && p1 == 1 && holdt <= 254) {
        //     holdt ++;
        // } else if ((ind & (1<<PD2)) && (p1 == 1) ) {     // bit high for first time so set to zero
        //     holdt = 0;
        //     p1 = 0;
        // } else {                                         // bit continuing low, do nothing or write zero de
        //     // holdt = 0;
        // }

        // // ..  do stuff based on how long button down
        // switch (holdt) {
        //     // for each push, debounced at 50 ms, increment counter        
        //     case 5 :                                
        //         ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        //             pcount = pcount+1;
        //         }
        //         eeprom_update_word (& eeprom_pcount, pcount );                
        //         itoa(pcount, buffer, RADIX);
        //         // wws_write_lcd(0, 1, 5, buffer);
        // 
        //         break;
        //     
        //     
        //     // at 1 sec, zero counter and write to eeprom and lcd            
        //     case 100:
        //         ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        //             pcount = 0;
        //         }    
        //         eeprom_update_word (& eeprom_pcount, pcount );
        //         itoa(pcount, buffer, RADIX);
        //         // wws_write_lcd(0, 1, 5, buffer);
        //         
        //         break;
        //         
        //     // default:    // XXX dont want always running
        //     //     break;
        //         
        // }  // holdt case
        
    }      // while loop
}          // main()


// ISR for ADC read.
ISR(ADC_vect){
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        adc_val = ADC;
    }
}

// ISR for TC0 CTC .  
ISR(TIMER0_COMPA_vect){
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
        tick++;
    }
}

// read a channel, quickly busy waiting from forum post
/*    adc_value0 = adc_read(0); // read ADC0 
      adc_value1 = adc_read(1); // read ADC1 
      adc_value2 = adc_read(2); // read ADC2 
*/      
uint16_t adc_read(uint8_t channel)
{
    uint16_t x;
    ADMUX = (1<<REFS0) | channel;   // set reference  with REFS0 and channel.  resetting ref each time bc clobber w channel byte
    ADCSRA |= (1<<ADSC);            // start conversion  
    while(ADCSRA & (1<<ADSC)) {}    // wait for conversion complete  
    ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { 
        x = ADC;
    };
    return x;
}

// write a string to lcd, blanking out w wide characters
void wws_write_lcd (int x, int y, int w, char* str){
    char buffer[32];       // to store lcd stuff
    
    memset(buffer, ' ', w);
    buffer[w] = '\0';
    lcd_gotoxy(x, y);  // x,y zero index
    lcd_puts(buffer);  // blank screen
    lcd_gotoxy(x, y);  // x,y zero index
    lcd_puts(str);     // write values

}

 

Bray's terminal screenshot:

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I don't know if either of these is the cause of the issue, but, %d expects an int (16 bits).  You need to use %ld for a long int (32 bits)

 

    uart_puts_P("String once a second\r\n");
    snprintf(buffer, 24, "%ld\r\n", tick);
    uart_puts(buffer);

Also, interrupts are disabled when an interrupt is acknowledged, so there is no need for an ATOMIC_BLOCK() inside an ISR (unless you have previously enabled interrupts in the ISR).

// ISR for ADC read.
ISR(ADC_vect){
    adc_val = ADC;
}

// ISR for TC0 CTC .  
ISR(TIMER0_COMPA_vect){
    tick++;
}

Having all those comments in the code you posted makes it difficult to read.  Why not just exclude them from the code?

Greg Muth

Portland, OR, US

Xplained/Pro/Mini Boards mostly

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

forkandwait wrote:
} else if ( tick % 100 == 0 ) {

I cannot tell exactly your program flow from a quick read, but I'd guess that this test is not coming true when you think it is.  The cause could be other branches of the code taking longer than 10ms so "tick" isn't checked every time.  Also, with your straightforward code, there is indeed an atomic race.  But it should be cured in the mainline, with an atomic block making a copy of "tick".

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:

forkandwait wrote:
} else if ( tick % 100 == 0 ) {

... but I'd guess that this test is not coming true when you think it is.  The cause could be other branches of the code taking longer than 10ms so "tick" isn't checked every time.

 

This particular timing set up works swimmingly with the ADC and LCD only, it just gets messed up when I try to  add the UART functionality. So I don't think the problem is my timing approach unless it is interacting weirdly with the UART library which might be taking longer than 10ms but I don't know.

 

I will probably write the UART functionality by hand and avoid interrupts altogether, especially since the overall architecture is based on waking up every 10 ms and checking.  Is there any collective wisdom on this approach especially with UARTs?

 

I will also add atomic blocks on the % timing calculation and take them out of the ISRs, because why not.

 

Thanks!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Don't pooh-pooh my speculation until it has been disproved.  Somewhere in your code there is a section that takes a long time. During that time, "tick" is still being updated.  So it is simply a matter of changing your check logic to avoid the exact modulus check.  Or set flags for the periodic operation in the ISR.  Or just use a tick event to increment auxiliary timers.

 

If you don't do that, you will still have the race lurking in your code regardless of how you do your UART operations. 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:

Don't pooh-pooh my speculation until it has been disproved. 

 

Hehe, that's  a pretty funny turn of phrase...

 

theusch wrote:
Somewhere in your code there is a section that takes a long time. During that time, "tick" is still being updated.  So it is simply a matter of changing your check logic to avoid the exact modulus check.  Or set flags for the periodic operation in the ISR.  Or just use a tick event to increment auxiliary timers.

 

 

I believe you that there is a race condition somewhere.  I moved the modulus calculation into an atomic block and check the variables later and confirmed it works with my simpler LCD only code. I don't know exactly how to apply your other suggestions, but I will think about it.

 

theusch wrote:

If you don't do that, you will still have the race lurking in your code regardless of how you do your UART operations. 

 

I just think the race condition may be in the uart library, and that to even get my head around I might need to (1) not use interrupts to drive the communications, and (2) not use a fairly opaque library.

 

Partly, I think this based on the perfect performance BEFORE adding the library to the overall code.  (I will try inititalizing the UART and not writing data just for grins later, it just occured to me.)

 

Thanks!

 

Code for modulus: changes

 

        ATOMIC_BLOCK (ATOMIC_RESTORESTATE) {
            if ( tick % 10 ) 
                ms100 = 1;
            else 
                ms100 = 0;
            
            if (tick % 100) 
                ms1000 = 1;
            else
                ms1000 = 0;
        }
        
        // .... snipped code 
        
        if ( ms100 ) {    

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are far better off with a shorter atomic block that basically just protects taking a local copy of "tick" then, when you have that you can resume interrupts and work with the copy (that does not risk being updated while you use it)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

forkandwait wrote:
I moved the modulus calculation into an atomic block and check the variables later

You aren't getting it.  It is not an "atomic" situation.  (but indeed atomic is important)   Sometimes, you are not reaching the %100 test within one "tick".

 

What could be in a UART "library"?  Interrupt-driven, and circular buffering?

 

If indeed a "send string" implemented with polling and wait loops, then each character takes about a millizecond at 9600 and ...

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.