Merge pull request #8 from HydraDancer/v1.1.3

v1.1.3

README.md

@@ -11,21 +11,11 @@ The following examples have been confirmed working:
 * template.py
 * usbproxy.py : USB Flash Drive in USB2 High-Speed
 
-**DISCLAIMER** : current results for the [highly-stressed stress test of Facedancer](https://github.com/greatscottgadgets/facedancer/blob/main/test/test_stress.py) with 20000 tries.
+**NOTE** : current results for the [highly-stressed stress test of Facedancer](https://github.com/greatscottgadgets/facedancer/blob/main/test/test_stress.py) with 20000 tries.
 
-The current Facedancer stress test results are the following.
-* USB2 High-Speed
-  * bulk IN/ctrl IN : pass
-  * bulk OUT/ctrl OUT : fails after a few hundred/thousand tries, never reaches 20000
-* USB2 Full-Speed
-  * bulk IN/ctrl IN : fails after a few hundred/thousand tries, never reaches 20000
-  * bulk OUT/ctrl OUT : fails after a few hundred/thousand tries, never reaches 20000
+TLDR : the stress test usually fails after a few thousand tries in both Full-Speed and High-Speed. In practice, Hydradancer is usable (see list of devices above) but might fail in highly-stressed situations. Note that we increased the requirements for this stress test (using the highly-stressed one by default and going up to 20000 tries instead of 100)
 
-We are currently working on fixing those issues and we have a few culprits in mind :
-* missed interrupts : the main culprit for now, it puts Hydradancer in a blocked state.
-* differences between HS/FS : HS has PING packets which reduces the amount of data transfers for OUT transactions. Since there are no FS examples from WCH and no indications in the datasheet, we experimented to solve this issue.
-
-We implemented a [firmware](https://github.com/hydrausb3/wch-ch56x-lib/tree/main/tests/test_firmware_usb_stress_test) to test the USB2 implementation of `wch-ch56x-lib` with the same stress test and it passes with 100000 tries in both HS and FS. However, Hydradancer's firmware is more complex (more interrupts, USB3 and USB2 at the same time, delays to synchronize with Facedancer).
+More about it [here](#stress-test-results).
 
 # Getting started (Hydradancer dongle)
 
@@ -236,6 +226,22 @@ For now, the tests in hydradancer/tests consist in loop-back devices, to test fo
 
 More information about the different scenarios can be found in [docs/Testing.md](docs/Testing.md).
 
+## Stress-test results
+
+The current Facedancer stress test results are the following.
+* USB2 High-Speed
+  * bulk IN/ctrl IN : pass
+  * bulk OUT/ctrl OUT : fails after a few hundred/thousand tries, rarely reaches 20000
+* USB2 Full-Speed
+  * bulk IN/ctrl IN : fails after a few hundred/thousand tries, rarely reaches 20000
+  * bulk OUT/ctrl OUT : fails after a few hundred/thousand tries, rarely reaches 20000
+
+Currently Hydradancer is usable (see the list of working devices above), however data corruption or timeouts might happen in very stressed conditions. The highly stressed stress test blasts USB transfers of random size and type (control/bulk) and then verifies the integrity of the transfer using USB control transfers.
+
+Solving this issue has proven difficult : it looks random, does not happen immediately (sometimes never). Adding logs or debugging using a USB sniffer can add additional delays and issues. Other architectures have been tried (FreeRTOS, doing all the processing in the interrupt handlers) however they proved to be slower and not more stable.
+
+We implemented a [firmware](https://github.com/hydrausb3/wch-ch56x-lib/tree/main/tests/test_firmware_usb_stress_test) to test the USB2 implementation of `wch-ch56x-lib` with the same stress test and it passes with 100000 tries in both HS and FS. However, Hydradancer's firmware is more complex (more interrupts, USB3 and USB2 at the same time, delays to synchronize with Facedancer).
+
 # How to contribute
 
 If you encounter bugs or want to suggest new features, please check the existing issues and create a new issue if necessary.

hydradancer/firmware_hydradancer/CMakeLists.txt

@@ -16,7 +16,7 @@ target_include_directories(${PROJECT_NAME} PUBLIC ${CMAKE_CURRENT_LIST_DIR}/User
 
 #### wch-ch56x-lib options
 
-target_compile_definitions(${PROJECT_NAME} PRIVATE POOL_BLOCK_SIZE=512 POOL_BLOCK_NUM=100 INTERRUPT_QUEUE_SIZE=40)
+target_compile_definitions(${PROJECT_NAME} PRIVATE POOL_BLOCK_SIZE=1024 POOL_BLOCK_NUM=50 INTERRUPT_QUEUE_SIZE=40)
 
 #### logging options
 

hydradancer/firmware_hydradancer/User/definitions.c

@@ -21,14 +21,16 @@ HYDRA_FIFO_DEF(event_queue, hydradancer_event_t, EVENT_QUEUE_SIZE);
 
 void hydradancer_send_event(void)
 {
-	if (event_transfer_finished)
+	BSP_ENTER_CRITICAL();
+	if (hydradancer_get_event_transfer_finished())
 	{
 		uint16_t events_count = fifo_count(&event_queue);
 		if (events_count > 0)
 		{
-			event_transfer_finished = false;
+			hydradancer_set_event_transfer_finished(false);
 			uint16_t count_read = fifo_read_n(&event_queue, (void*)_events_buffer, events_count);
 			endp_tx_set_new_buffer(&usb_device_1, 1, (uint8_t*)_events_buffer, count_read * sizeof(hydradancer_event_t));
 		}
 	}
+	BSP_EXIT_CRITICAL();
 }

hydradancer/firmware_hydradancer/User/definitions.h

@@ -6,7 +6,7 @@
 /* Global define */
 // DEF_ENDP_OUT_BURST_LEVEL / DEF_ENDP_IN_BURST_LEVEL maximum burst size 16 defined by the USB3 specification
 // Warning USB3 endpoint bulk with 8 or 16 burst can be problematic on some PC
-#define DEF_ENDP_OUT_BURST_LEVEL 4
+#define DEF_ENDP_OUT_BURST_LEVEL 1
 #define DEF_ENDP_IN_BURST_LEVEL (DEF_ENDP_OUT_BURST_LEVEL)
 #define DEF_ENDP_MAX_SIZE (DEF_ENDP1_OUT_BURST_LEVEL * ENDP_1_15_MAX_PACKET_SIZE)
 
@@ -89,67 +89,71 @@ void hydradancer_send_event(void);
 
 __attribute__((always_inline)) inline static void write_event(uint8_t type, uint8_t value)
 {
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	hydradancer_event_t event = {
 		.type = type,
 		.value = value,
 	};
 	fifo_write(&event_queue, &event, 1);
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
+}
+
+__attribute__((always_inline)) inline static bool hydradancer_get_event_transfer_finished()
+{
+	bool ret = false;
+	BSP_ENTER_CRITICAL();
+	ret = event_transfer_finished;
+	BSP_EXIT_CRITICAL();
+	return ret;
+}
+
+__attribute__((always_inline)) inline static void hydradancer_set_event_transfer_finished(bool _event_transfer_finished)
+{
+	BSP_ENTER_CRITICAL();
+	event_transfer_finished = _event_transfer_finished;
+	BSP_EXIT_CRITICAL();
 }
 
 __attribute__((always_inline)) inline static void hydradancer_status_set_out(uint8_t endp_num)
 {
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	hydradancer_status.ep_out_status |= (1 << endp_num);
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 }
 
 __attribute__((always_inline)) inline static void hydradancer_status_set_in(uint8_t endp_num)
 {
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	hydradancer_status.ep_in_status |= (1 << endp_num);
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 }
 
 __attribute__((always_inline)) inline static void hydradancer_status_set_nak(uint8_t endp_num)
 {
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	hydradancer_status.ep_in_nak |= (1 << endp_num);
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 }
 
 __attribute__((always_inline)) inline static void hydradancer_status_clear_out(uint8_t endp_num)
 {
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	hydradancer_status.ep_out_status &= ~(1 << endp_num);
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 }
 
 __attribute__((always_inline)) inline static void hydradancer_status_clear_in(uint8_t endp_num)
 {
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	hydradancer_status.ep_in_status &= ~(1 << endp_num);
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 }
 
 __attribute__((always_inline)) inline static void hydradancer_status_clear_nak(uint8_t endp_num)
 {
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	hydradancer_status.ep_in_nak &= ~(1 << endp_num);
-	bsp_enable_interrupt();
-}
-
-__attribute__((always_inline)) inline static void hydradancer_recover_out_interrupt(uint8_t endp_num)
-{
-	if (hydradancer_status.ep_out_status & (0x01 << endpoint_mapping_reverse[endp_num]))
-	{
-		ramx_pool_free(usb_device_1.endpoints.tx[endpoint_mapping[endp_num]].buffer);
-		hydradancer_status_clear_out(endp_num);
-		bsp_disable_interrupt();
-		endp_rx_set_state(&usb_device_0, endp_num, ENDP_STATE_ACK);
-		bsp_enable_interrupt();
-	}
+	BSP_EXIT_CRITICAL();
 }
 
 #endif

hydradancer/firmware_hydradancer/User/usb_control_device.c

@@ -78,62 +78,62 @@ void usb_control_init_endpoints(void)
 	usb_device_1.endpoints.tx[1].buffer = NULL;
 	usb_device_1.endpoints.tx[1].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.tx[1].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.tx[1].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.tx[1].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.rx[2].buffer = ramx_pool_alloc_bytes(ENDP_1_15_MAX_PACKET_SIZE);
 	usb_device_1.endpoints.rx[2].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.rx[2].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.rx[2].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.rx[2].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.tx[2].buffer = NULL;
 	usb_device_1.endpoints.tx[2].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.tx[2].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.tx[2].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.tx[2].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.rx[3].buffer = ramx_pool_alloc_bytes(ENDP_1_15_MAX_PACKET_SIZE);
 	usb_device_1.endpoints.rx[3].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.rx[3].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.rx[3].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.rx[3].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.tx[3].buffer = NULL;
 	usb_device_1.endpoints.tx[3].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.tx[3].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.tx[3].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.tx[3].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.rx[4].buffer = ramx_pool_alloc_bytes(ENDP_1_15_MAX_PACKET_SIZE);
 	usb_device_1.endpoints.rx[4].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.rx[4].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.rx[4].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.rx[4].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.tx[4].buffer = NULL;
 	usb_device_1.endpoints.tx[4].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.tx[4].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.tx[4].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.tx[4].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.rx[5].buffer = ramx_pool_alloc_bytes(ENDP_1_15_MAX_PACKET_SIZE);
 	usb_device_1.endpoints.rx[5].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.rx[5].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.rx[5].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.rx[5].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.tx[5].buffer = NULL;
 	usb_device_1.endpoints.tx[5].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.tx[5].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.tx[5].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.tx[5].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.rx[6].buffer = ramx_pool_alloc_bytes(ENDP_1_15_MAX_PACKET_SIZE);
 	usb_device_1.endpoints.rx[6].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.rx[6].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.rx[6].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.rx[6].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.tx[6].buffer = NULL;
 	usb_device_1.endpoints.tx[6].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.tx[6].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.tx[6].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.tx[6].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.rx[7].buffer = ramx_pool_alloc_bytes(ENDP_1_15_MAX_PACKET_SIZE);
 	usb_device_1.endpoints.rx[7].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
 	usb_device_1.endpoints.rx[7].max_burst = DEF_ENDP_OUT_BURST_LEVEL;
-	usb_device_1.endpoints.rx[7].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE;
+	usb_device_1.endpoints.rx[7].max_packet_size_with_burst = ENDP_1_15_MAX_PACKET_SIZE * DEF_ENDP_OUT_BURST_LEVEL;
 
 	usb_device_1.endpoints.tx[7].buffer = NULL;
 	usb_device_1.endpoints.tx[7].max_packet_size = ENDP_1_15_MAX_PACKET_SIZE;
@@ -145,10 +145,10 @@ bool usb_control_reinit(void)
 {
 	// // uncomment to empty log_buffer on emulated device disconnection
 	// LOG_DUMP();
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	usb_control_init_endpoints(); // update USB3 backend as well, without rebooting USB3 peripheral
 	usb30_init_endpoints();
 	boards_ready = 1;
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 	return true;
 }

hydradancer/firmware_hydradancer/User/usb_control_handlers.h

@@ -26,7 +26,7 @@
 
 __attribute__((always_inline)) static inline void usb_control_endp1_tx_complete(TRANSACTION_STATUS status)
 {
-	event_transfer_finished = true;
+	hydradancer_set_event_transfer_finished(true);
 }
 
 __attribute__((always_inline)) static inline void usb_control_endp_tx_complete(TRANSACTION_STATUS status, uint8_t endp_num)
@@ -89,9 +89,9 @@ static bool _usb_control_endp_rx_callback(uint8_t* data)
 	ep_queue_member_t* ep_queue_member = (ep_queue_member_t*)data;
 	while (true)
 	{
-		bsp_disable_interrupt();
+		BSP_ENTER_CRITICAL();
 		volatile uint16_t status = hydradancer_status.ep_in_status;
-		bsp_enable_interrupt();
+		BSP_EXIT_CRITICAL();
 		if (status & (0x01 << endpoint_mapping_reverse[ep_queue_member->ep_num]))
 			break;
 	}
@@ -168,15 +168,15 @@ bool _do_disable_usb(uint8_t* data)
 {
 	LOG_IF(LOG_LEVEL_DEBUG, LOG_ID_USER, "DISABLE_USB\r\n");
 	usb2_device_deinit();
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	boards_ready = 0;
-	event_transfer_finished = true;
+	hydradancer_set_event_transfer_finished(true);
 	start_polling = false;
 	hydra_interrupt_queue_free_all();
 	hydra_interrupt_queue_init();
 	hydra_pool_clean(&ep_queue);
 	ramx_pool_init();
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 	usb_emulation_reinit();
 	usb_control_reinit();
 	return true;

hydradancer/firmware_hydradancer/User/usb_emulation_device.c

@@ -6,7 +6,7 @@ void usb_emulation_reinit(void)
 	// // uncomment to empty log_buffer on emulated device disconnection
 	// LOG_DUMP();
 
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	// Setup ramx pool and interrupt_queue
 	usb_device_set_usb3_device_descriptor(&usb_device_0, NULL);
 	usb_device_set_usb2_device_descriptor(&usb_device_0, NULL);
@@ -24,7 +24,7 @@ void usb_emulation_reinit(void)
 	{
 		endp_rx_set_state(&usb_device_0, i, ENDP_STATE_ACK);
 	}
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 }
 
 void usb_emulation_init_endpoints(void)

hydradancer/firmware_hydradancer/User/usb_emulation_device.h

@@ -19,7 +19,7 @@
 #include "wch-ch56x-lib/USBDevice/usb_device.h"
 #include "wch-ch56x-lib/USBDevice/usb_endpoints.h"
 
-#define USB2_ENDP_1_15_MAX_PACKET_SIZE 512
+#define USB2_ENDP_1_15_MAX_PACKET_SIZE 1024
 
 void usb_emulation_reinit(void);
 

hydradancer/firmware_hydradancer/User/usb_emulation_handlers.h

@@ -139,9 +139,9 @@ bool _usb_emulation_endp0_passthrough_setup_callback(uint8_t* data)
 
 	while (true)
 	{
-		bsp_disable_interrupt();
+		BSP_ENTER_CRITICAL();
 		volatile uint16_t status = hydradancer_status.ep_out_status;
-		bsp_enable_interrupt();
+		BSP_EXIT_CRITICAL();
 		if (!(status & (0x01 << 0)))
 			break;
 		if (start_polling)
@@ -154,9 +154,9 @@ bool _usb_emulation_endp0_passthrough_setup_callback(uint8_t* data)
 
 	while (true)
 	{
-		bsp_disable_interrupt();
+		BSP_ENTER_CRITICAL();
 		volatile uint16_t status = hydradancer_status.ep_out_status;
-		bsp_enable_interrupt();
+		BSP_EXIT_CRITICAL();
 		if (!(status & (0x01 << 0)))
 			break;
 		if (start_polling)
@@ -190,9 +190,9 @@ static bool _usb_emulation_endp_rx_callback(uint8_t* data)
 
 	while (true)
 	{
-		bsp_disable_interrupt();
+		BSP_ENTER_CRITICAL();
 		volatile uint16_t status = hydradancer_status.ep_out_status;
-		bsp_enable_interrupt();
+		BSP_EXIT_CRITICAL();
 		if (!(status & (0x01 << endp_num)))
 		{
 			break;
@@ -341,7 +341,7 @@ void usb_emulation_nak_callback(uint8_t endp_num)
 void usb_emulation_usb2_device_handle_bus_reset(void);
 void usb_emulation_usb2_device_handle_bus_reset(void)
 {
-	bsp_disable_interrupt();
+	BSP_ENTER_CRITICAL();
 	hydradancer_status.ep_in_status = (0x1 << 0) & 0xff; // keep ep0
 	hydradancer_status.ep_out_status = 0;
 	hydradancer_status.ep_in_nak = 0;
@@ -356,7 +356,7 @@ void usb_emulation_usb2_device_handle_bus_reset(void)
 	}
 
 	boards_ready = 1;
-	bsp_enable_interrupt();
+	BSP_EXIT_CRITICAL();
 
 	write_event(EVENT_BUS_RESET, 0);
 }