I'm in the process of trying to write my own version of the RFM69 class. Not because there is anything wrong w/ the official one - I just find it a good way to understand things better and I'd like to experiment w/ some different interface ideas. Along the way, I've run into a couple of minor items that I think could be removed from the library (or it's equally likely that I'm missing something) and a few other questions.
Looking at:
https://github.com/LowPowerLab/RFM69/blob/master/RFM69.cpp1) In init(), there are these lines 93-96:
unsigned long start = millis();
uint8_t timeout = 50;
do writeReg(REG_SYNCVALUE1, 0xAA); while (readReg(REG_SYNCVALUE1) != 0xaa && millis()-start < timeout);
start = millis();
do writeReg(REG_SYNCVALUE1, 0x55); while (readReg(REG_SYNCVALUE1) != 0x55 && millis()-start < timeout);
But in CONFIG, lines 78-79, there are these lines:
/* 0x2F */ { REG_SYNCVALUE1, 0x2D }, // attempt to make this compatible with sync1 byte of RFM12B lib
/* 0x30 */ { REG_SYNCVALUE2, networkID }, // NETWORK ID
Does writing SYNCVALUE1 twice with different values do something if it's going to get over written during the CONFIG writing? And why is the do...while construct necessary in that case but not in any case where a register is being updated?
2) In setAddr() line 185 is calling:
writeReg(REG_NODEADRS, _address);
but the config line 82 has node address turned off. Obviously doesn't hurt anything - but it could be removed.
3) In interruptHandler() line 314:
if (_mode == RF69_MODE_RX && (readReg(REG_IRQFLAGS2) & RF_IRQFLAGS2_PAYLOADREADY))
I'm curious about why the second register check is needed. It seems like the only way the interrupt gets called in RX mode is if a payload is actually ready which seems like it makes the check unnecessary.
4) There are several cases where the AGC process is restarted by setting the PACKET2_RXRESTART bit. The comment for these lines says "avoid RX deadlock". The calls appears in receiveBegin() and all the send calls. I was wondering if anyone could explain what RX deadlock means and what this fixes?
5) I'm curious in general about the use of the interrupt system. It's used to trigger a packet received which I understand. I'm assuming that But in sendFrame, there is a while using a digitalRead on the interrupt pin to see find the send event rather than setting a flag in the interrupt handler to indicate that it was called. So why use the handler for receive but not transmit? And - what would happen if there was the same digitalRead() in receiveDone() to test for a received packet? That would allow for multiple radios to be created in one sketch (as a frequency bridge) by eliminating the static self pointer. There is also commented out code in sendFrame() that is watching the IRQFLAGS register to see if the packet was sent. Was this removed because the interrupt/pin system is faster than reading the register?
while (digitalRead(_interruptPin) == 0 && millis() - txStart < RF69_TX_LIMIT_MS); // wait for DIO0 to turn HIGH signalling transmission finish
//while (readReg(REG_IRQFLAGS2) & RF_IRQFLAGS2_PACKETSENT == 0x00); // wait for ModeReady
FYI - in case anyone cares: one of my goals is to see how hard it is to remove all of the while/timeout calls which could cause the radio to hold on to the execution for up to a second - especially in send() which might not return for that long because of the CSMA_LIMIT timeout. I'd prefer not to have my other code not run for that long if I can help it (even if it's unlikely to actually occur).
Other things I'm trying to accomplish:
- Remove macros from the header - I been bitten by macro conflicts (with other libraries) in the past that are hard to debug so I never like having #defines in any part of a public api.
- See if improving efficiency makes any difference. Each call to readReg/writeReg is triggering an spi select/unselect even though register calls tend to occur in groups. I'm going to try making it so there is a single select/unselect for each function whenever possible and see if there is any noticeable change speed.
- Have a state change return/callback poll() as the primary API method that can notify the caller on a variety of conditions like message received, message sent, ack received, ack sent, timeouts, errors, able to send, etc.
- Make certain calls more obvious as to what they're doing (at least to me). For example, there are a several calls like:
writeReg(REG_PACKETCONFIG2, (readReg(REG_PACKETCONFIG2) & 0xFE) | (key ? 1 : 0));
When I see this, I always have to check the bit pattern of 0xFE to make sure it's doing what it I think it should be. However, using an inlined function call (which generates the same assembly instructions) - it's easier for me to see that the following call is setting bit 0 of the register to 1 or 0 depending on the key.
setRegBit( REG_PACKETCONFIG2, BIT0, key ? 1: 0 );
Obviously these are just my opinions - I'm not saying there is anything wrong w/ the existing library at all (and if this comes across as critical of Felix's or anyone else's work - that's not my intent). If I wanted to actually get something working, I'd just use the library as is. This is really just more of an interesting experiment for me to try out some ideas and get a better understanding of how the radio works. They might not work, or might work and be worse, who knows.