| ??? 05/08/08 20:20 Modified: 05/08/08 20:57 Read: times |
#154592 - You can still do better Responding to: ???'s previous message |
Chris said:
void RS232_Write(unsigned char reg, unsigned char dat0, unsigned char dat1)
{
unsigned char REG_H, REG_L, DAT1_H, DAT1_L, DAT2_H, DAT2_L, CHECKSUM_H,
CHECKSUM_L, CHECKSUM_T;
REG_H = Hex2Ascii(reg >> 4); // do the Upper Nibble
REG_L = Hex2Ascii(reg & 0x0F); // do the Lower Nibble
DAT1_H = Hex2Ascii(dat0 >> 4); // do the Upper Nibble
DAT1_L = Hex2Ascii(dat0 & 0x0F); // do the Lower Nibble
DAT2_H = Hex2Ascii(dat1 >> 4); // do the Upper Nibble
DAT2_L = Hex2Ascii(dat1 & 0x0F); // do the Lower Nibble
CHECKSUM_T = (cam_header + cam_ID_H + cam_ID_L + REG_H + REG_L + DAT1_H +
DAT1_L + DAT2_H + DAT2_L);
CHECKSUM_H = Hex2Ascii(CHECKSUM_T >> 4); // do the Upper Nibble
CHECKSUM_L = Hex2Ascii(CHECKSUM_T & 0x0F); // do the Lower Nibble
//***********************************************************************
uart_init();
uart_transmit(cam_header); Wait(2); // header
uart_transmit(cam_ID_H); Wait(2); // ID H
uart_transmit(cam_ID_L); Wait(2); // ID L
uart_transmit(REG_H); Wait(2); // Register H
uart_transmit(REG_L); Wait(2); // Register L
uart_transmit(DAT1_H); Wait(2); // Data 1 H
uart_transmit(DAT1_L); Wait(2); // Data 1 L
uart_transmit(DAT2_H); Wait(2); // Data 2 H
uart_transmit(DAT2_L); Wait(2); // Data 2 L
uart_transmit(CHECKSUM_H); Wait(2); // Send calculated checksum 1
uart_transmit(CHECKSUM_L); Wait(2); // Send calculated checksum 2
//***********************************************************************
}
This is better than before. Although it works, there are still two problems. The first is the clumsy duplication of the same pattern over and over again in the last eleven lines. When you find yourself writing this kind of code, you should almost always be using a loop instead. That way, if the pattern changes, you only have to change it once instead of eleven times. Also, the loop will use lots less code space. The other problem is the way that you have decided to deal with the bytes in the packet as individual entities. As a result of this decision, you had to declare a whole bunch of temporary variables (REG_H, REG_L, ... etc.), and you had to name each one of them explicitly in the checksum calculation. What if the packet was 20 bytes long? Or 100? You'd eventually get tired of declaring variables, and the checksum calculation would eventually get to be unmanageably long. Consider rewriting the function yet again, according to these instructions:
-- Russ <heresy>PS: If you really have tons of code space to burn, OR if you are already using formatted I/O someplace else in your program, then you could consider using sprintf() to format the packet instead of all this manual horsing around.</heresy> |
