Reading from an Analog Port-PIC16F877A

Thread Starter

ActivePower

Joined Mar 15, 2012
155
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:

Rich (BB code):
//main.c
//To read a photoresistor input and flash an LED if it is dark

#include<htc.h>
#define _XTAL_FREQ 20000000

__CONFIG(FOSC_HS & CP_OFF & CPD_OFF & LVP_ON & PWRTE_OFF & DEBUG_ON & WDTE_OFF & BOREN_OFF & WRT_OFF);

void main(void)
{

	while(1)
	{
	unsigned int photo=0;
					//16 bit integer(0-65536)
	TRISB=0x0;
						//PORTB configured as output
	TRISA=0b11111111;
				//set TRISA registers to input
	ADCON1=0b00000000;		//all pins are analog; result is left justified 

        ADCON0=0b10000001;	
//switch on ADC module; sets clock to 32TOSC; input at RA0; GO bit is low	

	__delay_us(20);				//wait for 20us; acquisition time
	ADCON0=0b10000101;			//start the conversion
        while(ADCON0==0b10000101);	//wait for conversion to complete
	photo=ADRESH;
	if(photo<32768)				//if it is dark
	PORTB=0b10000000;			//light the LED
	}
}
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!
 

JohnInTX

Joined Jun 26, 2012
4,787
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:

Rich (BB code):
ADC_START_CONVERSION;
while (!ADC_DONE);
You never turn the LED off if the value drops below your threshold and there is no hysteresis. Maybe:

Rich (BB code):
 if (photo < PHOTO_THRESHOLD)
   LED_ON;
 if (photo > (PHOTO_THRESHOLD + PT_HYSTERESIS )
   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:
Rich (BB code):
 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:

Thread Starter

ActivePower

Joined Mar 15, 2012
155
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.
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!

You have not applied any bounds-checking to your ADC value.
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:

t06afre

Joined May 11, 2009
5,934
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.
Rich (BB code):
// Register: ADCON0
volatile unsigned char           ADCON0              @ 0x01F;
// bit and bitfield definitions
volatile bit ADON                @ ((unsigned)&ADCON0*8)+0;
volatile bit GO_nDONE            @ ((unsigned)&ADCON0*8)+2;
volatile bit GO                  @ ((unsigned)&ADCON0*8)+2;
volatile bit CHS0                @ ((unsigned)&ADCON0*8)+3;
volatile bit CHS1                @ ((unsigned)&ADCON0*8)+4;
volatile bit CHS2                @ ((unsigned)&ADCON0*8)+5;
volatile bit ADCS0               @ ((unsigned)&ADCON0*8)+6;
volatile bit ADCS1               @ ((unsigned)&ADCON0*8)+7;
volatile bit nDONE               @ ((unsigned)&ADCON0*8)+2;
volatile bit GO_DONE             @ ((unsigned)&ADCON0*8)+2;
#ifndef _LIB_BUILD
volatile union {
    struct {
        unsigned ADON                : 1;
        unsigned                     : 1;
        unsigned GO_nDONE            : 1;
        unsigned CHS                 : 3;
        unsigned ADCS                : 2;
    };
    struct {
        unsigned : 1;
        unsigned : 1;
        unsigned : 1;
        unsigned : 3;
        unsigned : 2;
    };
    struct {
        unsigned : 2;
        unsigned GO                  : 1;
        unsigned CHS0                : 1;
        unsigned CHS1                : 1;
        unsigned CHS2                : 1;
        unsigned ADCS0               : 1;
        unsigned ADCS1               : 1;
    };
    struct {
        unsigned : 2;
        unsigned nDONE               : 1;
    };
    struct {
        unsigned : 2;
        unsigned GO_DONE             : 1;
    };
} ADCON0bits @ 0x01F;
#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
Rich (BB code):
 ADCON0bits.CHS=0b111;
and to start a conversion you can write
Rich (BB code):
GO_nDONE=1;
and to wait for a conversion to finish write
Rich (BB code):
while(!GO_nDONE);
as some examples
 
Last edited:

Thread Starter

ActivePower

Joined Mar 15, 2012
155
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!
 

t06afre

Joined May 11, 2009
5,934
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?
All are equal just pick and mixs as you want,
Also, some bits go together always like CHS(three bits) and ADCS(two bits). Can they be used as such?
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
Rich (BB code):
volatile bit GO_nDONE            @ ((unsigned)&ADCON0*8)+2;

You can write like this
Rich (BB code):
GO_nDONE=0;
However for the bitfields like say CHS. They are part of the Ansi C bit-field union like this
Rich (BB code):
struct { unsigned lo : 1; unsigned dummy : 6; unsigned hi : 1;​
} foo;
And then correct way of doing this as shown
Rich (BB code):
ADCON0bits.CHS=0b101; //I like to use binary numers here but you can use whatever you like hex or decimal
ADCON0bits.ADCS=2;
If you write
Rich (BB code):
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
Rich (BB code):
volatile union {
    struct {
        unsigned ADON                : 1;
        unsigned                     : 1;
        unsigned GO_nDONE            : 1;
        unsigned CHS                 : 3;
        unsigned ADCS                : 2;
    };
Thirdly, does the A/D conversion need to take place in an infinite loop always?
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
 

ErnieM

Joined Apr 24, 2011
8,415
For example, is it okay if I use the GO bit instead of the GO_nDONE bit?
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:

Rich (BB code):
    GO = 1;            // start a conversion
    while (GO_nDONE);  // wait till complete
    ...
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):

Rich (BB code):
            ReferenceV = 0;
            adcon0.CHS0 = 1;    // A2D_Input = ReferenceV_Pin (AN1)
            delay_ms(2);        // hang here for a few for transients to
                            // settle out before we begin to sample the D2A

            // sample the Reference Pin Voltage
            for (LoopCounter = 1;  LoopCounter < D2A_SAMPLE_COUNT; LoopCounter++)
            { 
                delay_us(A2D_DELAY);// allow this sample to settle out
                adcon0.GO = GO;     // start conversion

                 while (adcon0.GO == NOT_DONE);          // wait till complete
                MAKESHORT( A2D_Result, adresl, adresh );// organize 
                ReferenceV =     ReferenceV + A2D_Result;  // add reading to total

                if (LoopCounter == 1)
                {
                    // first time thru, use this Reading as both the min and max
                    MinV = A2D_Result;
                    MaxV = A2D_Result;
                }
                else
                {
                    // 2nd or more time thru
                    // check for better min & max in ReferenceV
                    if (MinV > A2D_Result) MinV = A2D_Result;
                    if (MaxV < A2D_Result) MaxV = A2D_Result;
                }
            }

            ReferenceV  = ReferenceV  - MinV  - MaxV;
 

Thread Starter

ActivePower

Joined Mar 15, 2012
155
Here is the code after some modifications:
Rich (BB code):
//main.c
//To read a photoresistor input and flash an LED if it is dark

#include<htc.h>
#define _XTAL_FREQ 20000000
#define THRESHOLD_PHOTO 512
#define HYSTERESIS 100

__CONFIG(FOSC_HS & CP_OFF & CPD_OFF & LVP_ON & PWRTE_OFF & DEBUG_ON & WDTE_OFF & BOREN_OFF & WRT_OFF);

void main(void)
{

	while(1)
	{
		unsigned int photo=0;					//16 bit integer(0-65536)
		TRISB=0;						//PORTB configured as output
		TRISA=1;				//set TRISA registers to input
		
		ADCON1=0b00000000;				//configure all pins to analog; result is left justified 
		ADCON0=0b10000001;				// input at RA0; GO bit is low (default);sets clock to 32TOSC	//switches on ADC module	
		__delay_us(25);					//wait for 20us; acquisition time
		GO=1;						//start the conversion
		while(GO_nDONE);						//wait for conversion to complete
		photo=(unsigned int) ADRESH;
		if(photo<THRESHOLD_PHOTO)					//if it is dark
			RB0=0b1;			//light the LED
		else if (photo>(THRESHOLD_PHOTO+HYSTERESIS))
			RB0=0b0;
	}
}
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:

Thread Starter

ActivePower

Joined Mar 15, 2012
155
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:

Rich (BB code):
//main.c
//To read a photoresistor input and flash an LED if it is dark

#include<htc.h>
#define _XTAL_FREQ 20000000
#define THRESHOLD_PHOTO 512                   //for a 10 bit ADC; midway between 0V-5V


__CONFIG(FOSC_HS & CP_OFF & CPD_OFF & LVP_OFF & PWRTE_OFF & DEBUG_ON & WDTE_OFF & BOREN_OFF & WRT_OFF);

void main(void)
{

	while(1)
	{
		unsigned int photo=0;					//16 bit integer(0-65536)
		TRISB=0;						//PORTB configured as output
		TRISA=1;				//set TRISA registers to input
		
		ADCON1=0b00100000;				//configure all pins to analog; result is right justified 
		ADCON0=0b10000001;				// input at RA0; GO bit is low (default);sets clock to 32TOSC	//switches on ADC module	
		__delay_ms(1);					//wait for 20us; acquisition time
		GO=1;						//start the conversion
		while(GO_nDONE);						//wait for conversion to complete
		photo=(unsigned int) ADRESH<<8;
		photo=photo+ADRESL;
		if(photo<THRESHOLD_PHOTO)					//if it is dark
			RB0=0b1;			//light the LED
		else if (photo>THRESHOLD_PHOTO)
			RB0=0b0;
	}
}
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:

JohnInTX

Joined Jun 26, 2012
4,787
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:

ErnieM

Joined Apr 24, 2011
8,415
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.
 

osx-addict

Joined Feb 9, 2012
122
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..
 

Thread Starter

ActivePower

Joined Mar 15, 2012
155
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 :)
 

t06afre

Joined May 11, 2009
5,934
Thanks for all the advice! I would really consider using software debugging tools like MPLAB SIM in my future projects. :)
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
 
Top