Improved SerialUSB.read() reliability

cores/arduino/USB/CDC.cpp

@@ -33,14 +33,6 @@
 
 #define CDC_LINESTATE_READY		(CDC_LINESTATE_RTS | CDC_LINESTATE_DTR)
 
-struct ring_buffer {
-	uint8_t buffer[CDC_SERIAL_BUFFER_SIZE];
-	volatile uint32_t head;
-	volatile uint32_t tail;
-	volatile bool full;
-};
-ring_buffer cdc_rx_buffer = {{0}, 0, 0, false};
-
 typedef struct {
 	uint32_t dwDTERate;
 	uint8_t bCharFormat;
@@ -146,7 +138,6 @@ bool CDC_Setup(USBSetup& setup)
 	return false;
 }
 
-uint32_t _serialPeek = -1;
 void Serial_::begin(uint32_t /* baud_count */)
 {
 	// uart config is ignored in USB-CDC
@@ -161,45 +152,9 @@ void Serial_::end(void)
 {
 }
 
-void Serial_::accept(void)
-{
-	uint8_t buffer[CDC_SERIAL_BUFFER_SIZE];
-	uint32_t len = usb.recv(CDC_ENDPOINT_OUT, &buffer, CDC_SERIAL_BUFFER_SIZE);
-
-	uint8_t enableInterrupts = ((__get_PRIMASK() & 0x1) == 0);
-	__disable_irq();
-
-	ring_buffer *ringBuffer = &cdc_rx_buffer;
-	uint32_t i = ringBuffer->head;
-
-	// if we should be storing the received character into the location
-	// just before the tail (meaning that the head would advance to the
-	// current location of the tail), we're about to overflow the buffer
-	// and so we don't write the character or advance the head.
-	uint32_t k = 0;
-	while (len > 0 && !ringBuffer->full) {
-		len--;
-		ringBuffer->buffer[i++] = buffer[k++];
-		i %= CDC_SERIAL_BUFFER_SIZE;
-		if (i == ringBuffer->tail)
-			ringBuffer->full = true;
-	}
-	ringBuffer->head = i;
-	if (enableInterrupts) {
-		__enable_irq();
-	}
-}
-
 int Serial_::available(void)
 {
-	ring_buffer *buffer = &cdc_rx_buffer;
-	if (buffer->full) {
-		return CDC_SERIAL_BUFFER_SIZE;
-	}
-	if (buffer->head == buffer->tail) {
-		USB->DEVICE.DeviceEndpoint[CDC_ENDPOINT_OUT].EPINTENSET.reg = USB_DEVICE_EPINTENCLR_TRCPT(1);
-	}
-	return (uint32_t)(CDC_SERIAL_BUFFER_SIZE + buffer->head - buffer->tail) % CDC_SERIAL_BUFFER_SIZE;
+	return usb.available(CDC_ENDPOINT_OUT);
 }
 
 int Serial_::availableForWrite(void)
@@ -209,43 +164,38 @@ int Serial_::availableForWrite(void)
 	return (EPX_SIZE - 1);
 }
 
+int _serialPeek = -1;
+
 int Serial_::peek(void)
 {
-	ring_buffer *buffer = &cdc_rx_buffer;
-	if (buffer->head == buffer->tail && !buffer->full) {
-		return -1;
-	} else {
-		return buffer->buffer[buffer->tail];
-	}
+	if (_serialPeek != -1)
+		return _serialPeek;
+	_serialPeek = read();
+	return _serialPeek;
 }
 
-
-// if the ringBuffer is empty: try to fill it
-// if it's still empty: return -1
-// else return the last char
-// so the buffer is filled only when needed
 int Serial_::read(void)
 {
-	ring_buffer *buffer = &cdc_rx_buffer;
-
-	// if the head isn't ahead of the tail, we don't have any characters
-	if (buffer->head == buffer->tail && !buffer->full)
-	{
-		if (usb.available(CDC_ENDPOINT_OUT))
-			accept();
+	if (_serialPeek != -1) {
+		int res = _serialPeek;
+		_serialPeek = -1;
+		return res;
 	}
-	if (buffer->head == buffer->tail && !buffer->full)
-	{
-		return -1;
-	}
-	else
-	{
-		unsigned char c = buffer->buffer[buffer->tail];
-		buffer->tail = (uint32_t)(buffer->tail + 1) % CDC_SERIAL_BUFFER_SIZE;
-		buffer->full = false;
+	return usb.recv(CDC_ENDPOINT_OUT);
+}
 
-		return c;
+size_t Serial_::readBytes(char *buffer, size_t length)
+{
+	size_t count = 0;
+	_startMillis = millis();
+	while (count < length)
+	{
+		uint32_t n = usb.recv(CDC_ENDPOINT_OUT, buffer+count, length-count);
+		if (n == 0 && (millis() - _startMillis) >= _timeout)
+			break;
+		count += n;
 	}
+	return count;
 }
 
 void Serial_::flush(void)

cores/arduino/USB/SAMD21_USBDevice.h

@@ -62,8 +62,8 @@ class USBDevice_SAMD21G18x {
 	// USB Interrupts
 	inline bool isEndOfResetInterrupt()        { return usb.INTFLAG.bit.EORST; }
 	inline void ackEndOfResetInterrupt()       { usb.INTFLAG.reg = USB_DEVICE_INTFLAG_EORST; }
-	inline void enableEndOfResetInterrupt()    { usb.INTENSET.bit.EORST = 1; } 
-	inline void disableEndOfResetInterrupt()   { usb.INTENCLR.bit.EORST = 1; } 
+	inline void enableEndOfResetInterrupt()    { usb.INTENSET.bit.EORST = 1; }
+	inline void disableEndOfResetInterrupt()   { usb.INTENCLR.bit.EORST = 1; }
 
 	inline bool isStartOfFrameInterrupt()      { return usb.INTFLAG.bit.SOF; }
 	inline void ackStartOfFrameInterrupt()     { usb.INTFLAG.reg = USB_DEVICE_INTFLAG_SOF; }
@@ -195,3 +195,189 @@ void USBDevice_SAMD21G18x::calibrate() {
 	usb.PADCAL.bit.TRIM   = pad_trim;
 }
 
+/*
+ * Synchronization primitives.
+ * TODO: Move into a separate header file and make an API out of it
+ */
+
+class __Guard {
+public:
+	__Guard() : primask(__get_PRIMASK()), loops(1) {
+		__disable_irq();
+	}
+	~__Guard() {
+		if (primask == 0) {
+			__enable_irq();
+			// http://infocenter.arm.com/help/topic/com.arm.doc.dai0321a/BIHBFEIB.html
+			__ISB();
+		}
+	}
+	uint32_t enter() { return loops--; }
+private:
+	uint32_t primask;
+	uint32_t loops;
+};
+
+#define synchronized for (__Guard __guard; __guard.enter(); )
+
+
+/*
+ * USB EP generic handlers.
+ */
+
+class EPHandler {
+public:
+	virtual void handleEndpoint() = 0;
+	virtual uint32_t recv(void *_data, uint32_t len) = 0;
+	virtual uint32_t available() const = 0;
+};
+
+class DoubleBufferedEPOutHandler : public EPHandler {
+public:
+	DoubleBufferedEPOutHandler(USBDevice_SAMD21G18x &usbDev, uint32_t endPoint, uint32_t bufferSize) :
+		usbd(usbDev),
+		ep(endPoint), size(bufferSize),
+		current(0), incoming(0),
+		first0(0), last0(0), ready0(false),
+		first1(0), last1(0), ready1(false),
+		notify(false)
+	{
+		data0 = reinterpret_cast<uint8_t *>(malloc(size));
+		data1 = reinterpret_cast<uint8_t *>(malloc(size));
+
+		usbd.epBank0SetSize(ep, 64);
+		usbd.epBank0SetType(ep, 3); // BULK OUT
+
+		usbd.epBank0SetAddress(ep, const_cast<uint8_t *>(data0));
+
+		release();
+	}
+
+	virtual uint32_t recv(void *_data, uint32_t len)
+	{
+		uint8_t *data = reinterpret_cast<uint8_t *>(_data);
+
+		// R/W: current, first0/1, ready0/1, notify
+		// R  : last0/1, data0/1
+		if (current == 0) {
+			synchronized {
+				if (!ready0) {
+					return 0;
+				}
+			}
+			// when ready0==true the buffer is not being filled and last0 is constant
+			uint32_t i;
+			for (i=0; i<len && first0 < last0; i++) {
+				data[i] = data0[first0++];
+			}
+			if (first0 == last0) {
+				first0 = 0;
+				current = 1;
+				synchronized {
+					ready0 = false;
+					if (notify) {
+						notify = false;
+						release();
+					}
+				}
+			}
+			return i;
+		} else {
+			synchronized {
+				if (!ready1) {
+					return 0;
+				}
+			}
+			// when ready1==true the buffer is not being filled and last1 is constant
+			uint32_t i;
+			for (i=0; i<len && first1 < last1; i++) {
+				data[i] = data1[first1++];
+			}
+			if (first1 == last1) {
+				first1 = 0;
+				current = 0;
+				synchronized {
+					ready1 = false;
+					if (notify) {
+						notify = false;
+						release();
+					}
+				}
+			}
+			return i;
+		}
+	}
+
+	virtual void handleEndpoint()
+	{
+		// R/W : incoming, ready0/1
+		//   W : last0/1, notify
+		if (usbd.epBank0IsTransferComplete(ep))
+		{
+			// Ack Transfer complete
+			usbd.epBank0AckTransferComplete(ep);
+			//usbd.epBank0AckTransferFailed(ep); // XXX
+
+			// Update counters and swap banks
+			if (incoming == 0) {
+				last0 = usbd.epBank0ByteCount(ep);
+				incoming = 1;
+				usbd.epBank0SetAddress(ep, const_cast<uint8_t *>(data1));
+				ready0 = true;
+				synchronized {
+					if (ready1) {
+						notify = true;
+						return;
+					}
+					notify = false;
+				}
+			} else {
+				last1 = usbd.epBank0ByteCount(ep);
+				incoming = 0;
+				usbd.epBank0SetAddress(ep, const_cast<uint8_t *>(data0));
+				synchronized {
+					ready1 = true;
+					if (ready0) {
+						notify = true;
+						return;
+					}
+					notify = false;
+				}
+			}
+			release();
+		}
+	}
+
+	// Returns how many bytes are stored in the buffers
+	virtual uint32_t available() const {
+		return (last0 - first0) + (last1 - first1);
+	}
+
+	void release() {
+		// Release OUT EP
+		usbd.epBank0EnableTransferComplete(ep);
+		usbd.epBank0SetMultiPacketSize(ep, size);
+		usbd.epBank0SetByteCount(ep, 0);
+		usbd.epBank0ResetReady(ep);
+	}
+
+private:
+	USBDevice_SAMD21G18x &usbd;
+
+	const uint32_t ep;
+	const uint32_t size;
+	uint32_t current, incoming;
+
+	volatile uint8_t *data0;
+	uint32_t first0;
+	volatile uint32_t last0;
+	volatile bool ready0;
+
+	volatile uint8_t *data1;
+	uint32_t first1;
+	volatile uint32_t last1;
+	volatile bool ready1;
+
+	volatile bool notify;
+};
+

cores/arduino/USB/USBAPI.h

@@ -88,7 +88,7 @@ class USBDeviceClass {
 	uint32_t send(uint32_t ep, const void *data, uint32_t len);
 	void sendZlp(uint32_t ep);
 	uint32_t recv(uint32_t ep, void *data, uint32_t len);
-	uint32_t recv(uint32_t ep);
+	int recv(uint32_t ep);
 	uint32_t available(uint32_t ep);
 	void flush(uint32_t ep);
 	void stall(uint32_t ep);
@@ -112,14 +112,13 @@ extern USBDeviceClass USBDevice;
 class Serial_ : public Stream
 {
 public:
-	Serial_(USBDeviceClass &_usb) : usb(_usb) { }
+	Serial_(USBDeviceClass &_usb) : usb(_usb), stalled(false) { }
 	void begin(uint32_t baud_count);
 	void begin(unsigned long, uint8_t);
 	void end(void);
 
 	virtual int available(void);
 	virtual int availableForWrite(void);
-	virtual void accept(void);
 	virtual int peek(void);
 	virtual int read(void);
 	virtual void flush(void);
@@ -128,6 +127,8 @@ class Serial_ : public Stream
 	using Print::write; // pull in write(str) from Print
 	operator bool();
 
+	size_t readBytes(char *buffer, size_t length);
+
 	// This method allows processing "SEND_BREAK" requests sent by
 	// the USB host. Those requests indicate that the host wants to
 	// send a BREAK signal and are accompanied by a single uint16_t
@@ -168,8 +169,11 @@ class Serial_ : public Stream
 	};
 
 private:
+	int availableForStore(void);
+
 	USBDeviceClass &usb;
 	RingBuffer *_cdc_rx_buffer;
+	bool stalled;
 };
 extern Serial_ SerialUSB;