pbio/drv/usb: Refactor STM32 & EV3 driver, fix frequent host app lockup for EV3 #408
Conversation
| } else if (qctrld == (uint32_t)(&cppi_descriptors[CPPI_DESC_TX_STDOUT])) { | ||
| usb_tx_stdout_is_not_ready = false; | ||
| } else if (qctrld == (uint32_t)(&cppi_descriptors[CPPI_DESC_TX_PYBRICKS_EVENT])) { | ||
| transmitting = false; |
There was a problem hiding this comment.
These can be combined if the change to the single transmitting flag instead of three separate ones is OK.
| // This barrier prevents *subsequent* memory reads from being | ||
| // speculatively moved *earlier*, outside the if statement | ||
| // (which is technically allowed by the as-if rule). | ||
| pbdrv_compiler_memory_barrier(); |
There was a problem hiding this comment.
Is this barrier still necessary now that this portion of code is pulled out of the process loop and placed in this dedicated function?
| ep1_tx_response_buf[0] = PBIO_PYBRICKS_IN_EP_MSG_RESPONSE; | ||
| pbio_set_uint32_le(&ep1_tx_response_buf[1], result); | ||
| usb_send_response = true; | ||
| break; |
There was a problem hiding this comment.
The original version would immediately send the response here. In the revision, we only schedule it here and transmit it from the process when we get round to it.
| pbdrv_cache_prepare_before_dma(ep1_tx_stdout_buf, sizeof(ep1_tx_stdout_buf)); | ||
| usb_setup_tx_dma_desc(CPPI_DESC_TX_STDOUT, ep1_tx_stdout_buf, 2 + *size); | ||
| ep1_tx_stdout_sz = 2 + *size; | ||
| pbio_os_request_poll(); |
There was a problem hiding this comment.
stdout is scheduled rather than sent here so we can avoid conflicts with the status message.
On windows I can try to connect via USB to e.g. primehub seems not to do the connect to complete.
Want me to make a separate issue of this? Bert |
|
Yes, please move to a separate issue. Thanks! (Unless of course it wasn't happening on the master branch but it is here). |
laurensvalk
force-pushed
the
usb-fixes
branch
from
5b6b4d2 to
02b2206
Compare
|
Merged (GitHub indicator is incorrect). |
The STM32 version is intended to work almost the same as before (looking over the wire), but it is migrated from Contiki to
pbio/os. It drops the complexity of thePBIO_ONESHOTvariables to synchronize with the BCD process. These were making it run chronologically with transmissions, so we can do a bit of refactoring to do that in a conventional process.Both versions drop the keepalive timeout, as per the existing
REVISITin the code. Instead, we re-schedule the initial status update when the host subscribes.The EV3 version does have some changes, intended to fix Pybricks Code getting into a bad state when starting and stopping a script such as the following. You can run it with the labs version to confirm.
In this approach, I've combined the status and stdout descriptors so they are more like the single event descriptor we have in the STM32 USB version, as well as in the Bluetooth driver. I suppose there are other ways to achieve the same results, so I'm open to feedback.
enum { CPPI_DESC_RX, CPPI_DESC_TX_RESPONSE, + CPPI_DESC_TX_PYBRICKS_EVENT, - CPPI_DESC_TX_STATUS, - CPPI_DESC_TX_STDOUT, CPPI_DESC_COUNT = 32, };Also @dlech, does Pybricks Code always await the response before sending another command? It seems that both firmware drivers implicitly assume that this is the case (otherwise we can miss commands). If you can confirm I'll add this as a comment.
Would love to get feedback from @dlech and @ArcaneNibble on either version. Thanks!
This does still leave some edge cases on both the STM32 and EV3 version. They are much rarer, so we will defer these to new issues.