[CC1101] FIFO Refills to transmit packets up to 255 bytes

[Crsarmv7l]

Hi @jgromes,

Been a while, but finally submitting the CC1101 pull for FIFO refills per our long discussion here:
#1138

You may or may not want to incorporate it as I introduced some blocks in the startTransmit function.

I still get some corrupt packets when using this method (about 1 in 20 randomly) with both an ESP32-S2 and ESP32-S3. A 4.7uf ceramic capacitor certainly seems to help things as the issue is more frequent without the capacitor.

Based on all of the research I have done over the months I have also found open issues on both those chips regarding SPI on the espressif/arduino-esp32 github.

I have an SAMD21 coming to try testing with that

[Crsarmv7l]

Ha, some checks fail if I use min some fail if I use std::min. I am using min with arduino IDE

[jgromes]

@Crsarmv7l so great to see you are back, and so glad this is finally a PR! I'll do a proper review in the next few days.

Ha, some checks fail if I use min some fail if I use std::min

The internal RADIOLIB_MIN macro should work for everything. Arduino gets confused by std::min and non-Arduino stuff doesn't know what min is unless we ad using namespace std; everywhere.

[Crsarmv7l]

@Crsarmv7l so great to see you are back, and so glad this is finally a PR! I'll do a proper review in the next few days.

Ha, some checks fail if I use min some fail if I use std::min

The internal RADIOLIB_MIN macro should work for everything. Arduino gets confused by std::min and non-Arduino stuff doesn't know what min is unless we ad using namespace std; everywhere.

For sure. I'll do the fix for RADIOLIB_MIN along with anything else you find after review.

I know some of the blocks are probably going to be contentious. I could probably also define the MARC states I am checking instead of using magic numbers.

But I'll wait for your feedback.

[jgromes]

Both this as well as the previous do/while loop should have some limit so that they don't become infinite if the radio fails. Usually I do this with a time limit by calling this->mod->hal->millis() at the start and checking for a timeout. If the timeout is reached, we can return RADIOLIB_ERR_TX_TIMEOUT.

[Crsarmv7l]

I can certainly put a limit on this one. Going to be busy with work but I will try and get to it on Sun/Mon. I need to see what a reasonable timeout is.

The Do/while loop will fallout if the TXBYTES register is zero because you will at some point get two equal zero reads so we don't have to worry about forever blocking on that one. TBH the double read is accurate per the errata, but we can get by with just a single read of the fifo register in most cases. If you are ok with trading a bit more potential error with one less SPI transaction per loop we can go that direction. In favor of one less read is the argument that you will have less of an underflow risk.

Second thing I have been thinking about. Since I am using SPI to check the MARC state for tx completion, (SPI proved more reliable for refills, I did a bunch of interrupt testing as well) using an interrupt:

int16_t state = SPIsetRegValue(RADIOLIB_CC1101_REG_IOCFG2, RADIOLIB_CC1101_GDOX_SYNC_WORD_SENT_OR_PKT_RECEIVED, 5, 0);
  RADIOLIB_ASSERT(state);

Doesn't make much sense. I left it for usage with packets 64 bytes or less to maintain compatibility. I should probably also do something like:

if (len <= RADIOLIB_CC1101_FIFO_SIZE) {
    int16_t state = SPIsetRegValue(RADIOLIB_CC1101_REG_IOCFG2, RADIOLIB_CC1101_GDOX_SYNC_WORD_SENT_OR_PKT_RECEIVED, 5, 0);
    RADIOLIB_ASSERT(state);
}

I can also do this (plus a timeout)

// Check MARCSTATE for Idle only for packets bigger than FIFO size
if (len > RADIOLIB_CC1101_FIFO_SIZE) {
     while(SPIgetRegValue(RADIOLIB_CC1101_REG_MARCSTATE, 4, 0) != 0x01) {};
}

[jgromes]

the double read is accurate per the errata, but we can get by with just a single read of the fifo register in most cases

Usually I'd rather spend an extra SPI transaction checking it, this seems like something that can come back later to bite us.

Regarding the interrupt config and reading MARCSTATE - actually, when does startTransmit stop blocking? When the last user byte is written to the FIFO, or after transmitting is complete? If it's the second case, then that would turn startTransmit basically into the same thing as transmit.

[Crsarmv7l]

It currently blocks until the transmission is done. My suggested edit for the MARC state read would keep it from blocking on transmissions 64 bytes or less.

That would keep the old (non-blocking) functionality currently there, while still offering longer (blocking) tx of larger packets.

I can run some more tests without checking MARC state. My concern and the reason I added it, is we don't want to finish tx while there are still bytes in the FIFO waiting to go out.

Hmm here is an idea, I could move checking for Marc state idle to finishTransmit. Then it wouldn't go to standby until tx is done and the block in startTransmit would be done when the last bytes are added to the FIFO.

[jgromes]

I could move checking for Marc state idle to finishTransmit

I think that's the correct approach since that would no longer block once there is nothing more to add to the FIFO and we are just waiting for it to empty out. This is pretty much how other radios behave (wit hthe caveat that their FIFO is 265 bytes, so we can just write the whole packet, exit and wait for some Tx done interrupt to fire).

[Crsarmv7l]

Sounds like a plan!

[jgromes]

@Crsarmv7l I left some feedback. More generally, I think there are two additional points that should also be addressed:

1. All references to the 64-byte limit on CC1101 throughout the library should be udpated. It's at least in the examples, though maybe some of the doxygen comments in CC1101.h mention it as well.
2. A note to startTransmit Doxygen and the interrupt transmit example should be added, explaining that if the packet length exceeds 64 bytes, startTransmit will block until only 64 bytes remain in the outgoing packet. I think it is a valid solution given the limitations of CC1101 (since if this is a problem for the user, they can just use short packets only), but it needs to be documented since CC1101::startTransmit will now behave slightly differently than the other modules.

[Crsarmv7l]

@Crsarmv7l I left some feedback. More generally, I think there are two additional points that should also be addressed:

1. All references to the 64-byte limit on CC1101 throughout the library should be udpated. It's at least in the examples, though maybe some of the doxygen comments in CC1101.h mention it as well.
2. A note to startTransmit Doxygen and the interrupt transmit example should be added, explaining that if the packet length exceeds 64 bytes, startTransmit will block until only 64 bytes remain in the outgoing packet. I think it is a valid solution given the limitations of CC1101 (since if this is a problem for the user, they can just use short packets only), but it needs to be documented since CC1101::startTransmit will now behave slightly differently than the other modules.

Aww Documentation is no fun at all 🥲 . I think I can handle those.

[Crsarmv7l]

Going to mark as a draft for now.

Had another idea that I want to investigate that would be worth including if beneficial.