Critique my code, please.

[tracecom]

I am planning to build a copper/zinc penny separator that uses eddy currents, and the first step is a timer that can measure the speed of a falling penny. I have breadboarded the timer in the attached schematic, and written the code below. It is working, but I am looking for improvement suggestions.

' File Name:    Optical Timer.pbp
' Author:       tracecom
' Notice:       Copyright(c) 2013 Trace Communications; All Rights Reserved
' Compiler:     PICBASIC PRO 3
' Assembler:    MPASM  
' Target PIC:   12F509
' Oscillator:   4MHz Internal
' Hardware:     CRH Solderless Breadboard
' Schematic:    PIC12F509 Optical Timer.dch
' Keywords:     SEROUT, LCD, Photo Transistor
' Date:         4/23/2013
' Version:      1.0
' Notes:        
  
Include "modedefs.bas" ' Include serial modes.
serout GPIO.0,T9600,["?BFF"] ' Set LCD backlight to maximum brightness.
PAUSE 200 ' Pause to allow LCD EEPROM to program.
SEROUT GPIO.0,T9600,["?G216"]' Configure the LCD geometry: 2x16.
PAUSE 200 ' Pause to allow LCD EEPROM to program.
LCD var GPIO.0  ' Declare a serial out pin.  
PIP var GPIO.5  ' Declare a input pin.  
RDY var GPIO.1  ' Delcare a ready pin for LED indicator.
CNT Var word    ' Declare CNT as a word variable to store count. 
mainloop:
    high RDY
    if PIP = 0 then increment
    if PIP = 1 then display
increment:
    CNT = CNT + 1
    pause 1   ' Pause 1 millisecond.
    goto mainloop
     
display:
    If CNT = 0 Then no_data ' If count is zero, goto no_data.
    serout LCD,T9600,["?l", "?m"] ' Clear screen + carriage return.
    Serout LCD,T9600,[#CNT," mSec"]' If count is not zero, display count
                                        ' + " mSec"
    low RDY
    pause 3000  ' Pause 3 seconds to allow time to read display.
    serout LCD, T9600,["?l"]
    CNT = 0     ' Reset count to zero.
    goto mainloop 
no_data:
    Serout LCD,T9600,["?m","No Data"]   ' Display "No Data" + carriage return.
    goto mainloop
End

Here's the way it is currently working.

As long as there is nothing blocking the light from white LED1 to Q1, the collector of Q2 is high, the red LED2 is off, and the PIC sends "No Data" to the LCD. The green LED3 is lit to indicate that the PIC is ready to read the time.

When the light is blocked from LED1 to Q1, the collector of Q2 goes low, which causes the counter to increment in 1 mSec steps, and the red LED2 is lit to indicate a measurement is being taken, and stored in the word variable CNT. The green LED3 is off. When the light block is removed, the count stops and the total is displayed on the LCD for 3 seconds. The shortest period of time that I have been able to record is 17 mSecs; I did that by sort of thumping a coin through the gap between LED1 and Q1.

Then, the cycle repeats.

I am just trying to learn PBP3 and I know my code is clumsy. Comments, criticisms, and corrections on the code and the schematic are welcome.

I noticed that there are some capitalization inconsistencies in the code that are not there in the IDE. Apparently, something about copy/paste changes some lower case letters to capitals.


Thank you.

[wdmagic]

I see no TRISIO statments, so you might want to add that, you might want to go ahead and set input to low before main program runs to make sure theres no tristate bits left in ports before codes ran, well look at the modified code below

Include "modedefs.bas" ' Include serial modes.
TRISIO = %00100000
serout GPIO.0,T9600,["?BFF"] ' Set LCD backlight to maximum brightness.
PAUSE 200 ' Pause to allow LCD EEPROM to program.
SEROUT GPIO.0,T9600,["?G216"]' Configure the LCD geometry: 2x16.
PAUSE 200 ' Pause to allow LCD EEPROM to program.
LCD var GPIO.0 ' Declare a serial out pin.
PIP var GPIO.5 : PIP = 0 ' Declare a input pin.
RDY var GPIO.1 : RDY = 0 ' Delcare a ready pin for LED indicator.
CNT Var word : CNT = 0 ' Declare CNT as a word variable to store count.
mainloop:
high RDY
if PIP = 0 then increment
if PIP = 1 then display
increment:
CNT = CNT + 1
IF CNT > 65535 then ' This adds a overload catch so it doesnt run away.
CNT = 0
goto no_data2
ENDIF
pause 1 ' Pause 1 millisecond.
goto mainloop

display:
If CNT = 0 Then no_data ' If count is zero, goto no_data.
serout LCD,T9600,["?l", "?m"] ' Clear screen + carriage return.
Serout LCD,T9600,[#CNT," mSec"]' If count is not zero, display count
' + " mSec"
low RDY
pause 3000 ' Pause 3 seconds to allow time to read display.
serout LCD, T9600,["?l"]
CNT = 0 ' Reset count to zero.
goto mainloop
no_data:
Serout LCD,T9600,["?m","No Data"] ' Display "No Data" + carriage return.
goto mainloop
no_data2:
Serout LCD,T9600,["?m","Overload"]
goto mainloop

End

[tracecom]

Chris,

Thank you very much for the good suggestions.

[Demon]

I give you a 9 out of 10 because you added a schematic.

You needed a picture for a 10.

And a video for bonus points.

Robert "smart-ass critique"

[wdmagic]

I agree Robert, I wish more people would post schematics, pictures and full code. although I know whats its like if your afraid someone will steal your ideas... but the ideas may have only gotten into the head by being sparked by ideas here. I know there is alot like that for me. So.. share the love people and start posting more Pic's (no pun intended), schematics and code

[Jerson]

This seems redundant to me

    if PIP = 0 then increment
    if PIP = 1 then display
increment:

It could be

    if PIP = 1 then display
increment:

Another possible improvement would be to consider using an interrupt to capture the change in state of PIP. Without that, you could miss / misread the timing since the code could be updating the display and doing a pause thereby losing time before you poll the PIP pin.

[HenrikOlsson]

Hi,
Just a quick note

CNT = CNT + 1
IF CNT > 65535 then ' This adds a overload catch so it doesnt run away.
CNT = 0
goto no_data2
ENDIF

The above doesn't really work because CNT is declared as a WORD and can not contain values larger than 65535. The IF statement will never evaluate true so the program will never jump to no_data2. Once CNT is incremented "beyond" 65535 it will automatically wrap around to 0.

/Henrik.

[pedja089]

Correct way to do that is:

CNT = CNT + 1
IF CNT =0 then ' This adds a overload catch so it doesnt run away.
    CNT = 0
    goto no_data2
ENDIF

[wdmagic]

yep, thanks henrik, I forgot to subtract 1, should have been 65534

[tracecom]

Thank you for the input. I have implemented your suggestions.