Trying to learn Timers and it's not going so well

[jerryk]

In a previous thread I got some really helpful advice on managing program space on a pic16f18875.  Timers are the next step in the learning curve and I'm stumped.

The goal of the following code (provided by forum generosity) is intended to vary the on and off state of an led using two variables led_on_time and led_off_time.  I've modified it significantly but cannot figure out why it doesn't work as intended.

Code: [Select]

/**
  Included files
 */
#include <xc.h>
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>


//************************* Defines and enums ******************

#define led_on_time       500             // 500ms update interval
#define led_off_time      500

#define led1_Toggle()             do { LATDbits.LATD1 = ~LATDbits.LATD1; } while(0)
#define led2_Toggle()             do { LATDbits.LATD0 = ~LATCbits.LATD0; } while(0)
#define led3_Toggle()             do { LATCbits.LATC3 = ~LATCbits.LATC3; } while(0)
#define led4_Toggle()             do { LATCbits.LATC4 = ~LATCbits.LATC4; } while(0)
#define led5_Toggle()             do { LATCbits.LATC5 = ~LATCbits.LATC5; } while(0)
#define led6_Toggle()             do { LATCbits.LATC6 = ~LATCbits.LATC6; } while(0)
#define led7_Toggle()             do { LATCbits.LATC7 = ~LATCbits.LATC7; } while(0)
#define led8_Toggle()             do { LATBbits.LATB4 = ~LATBbits.LATB4; } while(0)
#define led9_Toggle()             do { LATBbits.LATB3 = ~LATBbits.LATB3; } while(0)
#define led10_Toggle()             do { LATBbits.LATB5 = ~LATBbits.LATB5; } while(0)


//#define led4_SetHigh()            do { LATCbits.LATC6 = 1; } while(0)
//#define led4_SetLow()             do { LATCbits.LATC6 = 0; } while(0)


enum {
    led_init  = 0,
    led_wait  = 1,
    led_blink = 2,
} led_status;






//************************* Global Variables ******************


//************************* Functions ******************
void SYSTEM_Initialize(void);
void Handle_LED1 (void);
uint16_t getTime(uint16_t last);
uint16_t readTimer0(void);
static uint16_t last_time = 0;


/*****************************************
    Main application
*****************************************/
void main (void){

    //Initialize functions
    SYSTEM_Initialize(); // Timer0 Internal_clock, prescaler_32768, postscaler_1);
 
    // Enable the Global Interrupts
    //INTCONbits.GIE = 1;

    // Enable the Peripheral Interrupts
    //INTCONbits.PEIE = 1;

 
   
    while(1) {
        Handle_LED1();
     
    }
}

void Handle_LED1(void)
{
    //static uint16_t wait_time = 0;
    //static uint16_t blink_time = 0;
    static uint16_t last_time = 0;
    static uint8_t status = 0;
    /* Check the application's current state. */
    switch (status)
    {
        /* Application's initial state. */
    case led_init:
    {

        status = led_wait;
        break;
    }

    case led_wait:
    {
   
        //if (getTime(wait_time) < led_off_time)
        if (getTime(last_time) < led_off_time)
           
        {
            return;
        }

        last_time = readTimer0();
        led6_Toggle();//Toggle LED
        status = led_blink;
        break;
    }
    case led_blink:

    {
     
        //if (getTime(blink_time) < led_on_time)
        if (getTime(last_time) < led_on_time)
        {
            return;
        }
        last_time = readTimer0();
        led6_Toggle(); //Toggle LED
        status = led_wait; // return to Wait state   
        break;
    }


        /* The default state should never be executed. */
    default:
    {
        /* TODO: Handle error in application's state machine. */
        break;
    }
    }
}

uint16_t getTime(uint16_t last){
    uint16_t current = readTimer0();
   
    if (last > current){
        //return ( (uint16_t)65536 - last + current );
        return ( (uint32_t)65536 - last + current );
    }
    return ( current - last );
}

uint16_t readTimer0 (void) {
    return (((uint16_t)TMR0H<<8) | TMR0L);
}


void SYSTEM_Initialize(void)
{
   

//OSCILLATOR_Initialize

    // NOSC HFINTOSC; NDIV 1;
    OSCCON1 = 0x60;
    // CSWHOLD may proceed; SOSCPWR Low power;
    OSCCON3 = 0x00;
    // MFOEN disabled; LFOEN disabled; ADOEN disabled; SOSCEN disabled; EXTOEN disabled; HFOEN disabled;
    OSCEN = 0x00;
    // HFFRQ 32_MHz;
    OSCFRQ = 0x06;
    // HFFRQ 16_MHz;
    //OSCFRQ = 0x05;
    // HFTUN 0;
    OSCTUNE = 0x00;

    //PORTS

    /**
    LATx registers
    */
    LATE = 0x00;
    LATD = 0x00;
    LATA = 0x00;
    LATB = 0x00;
    LATC = 0x00;

    /**
    TRISx registers
    */
    TRISE = 0x07;
    TRISA = 0xFF;
    TRISB = 0xC7;  //b11000111
    TRISC = 0x07;  //b00000111
    TRISD = 0xFC;  //b11111100

    /**
    ANSELx registers
    */
    ANSELD = 0xFF;
    ANSELC = 0xFF;
    ANSELB = 0xFF;
    ANSELE = 0x07;
    ANSELA = 0xFF;

    //CLOCK
    T0CON0 = 0x90;
   
    //T0CON1 = 0x7F; // not_syncronized and prescale = 1:32768
    T0CON1 = 0x6F; // syncronized and prescale = 1:32768
   
    //FOSC/4
    //T0CON1 = 0x40
   
    //TIMER0
    TMR0H = 0xFF;
 
    TMR0L = 0XFF;

 
} 


I have tried every variation I can think of but something is going on in how readTimer0() and getTime() work that I don't understand correctly.  It's seems logical that timer0 increments 1.024ms with an overflow every 67.12 s and starts over.  getTime() reads the timer compares it with last_time, the result of which is tested against led_on_time and led_off_time.

The problem is me for sure here and but after 3 days of hair loss I'm here again asking for help.

Jerry

[DavidAlfa]

Where are you enabling the timer? 
Check T0ON bit

Step the code in a debugging session, watching the variables, you'll detect what's wrong easily, ex. timer not ticking.
An advice: Make it simplest as possible for now!
Try toggling just one led.
When you get it working, proceed with extending the program.

[jerryk]

I believe T0CON0 = 0x90; covers turning the timer0 on and sets it to 16 bit.

I have tried toggling one at a time just using led_time_on.  That works and I get even 500ms on/off pulse.  The next step is to try to vary the off state using led_time_off.  That's where the problems start.  It seems that the last_time variable is not being set correctly.

Jerry

[DavidAlfa]

That's because I messed up badly the example code I posted earlier


First, by reading Timer 0  in the wrong order. Quoting the datasheet:

TMR0H is not the actual high byte of Timer0 in 16-bit mode.
It is actually a buffered version of the real high byte of Timer0, which is neither directly readable nor writable (see Figure 26-1).
TMR0H is updated with the contents of the high byte of Timer0 during a read of TMR0L.
This provides the ability to read all 16 bits of Timer0 without having to verify that the read of the high and low byte was valid, due to a rollover between successive reads of the high and low byte.

So TMR0L must be read first! Otherwise, TMRH will never get updated, and it will work like a 8-bit timer.
When the code detects the overflow, and does (65536-last)....last was 255 at best.
In that moment, it screwed up the timings, reporting 65seconds had passed since last call.

Also, it's never a good idea to enable and configure a peripheral in the same transfer, or reconfogure it while enabled.
You were doing both .
It's safer to initialize like this, enabling the peripheral on last place:

Code: [Select]

void configTimer0(void){

    // Clear timer counters
    TMR0H=0;
    TMR0L=0;

    // Set config (16bit, postscaler=1:1)
    T0CON0 = 0x10;   

    // Configure clock (syncronized with FOSC/4, prescale = 1:8192, Clock=HFINTOSC/4)
    // For 32MHz, F=8MHz, Timebase=8MHz/8192 = 1.024ms
    T0CON1 = 0x4D; 

    // Enable timer
    T0CON0bits.T0EN = 1;
}


The rest of the code was wrong in many other ways (My fault).

Reworked the code. Now works with absolute 32-bit time, can hold up to 49 days with 1ms precision.
So everytime you call getTime(), you get absolute 32bit time value.
Fixed some of your code to work with this timers and properly converted the LEDs to simple macros
Check here:
https://pastecode.io/s/o4q28qmn

[TomS_]

Be careful with readTimer0(), I think you are performing a non-atomic operation there.

Its entirely possible for the high byte of the counter to roll over once you have read it and ORed it together with the low byte, resulting in an invalid reading.

You might need to look at stopping the timer to take the reading, and then restart it. This could, of course, throw off any timing that you are trying to perform. So use it with care.

[DavidAlfa]

Nope, read the datasheet quote.
TMR0H is updated *only* when TMR0L is read.
You could keep reading only TMR0H, it wouldn't change even in 1000 years.

[TomS_]

Neat. I guess they've learned a few lessons with the newer PICs.

At least I was pretty sure thats how it worked on older PIC16's anyway. Maybe Im wrong.

[Ian.M]

Nope.  'Classic' midrange PIC12/16 (original 14 bit core) parts only had an 8 bit timer 0 so atomicity of reads or writes was never an issue.  You are probably mis-remembering the pain of accessing the 16 bit Timer 1 on that core.  It had two totally unbuffered registers TMR1L and TMR1H, and you had to deal with rollover in your code.  The algorithm for reading it reliably while it was running was:

Code: [Select]

uint16_t readtimer1(){
   uint8_t t1h, t1h2, t1l;

   t1h=TMR1H;
   t1l=TMR1L; // timestamp here
   t1h2=TMR1H;

   if(t1h!=t1h2) // got a rollover
      if(!(t1l&0x80)) // high bit of low byte clear
                      // => rollover before t1l read
         t1h=t1h2; // use second read of high byte

   return 256UL*t1h|t1l;
}Writing it was more problematic, as an increment during the write operation would be lost or if the timer was in externally clocked asynchronous mode, could even corrupt the value written.   If you knew the minimum T1CKI input period was sufficient, you could wait for bit 0 of TMR1L to toggle then update both TMR1L and TMR1H before it could increment again, otherwise it was safest to stop the timer, update it then restart it.
 

[TomS_]

Hmm yeah. Been a long time since I mucked around with PIC16. Life became so much easier when I moved to PIC32. 

[DavidAlfa]

Not sure about that, handling some pic32s was a nightmare due the erratas

[jerryk]

So the problem I'm having now is blocking code on two different timers that I'm experimenting with.
Timer0 uses @DaveAlfa's continuous overflowing timer that increments 1.024ms and Timer1 uses an interrupt that triggers every 1ms.  They both are working but the problem occurs once I try to refresh the LCD.  It takes 24.96ms to do a complete refresh because of the __delay_us() function in the LCD library functions.  This adds time to the delay constants (led_off/on_time) that I am using in both Handle_LED functions.  How much time is added depends on where the timer is when it exceeds the test conditions

Code: [Select]

if ((getTime() - last_time) < led_off_time1) or

Code: [Select]

if (led_count < led_off_time2)

I have tried to trim out all the delay time I can in the LCD library functions but this is about as close as I can trim in on this slow of device.

Code: [Select]

/*****************************************
    Main application
*****************************************/
void main (void){

    //Initialize functions
    SYSTEM_Initialize(); // Timer0 Internal_clock, prescaler_32768, postscaler_1);
    Lcd4_Init();
    configTimer0();
    configTimer1();
   

    while(1) {
        Handle_LED1();
        Handle_LED2();
        //Handle_ADC();
        //Handle_LCD();
    }
}
void Handle_LED1(void)
{
    static uint16_t last_time = 0;
    static uint8_t status = 0;
   
    // Check the application's current state.
    switch (status)
    {
        // Application's initial state.
    case led_init:
    {
        status = led_wait;
        break;
    }

    case led_wait:
    {
        if ((getTime() - last_time) < led_off_time1)
           
        {
            return;
        }

        last_time = getTime();
        led6_Toggle();//Toggle LED
        status = led_blink;
        break;
    }
    case led_blink:

    { 
        if ((getTime() - last_time) < led_on_time1)
        {
            return;
        }
        last_time = getTime();
        led6_Toggle(); //Toggle LED
        status = led_wait; // return to Wait state   
        break;
    }
 
    default:
    {
        break;
    }
    }
   
}

void Handle_LED2(void)
{
    static uint8_t led_status = led_init;

    switch (led_status)
    {
        // Application's initial state.
    case led_init:
    {
        led_count = 0;
        led_status = led_wait;
        break;
    }

    case led_wait:
    {
       if (led_count < led_off_time2)       
       {
           return;
       }
        led_count = 0;
        led5_Toggle();//Toggle LED
        led_status = led_blink;
        break;
    }
    case led_blink:

    { 
        if (led_count < led_on_time2)
        {
            return;
        }
        led_count = 0;
        led5_Toggle(); //Toggle LED
        led_status = led_wait; // return to Wait state   
        break;
    }
 
    default:
    {
        break;
    }
    }
}



/*
void Handle_LED(void){

    if ( elapsed < led_time){
        return;
    }

    last_time = now;

    // Do LED stuff
    // Everytime you get here you know "led_time" time has passed
}

*/
void Handle_ADC(void){
    static uint32_t last_time = 0;
    static uint8_t channel = 0;
    static uint8_t adc_status = adc_idle;
    static uint32_t voltage[4];
   
    // Do ADC stuff
    switch (adc_status)
    {

    case adc_idle:

        adc_status++;
        //ADPCH  = 5 - channel;
        // __delay_ms(10);
        break;

    case adc_sampling:

        adc_status++;
        ADCON0bits.ADGO = 1;
        break;

    case adc_converting:
        for (uint8_t i = 0; i < 4; i++)
        {
            voltage[i] = GET_Voltage(resistorRatioCell[i], ADC_Read(5-i));
        }

        //Converts readings at the balance wires to individual cell voltage
        for (uint8_t i = 0; i < 3; i++)
        {
            cell_voltages[i] = voltage[i] - voltage[i + 1];
        }
        cell_voltages[3] = voltage[3];

        adc_status = adc_all_done;
        break;

    case adc_all_done:
       
        adc_status = adc_idle;
        break;
    }

    //
}

void Handle_LCD(void)
{

    static uint8_t lcd_status = lcd_wait;

    switch (lcd_status)
    {
    case lcd_wait:

        if (lcd_count < lcd_delay_time)
        {
            return;
        }
       
        lcd_count = 0;
        lcd_status = lcd_display;
       
        break;

    case lcd_display:

/*     This contains 24.96 ms via __delay_us() function to refresh all four voltage readouts to LCD */

        LCD_Display(cell_voltages[0], 1, 0);
        LCD_Display(cell_voltages[1], 1, 8);
        LCD_Display(cell_voltages[2], 2, 0);
        LCD_Display(cell_voltages[3], 2, 8);

        lcd_status = lcd_wait;

        break;
    } 
}

void LCD_Display(uint32_t cell_voltages, uint8_t row, uint8_t column) {
    volt_str[0] = digit[ ((cell_voltages / 100000) % 10) ];
    volt_str[2] = digit[ ((cell_voltages / 10000) % 10) ];
    volt_str[3] = digit[ ((cell_voltages / 1000) % 10) ];
    Lcd4_Set_Cursor(row, column);
    Lcd4_Write_String(volt_str);
}

 


Code: [Select]

/***************Relevant LCD functions*****************/


void Lcd4_Set_Cursor(unsigned char a, unsigned char b)
{
    unsigned char temp, z, y;
    if(a == 1)
    {
      temp = 0x80 + b;
        z = temp >> 4;
        y = (0x80 + b) & 0x0F;
        Lcd4_Cmd(z);
        Lcd4_Cmd(y);
    }
    else if(a == 2)
    {
        temp = 0xC0 + b;
        z = temp >> 4;
        y = (0xC0 + b) & 0x0F;
        Lcd4_Cmd(z);
        Lcd4_Cmd(y);
    }
}

void Lcd4_Write_Char(char a)
{
   char temp,y;
   temp = a & 0x0F;
   y = a & 0xF0;
   RS = 1;             // => RS = 1
   Lcd4_Port(y >> 4);  //Data transfer
   EN = 1;
   __delay_us(500);
   EN = 0;
   Lcd4_Port(temp);
   EN = 1;
   //__delay_ms(2);
   __delay_us(40);
   EN = 0;
}

void Lcd4_Write_String(char *a)
{
    int i;
    for(i=0; a[i] != '\0'; i++)
     Lcd4_Write_Char(a[i]);
}



Any suggestions on how to avoid these blocking code delays?

Jerry

[DavidAlfa]

Is it the usual hd44780?
500us for EN pulse delay is crazy, 1-5us should be enough.
You only have to wait for the LCD instruction time (write RAM = 40us).
So at worst it'll be 50us per char, instead 540+.

Some optimization:

Code: [Select]

void Lcd4_Write_String(char *a)
{
    int i;
    for(i=0; a[i] != '\0'; i++)
     Lcd4_Write_Char(a[i]);
}
void Lcd4_Write_Char(char a)
{
   char temp,y;
   temp = a & 0x0F;
   y = a & 0xF0;
   RS = 1;             // => RS = 1
   Lcd4_Port(y >> 4);  //Data transfer
   EN = 1;
   __delay_us(500);
   EN = 0;
   Lcd4_Port(temp);
   EN = 1;
   //__delay_ms(2);
   __delay_us(40);
   EN = 0;
}


Code: [Select]

void Lcd4_Write_String(char *a)
{
    RS = 1;             // => RS = 1
    while(*a)
      Lcd4_Write_Char(*a++);
}
void Lcd4_Write_Char(char a)
{
   Lcd4_Port(a >> 4);  //Data transfer
   __delay_us(1);      // 1us to let data settle
   EN = 1;
   __delay_us(1);      // 1us EN high pulse
   EN = 0;

   __delay_us(1);      // let EN settle before writing data again

   Lcd4_Port(a & 0x0F);
   __delay_us(1);
   EN = 1;
   __delay_us(1);
   EN = 0;

   __delay_us(40);     // RAM Write execution time
}


Handle_ADC is all messed up (no ADC Code in it?) and you're using uint16 for the time... It'll overflow after 65 seconds! The code uses absolute time, it'll cause issues unless you modified something else.

Anyways, that's a microcontroller.
24ms every 500-1000ms (LCD update rate) is pretty ok.

[jerryk]

Is it the usual hd44780?
500us for EN pulse delay is crazy, 1-5us should be enough.
You only have to wait for the LCD instruction time (write RAM = 40us).
So at worst it'll be 50us per char, instead 540+.

The chips are potted but my assumption is it's using the hd44780.  That is what the library(so called library) code I'm using for the LCD is based on. 

I am at a loss on the EN timing needing so much time. I have tried to lower the EN pulse delay but so far anything below 500us makes the display garble.  I have a feeling it's because I'm not checking the busy flag on D7 and have RW tied to ground rather than controlled by the pic.  I'm also using the display in 4 bit mode if that adds to the timing.  I assume it would but only minimally.

Handle_ADC is all messed up (no ADC Code in it?) and you're using uint16 for the time... It'll overflow after 65 seconds! The code uses absolute time, it'll cause issues unless you modified something else.

Including the messed up adc code in the previous post was unintentional.  I have put off working on that part off until I sort out the LCD delays.  But since you caught the 67 second roll over, I did that just so I could see if there was any errors at the time of roll over.  I'll be heading to improving my understanding of the  ADC part once I'm more comfortable with working with timers in general.

Anyways, that's a microcontroller.
24ms every 500-1000ms (LCD update rate) is pretty ok.

This may answer a very general question that I have.  I'm I correct in thinking that no matter if I trim down the LCD instruction delays, there will be still some delay in my timing because blocking code will always exist to some degree.  That the goal is to reduce that delay and any associated code that blocks with FSM coding or other programming techniques so that delays are minimized to an acceptable level.  Is that what you mean by "that's a microcontroller"?

Anyway I'll see what the optimization you posted does and sure appreciate the help.

Jerry

[DavidAlfa]

You could add more states to the LCD so it sends one char at a time, greatly reducing the blocking and the timing jittering on the leds.

As you see, additional checks need to be done to ensure the ADC doesn't make a new conversion until the LCD is done.

There's some pseudo code but it's easy to understand.

Code: [Select]

char text[] = ”11.1 V";
uint8_t lcd_status, adc_status, led_status;
uint16_t adc_value[4], cell_voltage[4];

void main (void){
    while(1){
        Handle_LED();
        Handle_ADC();
        Handle_LCD();
    }
}

void Handle_ADC(void){
    static uint32_t last_time = 0, sample_time = 0;
    static uint8_t channel = 0;
    switch (adc_status){

        case ADC_channel_set:
            // Select ADC Channel etc
            sample_time = getTime();
            adc_state++;
            break;

        case ADC_sampling:
            if ( (getTime()-sample_time) > adc_sample_time ){
                // Start ADC conversion
                adc_state++;
            }
            break;

        case ADC_converting:
            if(conversion_done){
                adc_value[channel] = ADC_VALUE;
                if(++channel > adc_channels){
                    channel = 0;
                    adc_status = ADC_all_done;
                    // Convert adc_value[] and save to cell_voltage[]
                }
                else{
                    adc_status = ADC_channel_set;
                }
            }
            break;

        case ADC_all_done:
            if ( (getTime()-last_time) > adc_time ){
                last_time = getTime();
                adc_state = ADC_restart;
            }
            break;

        case ADC_restart:
            if(lcd_status == LCD_idle){        // Wait until LCD finished
                    adc_state = ADC_channel_set;
                }
            }
            break;
}

void Handle_LCD(void){

    static char * str;
    static uint32_t last_time = 0;
    static uint8_t cell = 0;

    switch (lcd_status){

        case LCD_idle:
            if ( (getTime()-last_time) > lcd_time ){
                last_time = getTime();
                lcd_status++;
            }
            break;

        case LCD_start:
            if(adc_status == ADC_all_done){        // Wait until ADC finished
                    lcd_status++;
            }
            break;

        case LCD_prepare:
            LCD_setCursor(cell); // as per current cell number
            // Other lcd commands required before sending the string
            Create_string(cell_voltage[cell]);
            str = text;
            RS = 1;
            lcd_status++;
            break;

        case LCD_send:
            if(*str){
                LCD_WriteChar(*str++);  // Write one char at a time
            {
            else{
                if(++cell > cell_number){   // all cell strings sent
                    cell = 0;
                    lcd_status = LCD_idle;
                }
                else{
                    lcd_status = LCD_prepare;    // prepare next cell string
                }
            }
            break;
    }
}


What do I mean by "it's just a mcu"?
Well, a mcu usually has small memories.
Creating good non-blocking code might use more program space.
So if the blocking way is acceptable, you can save a lot of it.


If you need better timing control for the leds, you could run another timer that overflows every 10ms and enable the interrupt, then it's just matter of using counters to determine when a led should be modified.

[jerryk]

I'm making progress in learning timers but a difficulty I have is in using the debugging tools to analyze/measure elapsed time.  I tried a work around by toggling an output pin on both sides of a section of code and using the oscilloscope to measure.  It works with exceptions.

In the code below I'm trying to measure the elapsed time of the two lines between setting LATD1 on then off.

Code: [Select]

    LATDbits.LATD1 = 1;
    Lcd4_Set_Cursor(row, column);
    Lcd4_Write_String(volt_str);
    LATDbits.LATD1 = 0;

The associated functions have delay blocking code via __delay_us(n) detailed below.

Code: [Select]

void Lcd4_Set_Cursor(unsigned char a, unsigned char b)
{
    unsigned char temp, z, y;
    if(a == 1)
    {
      temp = 0x80 + b;
        z = temp >> 4;
        y = (0x80 + b) & 0x0F;
       
        Lcd4_Cmd(z);
        Lcd4_Cmd(y);

        //__delay_us(1600);
    }
    else if(a == 2)
    {
        temp = 0xC0 + b;
        z = temp >> 4;
        y = (0xC0 + b) & 0x0F;
        Lcd4_Cmd(z);
        Lcd4_Cmd(y);
        //__delay_us(1600);
    }
}

void Lcd4_Cmd(unsigned char a)
{
    RS = 0;             // => RS = 0
    Lcd4_Port(a);
    EN  = 1;             // => E = 1
    //LATDbits.LATD1 = 1;
    __delay_us(400);
    EN  = 0;             // => E = 0
    //LATDbits.LATD1 = 0;
    __delay_us(400);
}

void Lcd4_Write_String(char *a)
{
    RS = 1;             // => RS = 1
    while(*a)
      Lcd4_Write_Char(*a++);
}
void Lcd4_Write_Char(char a)
{
    const char dt = 1;
   Lcd4_Port(a >> 4);  //Data transfer
   __delay_us(dt);      // 1us to let data settle
   EN = 1;
   __delay_us(dt);      // 1us EN high pulse
   EN = 0;

   __delay_us(dt);      // let EN settle before writing data again

   Lcd4_Port(a & 0x0F);
   __delay_us(dt);
   EN = 1;
   __delay_us(dt);
   EN = 0;

   __delay_us(40);     // RAM Write execution time
}

The goal was to turn LATD1 on then execute the two functions and embedded functions the turn off LATD1.  Using the scope I thought I would see the sum total of all time elapsed for both code execution and blocking code delay statements.

This not happening.  The elapsed time in the first code block above is just 6.4us.  Do the LATn register bits clear when you call another function?  Is there a better way to measure time with a scope?

Jerry 

[Ian.M]

It depends . . .
If the other code doesn't touch Port D, then no, they wont change in their own.

However badly written LCD library code might well change other pins on the same port.

[jerryk]

The problem appears to occur in this function included in my previous post.

Code: [Select]

void Lcd4_Cmd(unsigned char a)
{
    RS = 0;   //   ***** problem occurs here. When pin RD2 (defined as RS) is set to zero it also sets RD1 to zero.  Why?
    Lcd4_Port(a);
    EN  = 1;             // => E = 1
    //LATDbits.LATD1 = 1;
    __delay_us(400);
    EN  = 0;             // => E = 0
    //LATDbits.LATD1 = 0;
    __delay_us(400);
}

When pin RD2 (defined as RS) is set to zero it also sets RD1 to zero.  Relevant settings -- ANSD1 = 1 in the analog select ALSELD register and TRISD1 = 0.  Changing ANSD1 = 0 fixed the problem.  Why??  I'm not sure how the setting of RD2 (RS) to zero could effect RD1. 

More things to learn - Jerry

[Ian.M]

Because the 'short form' pin name defines are mapped to bits in the PORT registers so they can be used for both input and output for compatibility with legacy code, so its manipulating the PORTD SFR bit, and as 8 bit PICs implement BSF and BCF instructions as read byte from register, modify specified bit and write back whole byte to register, any other bit(s) that transiently or otherwise read as a level other than the state they were last set to will be corrupted.   That's why Microchip introduced LAT registers on the PIC18 and back-ported the newer I/O structure to the 14 bit core when they developed the Enhanced Midrange ( PIC12/16(L)F1xxx(x) ) family devices, as a LAT register always reads back what was last written to it, irrespective of the state of the pins or any peripheral modules.

Its a well known problem generally known as the "Read-Modify-Write effect" (aka: "RMW effect") that mostly went away when the Baseline and 'classic' (old) Midrange core PICs were superseded by the newer families.

See the seminal topic: RMW and solutions for it @microchip.com.  Unfortunately some of its links are suffering from bitrot (It *IS* a 2010 topic so positively geriatric by internet forum standards), and are best viewed on the Internet Archive.

Show us the LCD pin mapping #defines and we can tell you how to fix them. 

[DavidAlfa]

How is RS declared? Should be defined as LATx, not RAx.

What's the difference between LAT and PORT registers?
RAx reads the input buffer, connected to the output, while LAT reads the output latch, unaffected by whatever it's connected to the pin.
Imagine set a pin to 1, and inmediately read it.
LAT register will read 1, but PORT might still read 0 because the output didn't had enought time to rise.

Another example: Your output gets shorted to gnd.
If you toggle the output using RAx, your code will always see a 0, setting the pin to 1.
But if you do that using LAT, the code will correctly do the toggling, although, obviously, the pin voltage will never change.

So, the read-modify-write issue happens only when using PORT register.

[jerryk]

RS as well as all of the LCD pins were associated with RDx pins.  I have changed them all to LATDx.  The only thing that did fix the problems was changing (clearing) the appropriate ANSELx register bits to digital.

Some of the settings are the result of bad copy paste habits from other projects combined with some of the MCC code from previous projects.  Setting led pulses in my code is helping out considerably in my understanding of how the timing is executed.

Thanks for the education.

Jerry

[DavidAlfa]

Remember you can use the simulator in mplab, that way you'll know exactly the time and clock cycles between your breakpoints.
There're also other simulators like Proteus, but I don't know if the demo version lets you use the Pic simulator, anyways , for your own personal non-profit use, can be found everywhere...

If you wish, upload the code somewhere and I'll have a quick look