# PIC16F88 - compare ADC results with a constant value

I am trying to write my first program in assembler for PIC16F886. The program is supposed to do the following:

• If the value is smaller than 128 - turn LED off
• Otherwise - turn LED on

The schematic is rather simple:

And here is my assembly code.

    list        p=16f886        ; list directive to define processor
#include    <p16f886.inc>   ; processor specific variable definitions

__CONFIG    _CONFIG1, _LVP_OFF & _FCMEN_ON & _IESO_OFF & _BOR_OFF & _CPD_OFF & _CP_OFF & _MCLRE_ON & _PWRTE_ON & _WDT_OFF & _INTRC_OSC_NOCLKOUT
__CONFIG    _CONFIG2, _WRT_OFF & _BOR21V

org 00                      ;start at 0x00

VALUE EQU 0x80                  ;value to compare adc results

cblock  0x20
endc

port_config
movlw b'11111110'           ;set RB0 on PORTB to output
movwf TRISB

start
bsf PORTB, 0                ;turn LED on

movlw 0x00                  ;left justify
MOVWF ADCON1                ;Vdd and Vss as Vref
BANKSEL TRISA               ;
BSF TRISA,0                 ;Set RA0 to input
BANKSEL ANSEL               ;
BSF ANSEL,0                 ;Set RA0 to analog

GOTO $-1 ;No, test again BANKSEL ADRESH MOVF ADRESH,W ;Read lower 8 bits MOVWF RESULTLO ;Store in GPR space ; if RESULT <= VALUE movf RESULTLO,w addlw 255 - VALUE skpnc goto led_off ; if RESULT > VALUE movf VALUE,w addlw 255 - RESULTLO + 1 skpnc goto led_on ;loop goto adc_sample led_on bsf PORTB, 0 goto adc_sample led_off bcf PORTB, 0 goto adc_sample end  I expect the led to turn on/off when I change the resistance of the pot but instead the led constantly stays on. Update: I suspect it has something to do with the way I compare RESULT and VALUE since I get the following warning during compilation: Argument out of range. Least significant bits used. Update: As I've mention in comments, the schematic is taken directly from Proteus. On the real device the controller is powered from 5V source, there should be a limiting resistor in series with a led (unless I drive the mcu from a smaller voltage) and nMCLR is tied to +5V through a 5k resistor. • You'll want to add a resistor in series with that LED. Without one, it's likely overdriving both the LED and the PIC's output driver. It's possible that the PIC driver is already damaged, and is now "always on". You could test that by simply toggling the pin on and off (with a delay in between) and verifying that the LED does, in fact, turn off when it's supposed to. If the output driver is damaged, you can probably just move the LED to a different pin and change the code appropriately. – bitsmack May 20 '14 at 6:35 • Right now I'm only testing the circuit in Proteus, so I didn't add a resistor. However, pic datasheet says: "High current source/sink for direct LED drive". I suppose it means I can safely drive leds without a resistor? – Ashton H. May 20 '14 at 8:38 • No, don't drive a standard LED without a resistor or some current limiting device. No good will come of it if you do. BTW what is RV1 connected to? – Andy aka May 20 '14 at 8:47 • It forms a voltage divider between +5V bus and ground. – Ashton H. May 20 '14 at 8:50 • A "standard" LED can be driven at 20mA for maximum brightness. This varies; check the datasheet for the one you're using. The PIC's "direct LED drive" statement means that it can provide that much current. It used to be that a microcontroller had to drive a transistor, which then drove the LED... Anyway, to choose the resistance: R = (Vcc - Vf) / I, where Vcc is your supply voltage, Vf is the forward voltage of the LED (see the datasheet), and I is the desired current (keep it under 20mA). e.g.: (5V-2V)/0.02A = 150-Ohm. – bitsmack May 20 '14 at 8:52 ## 3 Answers You are setting the ADFM bit in ADCON1, which, as your comments indicate, right justifies the result. This results in the bits being stored as follows:  (ADRESH) 7 6 5 4 3 2 1 0 (ADRESL) 7 6 5 4 3 2 1 0 10-bit ADC reading: 9 8 7 6 5 4 3 2 1 0  You then read in the low 8-bits of the result, ADRESL. You never read in the high byte, ADRESH, containing the top two bits of the result. Thus you are throwing them away, which are the most signifcant (literally). When your pot reaches the mid-point, the top bit of the result changs, but you never see it. Instead, you want to left justify the result, by setting ADFM to 0, and read in just the top 8-bits (ADRESH). Your test for 128 is now valid. There is no harm in throwing away the lower 2 bits in ADRESL.  (ADRESH) 7 6 5 4 3 2 1 0 (ADRESL) 7 6 5 4 3 2 1 0 10-bit ADC reading: 9 8 7 6 5 4 3 2 1 0  • Thanks, I haven't noticed that - updated my code. Unfortunately, it still doesn't work – Ashton H. May 20 '14 at 8:33 I have managed to fix my code eventually. It had the following problems: 1. I have forgot to add a small delay for ADC to initialize properly 2. PORTB was not initialized properly 3. Comparison routine did no work as I expected  list p=16f886 ; list directive to define processor #include <p16f886.inc> ; processor specific variable definitions __CONFIG _CONFIG1, _LVP_OFF & _FCMEN_ON & _IESO_OFF & _BOR_OFF & _CPD_OFF & _CP_OFF & _MCLRE_ON & _PWRTE_ON & _WDT_OFF & _INTRC_OSC_NOCLKOUT __CONFIG _CONFIG2, _WRT_OFF & _BOR21V org 00 ;start at 0x00 VALUE EQU 0x80 cblock 0x20 RESULTHI ;ADC 8-bit result endc port_config bsf STATUS,RP0 ;select Registers at Bank 1 movlw b'11111111' ;set PORTA to input movwf TRISA movlw b'11111110' ;set RB0 on PORTB to output movwf TRISB bcf STATUS,RP0 ;select Registers at Bank 0 start bsf PORTB, 0 ;turn LED on adc_config BANKSEL ADCON1 movlw 0x00 ;left justify MOVWF ADCON1 ;Vdd and Vss as Vref BANKSEL TRISA ; BSF TRISA,0 ;Set RA0 to input BANKSEL ANSEL ; BSF ANSEL,0 ;Set RA0 to analog BANKSEL ADCON0; MOVLW b'11000001' ;ADC Frc clock, MOVWF ADCON0 ;AN0, On call delay ;Allow some time to settle CLRF ADRESH adc_sample BSF ADCON0,GO ;Start conversion BTFSC ADCON0, GO ;Is conversion done? GOTO$-1                    ;No, test again
call delay                  ;Allow some time to settle
MOVWF RESULTHI              ;Store in GPR space

compare_result
clrc
movlw VALUE
subwf RESULTHI,w
btfsc STATUS, Z             ;if (RESULTHI <= VALUE)
goto led_off                ;turn led off
btfsc STATUS, C
goto led_on                 ;otherwise: turn led on
goto led_off

led_on
bsf PORTB, 0
led_off
bcf PORTB, 0

delay
COUNT1 equ 0xFF
loopl0
decfsz COUNT1,1
goto loopl0
return

end

• Per my answer, when you modified your code, the comment for the instruction MOVF ADRESH,W should be ;Read high 8 bits. You are reading the high 8 bits of the 10 bit ADC result and throwing away the bottom two. Code is right, just the comment is wrong. – tcrosley May 20 '14 at 22:19
• You are absolutely right - fixed! – Ashton H. May 20 '14 at 22:31

There are some serious problems with your cicuit:

1. You forgot to power the PIC. Actually, the power and ground pins aren't even shown. Obviously something is wrong here.

2. There is nothing to limit the current thru the LED. Green LEDs typically drop about 2.1 V. Trying to drive it from higher voltage will either cause excessive current thru the LED or excessive current out the port pin. Depending on what exactly you eventually connect for power, that may also cause the whole power supply to collapse, possibly resetting the PIC.

Common T1-3/4 LEDs are rated for 20 mA, but will be plenty bright enough in a office environment at half that. Let's say you eventually power the PIC from a stiff 5 V supply, then you want a resistor that drops 5V - 2.1V = 2.9V at 10 mA. (2.9 V)/(10 mA) = 290 Ω, so the common value of 300 Ω should give you a clearly visible result without pushing either the LED or the PIC to its limits.

3. There is no bypass cap, even if there magically were power connections somewhere.

Until these things are fixed, it is pointless to think about the firmware.

• It's obviously powered, since he said the LED is always on. I agree with your other points. – tcrosley May 20 '14 at 20:17
• @tcrosley: Then there are important parts of the circuit the OP is not showing us. That is just as bad. We don't know, for example, what voltage the PIC is actually powered at, how stiff that supply is, and whether it is properly bypassed. It is not up to the OP to decide what is relevant information. Leaving out the power supply connections is just plain irresponsible and misleading. – Olin Lathrop May 20 '14 at 20:43
• @OlinLathrop as I've mentioned, I'm only testing the circuit in Proteus simulator. The schematic was just to show where the pot and led are connected. Applying the power to the uC is quite obvious and is not worth mentioning, especially if my question was about code. – Ashton H. May 20 '14 at 22:01
• @AshtonHearts, you'd be surprised at how difficult it can be to assess a questioners level of expertise. When you answer enough such questions, you realize nothing is obvious, and assuming a questioner knows more than what's posted is a losing proposition that leads to frustration. – Scott Seidman May 21 '14 at 3:19