Code review please! And some questions :D

T'Saavik · 2007-11-15 00:39

What i want:
I've just put the finishing touches on my program, and i was hoping i could get some comments/suggestions for improving it

It is attached to this post. I tried to follow the new programming styles outlined in the pdf/helpfile for 1.51 .

About the program:
This program is designed to read (via rb.0 schmitt triggered rising_edge) pulses from
an IR receiver that are being reflected off a spinning spindle of a micro milling machine.

Here are a few questions i have:
1. In the documentation for sx/b it says things like pullups and schmitt can't be used with the "PIN" command on SX28 chips.
However, i see lots of examples here doing that with SX28 in the DEVICE line, is this no longer true?
Is this valid on a SX28 now? "Sensor PIN RB.0 INPUT SCHMITT CMOS INTR_RISE"

2. If i use a definition like the one above, and i want to pullup the unused pins on RB, what do i do? Or should i not bother doing that?

3. I'm out of general memory. What should i do. I looked into using the arrays but i need to do more homework methinks (i failed miserably).

My wishlist:
Eventually i want to add an EEPROM to save rpm speeds. I might get fancy and try to load a database of rpm feed speeds into the EEPROM memory itself, we shall see. I'd also like to experiment with running this via a battery and "sleep" the SX, i dunno how practical that will be with a big 4 led display and ir emitter sucking juice while its on, but we shall see.

Thanks all!

VonSzarvas · 2007-11-15 00:59

1. pullups certainly work in the PIN command nowadays. Example: "MyPin PIN RA.0 INPUT PULLUP"

2. You can still use PLP command to pullup unused Pins in Start part of program

3. Look at bit more into arrays !!

Goodluck, Max.

JonnyMac · 2007-11-15 01:29

Have you considered putting the display and a counter into an ISR? The foreground program could display the counter contents (x 60) every second to give you RPM without all the crazy math.

Bean · 2007-11-15 01:57

Using SCHMITT and CMOS on the same pin doesn't make any sense.
There are 3 trigger levels: CMOS (0.5*Vdd), TTL(1.4V), and SCHMITT(0.15*Vdd & 0.85*Vdd) and they are all mutually exclusive.
Port RA does NOT have SCHMITT option.

Bean.

▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔▔
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
www.iElectronicDesigns.com

·

T'Saavik · 2007-11-15 18:58

Maxwin: 1. Cool 2. Whoops, i had it there before i SX/B 1.5'd the code, I'll add it back 3. Okay, sigh [noparse]:D[/noparse]

JonnyMac: That was the first thing that came to mind when i started. I did some research first and found this post
http://forums.parallax.com/forums/default.aspx?f=7&m=199012. The issues about accuracy and Bean's
great code example (RPM.xsb) pushed me into the route i used. I wanted to use the ISR on the display's POV but i thought
an ISR would interfere with the PULSIN commands. Would it not? /rubs_hands_excitingly

Bean: AH! I went back and re-read the help file....whoops! When i saw the 15% and 85% i thought you could choose TTL or CMOS level as well.
And thanks for the RPM.sxb! It is much more accurate then what i would have cobbled together! Quick bonus question, right now I'm feeding it
10HZ pulses from the PIC on the PDB for testing. When i run my SX at the full 50mhz, (versus say 5mhz) my expected rpm of 600 drops to 590. Any ideas?
I would just keep the speed lower, but someone gave me a whole bunch of SMD SX28s with SMD 50mhz resonators on boards.

JonnyMac · 2007-11-15 20:11

T,

Here's what I'd do:

1) Set the ISR to run at 10 uS (rate = 100_000); use the ISR to display RPM all the time.

2) Set a bit flag (I call mine "isrFlag") at the beginning of the ISR -- this will be used outside the ISR for 10 uS timing units

3) Replace PULSIN with a couple functions that work like this one:

FUNC HIGHTIME
  tmpW1 = 0
  \ JB  Sensor, @$                              ' let old high clear
  \ JNB Sensor, @$                              ' let low clear  
  DO
    \ CLRB isrFlag                              ' clear old ISR marker
    \ JNB  isrFlag, @$                          ' wait for new marker
    INC tmpW1
    IF tmpW1 = 0 THEN EXIT 
  LOOP UNTIL Sensor = 0
  RETURN tmpW1
  ENDFUNC

4) Use the math that you already have working.

5) Move the RPM digits to the display registers and start measuring again. You may find that you need to add a bit of a delay between RPM updates now that everything is happening simultaneously.

I use ISRs in most of my SX/B programs these days so learning how to replace standard functions like this is very useful.

Post Edited (JonnyMac) : 11/15/2007 8:28:32 PM GMT

Sparks-R-Fun · 2007-11-15 20:37

I will offer a few comemnts.

IRC_CAL         IRC_SLOW   'dunno if needed, used to add startup overhead

This command is used to trim the internal resonator circuit, usually trying to get it somewhat close to 4MHz. This calibration occurs only at the time the device is programmed. Here you have selected to have this trimmed to run as slowly as possible. Since you are using an external resonator or oscillator, this command is not needed.

Maximizing Global Variable Space

Some tips for maximizing global variable space include making global variables as generic, temporary and reusable as possible and, of course, making the most of variable arrays. As an example of reusing global variables, unless I have overlooked something, you presently use word variables pWidth0 and pWidth1 exclusively to determine the length of the IR pulse. (I do see where you reuse pWidth0 elsewhere.) To me these two variables look like good candidates for temporary variables. You might try using temporary variables for pWidth0 and pWidth1. It should save you an extra four global bytes, unless I have missed something. (That is quite possible.)

Also, it looks like your bit variables and your “theDug” and “dug” byte variables can easily be placed inside an array. This will save you three more global bytes bringing the total of freed global bytes up to seven, if everything works as I have described.

This should give you a good start toward better optimizing your global variable space. I will leave it up to you or others to notice further opportunities for optimization. These are things that I noticed in addition to what has already been said, not in place of them.

The following variable declarations illustrate what I have outlined above.

' -----[noparse][[/noparse] Variables ]-------------------------------------------------------

' tach vars
rpm           VAR WORD
dividendMSW   VAR WORD
dividendLSW   VAR WORD

'wanna be local variables
tmpW1        VAR    Word
tmpW2        VAR    Word
tmpB1        VAR    Byte
tmpB2        VAR    Byte


[color=#990000]pWidth0       VAR tmpW1                  ' Variable "pWidth0" will use the same memory space as variable "tmpW1".
pWidth1       VAR tmpW2                  ' Variable "pWidth1" will use the same memory space as variable "tmpW2".

Tach_Array    VAR BYTE(16)               ' Create a 16-byte array to hold tach variables.
overflow      VAR Tach_Array(0).0        &#8216; Uses byte 0, bit 0 of Tach_Array.
doneBit       VAR Tach_Array(0).1        &#8216; Uses byte 0, bit 1 of Tach_Array.
theDig        VAR Tach_Array(1)          &#8216; Uses byte 1 of Tach_Array.
dug           VAR Tach_Array(2)          &#8216; Uses byte 2 of Tach_Array.[/color]

This also caught my eye.

pwidth0=10 'how many times to show rpm to display. I need to add banking, recycling this var for now, locals please god!
  pwidth0= pwidth0 << 2 '(<< 2 is just *4)this number has to be a multiple of 4 since we need to loop 4 times to get 4 digits

It seems to me that it would be shorter to just say

pWidth0 = 40

Or better yet, consider using a constant that is declared at the top of your program listing. (I am talking PURELY for OPTIMIZATION sake here! Since you have plenty of program space you might prefer to leave it in place since, as you aptly point out, using the rotate ensures the value is a multiple of four.)

Thanks for opening your code to scrutiny. I enjoy looking at other people’s code!

- Sparks

PJMonty · 2007-11-16 20:35

Sparks,

Actually the IRC_CAL IRC_SLOW directive won't cause any calibration to occur. Calibration only happens using the IRC_4MHZ value. Both the IRC_SLOW and IRC_FAST directives tell the SX-Key to simply set the calibration to the slowest or highest of the 8 possible settings. The advantage to using one of these is that it eleminates the IDE giving you a warning message that no IRC-CAL directive has been specified while noty slowing programming down at all.

Back in the gnarly old days of the IDE, the IRC_CAL setting was a global setting stored in the configuration dialog. This meant you had to manually change it to whatever each project needed. This was prone to error as it was easy to forget to change the setting on the configuration dialog. By adding the directive, you store the info with the project so it automatically programs the chip correctly The side effect is that the IDE wants to make sure you give it some value for IRC_CAL. It will default to IRC_SLOW if none is given.

Thanks,
PeterM

Sparks-R-Fun · 2007-11-16 22:12

Peter,

I appreciate the clarification. The comment in the quoted first CODE section was that of the author’s and not my own. (It looks like I did not quote it after all!) I was responding to his comment, which seemed to question the necessity of the command. Personally, I have not noticed any warning messages when I omit the IRC_CAL directive. Perhaps this is a result of the default setting. I primarily use SX/B.

I do understand that the IRC_FAST and IRC_SLOW settings do not result in any calculations being performed. They simply set the fuses to the fastest or slowest possible settings. I may not have stated that clearly.

From the history related in your second paragraph I better understand why this directive might be necessary. I have always considered it optional. I am glad a well-written IDE is there to cover my omissions with default settings.

Thanks so much!

- Sparks

PJMonty · 2007-11-18 08:42

Sparks,

Oops. I mis-read your post a bit, and thought you misunderstood the details of IRC_CAL. Oh well, a bit of history for the masses.

Thanks,
PeterM

Code review please! And some questions :D

Comments