refactor!: replace open_tx_pipe() with tx_address

2bndy5 · 2bndy5 · commit 8a5f2d0676c3 · 2025-04-27T00:11:24.000-07:00
related to nRF24/RF24#1028 but takes a different approach from nRF24/RF24#1029 (more like nRF24/RF24#1030)
diff --git a/bindings/node/src/radio/interface.rs b/bindings/node/src/radio/interface.rs
@@ -761,20 +761,29 @@ impl RF24 {
             .map_err(|e| Error::new(Status::GenericFailure, format!("{e:?}")))
     }
 
-    /// Set the address used for transmitting on pipe 0.
+    /// The TX address used on pipe 0 for outgoing transmissions.
     ///
-    /// Only pipe 0 can be used for transmitting. It is highly recommended to
-    /// avoid using pipe 0 to receive because of this.
+    /// Set this before calling {@link RF24::asTx}.
     ///
-    /// @param address - The address to receive data from.
+    /// This is cached on the {@link RF24} instance, so {@link RF24::asTx} can
+    /// use it to ensure proper auto-ack behavior in TX mode,
+    /// if pipe 0 is also used for RX with a different address.
     ///
     /// @group Basic
-    #[napi]
-    pub fn open_tx_pipe(&mut self, address: Buffer) -> Result<()> {
+    #[napi(setter, js_name = "txAddress")]
+    pub fn set_tx_address(&mut self, address: Buffer) {
         let address = address.to_vec();
-        self.inner
-            .open_tx_pipe(&address)
-            .map_err(|e| Error::new(Status::GenericFailure, format!("{e:?}")))
+        let addr_len = address.len().min(5);
+        self.inner.tx_address[0..addr_len].copy_from_slice(&address[0..addr_len])
+    }
+
+    /// @group Basic
+    #[napi(getter, js_name = "txAddress")]
+    pub fn get_tx_address(&mut self) -> Result<Buffer> {
+        let mut address = [0; 5];
+        let addr_len = self.get_address_length()? as usize;
+        address[0..addr_len].copy_from_slice(&self.inner.tx_address[0..addr_len]);
+        Ok(Buffer::from(&address[0..addr_len]))
     }
 
     /// Close the specified pipe from receiving transmissions.
diff --git a/bindings/python/src/radio/interface.rs b/bindings/python/src/radio/interface.rs
@@ -648,17 +648,24 @@ impl RF24 {
             .map_err(|e| PyRuntimeError::new_err(format!("{e:?}")))
     }
 
-    /// Set the address used for transmitting on pipe 0.
+    /// The TX address used on pipe 0 for outgoing transmissions.
     ///
-    /// Only pipe 0 can be used for transmitting. It is highly recommended to
-    /// avoid using pipe 0 to receive because of this.
+    /// Set this before calling [`RF24.as_tx()`][rf24_py.RF24.as_tx].
     ///
-    /// Parameters:
-    ///     address: The address to receive data from.
-    pub fn open_tx_pipe(&mut self, address: &[u8]) -> PyResult<()> {
-        self.inner
-            .open_tx_pipe(address)
-            .map_err(|e| PyRuntimeError::new_err(format!("{e:?}")))
+    /// This is cached on the [`RF24`][rf24_py.RF24] instance, so
+    /// [`RF24.as_tx()`][rf24_py.RF24.as_tx] can
+    /// use it to ensure proper auto-ack behavior in TX mode,
+    /// if pipe 0 is also used for RX with a different address.
+    #[setter]
+    pub fn set_tx_address(&mut self, address: &[u8]) {
+        let addr_len = address.len().min(5);
+        self.inner.tx_address[0..addr_len].copy_from_slice(&address[0..addr_len])
+    }
+
+    #[getter]
+    pub fn get_tx_address(&mut self) -> PyResult<Cow<[u8]>> {
+        let addr_len = self.get_address_length()? as usize;
+        Ok(Cow::from(&self.inner.tx_address[0..addr_len]))
     }
 
     /// Close the specified pipe from receiving transmissions.
diff --git a/crates/rf24-rs/src/radio/prelude.rs b/crates/rf24-rs/src/radio/prelude.rs
@@ -21,7 +21,7 @@ pub trait EsbPipe {
     ///
     /// If the specified `pipe` is not in range [0, 5], then this function does nothing.
     ///
-    /// Up to 6 pipes can be open for reading at once.  Open all the required
+    /// Up to 6 pipes can be open for reading at once. Open all the required
     /// reading pipes, and then call [`EsbRadio::as_rx()`].
     ///
     /// ### About pipe addresses
@@ -41,11 +41,14 @@ pub trait EsbPipe {
     ///
     /// <div class="warning">
     ///
+    /// Only pipe 0 can be used for transmitting. It is highly recommended to
+    /// avoid using pipe 0 to receive because of this.
+    ///
     /// If the pipe 0 is opened for receiving by this function, the `address`
     /// passed to this function (for pipe 0) will be restored at every call to
     /// [`EsbRadio::as_rx()`].
     /// This address restoration is implemented because of the underlying necessary
-    /// functionality of [`EsbPipe::open_tx_pipe()`].
+    /// functionality of [`EsbRadio::as_tx()`].
     ///
     /// It is important that the `address` length for pipe 0
     /// is equal to the length configured by [`EsbPipe::set_address_length()`].
@@ -56,10 +59,6 @@ pub trait EsbPipe {
     /// to understand how to avoid using malformed addresses.
     fn open_rx_pipe(&mut self, pipe: u8, address: &[u8]) -> Result<(), Self::PipeErrorType>;
 
-    /// Set an address to pipe 0 for transmitting when radio is in TX mode.
-    ///
-    fn open_tx_pipe(&mut self, address: &[u8]) -> Result<(), Self::PipeErrorType>;
-
     /// Close a specified pipe from receiving data when radio is in RX mode.
     fn close_rx_pipe(&mut self, pipe: u8) -> Result<(), Self::PipeErrorType>;
 
@@ -480,14 +479,21 @@ pub trait EsbRadio {
     ///
     /// Conventionally, this should be called after setting the RX addresses via
     /// [`EsbPipe::open_rx_pipe()`]
+    ///
+    /// This function will restore the cached RX address set to pipe 0.
+    /// This is done because the [`EsbRadio::as_tx()`] will appropriate the
+    /// RX address on pipe 0 for auto-ack purposes.
     fn as_rx(&mut self) -> Result<(), Self::RadioErrorType>;
 
     /// Put the radio into inactive TX mode.
     ///
     /// This must be called at least once before calling [`EsbRadio::send()`] or
     /// [`EsbRadio::write()`].
-    /// Conventionally, this should be called after setting the TX address via
-    /// [`EsbPipe::open_tx_pipe()`].
+    /// Conventionally, this should be called after setting the
+    /// [`tx_address`](value@crate::radio::rf24::RF24::tx_address).
+    ///
+    /// For auto-ack purposes, this function will also restore the cached
+    /// [`tx_address`](value@crate::radio::rf24::RF24::tx_address) to the RX pipe 0.
     fn as_tx(&mut self) -> Result<(), Self::RadioErrorType>;
 
     /// Is the radio in RX mode?
diff --git a/crates/rf24-rs/src/radio/rf24/init.rs b/crates/rf24-rs/src/radio/rf24/init.rs
@@ -1,4 +1,4 @@
-use super::{data_rate::set_tx_delay, registers, Feature, Nrf24Error, RF24};
+use super::{commands, data_rate::set_tx_delay, registers, Feature, Nrf24Error, RF24};
 use crate::{
     radio::{
         prelude::{EsbChannel, EsbFifo, EsbInit, EsbPayloadLength, EsbPipe, EsbPower, EsbStatus},
@@ -57,7 +57,8 @@ where
         self.flush_rx()?;
         self.flush_tx()?;
 
-        self.set_address_length(config.address_length())?;
+        let addr_len = config.address_length();
+        self.set_address_length(addr_len)?;
 
         self.spi_write_byte(registers::SETUP_RETR, config.auto_retries.into_bits())?;
         self.spi_write_byte(registers::EN_AA, config.auto_ack())?;
@@ -85,8 +86,17 @@ where
                 self.close_rx_pipe(pipe)?;
             }
         }
-        config.tx_address(&mut address);
-        self.open_tx_pipe(&address)?;
+        config.tx_address(&mut self.tx_address);
+        // use `spi_transfer()` to avoid multiple borrows of self (`spi_write_buf()` and `tx_address`)
+        for reg in [registers::TX_ADDR, registers::RX_ADDR_P0] {
+            self.buf[0] = reg | commands::W_REGISTER;
+            self.buf[1..addr_len as usize + 1]
+                .copy_from_slice(&self.tx_address[0..addr_len as usize]);
+            self.spi_transfer(addr_len + 1)?;
+        }
+        // enable pipe 0 for TX mode
+        self.spi_read(1, registers::EN_RXADDR)?;
+        self.spi_write_byte(registers::EN_RXADDR, self.buf[1] | 1)?;
 
         self.set_payload_length(config.payload_length())?;
 
@@ -305,6 +315,12 @@ mod test {
                     ],
                     vec![0xEu8, 0, 0, 0, 0, 0],
                 ),
+                // open pipe 0 for TX (regardless of auto-ack)
+                (vec![registers::EN_RXADDR, 0u8], vec![0xEu8, 0u8]),
+                (
+                    vec![registers::EN_RXADDR | commands::W_REGISTER, 1u8],
+                    vec![0xEu8, 0u8],
+                ),
                 // set payload length to 32 bytes on all pipes
                 (
                     vec![registers::RX_PW_P0 | commands::W_REGISTER, 32u8],
diff --git a/crates/rf24-rs/src/radio/rf24/mod.rs b/crates/rf24-rs/src/radio/rf24/mod.rs
@@ -80,6 +80,14 @@ pub struct RF24<SPI, DO, DELAY> {
     config_reg: ConfigReg,
     feature: Feature,
     pipe0_rx_addr: Option<[u8; 5]>,
+    /// The TX address used on pipe 0 for outgoing transmissions.
+    ///
+    /// Set this before calling [`RF24::as_tx()`].
+    ///
+    /// This is cached on the [`RF24`] instance, so [`RF24::as_tx()`] can
+    /// use it to ensure proper auto-ack behavior in TX mode,
+    /// if pipe 0 is also used for RX with a different address.
+    pub tx_address: [u8; 5],
     payload_length: u8,
 }
 
@@ -104,6 +112,7 @@ where
             status: StatusFlags::from_bits(0),
             buf: [0u8; 33],
             pipe0_rx_addr: None,
+            tx_address: [0xE7; 5],
             feature: Feature::from_bits(0)
                 .with_address_length(5)
                 .with_is_plus_variant(true),
@@ -284,6 +293,29 @@ mod test {
                 vec![registers::CONFIG | commands::W_REGISTER, 0xCu8],
                 vec![0xEu8, 0u8],
             ),
+            //set cached TX address
+            (
+                vec![
+                    registers::TX_ADDR | commands::W_REGISTER,
+                    0xE7,
+                    0xE7,
+                    0xE7,
+                    0xE7,
+                    0xE7
+                ],
+                vec![0xE, 0, 0, 0, 0, 0]
+            ),
+            (
+                vec![
+                    registers::RX_ADDR_P0 | commands::W_REGISTER,
+                    0xE7,
+                    0xE7,
+                    0xE7,
+                    0xE7,
+                    0xE7
+                ],
+                vec![0xE, 0, 0, 0, 0, 0]
+            ),
             // open pipe 0 for TX (regardless of auto-ack)
             (vec![registers::EN_RXADDR, 0u8], vec![0xEu8, 0u8]),
             (
diff --git a/crates/rf24-rs/src/radio/rf24/pipe.rs b/crates/rf24-rs/src/radio/rf24/pipe.rs
@@ -42,11 +42,6 @@ where
         self.spi_write_byte(registers::EN_RXADDR, out)
     }
 
-    fn open_tx_pipe(&mut self, address: &[u8]) -> Result<(), Self::PipeErrorType> {
-        self.spi_write_buf(registers::TX_ADDR, address)?;
-        self.spi_write_buf(registers::RX_ADDR_P0, address)
-    }
-
     /// If the given `pipe` number is  not in range [0, 5], then this function does nothing.
     fn close_rx_pipe(&mut self, pipe: u8) -> Result<(), Self::PipeErrorType> {
         if pipe > 5 {
@@ -106,28 +101,6 @@ mod test {
         let address = [0x55u8; 5];
         radio.open_rx_pipe(9, &address).unwrap();
         radio.open_rx_pipe(5, &address).unwrap();
-        spi.done();
-        ce_pin.done();
-    }
-
-    #[test]
-    pub fn open_tx_pipe() {
-        let mut expected_buf = [0x55u8; 6];
-        expected_buf[0] = registers::TX_ADDR | commands::W_REGISTER;
-        let mut p0_buf = [0x55u8; 6];
-        p0_buf[0] = registers::RX_ADDR_P0 | commands::W_REGISTER;
-        let mut response = [0u8; 6];
-        response[0] = 0xEu8;
-
-        let spi_expectations = spi_test_expects![
-            // open_rx_pipe(5)
-            (expected_buf.to_vec(), response.clone().to_vec()),
-            (p0_buf.to_vec(), response.to_vec()),
-        ];
-        let mocks = mk_radio(&[], &spi_expectations);
-        let (mut radio, mut spi, mut ce_pin) = (mocks.0, mocks.1, mocks.2);
-        let address = [0x55u8; 5];
-        radio.open_tx_pipe(&address).unwrap();
         radio.close_rx_pipe(9).unwrap();
         spi.done();
         ce_pin.done();
diff --git a/crates/rf24-rs/src/radio/rf24/radio.rs b/crates/rf24-rs/src/radio/rf24/radio.rs
@@ -42,9 +42,17 @@ where
         self.config_reg = self.config_reg.as_tx();
         self.spi_write_byte(registers::CONFIG, self.config_reg.into_bits())?;
 
+        let addr_len = self.feature.address_length();
+        // use `spi_transfer()` to avoid multiple borrows of self (`spi_write_buf()` and `tx_address`)
+        for reg in [registers::TX_ADDR, registers::RX_ADDR_P0] {
+            self.buf[0] = reg | commands::W_REGISTER;
+            self.buf[1..addr_len as usize + 1]
+                .copy_from_slice(&self.tx_address[0..addr_len as usize]);
+            self.spi_transfer(addr_len + 1)?;
+        }
+
         self.spi_read(1, registers::EN_RXADDR)?;
-        let out = self.buf[1] | 1;
-        self.spi_write_byte(registers::EN_RXADDR, out)
+        self.spi_write_byte(registers::EN_RXADDR, self.buf[1] | 1)
     }
 
     fn is_rx(&self) -> bool {
@@ -98,23 +106,22 @@ where
             // TX FIFO is full already
             return Ok(false);
         }
-        let mut buf_len = buf.len().min(32) as u8;
+        let buf_len = buf.len().min(32);
         // to avoid resizing the given buf, we'll have to use self._buf directly
         self.buf[0] = if !ask_no_ack {
             commands::W_TX_PAYLOAD
         } else {
             commands::W_TX_PAYLOAD_NO_ACK
         };
-        self.buf[1..(buf_len + 1) as usize].copy_from_slice(&buf[..buf_len as usize]);
+        self.buf[1..buf_len + 1].copy_from_slice(&buf[..buf_len]);
         // ensure payload_length setting is respected
-        if !self.feature.dynamic_payloads() && buf_len < self.payload_length {
+        if !self.feature.dynamic_payloads() && (buf_len as u8) < self.payload_length {
             // pad buf with zeros
-            for i in (buf_len + 1)..(self.payload_length + 1) {
-                self.buf[i as usize] = 0;
-            }
-            buf_len = self.payload_length;
+            self.buf[buf_len + 1..self.payload_length as usize + 1].fill(0);
+            self.spi_transfer(self.payload_length + 1)?;
+        } else {
+            self.spi_transfer(buf_len as u8 + 1)?;
         }
-        self.spi_transfer(buf_len + 1)?;
         if start_tx {
             self.ce_pin.set_high().map_err(Nrf24Error::Gpo)?;
         }
@@ -150,9 +157,7 @@ where
             return Ok(0);
         }
         self.spi_read(buf_len, commands::R_RX_PAYLOAD)?;
-        for i in 0..buf_len {
-            buf[i as usize] = self.buf[i as usize + 1];
-        }
+        buf[0..buf_len as usize].copy_from_slice(&self.buf[1..buf_len as usize + 1]);
         let flags = StatusFlags::from_bits(mnemonics::MASK_RX_DR);
         self.clear_status_flags(flags)?;
         Ok(buf_len)
@@ -256,7 +261,7 @@ mod test {
                 vec![registers::STATUS | commands::W_REGISTER, 0x70u8],
                 vec![0xEu8, 0u8],
             ),
-            // write cached _pipe0_rx_addr
+            // write cached pipe0_rx_addr
             (buf_expected.to_vec(), vec![0xEu8, 0u8, 0u8, 0u8, 0u8, 0u8]),
         ];
         let mocks = mk_radio(&ce_expectations, &spi_expectations);
@@ -279,6 +284,29 @@ mod test {
                 vec![registers::CONFIG | commands::W_REGISTER, 0xCu8],
                 vec![0xEu8, 0u8],
             ),
+            // set cached TX address to RX pipe 0
+            (
+                vec![
+                    registers::TX_ADDR | commands::W_REGISTER,
+                    0xE7,
+                    0xE7,
+                    0xE7,
+                    0xE7,
+                    0xE7
+                ],
+                vec![0xE, 0, 0, 0, 0, 0]
+            ),
+            (
+                vec![
+                    registers::RX_ADDR_P0 | commands::W_REGISTER,
+                    0xE7,
+                    0xE7,
+                    0xE7,
+                    0xE7,
+                    0xE7
+                ],
+                vec![0xE, 0, 0, 0, 0, 0]
+            ),
             // open pipe 0 for TX (regardless of auto-ack)
             (vec![registers::EN_RXADDR, 0u8], vec![0xEu8, 0u8]),
             (
