Post by thread:[PIC] First Project

Hi all, I'm fairly new to the list and I also just "finished" my first (slightly trivial) project, so I thought I'd share. There's a website about it here: http://marcel.carrietech.com/pic/spiled.php . It's just a counter that interfaces with a MAX7221 via SPI to drive some LED displays. It works pretty well at this point, except for one thing: the two most significant digits (thousands and hundreds) aren't initialized for some reason, even though I'm clrf'ing them. They don't fluctuate once the pic is running, and they behave as expected, but I can't get them to start at 0 for some reason. If anyone has any ideas, please let me know. Also, any constructive criticism on code or whatever else is appreciated. Thanks, - Marcel

Well, I like the site - really nice layout. The circuit also looks pretty tidy (except the wires ;-)). I don't really understand the code, because I have never worked with software SPI before, but it looks nice. In general, it looks a lot better than my second project - It takes 8 LEDs on portb, puts 1 in port b, and rlf portb. ;-). On 6/12/05, Marcel Birthelmer <spam_OUTmarcelTakeThisOuTcarrietech.com> wrote: {Quote hidden}> Hi all, > I'm fairly new to the list and I also just "finished" my first (slightly > trivial) project, so I thought I'd share. There's a website about it > here: http://marcel.carrietech.com/pic/spiled.php . It's just a counter > that interfaces with a MAX7221 via SPI to drive some LED displays. It > works pretty well at this point, except for one thing: the two most > significant digits (thousands and hundreds) aren't initialized for some > reason, even though I'm clrf'ing them. They don't fluctuate once the pic > is running, and they behave as expected, but I can't get them to start > at 0 for some reason. If anyone has any ideas, please let me know. Also, > any constructive criticism on code or whatever else is appreciated. > Thanks, > - Marcel > -

Marcel Birthelmer wrote >It works pretty well at this point, except for one thing: the two most >significant digits (thousands and hundreds) aren't initialized for some >reason, even though I'm clrf'ing them. They don't fluctuate once the >pic is running, and they behave as expected, but I can't get them to >start at 0 for some reason. If anyone has any ideas, please let me >know. Also, any constructive criticism on code or whatever else is >appreciated. Hello, I can see a problem with the code. You've numbered the counters 0x28 to 0x31 rather than 0x28 to 0x2b. Then in the code you inc FSR so the most significant counters won't be updated when you expect. That's the sort of bug that's hard to find. -- Martin

Marcel Birthelmer wrote: > Also, any constructive > criticism on code or whatever else is appreciated. You used the <name>: construct for code labels. I wasn't even aware MPASM allowed this. In any case, labels should be clearly delineated from the rest of the code. The other MPASM label syntax enforces this by requiring labels to start in column 1 and opcodes in column 2 or further. This is a good idea you should follow anway. Also, it's a good idea to put labels on a line by themselves. That makes maintenence easier as inevitably you will need to add some code right after a label. I ran your code thru my ASPIC_FIX program to reformat it in a consistent way that I'm comfortable with. I had to fix the code labels manually, but I'll probably add the <name>: syntax to ASPIC_FIX so I don't have to do this in the future. > OUT1 EQU 0x20 > OUT2 EQU 0x21 > IN1 EQU 0x22 1 - Comment all your variables. 2 - Comment all your variables. 3 - You really should comment all your variables. 4 - This is a bad way to allocate variables. First, now that you've had your fun and gotten the LED blinker out of the way, it's time to do things right and use relocatable mode. Do it now before you've settled into the bad habit of absolute mode. In relocatable mode, use the RES directive to allocate variables. Even in absolute mode, CBLOCK is better for defining variable name symbols (note that it doesn't actually allocate RAM) than just giving each one an explicit value. > CTR1 EQU 0x28 > CTR2 EQU 0x29 > CTR3 EQU 0x30 > CTR4 EQU 0x31 I just had to laugh at this one. You are using explicit HEX constant, but you are counting in decimal. > CS EQU 0 > MISO EQU 2 > MOSI EQU 1 > SPICLK EQU 2 Whatever these are THEY SHOULD BE EXPLAINED in end of line comments at the very least. > org 0 > goto init > org 0x10 I don't get the point of this. You don't have an interrupt routine, so there is no reason your code can't start at 0 and continue directly from there. Even if not, what's special about 10h? > init clrf PORTB ;clear port b Again, I think it's bad form to put executable instructions on the same line as a label. > bsf STATUS, RP0 > bcf TRISA, SPICLK This sort of manual bank switching is dangerous. I think you got it right in this case, but sooner or later you'll make a mistake looking up the bank of an SFR in the data sheet or setting the RPx bits accordingly. For starter programs that are not space or speed cricital, just use BANKSEL. Once you get more comfort with PICs you'll probably want to use something like my DBANKIF and related macros in STD.INS.ASPIC at http://www.embedinc.com/pic. This particular case is particularly bad since you are assuming that RP1 is 0 but there is no mention of that restriction. Some day you'll insert code above here and unrelated stuff will break. > clrf PORTB Why? You already cleared it a few instructions earlier. > bsf PORTB, CS Ah, so CS stands for a particular bit number in port B. You should SAY SO. Also check out my /INBIT and /OUTBIT preprocessor directives. They handle I/O port bit definitions in a nicer way. > call xfer16 > > movlw 0x0A ;INADDRSITY : MAX > movwf OUT1 This is a serious bug because you are relying on XFER16 to either preserve the bank or set it to 0, but XFER16 is not defined to do either. (Actually none of its actions are defined, but that's another problem.) Just because you are getting away with it here doesn't make this any less of a bug. > mainloop > movlw CTR1 ;load starting address > movwf FSR > > bump incf INDF, 1 ;increase current digit Indirect bank setting? {Quote hidden}> movlw 0x0F ;restrict to last four bits > andwf INDF, 0 > xorlw 0x0A ;check if equal to ten > btfss STATUS, Z > goto bump_done ;if not, done > clrf INDF ;else, set to zero > > movf FSR, 0 ;check if this was the fourth digit > xorlw CTR4 > btfsc STATUS, Z ;if so, we're done > goto bump_done > incf FSR, 1 ;otherwise, go to next digit > goto bump Finally a few comments about the details, but all higher level explaination is completely missing. You may not think of it as a bug, but it certainly is, and a rather serious one at that. There should be block comments in front of important labels like MAINLOOP and BUMP. When do we get back here? What gets done each time thru the loop? When does the loop terminate? > bump_done > clrf ADDR Again, you are assuming the loop above terminates with bank 0 set, but I don't see anything specifying that, so it can't be relied on. There should also be a comment here that the bank setting is assumed to be 0. > xfer16 bcf PORTB, CS ;assert /CS >From previous context, this is apparently a subroutine. So WHERE ARE THE COMMENTS!!!!? Beginner or not, this sort of programming is simply inexcusible and irresponsible. What is the purpose of this subroutine? What does it take as input? What state does it change? What state does it trash? There should be something in the code that makes it visually easy to see a major break in flow. I use a comment line with all stars. A proper subroutine comment header should make the break in the code visually obvious, and it should tell a programmer everything he needs to know to call the subroutine. You should be able to give other programmers just the subroutine documentation header and they should be able to call the subroutine correctly and deal with all its side effects without needing the source code. ***************************************************************** Embed Inc, embedded system specialists in Littleton Massachusetts (978) 742-9014, http://www.embedinc.com

Olin Lathrop wrote: > Marcel Birthelmer wrote: > >>Also, any constructive >>criticism on code or whatever else is appreciated. > > > You used the <name>: construct for code labels. I wasn't even aware MPASM > allowed this. In any case, labels should be clearly delineated from the > rest of the code. The other MPASM label syntax enforces this by requiring > labels to start in column 1 and opcodes in column 2 or further. This is a > good idea you should follow anway. Also, it's a good idea to put labels on > a line by themselves. That makes maintenence easier as inevitably you will > need to add some code right after a label. I'm using gpasm to assemble, and the labels are actually in the first column, it's just that my php script on the website decided to mangle this and I didn't catch it. And I think the : after the label is just ignored. {Quote hidden}> > I ran your code thru my ASPIC_FIX program to reformat it in a consistent way > that I'm comfortable with. I had to fix the code labels manually, but I'll > probably add the <name>: syntax to ASPIC_FIX so I don't have to do this in > the future. > > >> OUT1 EQU 0x20 >> OUT2 EQU 0x21 >> IN1 EQU 0x22 > > > 1 - Comment all your variables. > 2 - Comment all your variables. > 3 - You really should comment all your variables. So you think I should comment my variables? > 4 - This is a bad way to allocate variables. First, now that you've had > your fun and gotten the LED blinker out of the way, it's time to do things > right and use relocatable mode. Do it now before you've settled into the > bad habit of absolute mode. I didn't know about this, but I will definitely research. {Quote hidden}> > In relocatable mode, use the RES directive to allocate variables. Even in > absolute mode, CBLOCK is better for defining variable name symbols (note > that it doesn't actually allocate RAM) than just giving each one an explicit > value. > > >> CTR1 EQU 0x28 >> CTR2 EQU 0x29 >> CTR3 EQU 0x30 >> CTR4 EQU 0x31 > > > I just had to laugh at this one. You are using explicit HEX constant, but > you are counting in decimal. And that would be the initialization bug. Thanks to you and the other person who pointed this out. {Quote hidden}> > >> CS EQU 0 >> MISO EQU 2 >> MOSI EQU 1 >> SPICLK EQU 2 > > > Whatever these are THEY SHOULD BE EXPLAINED in end of line comments at the > very least. > > >> org 0 >> goto init >> org 0x10 > > > I don't get the point of this. You don't have an interrupt routine, so > there is no reason your code can't start at 0 and continue directly from > there. Even if not, what's special about 10h? Yeah at one point I thought I was going to have an interrupt routine, and since I wasn't being very space-conscious with this code, so I spaced it out a little bit, and then I didn't bother reorganizing it. It's awkward, yes, but not really harmful. {Quote hidden}> > >>init clrf PORTB ;clear port b > > > Again, I think it's bad form to put executable instructions on the same line > as a label. > > >> bsf STATUS, RP0 >> bcf TRISA, SPICLK > > > This sort of manual bank switching is dangerous. I think you got it right > in this case, but sooner or later you'll make a mistake looking up the bank > of an SFR in the data sheet or setting the RPx bits accordingly. For > starter programs that are not space or speed cricital, just use BANKSEL. > Once you get more comfort with PICs you'll probably want to use something > like my DBANKIF and related macros in STD.INS.ASPIC at > http://www.embedinc.com/pic. > > This particular case is particularly bad since you are assuming that RP1 is > 0 but there is no mention of that restriction. Some day you'll insert code > above here and unrelated stuff will break. > > >> clrf PORTB > > > Why? You already cleared it a few instructions earlier. > > >> bsf PORTB, CS > > > Ah, so CS stands for a particular bit number in port B. You should SAY SO. > Also check out my /INBIT and /OUTBIT preprocessor directives. They handle > I/O port bit definitions in a nicer way. > > >> call xfer16 >> >> movlw 0x0A ;INADDRSITY : MAX >> movwf OUT1 > > > This is a serious bug because you are relying on XFER16 to either preserve > the bank or set it to 0, but XFER16 is not defined to do either. (Actually > none of its actions are defined, but that's another problem.) > > Just because you are getting away with it here doesn't make this any less of > a bug. OK, I can see how relying on the bank to stay the same can be dangerous, but considering that except for accessing the TRIS* registers, I'm not moving beyond the first bank at all in this program, I don't think it's a "serious bug". {Quote hidden}> > >>mainloop >> movlw CTR1 ;load starting address >> movwf FSR >> >>bump incf INDF, 1 ;increase current digit > > > Indirect bank setting? > > >> movlw 0x0F ;restrict to last four bits >> andwf INDF, 0 >> xorlw 0x0A ;check if equal to ten >> btfss STATUS, Z >> goto bump_done ;if not, done >> clrf INDF ;else, set to zero >> >> movf FSR, 0 ;check if this was the fourth digit >> xorlw CTR4 >> btfsc STATUS, Z ;if so, we're done >> goto bump_done >> incf FSR, 1 ;otherwise, go to next digit >> goto bump > > > Finally a few comments about the details, but all higher level explaination > is completely missing. You may not think of it as a bug, but it certainly > is, and a rather serious one at that. There should be block comments in > front of important labels like MAINLOOP and BUMP. When do we get back here? > What gets done each time thru the loop? When does the loop terminate? > > >>bump_done >> clrf ADDR > > > Again, you are assuming the loop above terminates with bank 0 set, but I > don't see anything specifying that, so it can't be relied on. There should > also be a comment here that the bank setting is assumed to be 0. > > >>xfer16 bcf PORTB, CS ;assert /CS > > >>From previous context, this is apparently a subroutine. So WHERE ARE THE > COMMENTS!!!!? Beginner or not, this sort of programming is simply > inexcusible and irresponsible. What is the purpose of this subroutine? > What does it take as input? What state does it change? What state does it > trash? > > There should be something in the code that makes it visually easy to see a > major break in flow. I use a comment line with all stars. A proper > subroutine comment header should make the break in the code visually > obvious, and it should tell a programmer everything he needs to know to call > the subroutine. You should be able to give other programmers just the > subroutine documentation header and they should be able to call the > subroutine correctly and deal with all its side effects without needing the > source code. OK, that's probably fair. But on the other hand, you wouldn't do a full doxygen-compatible documentation on "Hello World", either, so I think as long as I'm just screwing around and changing things every two seconds anyway, it's not a case of criminal negligence. Your point is definitely noted, though, and I promise the next time I submit something to the list it will be soaked with comments and documentation. Thanks very much for your in-depth review and also for pointing out the silly bug that was causing me trouble thus far. - Marcel

On Sun, 2005-06-12 at 10:25 -0700, Marcel Birthelmer wrote: > I'm using gpasm to assemble Olin said .... > > it's time to do things > > right and use relocatable mode. Do it now before you've settled into the > > bad habit of absolute mode. > I didn't know about this, but I will definitely research. Just to say gpasm DOES support relocatable code, and Olin is right, make the move now and save yourself the pain later ! Peter

Marcel Birthelmer wrote: > I'm using gpasm to assemble It would be good to point out you are using a non-standard assembler whenever you post code. Otherwise people will rightfully assume the code it intended for MPASM. > Yeah at one point I thought I was going to have an interrupt routine, > and since I wasn't being very space-conscious with this code, so I > spaced it out a little bit, and then I didn't bother reorganizing it. > It's awkward, yes, but not really harmful. This is another thing automatically handled in relocatable mode. The fixed code at the reset vector jumps to relocatable code, which the linker puts wherever it fits best. > OK, I can see how relying on the bank to stay the same can be > dangerous, > but considering that except for accessing the TRIS* registers, I'm not > moving beyond the first bank at all in this program, I don't think > it's > a "serious bug". It will be soon enough as you start writing anything beyond a simple LED blinker. If you don't religiously consider banking, you're going to waste a lot of time not so far down the road. ***************************************************************** Embed Inc, embedded system specialists in Littleton Massachusetts (978) 742-9014, http://www.embedinc.com

On Sun, 2005-06-12 at 13:42 -0400, Olin Lathrop wrote: > Marcel Birthelmer wrote: > > I'm using gpasm to assemble > > It would be good to point out you are using a non-standard assembler > whenever you post code. Otherwise people will rightfully assume the code it > intended for MPASM. Ok, I'm going to stick my neck out a bit here, but I believe gpasm's developers aim is to make it be as compatible as possible with MPASM. I've only ever used gpasm and I've never come across any code on the web that gpasm won't assemble, and while gpasm may have features missing I don't think it has any extras that would make valid gpasm code fail to work on MPASM. But I could be wrong. Peter

Peter Onion wrote: > Ok, I'm going to stick my neck out a bit here, but I believe gpasm's > developers aim is to make it be as compatible as possible with MPASM. > I've only ever used gpasm and I've never come across any code on the > web > that gpasm won't assemble, and while gpasm may have features missing I > don't think it has any extras that would make valid gpasm code fail to > work on MPASM. Does MPASM support the <name>: code label syntax? I hadn't seen that before in MPASM source. For a long time, Microchip had a bunch of secret features in MPASM that wasn't in the MPASM manual. I think they were listed in the MPLAB help, but of course you'd have to know to ask to get them. Is this possibly one of them, or is it an outright incompatibility in GPASM? ***************************************************************** Embed Inc, embedded system specialists in Littleton Massachusetts (978) 742-9014, http://www.embedinc.com

On Sun, 2005-06-12 at 15:16 -0400, Olin Lathrop wrote: > Does MPASM support the <name>: code label syntax? I hadn't seen that before > in MPASM source. You'll have to try it yourself Olin, as I don't have a windows machine here at all ! Peter

> Ok, I'm going to stick my neck out a bit here, but I believe gpasm's > developers aim is to make it be as compatible as possible with MPASM. > I've only ever used gpasm and I've never come across any code > on the web that gpasm won't assemble My WISP firmware? But I admit I used a lot of undocumented features. Wouter van Ooijen -- ------------------------------------------- Van Ooijen Technische Informatica: http://www.voti.nl consultancy, development, PICmicro products docent Hogeschool van Utrecht: http://www.voti.nl/hvu

> noted, though, and I promise the next time I submit something to the > list it will be soaked with comments and documentation. As a favour, would you (and everyone, as James often asks) also mind trimming your post to NOT include the entire message you're replying to. Thanks > as long as I'm just screwing around and changing things every two > seconds anyway, it's not a case of criminal negligence Not for you maybe, as you're working on this code and familiar with it, but someone coming in cold AND looking for a bug needs help to get up to speed

On Jun 12, 2005, at 5:00 AM, Martin Farrell wrote: > You've numbered the counters 0x28 to 0x31 rather than 0x28 to 0x2b. Heh. I did that myself recently. A 5bit mask of ones : 0x31, right? Hmmph. BillW

> Does MPASM support the <name>: code label syntax? Yep, sure does. I have copy-pasted lots of code with label: bla bla in, and it compiled fine without a twitch. I personally think this is quite nice to do, because the colon makes it a lot clearer that it's a label, and not just some instruction that no-one has heard of. I often get confused with what's labels when reading piclist emails - gmail does a rather nice job of screwing up tabs and indents. > Heh. I did that myself recently. A 5bit mask of ones : 0x31, right? Huh???? Yeh, i mean that 31 is a row of 5 ones, but how on earth is that relevent? Don't mean to flame, but it is a bit confusing. Donkey. Pencil sharpener. 7*6=42. -- Dave All us base are belong to you.

On Jun 12, 2005, at 10:25 AM, Marcel Birthelmer wrote: >>> Also, any constructive >>> criticism on code or whatever else is appreciated. >> [Olin's comments] > [counter-arguments] > The best code reviews ARE discussions; you need not do everything everyone suggests, but you should have reasons in your own mind to dismiss their comments. I think this has been a good example... BillW

On Jun 12, 2005, at 2:55 PM, .....dave.w.turnerKILLspam@spam@gmail.com wrote: >> Heh. I did that myself recently. A 5bit mask of ones : 0x31, right? > > Huh???? Yeh, i mean that 31 is a row of 5 ones, but how on earth is > that relevent? It's the same error as the original poster's program - thinking in decimal and writing in hex. 31 is 5 ones. 0x31 is something else! :-) BillW

Oh, e.g. 0x30 - 0x29 = 0x5 whereas: d'30' - d'29' = d'1' Well, you won't want to see my code then - I am constantly switching between decimal, hex and binary - binary for config registers, hex for things like delays which aren't specified exactly, and need to be fiddled with, decimal for exact numbers over 9. Somehow I manage.... Must be the number base conversion on my calculator ;-). On 6/12/05, William Chops Westfield <westfwKILLspammac.com> wrote: {Quote hidden}> > On Jun 12, 2005, at 2:55 PM, .....dave.w.turnerKILLspam.....gmail.com wrote: > > >> Heh. I did that myself recently. A 5bit mask of ones : 0x31, right? > > > > Huh???? Yeh, i mean that 31 is a row of 5 ones, but how on earth is > > that relevent? > > It's the same error as the original poster's program - thinking in > decimal and writing in hex. 31 is 5 ones. 0x31 is something else! > > :-) > BillW > -

Dave <EraseMEdave.w.turnerspam_OUTTakeThisOuTgmail.com> wrote: > Oh, e.g. > > 0x30 - 0x29 = 0x5 ???? Better let the assembler handle your arithmetic, rather than doing it in your head. -- Dave Tweed

> Does MPASM support the <name>: code label syntax? I hadn't seen that > before > in MPASM source. Yes, it does, as do most assemblers. I use it because then I can find the label easily in a text editor (instead of seeing all the references to the label). I also put the label on a line by itself for four reasons: 1: it can be long without messing up the code format 2: I can easily add code immediately after it 3: It gives me a place for comments to describe, for example, a loop invariant 4: I just like the way it looks ;-) Bob Ammerman RAm Systems

>> I just had to laugh at this one. You are using explicit >> HEX constant, but you are counting in decimal. > >And that would be the initialization bug. Thanks to you and >the other person who pointed this out. It is also the sort of bug that gets ruled out by using the RES directive, because you would define it as CTR RES 4 ; 4 byte counter - 1 byte per digit and then in your code refer to them as CTR, CTR+1, CTR+2, CTR+3 which also helps to show the relationship of the bytes, rather than as 4 separate variables, which could be independent counters for other uses.

Bob Ammerman wrote: > I use it because then I can find the label easily in a text editor > (instead of seeing all the references to the label). You can do the same thing with the other label format since those are forced to start in column 1. > I also put the label on a line by itself for four reasons: > > 1: it can be long without messing up the code format > 2: I can easily add code immediately after it > 3: It gives me a place for comments to describe, for example, a loop > invariant > 4: I just like the way it looks ;-) I agree completely. ***************************************************************** Embed Inc, embedded system specialists in Littleton Massachusetts (978) 742-9014, http://www.embedinc.com

> I also put the label on a line by itself for four reasons: Another reason (unless there's an error turn-off) is that a macro name can't share a line with a label eg, Illegal label (WAIT_SSP), build fails wait_ssp banksel pir1 but assembles OK as wait_ssp ;comment banksel pir1

Jinx wrote: > Another reason (unless there's an error turn-off) is that a macro > name can't share a line with a label I frequently put my UNBANK macro on the same line as a label. ***************************************************************** Embed Inc, embedded system specialists in Littleton Massachusetts (978) 742-9014, http://www.embedinc.com

> > Another reason (unless there's an error turn-off) is that a macro > > name can't share a line with a label > > I frequently put my UNBANK macro on the same line as a label. How do you stop MPLAB/MPASM failing the build ?

It isn't macros that can't be labeled, rather it is some of the standard pseudo-ops in the assember. Bob Ammerman RAm Systems {Original Message removed}

> It isn't macros that can't be labeled, rather it is some of the standard > pseudo-ops in the assember. Yes, that's true now I look more closely at what fails in a build. banksel for instance. My own macros don't cause a problem (working very long hours on the same project and had a bit of brain fade) What would be handy (jeepers, hope I get it right this time) is being able to "Set PC At Cursor" on a macro. Also, bookmarks saved when file/project closed. Breakpoints are

Jinx wrote: >> I frequently put my UNBANK macro on the same line as a label. > > How do you stop MPLAB/MPASM failing the build ? I don't do anything special. It just works. Before your post I had never considered this an issue (and still don't really believe it is). ***************************************************************** Embed Inc, embedded system specialists in Littleton Massachusetts (978) 742-9014, http://www.embedinc.com