Why does the HAL USB CDC driver call malloc in an ISR context ... again?

[Andrei Chichak]

Back in 2018 this question was posed and I have more information.

The answer to this question lays at the bottom of UM1734 where a static allocation code snippet is given.

There are two problems with this code snippet that will come back and bite the unwary developer (waves hand):

1) the snippet allocates 4 x 32-bit items in an array with no explanation as to how they will be used. Perhaps 4 pointers to 4 USB device descriptors? NO, NOT AT ALL.

These 4 x 32-bit values are used to house a 540 byte CDC device descriptor.

The various values in memory after this array are then sacrificed to the descriptor.

2) Their implementation allocates N x 32-bit values, whereas malloc expects N x 8-bit values, so 3/4 of your allocated space (if you managed to get past issue 1) will never get used. In a memory constrained system. Where every byte could be important.

Sigh, now I have to get ahold of my customer and let him know that the 250 units in the field need a firmware update. (insert fornication expletive here).

Andrei

[waclawek.jan]

Well, I don't use CubeMX so maybe it's different in the MX-generated code, but in the Cube examples I've seen so far it was *never* the device descriptor for which memory was allocated using malloc/USBD_malloc. And, it's relatively easy to use a search facility to find out that the only place where it is used is in allocating a class-specific struct in the class drivers.

And, actually, UM1734 says exactly that:

and, at the next page, it clearly says, that what's shown is an example, particularly for HID (where the respective class struct, USBD_HID_HandleTypeDef, as defined in c:\wek\stm32 misc\STM32Cube_FW_F4_V1.21.0\Middlewares\ST\STM32_USB_Device_Library\Class\HID\Inc\usbd_hid.h, consists of four uint32_t(*) - that's why uint32_t was chosen as the "unit" for that particular case):

and of course it's then the responsibility of the user to modify the example for *his* usage case - and that may be a single class different from HID, a compound of classes, or - as mentioned in UM1734 above - the same class in multiple USB device instances (using OTG_FS and OTG_HS).

Yes, the documentation may have been cleaner - there's *always* room for documentation enhancement - but I don't think this is the most urgent case for it.

JW

(*) To split some more hair: there are 3 explicit uint32_t and one enum variable in that USBD_HID_HandleTypeDef struct. While the C standard allows width of enum to be implementation defined, it most probably will be 32 bits compiler targeting 32-bit processor, by default; and even if not, it's highly unlikely it will be *more*.

[Bob S · Accepted solution]

Did you manually copy the code from UM1734 into your code or did CubeMX generate it? I ask because the CubeMX (ver 4.24, yes, old) USB CDC code I generated for my L4xx project has this as out-of-the-box USBD_static_malloc:

void *USDB_static_malloc(uint32_t size)
{
   static uint32_t mem[(sizeof(USBD_CDC_HandleTypeDef)/4)+1]; /* on 32-bit boundary*/
   UNUSED(size);
   return mem;
}

And this is only called once, from USBD_CDC_Init() to initialize the USBD_HandleTypeDef's pClassData pointer.

[Andrei Chichak]

I generated a new project for a F407 using 5.3 and there is no USBD_static_malloc generated, so I inherited this code from something that was taken from UM1734 and that would be from a version many versions ago.

There is no checkbox or any other control in cube to tell it to generate static allocators.

And Jan, how incredibly useless of you to rub the nose into the pile of crap of the user who has already said that they knew that they got stung.

A

[waclawek.jan]

> how incredibly useless

Useless? No. It makes me feel good, that's not useless... 😉

I just wanted to point out that it's not the *device descriptor* (that would also be shorter, btw) which goes into the malloc-ed space.

Also - as you mentioned, RAM is a valuable resource - the CDC class struct does not necessarily need to be 540 bytes - most of it is taken up by a packet buffer, but for example in FS, MAXPACKET is 64 bytes.

JW

[Andrei Chichak]

Okay, you're right, making yourself feel good isn't useless, it's just the way the internet works. 😉 (Oh I was ****** off this morning)

A few suggestions, since I have no idea who wrote any of this code:

1. the name mem *****, it is difficult to grep in a memory map. Call it something useful like USB_Handle_Buffer.
2. To avoid over allocating memory use: static uint32_t mem[(sizeof(USBD_CDC_HandleTypeDef) + 3) / 4];
3. Give a more immediately usable sample in UM1734. Assume TLDR.
4. Update the F4 1.24.1 code to include the suggested version of USBD_static_malloc

A

[Lix Paulian]

This is an issue going for a long time. I really don't understand why ST is not solving it in the easiest way possible, that is, enclose the malloc/free calls in the user_conf.h file inside "user code begin/end" tags. Thus, everyone can supply his own specific malloc/free code and subsequent generations by CubeMX won't overwrite it:

/* USER CODE BEGIN EM */
 
/** Alias for memory allocation. */
#define USBD_malloc         malloc
 
/** Alias for memory release. */
#define USBD_free           free
 
/** Alias for memory set. */
#define USBD_memset         memset
 
/** Alias for memory copy. */
#define USBD_memcpy         memcpy
 
/** Alias for delay. */
#define USBD_Delay          HAL_Delay
 
/* USER CODE END EM */

As a workaround I finally decided to edit the usbdconf_f7_h.ftl file in CubeMX database. This is a kludge, because after each CubeMX update I have to redo it. At least I don't have to redo after each code generation. So ST, what about a change for the next version?

Lix