STM32F4 - Czy kod do USB od ST naprawdę alokuje pamięć dynamicznie w przerwaniu?

Dziś w jednym z projektów postanowiłem włączyć profilaktycznie funkcję którą dodałem ostatnio do mojego RTOSa - sprawdzanie czy niektóre funkcje nie są używane w przerwaniu (m.in. mutexy, których użycie w przerwaniach jest błędem logicznym). No i co się okazuje? Że albo ja coś źle zrobiłem, ale programiści firmy ST w swojej bezkresnej mądrości stwierdzili, że przerwanie to jest świetne miejsce do alokowania pamięci przez malloc()...

Wniosek taki nasunął mi się po pierwszym spojrzeniu na callstack:


Widzimy tam następującą sekwencję:
- przerwanie OTG_FS_IRQHandler(),
- funkcja HAL_PCD_IRQHandler(),
- funkcja HAL_PCD_SetupStageCallback(),
- funkcja USBD_LL_SetupStage(),
- funkcja USBD_StdDevReq(),
- funkcja USBD_SetConfig(),
- funkcja USBD_SetClassConfig(),
- wywołanie funkcji spod wskaźnika Init w strukturze USBD_ClassTypeDef.

Wskaźnik "Init" w strukturze USBD_ClassTypeDef inicjalizowany jest funkcją USBD_CDC_Init(), która praktycznie na samym początku wywołuje malloc() poprzez makro USBD_malloc().

Teraz pytanie zasadnicze - czy ja coś zrobiłem źle czy może jednak ludzie z ST są... hmm... jakby to powiedzieć... "niezbyt mądrzy"?

Czyżby kolejny przykład idealnego rozwiązania jakim jest HAL.

I tak masz rację. W wielu przykładach jest to zaimplementowane:/

Na szczęście pamięć jest alokowana statycznie, a własny malloc zwraca statyczny adres stałego, ciut za dużego bufora.

A Twój post co dokładnie wnosi do dyskusji? Gdzie pamięć jest alokowana statycznie? W Twoich projektach, jak mniemam? Miło że napisałeś wcześniej na forum o tym że trzeba być świadomym takiego problemu! To że sobie można przerobić to na alokację statyczną to każdy wie, tylko już nie każdy wie, że dla kodu od ST sobie to trzeba koniecznie przerobić, chyba że ktoś ma fantazję szukać problemów z działaniem aplikacji przez miesiąc. Zwłaszcza, że dokładnie taki był plan ST względem tego kodu - w końcu makro nazwali USBD_malloc(), a nie USBD_get_storage() czy coś takiego.

BTW - jeśli Twój "własny malloc" jest używany też w innych miejscach, to absolutnie niczego nie rozwiązuje - ja nie mam problemu z alokacją dynamiczną. Żadnego. Problem mam tylko z alokacją dynamiczną z przerwań w wielowątkowej aplikacji, w której owa dynamiczna alokacja zabezpieczona jest mutexem.

Dodano po 3 [godziny] 46 [minuty]:

Napisałem też temat na forum ST - https://community.st.com/message/143009-does-...eally-allocate-dynamic-memory-from-interrupts

Przy okazji znalazłem też inny świetny temat o jakości tego kodu - https://community.st.com/thread/10392

Po głębszym przyjrzeniu się tej "bibliotece" informuję wszystkich zainteresowanych, że problem z tego tematu dotyczy wszystkich klas USB device - CDC, MSC, HID, DFU i audio - w każdej wersji funkcji USBD_..._Init() jest alokowana pamięć przy pomocy makra USBD_malloc().

Freddie Chopin napisał:

w każdej wersji funkcji USBD_..._Init() jest alokowana pamięć przy pomocy makra USBD_malloc().

Owszem. Ale to makro jest definiowane co najmniej na dwa sposoby, np. w konkretnych aplikacjach przykładowych z STM.
Czyli widać jasno że problem znany ludziom z STM i sami panują jakoś nad tym.

I też zauważ że wersja z wywołaniem malloc() jest tak właściwie w pliku szablonu konfiguracji (usbd_conf_template.h). Czyli domyślnie podlega modyfikacji w konkretnym zastosowaniu. Dlatego zarzucanie programistom STM błędu w sztuce jest może nieco śliskie, bo to niby user odpowiada za przygotowanie pliku z szablonu.

A z konkretnymi definicjami widocznie jest tak że albo zostaje jak jest (malloc()) tam gdzie to nie zaszkodzi, albo wymaga zmiany definicji, jak widać np. w aplikacjach przykładowych do USB DFU dla konkretnych płytek, gdzie to makro jest właściwie tylko dostarczeniem adresu zmiennej statycznej (funkcja USBD_static_malloc) a USBD_free jest tylko atrapą.

Jedyne o co mógłbym mieć pretensje do programistów STM, to że nie dali choćby krótkiego komentarza przy definicji USBD_malloc w pliku szablonu, skoro i tak z góry znali sprawę posługując się pośrednim makrem a nie prosto malloc().

rb401 napisał:

Owszem. Ale to makro jest definiowane co najmniej na dwa sposoby, np. w konkretnych aplikacjach przykładowych z STM.
Czyli widać jasno że problem znany ludziom z STM i sami panują jakoś nad tym.

Starasz się ich wybielić, co jest zupełnie niepotrzebnie, bo ich dokonania na przestrzeni lat pokazują, że o programowaniu zbyt wiele nie wiedzą. Przykładowo piszesz, że w aplikacjach to makro jest zdefiniowane na inną wartość, która nie alokuje dynamicznie. Sprawdźmy:



Jak więc widać na 49 przypadków statyczna alokacja została zastosowana DWA RAZY (słownie: 2x). Faktycznie - widać że problem jest im znany. Przy okazji jak robi się makro które miałoby zajmować się alokacją pamięci, potencjalnie w przerwaniu, to naprawdę używanie słowa "malloc" w jego nazwie nie jest najlepszą opcja, gdyż sugeruje co to makro powinno robić. Zwłaszcza, że ono jest używane _TYLKO_ do alokacji z przerwań, nie jest używane nigdzie indziej. Gdyby to makro nazywało się np. USBD_allocate_storage(), to już mógłbym się zastanowić czy na pewno malloc() jest tam dobrą opcją.

A zresztą - całą ta dyskusja jest daremna. Dlaczego? Bo czy to byłoby aż tak bardzo skomplikowane, żeby alokacja tego bloku pamięci odbywała się w USBD_Init(), USBD_RegisterClass(), USBD_CDC_RegisterInterface() lub USBD_Start()? Te funkcje są wywoływane normalnie z wątku głównego i mogą zwracać błędy, wtedy problem by nie istniał i już. Funkcje te mają oczywiście dostęp do wszystkich istotnych zmiennych i informacji, więc naprawdę nie ma żadnego powodu, aby to one nie mogły się tym zająć.

rb401 napisał:

I też zauważ że wersja z wywołaniem malloc() jest tak właściwie w pliku szablonu konfiguracji (usbd_conf_template.h). Czyli domyślnie podlega modyfikacji w konkretnym zastosowaniu. Dlatego zarzucanie programistom STM błędu w sztuce jest może nieco śliskie, bo to niby user odpowiada za przygotowanie pliku z szablonu.

Jak widzisz powyżej, wersja z malloc() jest w 47 na 49 przypadków, włącznie z przykładowymi aplikacjami we wszystkich możliwych odmianach.

rb401 napisał:

A z konkretnymi definicjami widocznie jest tak że albo zostaje jak jest (malloc()) tam gdzie to nie zaszkodzi, albo wymaga zmiany definicji,

Nie ma takiego przypadku, w którym używanie malloc() w przerwaniu nie zaszkodzi. To jest pułapka która tylko czeka na to, żeby w nią wpaść.

rb401 napisał:

jak widać np. w aplikacjach przykładowych do USB DFU dla konkretnych płytek, gdzie to makro jest właściwie tylko dostarczeniem adresu zmiennej statycznej (funkcja USBD_static_malloc) a USBD_free jest tylko atrapą.

Chyba że akurat masz inną płytkę niż STM32F446ZE-Nucleo i STM32F429ZI-Nucleo, bo wtedy jest zdefiniowane jako malloc(). Zresztą co kogo obchodzi DFU, jak to zapewne jedna z najrzadziej używanych klas.



Nawet nie pytam o sens tego rzutowania na uint32_t... Po prostu ten kod jest nie-do-obronienia.

Może nie mam kilkudziesięciu czy nawet kilkunastu lat doświadczenia w programowaniu embedded (albo zwykłym), ale trochę go już mam i moje zdanie od lat jest takie samo - kod pochodzący od ST (SPL, HAL, ich biblioteki, ...) nie nadaje się do niczego i nie powinien być używany w poważnych projektach. Powiem wam nawet "w sekrecie", że ST swego czasu było tego nawet świadome i mówiło o tym wprost. Na początku istnienia serii STM32 w plikach nagłówkowych które dostarczało ST było zawsze coś takiego na samej górze:

Cytat:

* THE PRESENT FIRMWARE WHICH IS FOR GUIDANCE ONLY AIMS AT PROVIDING CUSTOMERS
* WITH CODING INFORMATION REGARDING THEIR PRODUCTS IN ORDER FOR THEM TO SAVE TIME.

Potem ten disclaimer zniknął, ale kod się cudownie od tego nie poprawił - jak był badziewny, tak badziewny pozostał.

Freddie Chopin napisał:

Powiem wam nawet "w sekrecie", że ST swego czasu było tego nawet świadome i mówiło o tym wprost.

Kolego Freddie. Swego czasu zanim pojawił się CUBE ST twardo głosiło używajcie w projektach, zrobiliśmy udostępniamy i po to to jest. Teraz co? HAL to tylko materiały szkoleniowe, dla szybkiego startu... a potem to już wasz problem.

Wystarczy przeczytać dokładnie to co napisałem - "ST swego czasu było tego nawet świadome i mówiło o tym wprost". Było to jeszcze za czasów SPL i to jakoś przed 2010 rokiem lub gdzieś w tej okolicy.

Freddie Chopin napisał:

Jak widzisz powyżej, wersja z malloc() jest w 47 na 49 przypadków, włącznie z przykładowymi aplikacjami we wszystkich możliwych odmianach.

Statystycznie masz rację. Też to wcześniej sprawdziłem.
Ale nie o to chodzi, bo tematowi wariantowości USBD_malloc (statyczna vs. dynamiczna) od dawna w UM1734 poświęcona jest cała strona. Co prawda w innym kontekście, ale i tak że zwracam trochę honoru ludziom z STM.

Druga i istotniejsza kwestia. Jakoś nie zauważyłem by makro USBD_malloc było użyte w jakimś przerwaniu. Widzę że w każdej z klas występuje tylko raz w USBD_xxxx_Init, dokładnie tak jak tu postulujesz.

Tak że teraz już nie wiem o co Ci chodzi.

rb401 napisał:

Tak że teraz już nie wiem o co Ci chodzi.

Prześledź wywołania od przerwania to się dowiesz.

rb401 napisał:

od dawna w UM1734 poświęcona jest cała strona.

A ilość stron o czymś świadczy? np o treści merytorycznej?

Pomijając ze tytuł obrazka 21 jest błędny?

rb401 napisał:

Druga i istotniejsza kwestia. Jakoś nie zauważyłem by makro USBD_malloc było użyte w jakimś przerwaniu. Widzę że w każdej z klas występuje tylko raz w USBD_xxxx_Init, dokładnie tak jak tu postulujesz.

Tak że teraz już nie wiem o co Ci chodzi.

Przecież opisałem to w pierwszym poście - ze screenshotem i z pełnym opisem łańcucha wywołań. USBD_malloc() jest wywoływany w przerwaniu. Zawsze. Przy każdej klasie i przy każdej możliwej konfiguracji.

Freddie Chopin napisał:

Przecież opisałem to w pierwszym poście - ze screenshotem i z pełnym opisem łańcucha wywołań. USBD_malloc() jest wywoływany w przerwaniu. Zawsze. Przy każdej klasie i przy każdej możliwej konfiguracji.

Po prostu chciałem na własne oczy zobaczyć to w źródłach i ewentualnie dociec "co autor (czyli STM) miał na myśli". No i faktycznie jest dokładnie tak jak piszesz.
Ale w temacie, co STM miał na myśli robiąc to w ten sposób, nie mam pomysłu. Przez chwilę myślałem że to rezultat rozbudowy koncepcji z serii F1 ale tam jest praktycznie tak samo i już tez źle.

Tym bardziej wydaje się dziwne kategoryczne stwierdzenie w tym UM1734:

Cytat:

Is the USB device library compatible with Real Time operating system (RTOS)?
Yes, The USB device library could be used with RTOS, the CMSIS RTOS wrapper is
used to make abstraction with OS kernel.

rb401 napisał:

Ale w temacie, co STM miał na myśli robiąc to w ten sposób, nie mam pomysłu. Przez chwilę myślałem że to rezultat rozbudowy koncepcji z serii F1 ale tam jest praktycznie tak samo i już tez źle.

Tym bardziej wydaje się dziwne kategoryczne stwierdzenie w tym UM1734:

Pokładasz w nich zbyt dużą nadzieję (; Kod który oni produkują jest wyjątkowo słabej jakości. Nie mieli nic na myśli, po prostu kwestię jakości czy poprawności kodu mają gdzieś. Zobacz na ten drugi temat który zlinkowałem ( https://community.st.com/thread/10392 ) - po prostu szkolny błąd, którego nie da się popełnić mając choć minimalne pojęcie o tym jak działają programy wielowątkowe. A jednak go popełnili. Dostali informację o problemie 1.5 roku temu i zgadnij czy coś z tym zrobili? O tyle:



Szkoda tylko, że jeszcze powinno być sprawdzane, czy user używa przerwań i w takim wypadku powinien mu się wyświetlać error "Interrupts must not be used in current HAL release"... O volatile przy tym polu Lock to już nawet nie wspominam, bo co to za różnica czy jest bardzo źle czy jeszcze bardziej źle.