Code check and refinement suggestions?

[HenrikOlsson]

Hi Chris,
That's quite a bit of code to go thru but one thing I noticed is code like this (which you have 3 sets of):

    '----------------------------VOLTAGE-------------------------------    
    if (volts/10 < TripVoltsLow1) OR  (volts/10 < TripVoltsLow2) OR (volts/10 < TripVoltsLow3) then
        HIGH OUT1   'ON
    ELSE
        LOW OUT1    'OFF
    ENDIF
    IF (VOLTS/10 > TRIPVOLTSHIGH1) OR (VOLTS/10 > TRIPVOLTSHIGH2) OR (VOLTS/10 > TRIPVOLTSHIGH3) THEN
        HIGH OUT1   'ON
    ELSE
        LOW OUT1    'OFF
    ENDIF

Here you're making the calculation volts/10 no less than 6 times even though the result will be the same each time. It would be more efficient from a speed and program memory point to do the calculation one time and using the result of that calculation.

More, HIGH/LOW are convenient commands but if have your TRIS-registers set correctly (and I think you have) you might as well do OUT1 = 1 or OUT1 = 0 etc, that's also more efficient both in execution time and program space.

The display routine (or part of it):

    LCDout $fe, 1   ' XXXXXXXXXXXXXXXXXXXX  <-- 20 DIGITS
        
        'Voltage Display
        If Volts = 0 then
            Lcdout $fe, LINE1, "VOLTAGE:       0.0 V"
        endif
        if (Volts > 0) AND (Volts < 10) then
            Lcdout $fe, LINE1, "VOLTAGE:       0.",DEC Volts," V"
        endif
        if (Volts > 9) AND (Volts < 100) then
            Lcdout $fe, LINE1, "VOLTAGE:       ",DEC Volts DIG 1,".",DEC Volts DIG 0," V"
        endif
        if (Volts > 99) AND (Volts < 1000) then
            Lcdout $fe, LINE1, "VOLTAGE:      ",DEC Volts DIG 2,DEC Volts DIG 1,".",DEC Volts DIG 0," V"
        endif
        if Volts > 1000 then
            Lcdout $fe, LINE1, "VOLTAGE:     ",DEC Volts DIG 3,DEC Volts DIG 2,DEC Volts DIG 1,".",DEC Volts DIG 0," V"
        endif

Here you are storing the string VOLTAGE: in program memory 5 times. It would be more effecient to first print out VOLTAGE: and then, depending on the value of Volts, print the rest. I also think that it can be optimzed quite a bit by using the modulo operator ( // ) instead of printing the individual digits like that but I'm not sure if you're achieving anything special by doing it the way you are. If Volts are in units of 0.1 volts (ie 1234 is 123.4V) then you could simply do

LCDOUT $FE, 1
LCDOUT $FE, LINE1, "Voltage: ", DEC3 Volts/10, ".", DEC Volts//10

Well, that's a couple of ideas you might look into.

/Henrik.

[kevlar129bp]

Thank you a million Henrik,

I can see all of your points. I'll work on those suggestions and further refine from there. BTW, I could not make heads or tails from the first two responses to this thread...I read them six times each and still felt like I had just entered the twilight zone ...but I sincerely appreciate your post and guidance.

Thanks again,
Chris

[HenrikOlsson]

Hi Chris,
You're welcome.
I think the other Chris (wdmagic) is just trying to be funny and I believe what Norm is saying is that you have a couple of labels which you aren't GOTO'ing or GOSUB'ing from anywhere in the code.

However, FineLine (which is an IDE that Norm is developing) apparently isn't picking up on the fact that they ARE defined as interrupt handlers using DT-Ints and therefor ARE being "used".

Even if they weren't "used" it wouldn't really matter - you can have a label at each and every line in the code if you wanted to, it won't take up any more space and it won't run any slower.

I can see the unused feature in FineLine being good for finding variables which are declared but not used though.

Keep it up!

/Henrik.