Reading from an Analog Port-PIC16F877A

Discussion in 'Programmer's Corner' started by ActivePower, Jul 10, 2012.

  1. ActivePower

    Thread Starter Member

    Mar 15, 2012
    155
    23
    Well, after I had LEDs blinking away on my board in various fashions I wished to progress to reading analog inputs from sensors (in this case, a photoresistor).

    I couldn't find some example code for the Hi-Tech C compiler off the internet so I wrote one using the PIC datasheet and the reference manual and some sample code in an word document I found online. It builds successfully, however, I still am not convinced that it is correct. Here it is:

    Code ( (Unknown Language)):
    1. //main.c
    2. //To read a photoresistor input and flash an LED if it is dark
    3.  
    4. #include<htc.h>
    5. #define _XTAL_FREQ 20000000
    6.  
    7. __CONFIG(FOSC_HS & CP_OFF & CPD_OFF & LVP_ON & PWRTE_OFF & DEBUG_ON & WDTE_OFF & BOREN_OFF & WRT_OFF);
    8.  
    9. void main(void)
    10. {
    11.  
    12.     while(1)
    13.     {
    14.     unsigned int photo=0;
    15.                     //16 bit integer(0-65536)
    16.     TRISB=0x0;
    17.                         //PORTB configured as output
    18.     TRISA=0b11111111;
    19.                 //set TRISA registers to input
    20.     ADCON1=0b00000000;      //all pins are analog; result is left justified
    21.  
    22.         ADCON0=0b10000001; 
    23. //switch on ADC module; sets clock to 32TOSC; input at RA0; GO bit is low  
    24.  
    25.     __delay_us(20);             //wait for 20us; acquisition time
    26.     ADCON0=0b10000101;          //start the conversion
    27.         while(ADCON0==0b10000101);  //wait for conversion to complete
    28.     photo=ADRESH;
    29.     if(photo<32768)             //if it is dark
    30.     PORTB=0b10000000;           //light the LED
    31.     }
    32. }
    33.    
    34.    
    35.  
    Would this code work? I still have some qualms regard setting individual bits in registers (see ADCON0 up there).
    If something appears terribly wrong with the code above please point it out and help me fix it.

    Thanks!
     
  2. JohnInTX

    Moderator

    Jun 26, 2012
    2,341
    1,024
    The basic ADC ops seem to be OK but some observations and suggestions for improvement:

    When testing one bit in a register or setting one on a port you should use logical operators to isolate the bit. Its not a big deal here if this is test code but in most cases, registers contain multiple bits that you don't have control over. In those cases, your construct will fail. The same applies to outputting to a port. Your way works only until you hook something else to PORTB. A better way is to define individual bits something like this (your compiler may already have a .h file that defines some of these):

    #define ADC_START_CONVERSION (ADCON0 |= 0x04)
    #define ADC_DONE (~ADCON0 & 0x04)
    #define LED_ON (PORTB |= 0x80)
    #define LED_OFF (PORTB &= ~0x80)
    #define PHOTO_THRESHOLD 32768 // where to turn LED ON
    #define PT_HYSTERESIS 500 // on-off hysteresis

    then something like:

    Code ( (Unknown Language)):
    1.  
    2. ADC_START_CONVERSION;
    3. while (!ADC_DONE);
    You never turn the LED off if the value drops below your threshold and there is no hysteresis. Maybe:

    Code ( (Unknown Language)):
    1.  if (photo < PHOTO_THRESHOLD)
    2.    LED_ON;
    3.  if (photo > (PHOTO_THRESHOLD + PT_HYSTERESIS )
    4.    LED_OFF;
    You also have an implicit cast in photo=ADRESH. I like to explicitly cast, not only for readability but also to make sure that 1) I have documented that I'm making the conversion and 2) that things work as you expect across different compilers and versions of the same compilers.. I KNOW its supposed to be all defined and all but I can cite examples when it wasn't.. So:
    Code ( (Unknown Language)):
    1.  photo = (unsigned int) ADRESH; // explicit cast
    You also should consider typedef to 'uncouple' 'photo' from a specific type in case you ever need to change it

    You have not applied any bounds-checking to your ADC value. Whenever you interface to the 'outside world' its important to account for any and ALL possibilities. What are the expected normal values from the ADC? What are the values if the sensor FAILS and what can you do about it? (Has the sensor failed or is it just REALLY bright?) Can any ADC values generate arithmetic overflows/wraparounds downstream? I realize that you are playing around here and I am piling on, but get into the habit of examining all possibilities early and account for them from the start.

    Picking nits:
    Put your comments either on the source line or before it, not after. Be consistent. Comment like someone after you has to understand what you have done.
    The name 'photo' could be more descriptive. We talkin' Polaroids here or what?
    I like writing 'test' code like its going to be released.. because, it frequently IS released, as soon as it works. I DO like that you have good comments even in your initial efforts.

    Don't take ANY of my criticisms personally. I offer these suggestions based on my experience. You have researched the datasheets and written a working program, (for at least one pass). Well done.
     
    Last edited: Jul 11, 2012
    ActivePower likes this.
  3. ActivePower

    Thread Starter Member

    Mar 15, 2012
    155
    23
    1. Thanks for pointing those out! I now realize that I really need to get used to examining all possibilities if I ever write some useful piece of code.

    2. Are you suggesting that I should use interrupts for any unexpected operation/sensor failure. I have just started reading about them and I am not entirely comfortable as yet. But I did consider the possibility of setting the ADC interrupt. I do not know whether it was the right thing to do. I am still unaware of many things!

    3. How do you apply those? I am afraid I haven't read about them yet.

    4. Also, could you explain the HYSTERESIS stuff up there? I could not exactly make it out.

    Thanks for your suggestions! I will certainly keep those in mind :)

    EDIT: Also the 10-bit data from the ADC is stored in the register pair ADRESH:ADRESL and I could not figure out a way to read both of them together. Are they concatenated during the ADC operation? Would reading the leading register also imply reading the next register?
     
    Last edited: Jul 11, 2012
  4. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    Active power. Have you looked at your PICs include file <pic16f877a.h>. Here you will find that many bits or group of bits are defined as variables. As an example let us take a look at the definitions for the ADCON0 register.
    Code ( (Unknown Language)):
    1. // Register: ADCON0
    2. volatile unsigned char           ADCON0              @ 0x01F;
    3. // bit and bitfield definitions
    4. volatile bit ADON                @ ((unsigned)&ADCON0*8)+0;
    5. volatile bit GO_nDONE            @ ((unsigned)&ADCON0*8)+2;
    6. volatile bit GO                  @ ((unsigned)&ADCON0*8)+2;
    7. volatile bit CHS0                @ ((unsigned)&ADCON0*8)+3;
    8. volatile bit CHS1                @ ((unsigned)&ADCON0*8)+4;
    9. volatile bit CHS2                @ ((unsigned)&ADCON0*8)+5;
    10. volatile bit ADCS0               @ ((unsigned)&ADCON0*8)+6;
    11. volatile bit ADCS1               @ ((unsigned)&ADCON0*8)+7;
    12. volatile bit nDONE               @ ((unsigned)&ADCON0*8)+2;
    13. volatile bit GO_DONE             @ ((unsigned)&ADCON0*8)+2;
    14. #ifndef _LIB_BUILD
    15. volatile union {
    16.     struct {
    17.         unsigned ADON                : 1;
    18.         unsigned                     : 1;
    19.         unsigned GO_nDONE            : 1;
    20.         unsigned CHS                 : 3;
    21.         unsigned ADCS                : 2;
    22.     };
    23.     struct {
    24.         unsigned : 1;
    25.         unsigned : 1;
    26.         unsigned : 1;
    27.         unsigned : 3;
    28.         unsigned : 2;
    29.     };
    30.     struct {
    31.         unsigned : 2;
    32.         unsigned GO                  : 1;
    33.         unsigned CHS0                : 1;
    34.         unsigned CHS1                : 1;
    35.         unsigned CHS2                : 1;
    36.         unsigned ADCS0               : 1;
    37.         unsigned ADCS1               : 1;
    38.     };
    39.     struct {
    40.         unsigned : 2;
    41.         unsigned nDONE               : 1;
    42.     };
    43.     struct {
    44.         unsigned : 2;
    45.         unsigned GO_DONE             : 1;
    46.     };
    47. } ADCON0bits @ 0x01F;
    48. #endif
    This is a snippet from the data sheet
    To do an A/D Conversion, follow these steps:
    1. Configure the A/D module:
    • Configure analog pins/voltage reference and
    digital I/O (ADCON1)
    • Select A/D input channel (ADCON0)
    • Select A/D conversion clock (ADCON0)
    • Turn on A/D module (ADCON0)
    .
    .
    4. Start conversion:
    • Set GO/DONE bit (ADCON0)
    5. Wait for A/D conversion to complete by either:
    • Polling for the GO/DONE bit to be cleared
    (interrupts disabled); OR
    • Waiting for the A/D interrupt
    6. Read A/D Result register pair
    (ADRESH:ADRESL), clear bit ADIF if required.
    7. For the next conversion, go to step 1 or step 2
    as required. The A/D conversion time per bit is
    defined as TAD
    So in order to say select say RE2/AN7 as analog input you can write in your code
    Code ( (Unknown Language)):
    1.  ADCON0bits.CHS=0b111;
    and to start a conversion you can write
    Code ( (Unknown Language)):
    1. GO_nDONE=1;
    and to wait for a conversion to finish write
    Code ( (Unknown Language)):
    1. while(!GO_nDONE);
    as some examples
     
    Last edited: Jul 11, 2012
    ActivePower likes this.
  5. ActivePower

    Thread Starter Member

    Mar 15, 2012
    155
    23
    Thanks! That was a really great tip. Now I know from where to get the names of the register bits for my PIC!

    Just a little query though, I notice that there are four GO/DONE bits defined in the ADCON0 register. Are they all alike or for different situations/bit configurations?
    For example, is it okay if I use the GO bit instead of the GO_nDONE bit?

    Also, some bits go together always like CHS(three bits) and ADCS(two bits). Can they be used as such?

    Thirdly, does the A/D conversion need to take place in an infinite loop always?

    For example, suppose I have a continuous sensor input analog signal based on which I will decide the movement of my motors. At the same time, I need my device to do other stuff such as send some data via a serial connection (just for example, I do not know if it can be done simultaneously). If I enter the super-loop for reading the sensor data, I may never come out of it. How will my program perform the other operations?
    I realize that I am missing some very fundamental MCU operating principles here but I would be glad if anyone could provide some pointers in this regard.

    Thanks!
     
  6. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    All are equal just pick and mixs as you want,
    Here I can see I made a mistake. I was writing and waiting in a phone cue at the same time. So to be clear
    Then defined this way
    Code ( (Unknown Language)):
    1. volatile bit GO_nDONE            @ ((unsigned)&ADCON0*8)+2;
    2.  

    You can write like this
    Code ( (Unknown Language)):
    1. GO_nDONE=0;
    However for the bitfields like say CHS. They are part of the Ansi C bit-field union like this
    Code ( (Unknown Language)):
    1.  
    2. [LEFT]struct {
    3. unsigned lo : 1;
    4. unsigned dummy : 6;
    5. unsigned hi : 1;[/LEFT]
    6. } foo;
    7.  
    And then correct way of doing this as shown
    Code ( (Unknown Language)):
    1.  
    2. ADCON0bits.CHS=0b101; //I like to use binary numers here but you can use whatever you like hex or decimal
    3. ADCON0bits.ADCS=2;
    4.  
    If you write
    Code ( (Unknown Language)):
    1. ADCON0bits.CHS=0b1101
    you will get a compiler warning
    You can see from here that CHS is 3 bit wide and ADCS is two bit wide
    Code ( (Unknown Language)):
    1. volatile union {
    2.     struct {
    3.         unsigned ADON                : 1;
    4.         unsigned                     : 1;
    5.         unsigned GO_nDONE            : 1;
    6.         unsigned CHS                 : 3;
    7.         unsigned ADCS                : 2;
    8.     };
    9.  
    No the endless loop are just used in some quick and dirty examples. You can program as you want. Take one sample or a number of samples
     
  7. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,386
    1,605
    Of course, the same thing is defined 4 different ways so it makes sense inside your code for the human trying to read it. For example:

    Code ( (Unknown Language)):
    1.  
    2.     GO = 1;            // start a conversion
    3.     while (GO_nDONE);  // wait till complete
    4.     ...
    5.  
    Here I refer to the same bit by two different ways since I am doing two different things with that bit. Setting GO to begin makes human intuitive sense as does checking GO_nDONE for completion.

    I typically have the luxury of having time to waste so I can wait until my reading is complete. To keep your unit actively processing you may instead wish to periodically test GO_nDONE while you also check other inputs or do other work. Once the conversion is done you process it without wasting time for the reading to complete.

    The other way is to set up an interrupt to note the completion, but that can get messy and depending on your compiler it may not be the best thing to do a lot of computation inside the ISR, so you end up setting a flag bit you pole anyway. So just polling for GO_nDONE may well be the simplest way to go.

    FYI, here's a snippet of some production code I wrote. I have a timer interrupt running in the background, and long before that changes state I need to sample some voltages. The voltage is samples 18 times, the highest and lowest readings are tossed away, and the remaining sum of 16 readings is used later on. Here's the code (Sourceboost C compiler):

    Code ( (Unknown Language)):
    1.  
    2.             ReferenceV = 0;
    3.             adcon0.CHS0 = 1;    // A2D_Input = ReferenceV_Pin (AN1)
    4.             delay_ms(2);        // hang here for a few for transients to
    5.                             // settle out before we begin to sample the D2A
    6.  
    7.             // sample the Reference Pin Voltage
    8.             for (LoopCounter = 1;  LoopCounter < D2A_SAMPLE_COUNT; LoopCounter++)
    9.             {
    10.                 delay_us(A2D_DELAY);// allow this sample to settle out
    11.                 adcon0.GO = GO;     // start conversion
    12.  
    13.                  while (adcon0.GO == NOT_DONE);          // wait till complete
    14.                 MAKESHORT( A2D_Result, adresl, adresh );// organize
    15.                 ReferenceV =     ReferenceV + A2D_Result;  // add reading to total
    16.  
    17.                 if (LoopCounter == 1)
    18.                 {
    19.                     // first time thru, use this Reading as both the min and max
    20.                     MinV = A2D_Result;
    21.                     MaxV = A2D_Result;
    22.                 }
    23.                 else
    24.                 {
    25.                     // 2nd or more time thru
    26.                     // check for better min & max in ReferenceV
    27.                     if (MinV > A2D_Result) MinV = A2D_Result;
    28.                     if (MaxV < A2D_Result) MaxV = A2D_Result;
    29.                 }
    30.             }
    31.  
    32.             ReferenceV  = ReferenceV  - MinV  - MaxV;
     
  8. ActivePower

    Thread Starter Member

    Mar 15, 2012
    155
    23
    Here is the code after some modifications:
    Code ( (Unknown Language)):
    1. //main.c
    2. //To read a photoresistor input and flash an LED if it is dark
    3.  
    4. #include<htc.h>
    5. #define _XTAL_FREQ 20000000
    6. #define THRESHOLD_PHOTO 512
    7. #define HYSTERESIS 100
    8.  
    9. __CONFIG(FOSC_HS & CP_OFF & CPD_OFF & LVP_ON & PWRTE_OFF & DEBUG_ON & WDTE_OFF & BOREN_OFF & WRT_OFF);
    10.  
    11. void main(void)
    12. {
    13.  
    14.     while(1)
    15.     {
    16.         unsigned int photo=0;                   //16 bit integer(0-65536)
    17.         TRISB=0;                        //PORTB configured as output
    18.         TRISA=1;                //set TRISA registers to input
    19.        
    20.         ADCON1=0b00000000;              //configure all pins to analog; result is left justified
    21.         ADCON0=0b10000001;              // input at RA0; GO bit is low (default);sets clock to 32TOSC   //switches on ADC module   
    22.         __delay_us(25);                 //wait for 20us; acquisition time
    23.         GO=1;                       //start the conversion
    24.         while(GO_nDONE);                        //wait for conversion to complete
    25.         photo=(unsigned int) ADRESH;
    26.         if(photo<THRESHOLD_PHOTO)                   //if it is dark
    27.             RB0=0b1;            //light the LED
    28.         else if (photo>(THRESHOLD_PHOTO+HYSTERESIS))
    29.             RB0=0b0;
    30.     }
    31. }
    32.    
    33.    
    34.    
    Does it look correct enough?

    EDIT: Tested it. Looks like either the ADC is not reading input from the port or the conversion is not taking correctly, for the LED lights up irrespective of the voltage at RA0! Is this because of the faulty reading from the ADRESH:ADRESL register pair? The result config is left-justified and I haven't figured out a way of reading ADRESL (if it isn't concatenated with ADRESH, I mean, in which case the above code should work (?)) yet.
     
    Last edited: Jul 13, 2012
  9. ActivePower

    Thread Starter Member

    Mar 15, 2012
    155
    23
    Tried a new code (this one off the internet, with some modifications) http://tutorial.cytron.com.my/2011/08/09/project-4-%E2%80%93-analog-sensor-potentiometer-bb-psj/

    Here it goes:

    Code ( (Unknown Language)):
    1. //main.c
    2. //To read a photoresistor input and flash an LED if it is dark
    3.  
    4. #include<htc.h>
    5. #define _XTAL_FREQ 20000000
    6. #define THRESHOLD_PHOTO 512                   //for a 10 bit ADC; midway between 0V-5V
    7.  
    8.  
    9. __CONFIG(FOSC_HS & CP_OFF & CPD_OFF & LVP_OFF & PWRTE_OFF & DEBUG_ON & WDTE_OFF & BOREN_OFF & WRT_OFF);
    10.  
    11. void main(void)
    12. {
    13.  
    14.     while(1)
    15.     {
    16.         unsigned int photo=0;                   //16 bit integer(0-65536)
    17.         TRISB=0;                        //PORTB configured as output
    18.         TRISA=1;                //set TRISA registers to input
    19.        
    20.         ADCON1=0b00100000;              //configure all pins to analog; result is right justified
    21.         ADCON0=0b10000001;              // input at RA0; GO bit is low (default);sets clock to 32TOSC   //switches on ADC module   
    22.         __delay_ms(1);                  //wait for 20us; acquisition time
    23.         GO=1;                       //start the conversion
    24.         while(GO_nDONE);                        //wait for conversion to complete
    25.         photo=(unsigned int) ADRESH<<8;
    26.         photo=photo+ADRESL;
    27.         if(photo<THRESHOLD_PHOTO)                   //if it is dark
    28.             RB0=0b1;            //light the LED
    29.         else if (photo>THRESHOLD_PHOTO)
    30.             RB0=0b0;
    31.     }
    32. }
    33.    
    34.    
    35.    
    Still no success! Please help me figure out where I am going wrong. Sorry for the back-to-back posts but I am really eager to see my code work!

    EDIT: And work it does! I need to tone down the delay time though, it works pretty sluggishly. It might have looked as though I have been talking to myself for sometime in this thread and I am really sorry for that! I will try making some changes in the code and will let you know what happens!

    EDIT 2: On second thoughts, can you explain how the ADRES register read lines work? And what difference does if make to the reading process if the result is left/right justified?

    Thanks for bearing with me! :)
     
    Last edited: Jul 13, 2012
  10. JohnInTX

    Moderator

    Jun 26, 2012
    2,341
    1,024
    You are mixing signed and unsigned ints:

    #define THRESHOLD_PHOTO (unsigned int)512
    Cast ADRESL as (unsigned int) as well

    Since your ADC is left-justified, all counts are *2^6 i.e.
    1 ADC count = 00000000 01000000 which is 64 (not 1) so midrange
    is actually 32768, not 512.

    I actually prefer left-justified since migrating to an ADC with more precision (e.g. 12 bits) just adds significant LSBs without changing the ranges of the numbers. The downside is that the numbers are bigger and that things like integer overflow etc. can be an issue.
     
    Last edited: Jul 13, 2012
  11. ErnieM

    AAC Fanatic!

    Apr 24, 2011
    7,386
    1,605
    First, for a 2MHz (which I think you're using) I would make ADCS 0b100. Next, you have ther result left justified via ADFM. You want it right justified to use the full 10 bits. I believe the way you have it photo will range from 0x0000 to 0x00F0 in 0x10 steps. Then you'll still have to adjust THRESHOLD_PHOTO to work in the range of 0 to 1023 (the output of a 10 bit A2D).

    This is an excellent time to use an in circuit debugger to see exactly what the A2D converter is doing and how that output is processed.
     
  12. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    Also not to be grumpy. But this is something you could have cleared up by doing some very simple debugging. You are really not a programmer before you understand at least some simple debugging concepts. Like single stepping, breakpoints, register injection/changing and watches. Take a look at the first post here
    http://forum.allaboutcircuits.com/showthread.php?t=44852
     
    ActivePower likes this.
  13. osx-addict

    Member

    Feb 9, 2012
    122
    9
    I agree with the above post and particularly after reading a handful of threads elsewhere on the forums.. Consider taking the time to try to learn how to use the debugger! As a programmer/software engineer, it's your friend! Don't be afraid of it.. I've been doing software engineering for close to 30 years and I still find people that refuse to use a debugger and would rather guess why a program won't run or prefer to use printf() statements.. Of course, that assumes that you can even use printf()'s -- which usually isn't the case for embedded work. For these types of tasks specifically, a voltmeter (or O-scope) and a debugger are your friend! Learn how to single step, step in functions, over functions, set breakpoints, resume,etc.. It's all good and probably only ~30 minutes to an hour of time well spent learning this stuff.. It's kinda like learning to ride a bike -- you can't compete in the Tour de France without knowing how to ride the bike first -- right??

    P.S. Some debuggers even do watchpoints which are cool particularly if they're hardware debug supported -- these are nice features that allow you to stop a program IF some sort of event is triggered (a variable is a certain value,etc).. I've not yet programmed anything on the PIC's but have done TONS on embedded RAD-hard PPCs, Solaris boxes using GDB (command line only), Arduinos, 6800's, 68000's, 8086 and various others along the way..
     
    ActivePower likes this.
  14. ActivePower

    Thread Starter Member

    Mar 15, 2012
    155
    23
    Thanks for all the advice! I would really consider using software debugging tools like MPLAB SIM in my future projects. I can not actually afford a hardware debugger being a hobbyist. Also, as my JDM programmer doesn't support a USB-Serial controller, it is a little tough for me to make instantaneous changes to the code unless I hook it up with my age-old PC (too old to run MPLAB).

    All that grumpiness aside, I really need to develop a method for beginning some useful work in this field and your suggestions would go a long way in helping me for it.

    Thanks a lot :)
     
  15. t06afre

    AAC Fanatic!

    May 11, 2009
    5,939
    1,222
    You can do a lot with MPLAB SIM. Just learn it step by step. From your postings I can you have improved a lot. I know from teaching in programming. That debugging is the last skill to sink in. So the "grumpy" post was more or less meant as a push in the correct direction. By the way for the next two weeks I will off-line. So I will not be able to help you much in debugging skills. But good luck with that project
     
    ActivePower likes this.
  16. osx-addict

    Member

    Feb 9, 2012
    122
    9
    No problem.. I'm in the same boat for my upcoming PIC project.. I've not even bought a programmer yet.... :D
     
Loading...