Long delay causes error in ADC reading - PIC16F1937)

Discussion in 'Embedded Systems and Microcontrollers' started by Stuntman, Jun 27, 2014.

  1. Stuntman

    Thread Starter Active Member

    Mar 28, 2011
    181
    47
    So I was working with a student on reading an analog input (linear pot). The end goal was to read the 10-bit ADC value, then interpret this value as to how long (in arbitrary units) to turn on and off an LED.

    Here is the code:


    Code ( (Unknown Language)):
    1.  
    2.  
    3. void main(void)
    4. {
    5.  
    6. init(); //  Initialization code to setup ports, oscillator, etc.
    7.  
    8.  
    9. while(1)
    10. {
    11.     ADCvalue=ADCpoll(13); // Get ADC Value
    12.     time_Switch=(long)ADCvalue*10;//  Multiply the ADC value by scaler
    13.  
    14.     lcdwrite(time_Switch,0);//Displays time_switch on LCD
    15.  
    16.    RC1=1; //Turn off LED
    17.  
    18.     for(time_ON=0; time_ON<=time_Switch; time_ON++) //Wait time_switch
    19.     {
    20.        
    21.     }
    22.    RC1=0;  //  Turn On LED
    23.     for(time_OFF=0; time_OFF<=time_Switch; time_OFF++)  //Wait time_switch
    24.     {
    25.  
    26.     }
    27.  
    28.  
    29. }
    30. }
    31.  
    32.  
    This code will work just as expected. However, if you change

    Code ( (Unknown Language)):
    1. time_Switch=(long)ADCvalue*10;
    to
    Code ( (Unknown Language)):
    1. time_Switch=(long)ADCvalue*100;
    The ADC no longer reads properly. Both the LCD and the flashing LED hint that the ADC value randomly jumps from say 512, down to 234, then back to 502 etc.

    In addition, when things got shakey, the ADC value will not make it all the way to 1023 even at full voltage.

    My first thought was data type issues. So we typecasted everything, removed the LCD function, and finally reverted back to the *10 multiplier, and repeated the for loop 10 times (emulating the 100x multiplier).

    Nothing worked. Even hardcoding the time_switch = 102300 made the thing go crazy.

    Any ideas on what to try next? It's almost like the for loop cannot handle this many iterations. But even if it can't, why would it mess up my ADC values?

    Interested in any thoughts.

    Thanks
     
  2. takao21203

    Distinguished Member

    Apr 28, 2012
    3,577
    463
    Do you have a stabilized supply for the ADC?

    I use for instance a 78L05, with 1/2 divider.

    The regular supply might be too much polluted. What do you use as power supply?
     
  3. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,386
    1,605
    Not much to go on. You don't spec the compiler you use, nor give a hint as to how any of your values are defined.

    How big is a "long"?

    Finally, I believe the type cast binds first, but you might want to try:

    Code ( (Unknown Language)):
    1.  time_Switch=((long)ADCvalue)*100;
     
  4. tshuck

    Well-Known Member

    Oct 18, 2012
    3,531
    675
    Have you tried casting it as:

    Code ( (Unknown Language)):
    1. time_Switch=((long)ADCvalue)*100;
    Also, your LCD write function may only work for integer-sized variables...

    Edit: The_RB beat me to the cast suggestion...

    Edit #2: what is the size of time_ON and time_OFF?
     
  5. Stuntman

    Thread Starter Active Member

    Mar 28, 2011
    181
    47
    takao21203 - The board has a LT1121 linear 5V reg and is sufficiently decoupled. Also, this "error" comes from LONG delays between ADC polls, never during shorter, more frequent polling, which would seem odd for true noise.

    Ernie - The variables are declared as:
    unsigned int ADCvalue;
    long time_Switch;
    long time_ON;
    long time_OFF;

    Compiler is Hitech C 10-12-16 9.83

    My first thought was data type as well. But consider that we rewrote the function using the *10 multiplier, then running the FOR loop 10 times (nested). Doing this should negate overrun on data types no?

    Long is 32 bits and should range +/- 2,147,483,647

    tshuck - My LCD function limits out when the numbers are too large (>9999999). Even if that were an issue, that should not be redefining the time_switch (IE, the LED's should not be flashing wrong just because the display is incorrect). I thought of this as well, so as a "just in case" I actually removed the LCDwrite function and still, the LED's blinked haphazardly with the 100 multiplier. Time_ON and Time_OFF have all been defined as long. When we first encountered this error, the first thing I did is define all the variables to long thinking it would fix the issue.


    I look at the entire problem like this:

    All the variables are declared as LONG having sufficient room for 1023*100(102300). I would say it's just the display messing up/converting to BCD wrong, but the LED's are reciprocating this issue. Then the opposite is true, if the FOR loop is not executing as I expected, why would that have anything to do with the ADC value or the time_switch which is being shown on the LCD.

    Thank you for the suggestions thus far.

    P.S. I will try the typecasting in parenthesis, but know that I attempted typecasting everything at one point to no avail.
     
  6. THE_RB

    AAC Fanatic!

    Feb 11, 2008
    5,435
    1,305
    I'd love to take the credit for the 1 minute gazump but it was actually ErnieM.

    All us smartass geeks look the same... ;)
    :D:D
     
  7. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,386
    1,605
    While we're all still fishin' for the root problem here, could you please post the code for the ADCpoll() function?

    Might as well zip up the whole program and attach that too.


    And here I thought I was the handsome one.
     
  8. tshuck

    Well-Known Member

    Oct 18, 2012
    3,531
    675
    Oh, whoops!:(

    That's what happens when I try to recognize a person by their avatar when trying to remember which colors were there without looking again...

    Sorry, about that Ernie, the credit is yours!:p

    @OP: declare time_ON and time_OFF as volatile long... your program may be optimizing those statements out...
     
  9. THE_RB

    AAC Fanatic!

    Feb 11, 2008
    5,435
    1,305
    Code ( (Unknown Language)):
    1.  
    2.   lcdwrite(time_Switch,0);   //Displays time_switch on LCD
    3.  
    Normally lcdwrite(,) requires a pointer to a text string. This looks to be trying to display a numerical variable directly?
     
  10. BobTPH

    Active Member

    Jun 5, 2013
    782
    114
    Looks like a possible compiler bug to me. The behavior sounds like what would happen if the value was truncated to 16 bits. Which it should not be, given the declarations and code you have shown us.

    I would debug the code and check the values after assignment. Also check the actual iterations of the for loop, the bug could be there.


    Bob
     
  11. Stuntman

    Thread Starter Active Member

    Mar 28, 2011
    181
    47
    ErnieM -

    Code ( (Unknown Language)):
    1. unsigned int ADCpoll (unsigned char ch)  //  Use to Poll ADC port
    2. {
    3.  
    4.  
    5.     if(ch>13) return 0;  //Invalid Channel
    6.  
    7.     ADCON0=0x00;
    8.  
    9.     ADCON0=(ch<<2);   //Select ADC Channel
    10.  
    11.     ADON=1;  //switch on the adc module
    12.  
    13.     GO_nDONE=1;//Start conversion
    14.  
    15.     while(GO_nDONE); //wait for the conversion to finish
    16.  
    17.     ADON=0;  //switch off adc
    18.  
    19.     return ADRES;
    20.  
    21. }
    THE_RB -
    The lcdwrite() is a function I wrote for the 3 digit LCD w/2 DP. I wanted to do everything in fixed point, so this lcdwrite takes either a long or int

    Code ( (Unknown Language)):
    1. void lcdwrite (long output, int out)
    in .0001 and .01 decimal place respectively and does all the truncation and decimal point assignments. I have a hard time blaming this function as A.) it does not modify time_switch and I have even removed the function and the code still falters in seemingly the same fashion.

    To clear the air, I need to retry simply having the LCD print ADCvalue without the scaler multiplication and see how it reacts. I believe I did this for most of my testing, but want to make sure. I will also try the typecasting in parenthesis.
     
  12. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,386
    1,605
    re the ADCpoll() function:

    1) For an invalid ch input the function returns the perfectly legitimate value of zero.

    B) Each time you go thru you turn the A2D on and off again. Just turn it on and leave it on, it only uses a small amount of current.

    III) After selecting (and connecting) the channel to read there needs be a small delay to allow the internal cap to charge. This isn't the problem with your code as you never change the input, but one day it will be the problem if you reuse this function.

    I still don't see what may be causing the problem, but you only provide very small snapshots of your code which makes it very hard to see. If you zip up the entire project someone may be able to put a finger on it.
     
  13. Stuntman

    Thread Starter Active Member

    Mar 28, 2011
    181
    47
    Ernie - Thank you for the feedback. The ADC module turn-on/off is just a creature of habit. I never knew if there were more serious repercussions for leaving it on, now I know. I will add this delay. I take it this is necessary after turning on the ADC module OR switching channels?

    So I tried the code changes I spoke of last e-mail and truncated the code to a more compact version that still causes issue:

    Code ( (Unknown Language)):
    1.  
    2. unsigned int ADCvalue;
    3. long time_Switch;
    4. long time_ON;
    5. long time_OFF;
    6.  
    7. void main(void) ///  Where main code starts
    8. {
    9. init(); //  Initialization code to setup ports, oscillator, etc.
    10.  
    11. while(1)
    12. {
    13.  
    14.     ADCvalue=ADCpoll(13);
    15.     lcdwrite(0,ADCvalue);
    16.     (long)time_Switch=((long)ADCvalue)*100;
    17.  
    18.    
    19.     RC1=1;
    20.     for(time_ON=0; time_ON<=time_Switch; time_ON++)
    21.     {
    22.     }
    23.   /*  RC1=0;
    24.     for(time_OFF=0; time_OFF<=time_Switch; time_OFF++)
    25.     {
    26.     }
    27. */
    28.  
    29. }
    30.  
    31. }
    Note, even with the second FOR loop commented out, the error is present.

    If I turn the potentiometer up to 5V (AN13), and use a the *10 multiplier (instead of the 100), I get a reasonable readout on the LCD (~1010). By changing the multiplier to 100, the ADC value (as shown on LCD) changes to around 800 and oscillates down to near the 600 level.

    This code is quite long and contains many custom function I have implemented on this board. I could certainly trim it down and package if that would help.
     
  14. NorthGuy

    Active Member

    Jun 28, 2014
    603
    121
    Sorry for joining the thread late.

    You completely missed the acquisition step in your code. Therefore, the sampling capacitor never gets charged to the full voltage.

    Read the "15.4 A/D Acquisition Requirements" in the datasheet.
     
  15. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,386
    1,605
    Switching channels is the important place to add the delay. You have to allow the internal cap of the A2D to fully charge to the input voltage. (See the data sheet for an overly complicated explanation (where 1/2 LSB means the largest change that doesn't affect the output state) or just go with their recommend minimum.

    I ran the code you posted thru the MPLAB simulator using the XC8 compiler, which I believe internally is the same as Hitech C. I made a dummy A2D routine to return the max 1023 value, and commented out the LCD thing.

    Since there were no runs, no hits, and no errors I would suggest you make sure the delay inside your A2D read routine is adequate, and STOP TURNING THE DANG THING ON AND OFF. :D
     
Loading...