Critique my code, please.

Thread Starter

tracecom

Joined Apr 16, 2010
3,944
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.

Rich (BB code):
' 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.

Thanks.

ETA: I just 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.
 

Attachments

Last edited:

JohnInTX

Joined Jun 26, 2012
4,787
With no coin yet, you got0 to display then no_data where it displays "No Data" then loops to do the same thing at machine speed. Your display will be repeatedly written to with the same info..

You should have a true idle loop where it looks for the transition from 1-0 on the opto input, proceed to something like your current main loop until the opto input goes high again then drop down ONCE to display the result. That way you don't need the 3 sec delay, it displays then goes right back to idle

Change the two if PIP = x statements to a single test. If PIP is not == to 0 it must be 1. No need to retest it.
if PIP=1 then display
else, just fall through to increment when PIP==0.

Some pseudocode:
Rich (BB code):
idle:
 high RDY
idleloop:
 if PIP=1 then idleloop  ' wait for a drop..(I don't know if this basic uses while..) 

countloop: ' fell through to here when drop detected, count while coin in sensor
 CNT = CNT+1 ' accumulate counts while PIP==0
 pause 1
 if PIP=0 then countloop

display: ' fall through to display stuff when coin clears sensor
 low RDY
 DISPLAY bla bla bla ' report results
 goto idle  ' again..
EDIT: you should also monitor CNT for overflow in case the coin gets stuck etc.
 
Last edited:

Thread Starter

tracecom

Joined Apr 16, 2010
3,944
Thanks John. On your first point, you are correct that the display is repeatedly rewritten with No Data, and I would like to fix that. I will study your suggestion and try to implement it. On your second point, I agree; in fact, just after I posted the code, I realized that I didn't need that second if statement. I will definitely fix that. And that's a good suggestion about monitoring CNT; I'll try to figure that out as well.

BTW, I posted an identical thread on the PBP forum, but I don't know anyone there, so I don't know what to expect. They may be fond of saying RTM, which always annoys me.
 

JohnInTX

Joined Jun 26, 2012
4,787
BTW, I posted an identical thread on the PBP forum, but I don't know anyone there, so I don't know what to expect. They may be fond of saying RTM, which always annoys me.
People who go out of their way to be trite and unhelpful annoy me as well.

Good luck with your revs.
 

Thread Starter

tracecom

Joined Apr 16, 2010
3,944
I still need to make one change to the code per John's suggestion, but I went ahead with some experimentation.

Here are the results.



The good news is the I didn't have any overlap between the slowest copper penny and the fastest zinc penny. The bad news is that there isn't much of a gap between the range for copper (85-98) and the range for zinc (73-83.) What do you guys think? Do I need to try for more consistency?
 

Attachments

whatsthatsmell

Joined Oct 9, 2009
102
BTW, I posted an identical thread on the PBP forum, but I don't know anyone there, so I don't know what to expect. They may be fond of saying RTM, which always annoys me.
I have found them to be very helpful and patient for the most part. The only problem is that the forum is not as popular as this one and it may take awhile to get an answer.

One suggestion to save a couple of keystrokes in your program is to use the mode number in your serout command as opposed to the mode. ( 2 is the mode number for T9600)

So:
serout LCD, T9600,["?l"]

becomes

serout LCD, 2,["?l"]
 
Top