Can you optimise this >>> ?

[Darren Logan]

Hello all,

Im trying to shave off some code in order to gain some ROM space.

I have a procedure shown below which turns on various LEDs according to the passed argument. My question is, can you see a way I can trim this procedure down?

void switchled(int whichled,boolean state) {

int i;

i=whichled;

if(whichled==ALL) {
whichled=54;
}

do {
switch(whichled) {
case(56) : output_bit(PIN_S1LEDG,state);
break;
case(57) : output_bit(PIN_S1LEDR,state);
break;
case(58) : output_bit(PIN_S2LEDG,state);
break;
case(59) : output_bit(PIN_S2LEDR,state);
break;
case(60) : output_bit(PIN_EDVLEDG,state);
break;
case(61) : output_bit(PIN_EDVLEDR,state);
break;
case(55) : output_bit(PIN_PLEDG,state);
break;
case(54) : output_bit(PIN_PLEDR,state);
break;
}
} while((i==ALL) && (++whichled<62));
}




Regards,
Darren
___________________________
This message was ported from CCS's old forum
Original Post ID: 13710

[Neutone]

:=Hello all,
:=
:=Im trying to shave off some code in order to gain some ROM space.
:=
:=I have a procedure shown below which turns on various LEDs according to the passed argument. My question is, can you see a way I can trim this procedure down?
:=
:=void switchled(int whichled,boolean state) {
:=
:= int i;
:=
:= i=whichled;
:=
:= if(whichled==ALL) {
:= whichled=54;
:= }
:=
:= do {
:= switch(whichled) {
:= case(56) : output_bit(PIN_S1LEDG,state);
:= break;
:= case(57) : output_bit(PIN_S1LEDR,state);
:= break;
:= case(58) : output_bit(PIN_S2LEDG,state);
:= break;
:= case(59) : output_bit(PIN_S2LEDR,state);
:= break;
:= case(60) : output_bit(PIN_EDVLEDG,state);
:= break;
:= case(61) : output_bit(PIN_EDVLEDR,state);
:= break;
:= case(55) : output_bit(PIN_PLEDG,state);
:= break;
:= case(54) : output_bit(PIN_PLEDR,state);
:= break;
:= }
:= } while((i==ALL) && (++whichled<62));
:=}
:=
:=
:=
:=
:=Regards,
:=Darren

This is not quite what you are asking but I think it may help. You have a few discrete outputs. Assign the output status for each to of these to the bits within a single byte. If you need to toggle bit 0 do an XOR with 1. To turn on bit 0 do an OR with 1. To turn off bit 0 AND with FE. If you need to turn them all on simple set byte to FF. If you want to flash some of the bits simply make a flash mask like 0b00001010 and do an XOR on your byte when it's time to flash. I have an application that has to do this kind of thing. Each output can be normaly open or closed as the user wishes as well as flashing or steady when active. I create a byte for each user setting as well as the triggers. Processing time and code size are very small.
___________________________
This message was ported from CCS's old forum
Original Post ID: 13713

[Hans Wedemeyer]

Of course that assumes you are going to put that byte to a single port.

What is not explained is if all LED's are on one port !

They could be spread around various spare pins !
___________________________
This message was ported from CCS's old forum
Original Post ID: 13716

[Hans Wedemeyer]

Are the LED's all on the smae port ? or are they spread around on various spear pins ?
___________________________
This message was ported from CCS's old forum
Original Post ID: 13717

[Darren Logan]

Hello,

Good question!

6 of the 8 leds are on port C (C0 to C5), the other 2 are on port B (B6 and B7)

The constant assigned to the PINS by CCS are sequential though.
B6 = 54
B7 = 55
C0 = 56
:
:
C6 = 61

Regards,
Darren
___________________________
This message was ported from CCS's old forum
Original Post ID: 13720

[Neutone]

:=Of course that assumes you are going to put that byte to a single port.
:=
:=What is not explained is if all LED's are on one port !
:=
:=They could be spread around various spare pins !

I'm actually doing half of mine over an SPI bus. Having them all on the same port would make it a bit easier though. Even if the pins were all over the place the idea of keeping track of outputs with a byte and then matching the outputs to the byte is sound.
___________________________
This message was ported from CCS's old forum
Original Post ID: 13722

[Hans Wedemeyer]

Yes agree and there is nothing wrong with that, in fact it's "normal" ! if you ever worked with the Rabbot uP you have to do to keep track of lots of port registers.

However the question was how optimize the example code.
Using a byte would be effective IF all led's are on the same port.

At the end where the "keep track byte" is used you would still have to set port pins if the LED's are not on the same port, and that would need extra code.
___________________________
This message was ported from CCS's old forum
Original Post ID: 13726

[Hans Wedemeyer]

Neutone's idea of using a byte and setting bits, the putting that byte to the port is pretty standard stuff and would apply here if all LED's are on the same port.
As they are not, you may be be able to do something like this

switchled(int port, int whichled, boolean state)
{
int i;
if ( whichled != ALL )
{
output_bit( port+whichled, state);
}
else
{
// as there are 6 leds on port C it may produce smaller
// code using a for loop. need to check this
for ( i=0; i<6; i++)
{
output_bit(portc+i, state); // on or off
}

// as last two are on port B simply hard wire them
output_bit(PIN_B6, state); // on or off
output_bit(PIN_B7, state); // on or off
}
}

Then call the above function with the port var constant to ensure you are setting the correct pin. CCS describes how to calculate the port pin constants.

I have not tested this and who knows it may be as fat as the original code, but I doubt it.
good luck.
___________________________
This message was ported from CCS's old forum
Original Post ID: 13727

[Darren Logan]

hello,

thanks for this but the x in the following line:

output_bit(x,?)

must be a constant!

8-(

Regards,
Darren
___________________________
This message was ported from CCS's old forum
Original Post ID: 13734

[Hans Wedemeyer]

Simpley use bit_set(x,0) and bit_clear(x,0) it will still reduce the code.

if ( state == 0 )
{
set
}
else
{
clear
}
___________________________
This message was ported from CCS's old forum
Original Post ID: 13735

[j_purbrick]

:=Hello all,
:=
:=Im trying to shave off some code in order to gain some ROM space.
:=
:=I have a procedure shown below which turns on various LEDs according to the passed argument. My question is, can you see a way I can trim this procedure down?

I've taken the liberty of rewriting your function and I may have made some mistakes along the way, and I ignored the use of ALL, but the point was to try to make the compiler use the highly-efficient DECFSZ instruction. I don't think the case-switch inside the do-while is very efficient in terms of space or execution time.

Then, it would be better not to pass "state" as a parameter to the function. I always make booleans global, so the compiler can pack them into bytes most efficiently, and if you did that you'd call the function by using something like

state = 1;
switchled(whichled);

which would save on loading of the extra parameter too.

Depending on how your RAM is used, it might also be an advantage to make i into a global, to ensure that it gets put into the lowest RAM bank. That minimizes usage of switching between banks. Take a look at the LST file and see how much of that is going on.

Lastly, an instruction of the form

output_bit(PIN_S2LEDR,state);

uses 5 bytes but

output_bit(PIN_S2LEDR, 0);
if (state)
output_bit(PIN_S2LEDR,1);

only uses 3, if you can tolerate a very brief blink in the LED's operation.


void switchled(int whichled, boolean state) {

int i;

i = 62 - whichled; // 62-->0 (change none), 54-->8 (change all)

if (i == 0)
state = 0;

output_bit(PIN_EDVLEDR,state);
if (--i == 0)
return;
output_bit(PIN_EDVLEDG,state);
if (--i == 0)
return;
output_bit(PIN_S2LEDR,state);
if (--i == 0)
return;
output_bit(PIN_S2LEDG,state);
if (--i == 0)
return;
output_bit(PIN_S1LEDR,state);
if (--i == 0)
return;
output_bit(PIN_S1LEDG,state);
if (--i == 0)
return;
output_bit(PIN_PLEDG,state);
if (--i == 0)
return;
if (--i == 0)
return;
output_bit(PIN_PLEDR,state);
}
___________________________
This message was ported from CCS's old forum
Original Post ID: 13744