Critique my code, please.

Discussion in 'The Projects Forum' started by tracecom, Apr 23, 2013.

  1. tracecom

    Thread Starter AAC Fanatic!

    Apr 16, 2010
    3,869
    1,393
    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.

    Code ( (Unknown Language)):
    1. ' File Name:    Optical Timer.pbp
    2. ' Author:       tracecom
    3. ' Notice:       Copyright(c) 2013 Trace Communications; All Rights Reserved
    4. ' Compiler:     PICBASIC PRO 3
    5. ' Assembler:    MPASM  
    6. ' Target PIC:   12F509
    7. ' Oscillator:   4MHz Internal
    8. ' Hardware:     CRH Solderless Breadboard
    9. ' Schematic:    PIC12F509 Optical Timer.dch
    10. ' Keywords:     SEROUT, LCD, Photo Transistor
    11. ' Date:         4/23/2013
    12. ' Version:      1.0
    13. ' Notes:        
    14.  
    15. Include "modedefs.bas" ' Include serial modes.
    16. serout GPIO.0,T9600,["?BFF"] ' Set LCD backlight to maximum brightness.
    17. PAUSE 200 ' Pause to allow LCD EEPROM to program.
    18. SEROUT GPIO.0,T9600,["?G216"]' Configure the LCD geometry: 2x16.
    19. PAUSE 200 ' Pause to allow LCD EEPROM to program.
    20. LCD var GPIO.0  ' Declare a serial out pin.  
    21. PIP var GPIO.5  ' Declare a input pin.  
    22. RDY var GPIO.1  ' Delcare a ready pin for LED indicator.
    23. CNT Var word    ' Declare CNT as a word variable to store count.
    24. mainloop:
    25.     high RDY
    26.     if PIP = 0 then increment
    27.     if PIP = 1 then display
    28. increment:
    29.     CNT = CNT + 1
    30.     pause 1   ' Pause 1 millisecond.
    31.     goto mainloop
    32.  
    33. display:
    34.     If CNT = 0 Then no_data ' If count is zero, goto no_data.
    35.     serout LCD,T9600,["?l", "?m"] ' Clear screen + carriage return.
    36.     Serout LCD,T9600,[#CNT," mSec"]' If count is not zero, display count
    37.                                         ' + " mSec"
    38.     low RDY
    39.     pause 3000  ' Pause 3 seconds to allow time to read display.
    40.     serout LCD, T9600,["?l"]
    41.     CNT = 0     ' Reset count to zero.
    42.     goto mainloop
    43. no_data:
    44.     Serout LCD,T9600,["?m","No Data"]   ' Display "No Data" + carriage return.
    45.     goto mainloop
    46. 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.
     
    Last edited: Apr 23, 2013
  2. JohnInTX

    Moderator

    Jun 26, 2012
    2,347
    1,029
    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:
    Code ( (Unknown Language)):
    1.  
    2. idle:
    3.  high RDY
    4. idleloop:
    5.  if PIP=1 then idleloop  ' wait for a drop..(I don't know if this basic uses while..)
    6.  
    7. countloop: ' fell through to here when drop detected, count while coin in sensor
    8.  CNT = CNT+1 ' accumulate counts while PIP==0
    9.  pause 1
    10.  if PIP=0 then countloop
    11.  
    12. display: ' fall through to display stuff when coin clears sensor
    13.  low RDY
    14.  DISPLAY bla bla bla ' report results
    15.  goto idle  ' again..
    EDIT: you should also monitor CNT for overflow in case the coin gets stuck etc.
     
    Last edited: Apr 23, 2013
    tracecom likes this.
  3. tracecom

    Thread Starter AAC Fanatic!

    Apr 16, 2010
    3,869
    1,393
    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.
     
  4. JohnInTX

    Moderator

    Jun 26, 2012
    2,347
    1,029
    People who go out of their way to be trite and unhelpful annoy me as well.

    Good luck with your revs.
     
  5. tracecom

    Thread Starter AAC Fanatic!

    Apr 16, 2010
    3,869
    1,393
    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.

    [​IMG]

    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?
     
  6. whatsthatsmell

    Active Member

    Oct 9, 2009
    102
    4
    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"]
     
    tracecom likes this.
Loading...