David,

Just a few style points if you don't mind (I spotted that your Interrupt routine doesn't do anything -
including affecting the delays, but it's already been mentioned by someone else).

You don't set the origin at the start of the program - the compiler will assume zero I'm sure, but it's
probably best to put in  ORG 0x000  anyway to make sure, and to make it quicker to spot the actual start of
the program.

You start out well with comments describing what's going on, and this is excellent:

    MOVWF TMR0L      ;load timer0 with 255 so as to get a 16us delay
                     ;Delay = (256 - TMR0 * prescaler)
                     ;-----------------------------------------
                     ;          Clock Frequency / 4

but you seem to have got bored with that by the time you got to the Main Loop:

    MOVLW B'11110100'
    MOVWF CNT1

so I (and perhaps you!) can't see what you're doing there.  I think this is another timing value, but how
long?

You use Binary for all your literals, which in some cases is appropriate, but in others it would be easier if
you'd used hex or decimal, for example:

     movlw B'00010000'       ; RB4 input, all other output (my switch)
     MOVWF TRISB

is right because you're dealing with bits and you can easily see which one is set, but:

     MOVLW B'11110100'
     MOVWF CNT1

is hard to read because it's representing a number (of counter-ticks, I presume) and expressing it in decimal
would make it easier to understand - try decreasing it by 5, and you'll see what I mean!  :-)

In this section:

     btfsc PORTB, RB4     ; check rb4
     goto ROTATE         ; rb4 = 1
     goto BINCNT         ; rb4 = 0
 ROTATE:

You could have saved an instruction by flipping the test:

     btfss PORTB, RB4    ; check rb4
     goto BINCNT         ; rb4 = 0
 ROTATE:                 ; rb4 = 1

now there may be occasions where you don't want this, for example if you want to keep the same number of
cycles in each path, but I don't think that's an issue here.

You are very inconsistent in your use of case, with this being the best example:

     Rrncf PORTD, 1        ; rotate right port D
     CALL Delay
     goto loop

where you've used Proper, UPPER, and lower case for the instructions and parameters all in three lines :-)
Please pick a style and stick to it - it's much faster to read when it's all the same.  My own "Standard" is
to use UPPER case for the instructions and special registers like ports, and Proper case for labels and my own
variables, so this is how I would have written it:

    RRNCF PORTD, 1        ; Rotate Right Port D
    CALL Delay
    GOTO Loop

But this is very personal and could well start a flame war!  :-)  My point is not that you should do it my
way, but to pick a style and stick with it.

Please don't take any of this as personal criticism - I'm trying to help you write more readable programs, and
(as I used to tell people in my team) almost all programs have much more time spent reading than writing them,
so saving a few keystrokes as you're writing will cost you, or someone else, much more later.  (I can just
hear my former colleagues groaning:  "He's not STILL banging on about that, is he?" :-)

Cheers,

Howard Winter
St.Albans, England

--
http://www.piclist.com hint: PICList Posts must start with ONE topic:
[PIC]:,[SX]:,[AVR]: ->uP ONLY! [EE]:,[OT]: ->Other [BUY]:,[AD]: ->Ads