Beginner in need of help !!

[Tabsoft]

Rob,

Why don't you check this out.

'PIC 12F683

#CONFIG 
   __config _INTRC_OSC_NOCLKOUT & _WDT_OFF & _MCLRE_OFF & _CP_OFF
#ENDCONFIG


DEFINE OSC 4 '4mhz ocsillator
ANSEL = %00000010 'pin 6 analog
CMCON0 = %00000100 'comparator mode
VRCON = %10101000 'voltage reference
TRISIO = %00000010 'pin 6 input

PAUSE 50 'wait for hardware to settle

ON INTERRUPT GOTO int1 'interrupt handler is int1
INTCON = %11001000 'enable GIE and GPIE; clear GPIF
PIE1 = %00001000 'enable comparator interrupt

POT1 VAR CMCON0.6 'read potentiometer
LED VAR GPIO.2 'led pin 5
loop1 var word 'loop1 counter

bitTest var bit 'Debug test bit.  Set/Clear in ISR and Check in main


high LED    'Start with LED ON
pause 500   'Pause .5 secs so you can see the LED turn on

bitTest = POT1  'Take initial reading of input voltage state
Loop1 = 0

ENABLE
main:

    IF bitTest = 1 THEN 'input voltage is < reference voltage  
        low LED
        nap 1
    else    'bitTest = 0 - input voltage is > reference voltage
        GOTO FLASH
    ENDIF
        
    GOTO main


ENABLE
FLASH:
    do
        for loop1 = 1 to 1000
            HIGH LED
            PAUSE 1
        NEXT LOOP1
        
        for loop1 = 1 to 1000
            LOW LED
            PAUSE 1
        NEXT LOOP1
        
        IF bitTest = 1 THEN
            GOTO main
        ENDIF
    
    LOOP
  
    
DISABLE 'disable interrupts in handler
int1:
    
if PIR1.3 = 1 then  'Comparator Interrupt fired (must be cleared before resume)
bitTest = pot1
ENDIF 

INTCON =%11001000 
PIR1 = %00000000 'reset CMIF
RESUME 'where left off
ENABLE 'enable interrupts

[HenrikOlsson]

I will drop the pause below the osc value, thanks for that tip

Like Tabsoft wrote, you generally want your DEFINEs at the top of the program but it's for readabillity. In reallity it makes no difference because they are not runtime commands.

Another little note

        for loop1 = 1 to 1000
            HIGH LED
            PAUSE 1
        NEXT LOOP1

What's the purpose of setting the LED high inside the loop?
There's nothing inherently wrong in doing it but it does take (a little) bit of time.

Also, now that you're starting to get the hang of it you might want to consider ditching the HIGH/LOW commands and simply set the pin directly, LED=1, LED=0. For this to work you need to clear the corresponding TRIS-bit first (TRISO.2 = 0 ' Set GPIO.2 to output). Doing it this way executes faster and consumes less codespace since it doesn't waste resources clearing the TRIS bit each and every time (which is what HIGH/LOW does for you).

Again, you're not doing anything "wrong" - it all works fine, I'm just pointing out a couple of things you could improve and take with you to the next project.

/Henrik.

[robbo78]

Thanks guys, I will have a play

Rob