Hello!
It seems (at least to me) that you have some redundancy in your code.
You keep a variable called "port_status", which contains the status of all
your relays (I guess you have 8 relays).
Now why do you use an array RelayStatus since you have already the info
in port_status. I guess the relays reflect exactly your port 2, so why
don't you say directly:
P2 = port_status;
This would set all your relays status as an exact match of the variable.
You would not need an array to keep the status (therefore you save 8 bytes
space).
Now if you want to modify one relay only, then you can do:
Code:
const uint8 const uint8 BIT[8] = {
0x01, 0x02, 0x04, 0x08, 0x10, 0x20, 0x40, 0x80
};
set_relay(uint8 whichone, uint8 on_off) {
if(on_off) { // If you want to set the relay pointed by whichone
port_status |= BIT[whichone]; // Set bit "whichone" of P2
}
else { // You want to set the same relay off
port_status &= ~BIT[whichone]; // Clear bit "whichone" of P2
}
P2 = port_status; // Update the port
}
Just in case it was not clear enough:
In the above code, the variable whichone specifies the relay you want to update,
and the variable on_off specifies how you want to update it
(0 = off, nonzero = on).
By doing this, you can keep the port 2 as an exact image of the port_status
vector, and you can set all port bits at once instead of setting them in 8 steps.
Using the constant array BIT is faster than using a shifter function. And it will
be kept in flash, so it will not take any space in your RAM.
Note that I don't have access to the code and therefore cannot know if you
really need the array of relay status. Maybe you have good reasons for that.
But only looking at what you wrote, I think there is redundancy.
Last thing:
I still suppose that the relay is directly at the processor output (with a transistor
of some kind). In this case, testing if it's in a given state is useless.
Just set it the way you want it to be.
If it was already in the state you want it to be, then the function will do nothing.
If it was in the opposite state, then the function will change its status.
penoy_balut said:
Use pointers or index to an array.
It would be nice to explain him (and I would also be curious about the reason)
why he should use pointers instead of the array itself.
Dora.
visiontec said:
Im trying to get values out of an array and assign them to a P2 pins.
this is my code:
Code:
unsigned char xdata Relay_state[8];
void main (void)
{
.......
update_relay();
}
void update_relay(void)
{
int port_status = P2;
if(port_status & 2^0 !=Relay_state[0]) P2_0 = !Relay_state[0];
if(port_status & 2^1 !=Relay_state[1]) P2_1 = !Relay_state[1];
if(port_status & 2^2 !=Relay_state[2]) P2_2 = !Relay_state[2];
if(port_status & 2^3 !=Relay_state[3]) P2_3 = !Relay_state[3];
if(port_status & 2^4 !=Relay_state[4]) P2_4 = !Relay_state[4];
if(port_status & 2^5 !=Relay_state[5]) P2_5 = !Relay_state[5];
if(port_status & 2^6 !=Relay_state[6]) P2_6 = !Relay_state[6];
if(port_status & 2^7 !=Relay_state[7]) P2_7 = !Relay_state[7];
}
but is there any way of doin this more effeciently?
I tried doing this:
Code:
int shifter = 0x01;
int c = 0;
while(c<8)
{
if((port_status & shifter) != Relay_state[c])
{
P2 |= shifter;
}
c++;
shifter = shifter << c;
}
but this didnt really work out.
guys can someone tell me how i should do this?