32F417, ST HAL USB FS: USBD_CDC_ReceivePacket() random data loss

[peter-h]

I originally posted this here but usually nobody reads posts there
https://community.st.com/s/question/0D53W00001m74r2SAA/32f417-cdcreceivefs-or-usbdcdcreceivepacket-any-known-bugs?t=1661949264599

It's a strange one and I wonder if anyone has ideas.

I don't think it is a straight interrupt latency issue. I tried highest priority for USB. Also the apparently offending code - a DMA transfer with a yield to RTOS during it - doesn't disable interrupts. But USB does use interrupts. I am not sure if USB FS can fill the 64 byte FIFO without them; if it can then this almost certainly can't be the issue.

The fix seems good but is unsavoury because there is no reason for it. Removing the yield in the while loop fixes the data loss

Code: [Select]

// Config SPI3 to let DMA handle the data. These need to be cleared when transfer complete!
// This starts the transfer

SPI3->CR2 |= 3; // TXDMAEN, RXDMAEN: 11 - both set in one go
SPI3->CR1 |= (1<<6); // SPE=1 enable SPI

// Wait for DMA to finish. Blocking is necessary to prevent device CS=1 too early.
// There could be a timeout here but a failure is impossible short of duff silicon, because
// we are a Master and generating the SPI clock.

while(true)
{
uint32_t temp2;
temp2 = DMA1->LISR;
if ( (temp2 & (1<<5)) !=0 ) break; // TCIF0
taskYIELD();         // release to RTOS
}

SPI3->CR2 &= ~3; // TXDMAEN, RXDMAEN: 00 - both cleared in one go

// Clear int pending flags. They get cleared at the top of this function anyway, but...

DMA1->LIFCR = (0x03d << 0); // clear int flags & transfer complete - 111101 stream 0
DMA1->HIFCR = (0x03d << 22); // clear int flags & transfer complete - 111101 stream 7

    // Clear any rx data and the overrun flag in case not all received data was read

SPI3->CR1 &= ~(1<<6); // SPE=0 disable SPI


[newbrain]

Is that code inside an ISR?

AFAIK, It's not possible to yield willy nilly in that case, there are a number of limitations.

But I don't like it even if it's outside an ISR.
When an RTOS is used, it should be used all the way, here the code is instead doing a busy loop coitus interruptus style.

A better way, IMO, would be to just set up an ISR for the end of DMA transfer, and signal the waiting task with a direct to task notification, in FreeRTOS they are much cheaper (both memory and cycles) than a queue or semaphore. Minor register clean up could still be in the DMA ISR.

Pseudocoding:

Code: [Select]

...
setup transfer and DMA
start transfer
xTaskNotifyWait() // forever or with timeout, your choice.
do the rest
...


DMA_transfer_complete_ISR()
  cleanup
  vTaskNotifyGiveFromISR()
  yielding according to FreeRTOS rules


[peter-h]

I don't really think the USB data corruption is related to the DMA transfer; it is some sort of timing issue, because removing that yield statement merely reduces the corruption from 5-10% to about 1%.

But, yes, the reason for waiting for the DMA to finish is because it is feeding SPI Master and one needs to lift /CS at the end of the transfer. The actual end of data is detected by receiving the last byte (SPI TX always receives data at the same time, even if not same data).

It is AFAIK normal to yield to the RTOS while waiting in some loop. It enables lower priority tasks to run. But it may be a while before you get back control, so there is a tradeoff. Sometimes you do not want to get control back soon, so one can use osDelay(10) for example.

No, the code is an RTOS task. There are ISRs related to USB but I don't know how that side of it works. I know how MSC works, fairly well, and that is robust. It is CDC which is getting corrupted.

I noticed that the first byte after startup (direction PC -> target) is always corrupted. That should make it easier to track down.

EDIT: unfortunately this is not possible for me to track down. This is the chain of functions and ISRs involved just in passing that first (corrupted) byte, and at each undocumented stage is a load of structs of structs, handles to this or that, etc. That code is probably just broken. It works fine target -> PC, but 10% corruption PC -> target, reducing to 1% if I change that RTOS yield code (which I am sure is not related, except timing-wise).



You either know this stuff or you don't, and the code is as complicated as code gets.

[newbrain]

It is AFAIK normal to yield to the RTOS while waiting in some loop. It enables lower priority tasks to run. But it may be a while before you get back control, so there is a tradeoff. Sometimes you do not want to get control back soon, so one can use osDelay(10) for example.

Maybe - I never did that.
If that's a high priority task, there'll be a lot of useless call into the RTOS kernel and searches for the next task. If it's a low priority task it will just sit there.
With the way I suggested, the task goes to sleep (one context change and search for the task to run) and gets woken up only when it's needed again (and prio allows, ça va sans dire).

And beware: yielding will not allow lower priority tasks to run.

The scheduler will select this task again and again over any other at a lower priority, as the task is ready to run: it is not waiting on anything the RTOS knows about (queue, mutex, semaphore, notification, etc.). It will only allow higher priority tasks to run but, if you are using preemption (yes IIRC?), this already happens - well unless you are are doing this dance also for them - but in that case, this task will never run...

As I, maybe a bit colourfully, said, this is only a glorified busy loop, and blocks all lower prio tasks: it's a CPU black hole that will consume all CPU time left, bypassing the usefulness of an RTOS.

Only a RTOS delay (vTaskDelay()/vTaskDelayUntil()) will permit lower priority tasks to run, at least until the delay expires.
But why? You can have task woken up at the right time, without need to wait for a delay to expire, using the facilities the RTOS makes available.

taskYIELD
task. h

taskYIELD() is used to request a context switch to another task. However, if there are no other tasks at a higher or equal priority to the task that calls taskYIELD() then the RTOS scheduler will simply select the task that called taskYIELD() to run again.

If configUSE_PREEMPTION is set to 1 then the RTOS scheduler will always be running the highest priority task that is able to run, so calling taskYIELD() will never result in a switch to a higher priority task.

You either know this stuff or you don't, and the code is as complicated as code gets.

Yes STM32 USB stack is really complicated. I tend to use tinyusb - might not be 100% complete and perfect, but it is way easier to make it work, with or without an RTOS.
I just set it up for a CDC on an iMX RT1062, FreeRTOS and works like a charm.

[peter-h]

if you are using preemption

Yes I am. An RTOS without that would not do much useful stuff. However I write all code to yield when waiting.

yielding will not allow lower priority tasks to run.

I didn't know that, but

taskYIELD() is used to request a context switch to another task. However, if there are no other tasks at a higher or equal priority to the task that calls taskYIELD() then the RTOS scheduler will simply select the task that called taskYIELD() to run again.

is better than just hanging there, isn't it, because just hanging there will completely waste CPU time until the next 1ms tick.

Only a RTOS delay (vTaskDelay()/vTaskDelayUntil()) will permit lower priority tasks to run, at least until the delay expires.

OK; that's good news. I was using osDelay(x) before I moved to taskyield, believing the latter to be better in some cases because if the RTOS has no other tasks to run, it will come back to me before x. Well, I was right, except that yielding doesn't allow lower priority tasks to run, whereas osDelay does. That's a good lesson for me

There is more detail: almost all my RTOS tasks run at IDLE priority anyway, so the above is moot.

Also osDelay(1), which I used generally, isn't really 1ms. It is anything from near-zero (depending on how soon is the next tick) to 1ms. If you want 1ms then you need to settle for 1-2ms, with osDelay(2). Or use other non-RTOS delay means of course... there are several options.

Yes STM32 USB stack is really complicated. I tend to use tinyusb - might not be 100% complete and perfect, but it is way easier to make it work, with or without an RTOS.
I just set it up for a CDC on an iMX RT1062, FreeRTOS and works like a charm.

This project was set up by someone else a few years ago. That USB code seems to have come entirely from the Cube MX code generator. And it has been working OK. The MSC (removable drive, done with FatFS) portion is solid, and the CDC output (data to PC) is solid (if rather low performance, but ok for debugs). I think the data rate from PC is poor, of the order of tens of bytes/sec if sending single bytes.

I have decided to not waste more days on the CDC from PC code. The code is so convoluted I don't even understand some of the C. And I certainly don't understand the program flow. It works well enough for another (undocumented) debug function where I can discard incoming non-ascii bytes (the corrupted bytes are mostly non-ascii). It is 90% reliable, increasing to 99% with that RTOS yield line removed (which is a worryingly bad timing dependency, I think, but the rest of the product is rock solid).

It would not surprise me if nobody has tested this CDC feature before.

[newbrain]

I'm still convinced you are holding it wrong.
What good is preemption when all tasks are at the same priority?
What use is doing a pointless context switch to a task that is blocked (unbeknownst to the RTOS) and then immediately switching away?
Why not let the RTOS handle that for you and activate tasks only when they have real work to do?

Honestly, this looks extremely fragile to me: a change in priority will easily break it, timing can be marginal as a task will be awoken only at time slices and not as soon as possible.
But eh, to each their own.

[peter-h]

What good is preemption when all tasks are at the same priority?

If everything was same priority, tasks will switch at 1ms rate, only - AIUI.

For many applications that is ok e.g. processing comms on multiple serial ports, with a 1k interrupt-driven buffer on each. I did my own RTOS for one such application, switching at 10ms rate. It worked great.

Yielding to RTOS enables an earlier switch to the next task.

But eh, to each their own.

I am always happy to learn, but in this case I don't see the problem. This system has about 20 RTOS tasks, most base priority and some higher.

[eutectique]

Side note: I've just stumbled across the following line in the FreeRTOS file heap_useNewlib_ST.c :

Code: [Select]

#define MALLOCS_INSIDE_ISRs // #define iff you have this crap code (ie unrepaired STM USB CDC)


[peter-h]

I searched for that string (with and without the typo) and cannot find it in my project.

Mallocs are a huge liability and I have spent a chunk of my life recently weeding them out. ST were dumb enough to use the heap for some USB buffers, which seems ok superficially, but as windoze USB goes off to sleep periodically, the heap grew and grew / got fragmented, and my board crashed, about once a day. Those buffers are now static. And I posted enough here about heap usage in printf family code. Definitely no heap usage in ISRs now.

LWIP uses a private heap for whatever stuff. FreeRTOS also uses a private heap. AFAICT those two are safe because once allocated, they keep it. The big one is MbedTLS which uses a big private heap, but dumps it when the connection is closed, and that one cannot be avoided.

A google on "unrepaired STM USB CDC" digs up a lot of reading material - thanks It leads to this well known writer
https://github.com/gmgunderground/STM32-RTOS-USB-HowToFix/blob/master/heap_useNewlib.c

but mine is just this

Code: [Select]

static uint8_t  USBD_MSC_CDC_Init (USBD_HandleTypeDef *pdev,
   uint8_t cfgidx)
{
/* MSC initialization */
uint8_t ret = USBD_MSC_Init (pdev, cfgidx);
if(ret != 0)
return ret;

/* CDC initialization */
ret = USBD_CDC_Init (pdev, cfgidx);
if(ret != 0)
return ret;

return USBD_OK;
}
and I checked that one is actually executed. No mallocs anywhere else either.

If anyone can find any detail of this dodgy CDC code, I am all ears.

[voltsandjolts]

I seem to recall that in another thread you mentioned disabling USB interrupts for some tens of ms in an STM32 project you were working on. I can't remember why and it may not be related at all to this project. But thought I'd mention it incase it jogs your memory about some code you are developing or copy-pasted into this project.

[peter-h]

Yes. The USB MSC process (a removable disk drive, constructed as a 2MB FatFS volume in an Adesto SPI FLASH) does the disk r/w inside an ISR.

Reading the disk is pretty easy, because windoze reads 1 sector at a time (512 bytes) which with a 21MHz SPI clock is ~200us. May be a long time for an ISR but normally the USB port has nothing plugged in so it doesn't get invoked. If you do plug in a PC, windoze will poll the device at about 1Hz just for fun. So if I was getting corruption at 1Hz I would have a dig around there. Also I know, experimentally, that a 200us ISR does not break USB. Even a 16ms ISR (see below) does not break USB!

Writing the disk is slow no matter how you shake it because of the ~16ms sector write time. This time has to somehow propagate back to the host (unless your target has 2MB of RAM in which case you can just cache the biggest possible file ) and we found no way to do that which actually worked. People suggested writing a "driver" for the filesystem but that just pushes the issue further back. From memory, ~1 year ago and extensively posted here, it is a "SCSI" device profile and it can return "busy; try again later" but we could not get that to work, and never found anybody who knew anything about it. All the code is from Cube MX but there is no input on the ST forum. So I just do the 16ms sector write inside the USB ISR. It's inelegant, but it 100% works, and it works with various windoze versions, linux, etc. It is not the cause of the CDC data corruption because I am definitely not writing to the FLASH (and would almost never be writing in the normal use of the product anyway).

I now find the corruption is down to 1% and, having undocumented that feature, I can use it for what I want it for. Maybe one day some CDC fix will turn up.

The taskyield versus osdelay is interesting. With FreeRTOS, if you have pre-emption enabled (very desirable) the RTOS will only ever run the top priority task and no others - unless that task somehow yields. But taskyield will switch only to same or higher priority tasks. So you should not use taskyield (to "spread the CPU around" more evenly, which after all is the objective) unless in an IDLE priority thread. And that is what I am doing. All the "application" code is running in IDLE. If the task runs at a higher than IDLE priority, then it should yield with osDelay(1) (implemented as vTaskDelay(1)) instead. And I have no idea why removing the taskyield from the "wait while DMA busy" improves CDC corruption from 10% to 1%. Replacing the taskyield with osdelay(1) in that DMA code produces a ~2% error rate, so worse than no "yield" but much better than taskyield(). Bizzare and I am sure not relevant.

[Silenos]

... corruption from 10% to 1% ...  ~2% error rate ...

Can't you capture the usb stream when you get the error and then reinject it, with possibility to dump trace of any mcu operations/memory at around the error occurance? It really feels like you are  spinning in darkness, trying to modify random things and hope for success. It also applies to your eth stuff...
Eg. I'm currently writing an extension of server which runs certain protocols over tcp and udp. It is also a pile of differrent third party libs, concurrency mess, and so on. But I'm using testing scenarios which mimick real eth traffic to run my software, and I'm writing/scripting the scenarios along the actual software. And the colleague prepared me scripts to generate thousands of those scenarios which touches different issues: unexpected data in packets, malformed packets, malicious packets, timings of all kinds, combinations of those, attacks attempts, stressess, etc.
Yes, it takes time, but at least my softawre doesn't have "x% of failure rate", it actually works (or not). And it is before I get access to some serious and very expensive certified traffic generator, and that will happen before the server even gets a chance to interface target network... I didn't notice any teseting approach on your side.

[peter-h]

Can't you capture the usb stream when you get the error and then reinject it, with possibility to dump trace of any mcu operations/memory at around the error occurance? It really feels like you are  spinning in darkness, trying to modify random things and hope for success.

It is hugely complex. Take a look at some explanations in the ST thread in post #1. One needs deep understanding to even know where the data can be found.

It also applies to your eth stuff...

Does it? That is rock solid.

As is USB MSC. I have 1 mbyte/sec transfer speed now, filesystem -> browser via ETH. Similar speed filesystem -> PC via USB MSC.

I just sometimes approach things in a different way.

but at least my softawre doesn't have "x% of failure rate"

CDC (PC -> target direction) is not a core feature for me and I am happy to dump it. I was hoping someone here might recognise those functions. They are auto generated by ST Cube MX.

I didn't notice any teseting approach on your side.

That's because I don't post everything

I test everything, exhaustively. And document it as I go. Bits which worked and bits which didn't work.

[peter-h]

I've had a dig around looking whether anybody has published other versions of these ex Cube MX functions and the only ones I can find are 2016 or older. So the next step might be to re-run Cube MX and regenerate that code and see if ST changed anything. I will need to get somebody else to do that because I never used MX.

[peter-h]

Finally fixed.

Nothing to do with the RTOS, although the RTOS contributed to the timing-related issues.

It was junk code in one of the ex-MX functions, modded some years ago but never tested. It did never quite work originally though AFAICT.