From 76302fce5645da90c49752ea5e740ec79bc8330f Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Mon, 15 Jul 2024 03:05:50 -0700 Subject: [PATCH 1/5] Use `read_register()` to write 1 byte SPI commands I came across this tactic when writing the rust implementation. By telling `read_register()` to read 0 bytes from a register that is actually a reserved SPI command byte, we don't need to use a special form of `write_register(uint8_t, uint8_t)`. The same transaction still occurs with less code executed to do it. This only affects `get_status()`, `flush_tx()`, and `flush_rx()`. Luckily, those SPI commands already have 0x20 bit asserted, so there's no need to mask the command byte with `W_REGISTER` as is done in `write_register()`. This needs testing on the various supported platforms to ensure a 1-byte buffer behaves as expected. --- RF24.cpp | 60 ++++++++++++++++++++------------------------------------ RF24.h | 4 +--- 2 files changed, 22 insertions(+), 42 deletions(-) diff --git a/RF24.cpp b/RF24.cpp index bf2b6436..c1df5fd3 100644 --- a/RF24.cpp +++ b/RF24.cpp @@ -282,54 +282,36 @@ void RF24::write_register(uint8_t reg, const uint8_t* buf, uint8_t len) /****************************************************************************/ -void RF24::write_register(uint8_t reg, uint8_t value, bool is_cmd_only) +void RF24::write_register(uint8_t reg, uint8_t value) { - if (is_cmd_only) { - if (reg != RF24_NOP) { // don't print the get_status() operation - IF_RF24_DEBUG(printf_P(PSTR("write_register(%02x)\r\n"), reg)); - } - beginTransaction(); -#if defined(RF24_LINUX) - status = _SPI.transfer(W_REGISTER | reg); -#else // !defined(RF24_LINUX) || defined (RF24_RP2) - #if defined(RF24_SPI_PTR) - status = _spi->transfer(W_REGISTER | reg); - #else // !defined (RF24_SPI_PTR) - status = _SPI.transfer(W_REGISTER | reg); - #endif // !defined (RF24_SPI_PTR) -#endif // !defined(RF24_LINUX) || defined(RF24_RP2) - endTransaction(); - } - else { - IF_RF24_DEBUG(printf_P(PSTR("write_register(%02x,%02x)\r\n"), reg, value)); + IF_RF24_DEBUG(printf_P(PSTR("write_register(%02x,%02x)\r\n"), reg, value)); #if defined(RF24_LINUX) || defined(RF24_RP2) - beginTransaction(); - uint8_t* prx = spi_rxbuff; - uint8_t* ptx = spi_txbuff; - *ptx++ = (W_REGISTER | reg); - *ptx = value; + beginTransaction(); + uint8_t* prx = spi_rxbuff; + uint8_t* ptx = spi_txbuff; + *ptx++ = (W_REGISTER | reg); + *ptx = value; #if defined(RF24_RP2) - _spi->transfernb((const uint8_t*)spi_txbuff, spi_rxbuff, 2); + _spi->transfernb((const uint8_t*)spi_txbuff, spi_rxbuff, 2); #else // !defined(RF24_RP2) - _SPI.transfernb(reinterpret_cast(spi_txbuff), reinterpret_cast(spi_rxbuff), 2); + _SPI.transfernb(reinterpret_cast(spi_txbuff), reinterpret_cast(spi_rxbuff), 2); #endif // !defined(RF24_RP2) - status = *prx++; // status is 1st byte of receive buffer - endTransaction(); + status = *prx++; // status is 1st byte of receive buffer + endTransaction(); #else // !defined(RF24_LINUX) && !defined(RF24_RP2) - beginTransaction(); + beginTransaction(); #if defined(RF24_SPI_PTR) - status = _spi->transfer(W_REGISTER | reg); - _spi->transfer(value); + status = _spi->transfer(W_REGISTER | reg); + _spi->transfer(value); #else // !defined(RF24_SPI_PTR) - status = _SPI.transfer(W_REGISTER | reg); - _SPI.transfer(value); + status = _SPI.transfer(W_REGISTER | reg); + _SPI.transfer(value); #endif // !defined(RF24_SPI_PTR) - endTransaction(); + endTransaction(); #endif // !defined(RF24_LINUX) && !defined(RF24_RP2) - } } /****************************************************************************/ @@ -486,7 +468,7 @@ void RF24::read_payload(void* buf, uint8_t data_len) uint8_t RF24::flush_rx(void) { - write_register(FLUSH_RX, RF24_NOP, true); + read_register(FLUSH_RX, (uint8_t*)nullptr, 0); return status; } @@ -494,7 +476,7 @@ uint8_t RF24::flush_rx(void) uint8_t RF24::flush_tx(void) { - write_register(FLUSH_TX, RF24_NOP, true); + read_register(FLUSH_TX, (uint8_t*)nullptr, 0); return status; } @@ -502,7 +484,7 @@ uint8_t RF24::flush_tx(void) uint8_t RF24::get_status(void) { - write_register(RF24_NOP, RF24_NOP, true); + read_register(RF24_NOP, (uint8_t*)nullptr, 0); return status; } @@ -1325,7 +1307,7 @@ bool RF24::writeBlocking(const void* buf, uint8_t len, uint32_t timeout) void RF24::reUseTX() { write_register(NRF_STATUS, _BV(MAX_RT)); //Clear max retry flag - write_register(REUSE_TX_PL, RF24_NOP, true); + read_register(REUSE_TX_PL, (uint8_t*)nullptr, 0); ce(LOW); //Re-Transfer packet ce(HIGH); } diff --git a/RF24.h b/RF24.h index cdd72b77..2a8af231 100644 --- a/RF24.h +++ b/RF24.h @@ -1929,12 +1929,10 @@ class RF24 * * @param reg Which register. Use constants from nRF24L01.h * @param value The new value to write - * @param is_cmd_only if this parameter is true, then the `reg` parameter - * is written, and the `value` param is ignored. * @return Nothing. Older versions of this function returned the status * byte, but that it now saved to a private member on all SPI transactions. */ - void write_register(uint8_t reg, uint8_t value, bool is_cmd_only = false); + void write_register(uint8_t reg, uint8_t value); /** * Write the transmit payload From 7a537811684bc3b54df75d9c3a7305bfdfce8d31 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Mon, 15 Jul 2024 04:00:56 -0700 Subject: [PATCH 2/5] replace `reg | R_REGISTER` with just `reg` `R_REGISTER` is defined as 0. `reg | 0` always results in `reg` --- RF24.cpp | 8 ++++---- .../rf24_ATTiny/timingSearch3pin/timingSearch3pin.ino | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/RF24.cpp b/RF24.cpp index c1df5fd3..1c910b31 100644 --- a/RF24.cpp +++ b/RF24.cpp @@ -154,7 +154,7 @@ void RF24::read_register(uint8_t reg, uint8_t* buf, uint8_t len) uint8_t* ptx = spi_txbuff; uint8_t size = static_cast(len + 1); // Add register value to transmit buffer - *ptx++ = (R_REGISTER | reg); + *ptx++ = reg; while (len--) { *ptx++ = RF24_NOP; // Dummy operation, just for reading @@ -179,13 +179,13 @@ void RF24::read_register(uint8_t reg, uint8_t* buf, uint8_t len) beginTransaction(); #if defined(RF24_SPI_PTR) - status = _spi->transfer(R_REGISTER | reg); + status = _spi->transfer(reg); while (len--) { *buf++ = _spi->transfer(0xFF); } #else // !defined(RF24_SPI_PTR) - status = _SPI.transfer(R_REGISTER | reg); + status = _SPI.transfer(reg); while (len--) { *buf++ = _SPI.transfer(0xFF); } @@ -206,7 +206,7 @@ uint8_t RF24::read_register(uint8_t reg) uint8_t* prx = spi_rxbuff; uint8_t* ptx = spi_txbuff; - *ptx++ = (R_REGISTER | reg); + *ptx++ = reg; *ptx++ = RF24_NOP; // Dummy operation, just for reading #if defined(RF24_RP2) diff --git a/examples/rf24_ATTiny/timingSearch3pin/timingSearch3pin.ino b/examples/rf24_ATTiny/timingSearch3pin/timingSearch3pin.ino index d554c045..c2b79b3a 100644 --- a/examples/rf24_ATTiny/timingSearch3pin/timingSearch3pin.ino +++ b/examples/rf24_ATTiny/timingSearch3pin/timingSearch3pin.ino @@ -79,7 +79,7 @@ void csn(bool mode) { /****************************************************************************/ uint8_t read_register(uint8_t reg) { csn(LOW); - SPI.transfer(R_REGISTER | reg); + SPI.transfer(reg); uint8_t result = SPI.transfer(0xff); csn(HIGH); return result; From 8aba459e200e73bbfd8faae415edd349d6a46279 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Mon, 15 Jul 2024 04:18:07 -0700 Subject: [PATCH 3/5] don't `reg & REGISTER_MASK` `REGISTER_MASK` is defined as 0x1F. All register offsets are all under 0x1F. Any register offset masked with REGISTER_MASK results in the same register offset value, so we don't need to compute this at runtime. Note: all register offsets used are a known number. There is no public API that allows users to read a register offset larger than 0x1F. --- RF24.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/RF24.cpp b/RF24.cpp index 1c910b31..3bedbdf3 100644 --- a/RF24.cpp +++ b/RF24.cpp @@ -223,11 +223,11 @@ uint8_t RF24::read_register(uint8_t reg) beginTransaction(); #if defined(RF24_SPI_PTR) - status = _spi->transfer(R_REGISTER | reg); + status = _spi->transfer(reg); result = _spi->transfer(0xff); #else // !defined(RF24_SPI_PTR) - status = _SPI.transfer(R_REGISTER | reg); + status = _SPI.transfer(reg); result = _SPI.transfer(0xff); #endif // !defined(RF24_SPI_PTR) @@ -247,7 +247,7 @@ void RF24::write_register(uint8_t reg, const uint8_t* buf, uint8_t len) uint8_t* ptx = spi_txbuff; uint8_t size = static_cast(len + 1); // Add register value to transmit buffer - *ptx++ = (W_REGISTER | (REGISTER_MASK & reg)); + *ptx++ = (W_REGISTER | reg); while (len--) { *ptx++ = *buf++; } @@ -527,7 +527,7 @@ void RF24::print_address_register(const char* name, uint8_t reg, uint8_t qty) name); while (qty--) { uint8_t* buffer = new uint8_t[addr_width]; - read_register(reg++ & REGISTER_MASK, buffer, addr_width); + read_register(reg++, buffer, addr_width); printf_P(PSTR(" 0x")); uint8_t* bufptr = buffer + addr_width; @@ -546,7 +546,7 @@ uint8_t RF24::sprintf_address_register(char* out_buffer, uint8_t reg, uint8_t qt uint8_t offset = 0; uint8_t* read_buffer = new uint8_t[addr_width]; while (qty--) { - read_register(reg++ & REGISTER_MASK, read_buffer, addr_width); + read_register(reg++, read_buffer, addr_width); uint8_t* bufptr = read_buffer + addr_width; while (--bufptr >= read_buffer) { offset += sprintf_P(out_buffer + offset, PSTR("%02X"), *bufptr); From 8c5dda0fcc6c76c899be0449c5759a097969c76a Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Mon, 15 Jul 2024 16:03:00 -0700 Subject: [PATCH 4/5] writeAckPayload does not need to check for static payload size When writing an ACK payload to TX FIFO, static payload size is not applicable. So, bypass the additional checks in `write_payload()` about ensuring static payload size is satisfied. Note, the `W_ACK_PAYLOAD` command already has 0x20 bit (`W_REGISTER`) asserted. --- RF24.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RF24.cpp b/RF24.cpp index 3bedbdf3..f4e6a3b6 100644 --- a/RF24.cpp +++ b/RF24.cpp @@ -1756,7 +1756,7 @@ bool RF24::writeAckPayload(uint8_t pipe, const void* buf, uint8_t len) if (ack_payloads_enabled) { const uint8_t* current = reinterpret_cast(buf); - write_payload(current, len, W_ACK_PAYLOAD | (pipe & 0x07)); + write_register(W_ACK_PAYLOAD | (pipe & 0x07), current, rf24_min(len, static_cast(32))); return !(status & _BV(TX_FULL)); } return 0; From 0829b87cc4a18cba99c20a92bc0cb3f01940ca36 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sat, 20 Jul 2024 02:52:32 -0700 Subject: [PATCH 5/5] reviewed debug output --- RF24.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/RF24.cpp b/RF24.cpp index f4e6a3b6..92309f0c 100644 --- a/RF24.cpp +++ b/RF24.cpp @@ -330,7 +330,7 @@ void RF24::write_payload(const void* buf, uint8_t data_len, const uint8_t writeT } //printf("[Writing %u bytes %u blanks]",data_len,blank_len); - IF_RF24_DEBUG(printf("[Writing %u bytes %u blanks]\n", data_len, blank_len);); + IF_RF24_DEBUG(printf_P("[Writing %u bytes %u blanks]\n", data_len, blank_len);); #if defined(RF24_LINUX) || defined(RF24_RP2) beginTransaction(); @@ -402,7 +402,7 @@ void RF24::read_payload(void* buf, uint8_t data_len) //printf("[Reading %u bytes %u blanks]",data_len,blank_len); - IF_RF24_DEBUG(printf("[Reading %u bytes %u blanks]\n", data_len, blank_len);); + IF_RF24_DEBUG(printf_P("[Reading %u bytes %u blanks]\n", data_len, blank_len);); #if defined(RF24_LINUX) || defined(RF24_RP2) beginTransaction(); @@ -469,6 +469,7 @@ void RF24::read_payload(void* buf, uint8_t data_len) uint8_t RF24::flush_rx(void) { read_register(FLUSH_RX, (uint8_t*)nullptr, 0); + IF_RF24_DEBUG(printf_P("[Flushing RX FIFO]");); return status; } @@ -477,6 +478,7 @@ uint8_t RF24::flush_rx(void) uint8_t RF24::flush_tx(void) { read_register(FLUSH_TX, (uint8_t*)nullptr, 0); + IF_RF24_DEBUG(printf_P("[Flushing RX FIFO]");); return status; } @@ -1308,6 +1310,7 @@ void RF24::reUseTX() { write_register(NRF_STATUS, _BV(MAX_RT)); //Clear max retry flag read_register(REUSE_TX_PL, (uint8_t*)nullptr, 0); + IF_RF24_DEBUG(printf_P("[Reusing payload in TX FIFO]");); ce(LOW); //Re-Transfer packet ce(HIGH); } @@ -1672,7 +1675,7 @@ void RF24::enableDynamicPayloads(void) //toggle_features(); write_register(FEATURE, read_register(FEATURE) | _BV(EN_DPL)); - IF_RF24_DEBUG(printf("FEATURE=%i\r\n", read_register(FEATURE))); + IF_RF24_DEBUG(printf_P("FEATURE=%i\r\n", read_register(FEATURE))); // Enable dynamic payload on all pipes // @@ -1692,7 +1695,7 @@ void RF24::disableDynamicPayloads(void) //toggle_features(); write_register(FEATURE, 0); - IF_RF24_DEBUG(printf("FEATURE=%i\r\n", read_register(FEATURE))); + IF_RF24_DEBUG(printf_P("FEATURE=%i\r\n", read_register(FEATURE))); // Disable dynamic payload on all pipes // @@ -1713,7 +1716,7 @@ void RF24::enableAckPayload(void) if (!ack_payloads_enabled) { write_register(FEATURE, read_register(FEATURE) | _BV(EN_ACK_PAY) | _BV(EN_DPL)); - IF_RF24_DEBUG(printf("FEATURE=%i\r\n", read_register(FEATURE))); + IF_RF24_DEBUG(printf_P("FEATURE=%i\r\n", read_register(FEATURE))); // Enable dynamic payload on pipes 0 & 1 write_register(DYNPD, read_register(DYNPD) | _BV(DPL_P1) | _BV(DPL_P0)); @@ -1730,7 +1733,7 @@ void RF24::disableAckPayload(void) if (ack_payloads_enabled) { write_register(FEATURE, static_cast(read_register(FEATURE) & ~_BV(EN_ACK_PAY))); - IF_RF24_DEBUG(printf("FEATURE=%i\r\n", read_register(FEATURE))); + IF_RF24_DEBUG(printf_P("FEATURE=%i\r\n", read_register(FEATURE))); ack_payloads_enabled = false; } @@ -1746,7 +1749,7 @@ void RF24::enableDynamicAck(void) //toggle_features(); write_register(FEATURE, read_register(FEATURE) | _BV(EN_DYN_ACK)); - IF_RF24_DEBUG(printf("FEATURE=%i\r\n", read_register(FEATURE))); + IF_RF24_DEBUG(printf_P("FEATURE=%i\r\n", read_register(FEATURE))); } /****************************************************************************/