Merge pull request #3970 from elagil/fix_usb_iso_in_ep_stat

Fix USB ISO IN EP stat and ISO OUT buffer order

1 files changed, 83 insertions, 75 deletions

diff --git a/embassy-stm32/src/usb/usb.rs b/embassy-stm32/src/usb/usb.rs
index b9a16bbf1..8dabfc78f 100644
--- a/embassy-stm32/src/usb/usb.rs
+++ b/embassy-stm32/src/usb/usb.rs
@@ -80,10 +80,10 @@ impl<T: Instance> interrupt::typelevel::Handler<T::Interrupt> for InterruptHandl
 
         if istr.ctr() {
             let index = istr.ep_id() as usize;
-            CTR_TRIGGERED[index].store(true, Ordering::Relaxed);
 
             let mut epr = regs.epr(index).read();
             if epr.ctr_rx() {
+                RX_COMPLETE[index].store(true, Ordering::Relaxed);
                 if index == 0 && epr.setup() {
                     EP0_SETUP.store(true, Ordering::Relaxed);
                 }
@@ -91,6 +91,7 @@ impl<T: Instance> interrupt::typelevel::Handler<T::Interrupt> for InterruptHandl
                 EP_OUT_WAKERS[index].wake();
             }
             if epr.ctr_tx() {
+                TX_PENDING[index].store(false, Ordering::Relaxed);
                 //trace!("EP {} TX", index);
                 EP_IN_WAKERS[index].wake();
             }
@@ -122,7 +123,8 @@ const USBRAM_ALIGN: usize = 4;
 static BUS_WAKER: AtomicWaker = AtomicWaker::new();
 static EP0_SETUP: AtomicBool = AtomicBool::new(false);
 
-static CTR_TRIGGERED: [AtomicBool; EP_COUNT] = [const { AtomicBool::new(false) }; EP_COUNT];
+static TX_PENDING: [AtomicBool; EP_COUNT] = [const { AtomicBool::new(false) }; EP_COUNT];
+static RX_COMPLETE: [AtomicBool; EP_COUNT] = [const { AtomicBool::new(false) }; EP_COUNT];
 static EP_IN_WAKERS: [AtomicWaker; EP_COUNT] = [const { AtomicWaker::new() }; EP_COUNT];
 static EP_OUT_WAKERS: [AtomicWaker; EP_COUNT] = [const { AtomicWaker::new() }; EP_COUNT];
 static IRQ_RESET: AtomicBool = AtomicBool::new(false);
@@ -204,15 +206,13 @@ mod btable {
 mod btable {
     use super::*;
 
-    pub(super) fn write_in_tx<T: Instance>(_index: usize, _addr: u16) {}
-
-    pub(super) fn write_in_rx<T: Instance>(_index: usize, _addr: u16) {}
-
     pub(super) fn write_in_len_tx<T: Instance>(index: usize, addr: u16, len: u16) {
+        assert_eq!(addr & 0b11, 0);
         USBRAM.mem(index * 2).write_value((addr as u32) | ((len as u32) << 16));
     }
 
     pub(super) fn write_in_len_rx<T: Instance>(index: usize, addr: u16, len: u16) {
+        assert_eq!(addr & 0b11, 0);
         USBRAM
             .mem(index * 2 + 1)
             .write_value((addr as u32) | ((len as u32) << 16));
@@ -363,10 +363,11 @@ impl<'d, T: Instance> Driver<'d, T> {
                 return false; // reserved for control pipe
             }
             let used = ep.used_out || ep.used_in;
-            if used && (ep.ep_type == EndpointType::Isochronous || ep.ep_type == EndpointType::Bulk) {
-                // Isochronous and bulk endpoints are double-buffered.
+            if used && (ep.ep_type == EndpointType::Isochronous) {
+                // Isochronous endpoints are always double-buffered.
                 // Their corresponding endpoint/channel registers are forced to be unidirectional.
                 // Do not reuse this index.
+                // FIXME: Bulk endpoints can be double buffered, but are not in the current implementation.
                 return false;
             }
 
@@ -412,11 +413,23 @@ impl<'d, T: Instance> Driver<'d, T> {
                 let len = align_len_up(max_packet_size);
                 let addr = self.alloc_ep_mem(len);
 
-                // ep_in_len is written when actually TXing packets.
-                btable::write_in_tx::<T>(index, addr);
+                #[cfg(not(any(usbram_32_2048, usbram_32_1024)))]
+                {
+                    // ep_in_len is written when actually transmitting packets.
+                    btable::write_in_tx::<T>(index, addr);
 
-                if ep_type == EndpointType::Isochronous {
-                    btable::write_in_rx::<T>(index, addr);
+                    if ep_type == EndpointType::Isochronous {
+                        btable::write_in_rx::<T>(index, addr);
+                    }
+                }
+
+                #[cfg(any(usbram_32_2048, usbram_32_1024))]
+                {
+                    btable::write_in_len_tx::<T>(index, addr, 0);
+
+                    if ep_type == EndpointType::Isochronous {
+                        btable::write_in_len_rx::<T>(index, addr, 0);
+                    }
                 }
 
                 EndpointBuffer {
@@ -640,22 +653,25 @@ impl<'d, T: Instance> driver::Bus for Bus<'d, T> {
     fn endpoint_set_enabled(&mut self, ep_addr: EndpointAddress, enabled: bool) {
         trace!("set_enabled {:?} {}", ep_addr, enabled);
         // This can race, so do a retry loop.
-        let reg = T::regs().epr(ep_addr.index() as _);
-        trace!("EPR before: {:04x}", reg.read().0);
+        let epr = T::regs().epr(ep_addr.index() as _);
+        trace!("EPR before: {:04x}", epr.read().0);
         match ep_addr.direction() {
             Direction::In => {
                 loop {
                     let want_stat = match enabled {
                         false => Stat::DISABLED,
-                        true => Stat::NAK,
+                        true => match epr.read().ep_type() {
+                            EpType::ISO => Stat::VALID,
+                            _ => Stat::NAK,
+                        },
                     };
-                    let r = reg.read();
+                    let r = epr.read();
                     if r.stat_tx() == want_stat {
                         break;
                     }
                     let mut w = invariant(r);
                     w.set_stat_tx(Stat::from_bits(r.stat_tx().to_bits() ^ want_stat.to_bits()));
-                    reg.write_value(w);
+                    epr.write_value(w);
                 }
                 EP_IN_WAKERS[ep_addr.index()].wake();
             }
@@ -665,18 +681,18 @@ impl<'d, T: Instance> driver::Bus for Bus<'d, T> {
                         false => Stat::DISABLED,
                         true => Stat::VALID,
                     };
-                    let r = reg.read();
+                    let r = epr.read();
                     if r.stat_rx() == want_stat {
                         break;
                     }
                     let mut w = invariant(r);
                     w.set_stat_rx(Stat::from_bits(r.stat_rx().to_bits() ^ want_stat.to_bits()));
-                    reg.write_value(w);
+                    epr.write_value(w);
                 }
                 EP_OUT_WAKERS[ep_addr.index()].wake();
             }
         }
-        trace!("EPR after: {:04x}", reg.read().0);
+        trace!("EPR after: {:04x}", epr.read().0);
     }
 
     async fn enable(&mut self) {}
@@ -712,6 +728,7 @@ impl Dir for Out {
 /// For double-buffered endpoints, both the `Rx` and `Tx` buffer from a channel are used for the same
 /// direction of transfer. This is opposed to single-buffered endpoints, where one channel can serve
 /// two directions at the same time.
+#[derive(Clone, Copy, Debug)]
 enum PacketBuffer {
     /// The RX buffer - must be used for single-buffered OUT endpoints
     Rx,
@@ -836,7 +853,8 @@ impl<'d, T: Instance> driver::EndpointOut for Endpoint<'d, T, Out> {
             if self.info.ep_type == EndpointType::Isochronous {
                 // The isochronous endpoint does not change its `STAT_RX` field to `NAK` when receiving a packet.
                 // Therefore, this instead waits until the `CTR` interrupt was triggered.
-                if matches!(stat, Stat::DISABLED) || CTR_TRIGGERED[index].load(Ordering::Relaxed) {
+                if matches!(stat, Stat::DISABLED) || RX_COMPLETE[index].load(Ordering::Relaxed) {
+                    assert!(matches!(stat, Stat::VALID | Stat::DISABLED));
                     Poll::Ready(stat)
                 } else {
                     Poll::Pending
@@ -851,7 +869,7 @@ impl<'d, T: Instance> driver::EndpointOut for Endpoint<'d, T, Out> {
         })
         .await;
 
-        CTR_TRIGGERED[index].store(false, Ordering::Relaxed);
+        RX_COMPLETE[index].store(false, Ordering::Relaxed);
 
         if stat == Stat::DISABLED {
             return Err(EndpointError::Disabled);
@@ -859,31 +877,26 @@ impl<'d, T: Instance> driver::EndpointOut for Endpoint<'d, T, Out> {
 
         let regs = T::regs();
 
-        let packet_buffer = if self.info.ep_type == EndpointType::Isochronous {
+        let rx_len = if self.info.ep_type == EndpointType::Isochronous {
             // Find the buffer, which is currently in use. Read from the OTHER buffer.
-            if regs.epr(index).read().dtog_rx() {
-                PacketBuffer::Rx
-            } else {
+            let packet_buffer = if regs.epr(index).read().dtog_rx() {
                 PacketBuffer::Tx
-            }
-        } else {
-            PacketBuffer::Rx
-        };
-
-        let rx_len = self.read_data_double_buffered(buf, packet_buffer)?;
-
-        regs.epr(index).write(|w| {
-            w.set_ep_type(convert_type(self.info.ep_type));
-            w.set_ea(self.info.addr.index() as _);
-            if self.info.ep_type == EndpointType::Isochronous {
-                w.set_stat_rx(Stat::from_bits(0)); // STAT_RX remains `VALID`.
             } else {
+                PacketBuffer::Rx
+            };
+            self.read_data_double_buffered(buf, packet_buffer)?
+        } else {
+            regs.epr(index).write(|w| {
+                w.set_ep_type(convert_type(self.info.ep_type));
+                w.set_ea(self.info.addr.index() as _);
                 w.set_stat_rx(Stat::from_bits(Stat::NAK.to_bits() ^ Stat::VALID.to_bits()));
-            }
-            w.set_stat_tx(Stat::from_bits(0));
-            w.set_ctr_rx(true); // don't clear
-            w.set_ctr_tx(true); // don't clear
-        });
+                w.set_stat_tx(Stat::from_bits(0));
+                w.set_ctr_rx(true); // don't clear
+                w.set_ctr_tx(true); // don't clear
+            });
+
+            self.read_data(buf)?
+        };
         trace!("READ OK, rx_len = {}", rx_len);
 
         Ok(rx_len)
@@ -895,18 +908,31 @@ impl<'d, T: Instance> driver::EndpointIn for Endpoint<'d, T, In> {
         if buf.len() > self.info.max_packet_size as usize {
             return Err(EndpointError::BufferOverflow);
         }
+        trace!("WRITE WAITING, buf.len() = {}", buf.len());
 
+        let regs = T::regs();
         let index = self.info.addr.index();
 
-        trace!("WRITE WAITING");
+        if self.info.ep_type == EndpointType::Isochronous {
+            // Find the buffer, which is currently in use. Write to the OTHER buffer.
+            let packet_buffer = if regs.epr(index).read().dtog_tx() {
+                PacketBuffer::Rx
+            } else {
+                PacketBuffer::Tx
+            };
+
+            self.write_data_double_buffered(buf, packet_buffer);
+        }
+
         let stat = poll_fn(|cx| {
             EP_IN_WAKERS[index].register(cx.waker());
             let regs = T::regs();
             let stat = regs.epr(index).read().stat_tx();
             if self.info.ep_type == EndpointType::Isochronous {
-                // The isochronous endpoint does not change its `STAT_RX` field to `NAK` when receiving a packet.
+                // The isochronous endpoint does not change its `STAT_TX` field to `NAK` after sending a packet.
                 // Therefore, this instead waits until the `CTR` interrupt was triggered.
-                if matches!(stat, Stat::DISABLED) || CTR_TRIGGERED[index].load(Ordering::Relaxed) {
+                if matches!(stat, Stat::DISABLED) || !TX_PENDING[index].load(Ordering::Relaxed) {
+                    assert!(matches!(stat, Stat::VALID | Stat::DISABLED));
                     Poll::Ready(stat)
                 } else {
                     Poll::Pending
@@ -921,41 +947,23 @@ impl<'d, T: Instance> driver::EndpointIn for Endpoint<'d, T, In> {
         })
         .await;
 
-        CTR_TRIGGERED[index].store(false, Ordering::Relaxed);
-
         if stat == Stat::DISABLED {
             return Err(EndpointError::Disabled);
         }
 
-        let regs = T::regs();
-
-        let packet_buffer = if self.info.ep_type == EndpointType::Isochronous {
-            // Find the buffer, which is currently in use. Write to the OTHER buffer.
-            if regs.epr(index).read().dtog_tx() {
-                PacketBuffer::Tx
-            } else {
-                PacketBuffer::Rx
-            }
-        } else {
-            PacketBuffer::Tx
-        };
-
-        self.write_data_double_buffered(buf, packet_buffer);
+        if self.info.ep_type != EndpointType::Isochronous {
+            self.write_data(buf);
 
-        let regs = T::regs();
-        regs.epr(index).write(|w| {
-            w.set_ep_type(convert_type(self.info.ep_type));
-            w.set_ea(self.info.addr.index() as _);
-            if self.info.ep_type == EndpointType::Isochronous {
-                w.set_stat_tx(Stat::from_bits(0)); // STAT_TX remains `VALID`.
-            } else {
+            regs.epr(index).write(|w| {
+                w.set_ep_type(convert_type(self.info.ep_type));
+                w.set_ea(self.info.addr.index() as _);
                 w.set_stat_tx(Stat::from_bits(Stat::NAK.to_bits() ^ Stat::VALID.to_bits()));
-            }
-            w.set_stat_rx(Stat::from_bits(0));
-            w.set_ctr_rx(true); // don't clear
-            w.set_ctr_tx(true); // don't clear
-        });
-
+                w.set_stat_rx(Stat::from_bits(0));
+                w.set_ctr_rx(true); // don't clear
+                w.set_ctr_tx(true); // don't clear
+            });
+        }
+        TX_PENDING[index].store(true, Ordering::Relaxed);
         trace!("WRITE OK");
 
         Ok(())