Port FreeRTOS CMSIS interface from V1 to V2. Fix touch issue.#4
Port FreeRTOS CMSIS interface from V1 to V2. Fix touch issue.#4AndrzejDa wants to merge 2 commits into
Conversation
…CTP_RST and CTP_INT pins.
There was a problem hiding this comment.
6 issues found across 27 files
Prompt for AI agents (all 6 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c">
<violation number="1" location="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c:2022">
P1: Comparison `==` is used instead of assignment `=`. This statement does nothing and leaves `mp->sem` uninitialized when neither static nor dynamic allocation is configured, causing undefined behavior.</violation>
</file>
<file name="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.h">
<violation number="1" location="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.h:704">
P3: Documentation error: Comment says 'Memory Pool' but this function operates on a Message Queue. Consider changing to 'Get maximum message size in a Message Queue.'</violation>
</file>
<file name="Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h">
<violation number="1" location="Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h:75">
P3: Duplicate word "using using" in the documentation comment. Should be "using unitary".</violation>
<violation number="2" location="Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h:135">
P3: Typo "Tilmer" should be "Timer" in the documentation comment.</violation>
</file>
<file name="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os.h">
<violation number="1" location="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os.h:747">
P2: Message queue data array over-allocates memory by 4x. Using `uint32_t` as array type with `(queue_sz) * sizeof(type)` as element count results in 4x more memory than needed. Consider using `uint8_t` as the array type to match the intended byte size.</violation>
</file>
<file name="CM7/Core/Src/lvgl_port_touch.c">
<violation number="1" location="CM7/Core/Src/lvgl_port_touch.c:35">
P0: `vTaskDelay()` cannot be called before the FreeRTOS scheduler starts. Looking at `main.c`, `lvgl_touchscreen_init()` is called at line 200, but `osKernelStart()` doesn't happen until line 218. This will cause undefined behavior or a crash. Keep using `HAL_Delay()` for pre-scheduler initialization, or move the touchscreen initialization into a task.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| #elif (configSUPPORT_DYNAMIC_ALLOCATION == 1) | ||
| mp->sem = xSemaphoreCreateCounting (block_count, block_count); | ||
| #else | ||
| mp->sem == NULL; |
There was a problem hiding this comment.
P1: Comparison == is used instead of assignment =. This statement does nothing and leaves mp->sem uninitialized when neither static nor dynamic allocation is configured, causing undefined behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c, line 2022:
<comment>Comparison `==` is used instead of assignment `=`. This statement does nothing and leaves `mp->sem` uninitialized when neither static nor dynamic allocation is configured, causing undefined behavior.</comment>
<file context>
@@ -0,0 +1,2482 @@
+ #elif (configSUPPORT_DYNAMIC_ALLOCATION == 1)
+ mp->sem = xSemaphoreCreateCounting (block_count, block_count);
+ #else
+ mp->sem == NULL;
+ #endif
+
</file context>
| mp->sem == NULL; | |
| mp->sem = NULL; |
| /// \return maximum number of messages. | ||
| uint32_t osMessageQueueGetCapacity (osMessageQueueId_t mq_id); | ||
|
|
||
| /// Get maximum message size in a Memory Pool. |
There was a problem hiding this comment.
P3: Documentation error: Comment says 'Memory Pool' but this function operates on a Message Queue. Consider changing to 'Get maximum message size in a Message Queue.'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.h, line 704:
<comment>Documentation error: Comment says 'Memory Pool' but this function operates on a Message Queue. Consider changing to 'Get maximum message size in a Message Queue.'</comment>
<file context>
@@ -0,0 +1,734 @@
+/// \return maximum number of messages.
+uint32_t osMessageQueueGetCapacity (osMessageQueueId_t mq_id);
+
+/// Get maximum message size in a Memory Pool.
+/// \param[in] mq_id message queue ID obtained by \ref osMessageQueueNew.
+/// \return maximum message size in bytes.
</file context>
| /// Get maximum message size in a Memory Pool. | |
| /// Get maximum message size in a Message Queue. |
| * @{ | ||
| */ | ||
| #define LL_LPTIM_OPERATING_MODE_CONTINUOUS LPTIM_CR_CNTSTRT /*!<LP Timer starts in continuous mode*/ | ||
| #define LL_LPTIM_OPERATING_MODE_ONESHOT LPTIM_CR_SNGSTRT /*!<LP Tilmer starts in single mode*/ |
There was a problem hiding this comment.
P3: Typo "Tilmer" should be "Timer" in the documentation comment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h, line 135:
<comment>Typo "Tilmer" should be "Timer" in the documentation comment.</comment>
<file context>
@@ -0,0 +1,1514 @@
+ * @{
+ */
+#define LL_LPTIM_OPERATING_MODE_CONTINUOUS LPTIM_CR_CNTSTRT /*!<LP Timer starts in continuous mode*/
+#define LL_LPTIM_OPERATING_MODE_ONESHOT LPTIM_CR_SNGSTRT /*!<LP Tilmer starts in single mode*/
+/**
+ * @}
</file context>
| #define LL_LPTIM_OPERATING_MODE_ONESHOT LPTIM_CR_SNGSTRT /*!<LP Tilmer starts in single mode*/ | |
| #define LL_LPTIM_OPERATING_MODE_ONESHOT LPTIM_CR_SNGSTRT /*!<LP Timer starts in single mode*/ |
| uint32_t Prescaler; /*!< Specifies the prescaler division ratio. | ||
| This parameter can be a value of @ref LPTIM_LL_EC_PRESCALER. | ||
|
|
||
| This feature can be modified afterwards using using unitary |
There was a problem hiding this comment.
P3: Duplicate word "using using" in the documentation comment. Should be "using unitary".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h, line 75:
<comment>Duplicate word "using using" in the documentation comment. Should be "using unitary".</comment>
<file context>
@@ -0,0 +1,1514 @@
+ uint32_t Prescaler; /*!< Specifies the prescaler division ratio.
+ This parameter can be a value of @ref LPTIM_LL_EC_PRESCALER.
+
+ This feature can be modified afterwards using using unitary
+ function @ref LL_LPTIM_SetPrescaler().*/
+
</file context>
| #else // define the object | ||
| #define osMessageQDef(name, queue_sz, type) \ | ||
| static StaticQueue_t os_mq_cb_##name; \ | ||
| static uint32_t os_mq_data_##name[(queue_sz) * sizeof(type)]; \ |
There was a problem hiding this comment.
P2: Message queue data array over-allocates memory by 4x. Using uint32_t as array type with (queue_sz) * sizeof(type) as element count results in 4x more memory than needed. Consider using uint8_t as the array type to match the intended byte size.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os.h, line 747:
<comment>Message queue data array over-allocates memory by 4x. Using `uint32_t` as array type with `(queue_sz) * sizeof(type)` as element count results in 4x more memory than needed. Consider using `uint8_t` as the array type to match the intended byte size.</comment>
<file context>
@@ -0,0 +1,846 @@
+#else // define the object
+#define osMessageQDef(name, queue_sz, type) \
+static StaticQueue_t os_mq_cb_##name; \
+static uint32_t os_mq_data_##name[(queue_sz) * sizeof(type)]; \
+const osMessageQDef_t os_messageQ_def_##name = \
+{ (queue_sz), \
</file context>
| static uint32_t os_mq_data_##name[(queue_sz) * sizeof(type)]; \ | |
| static uint8_t os_mq_data_##name[(queue_sz) * sizeof(type)]; \ |
| * here we just need to reset touchscreen controller */ | ||
| HAL_GPIO_WritePin(CTP_RST_GPIO_Port, CTP_RST_Pin, GPIO_PIN_SET); | ||
| HAL_Delay(10); | ||
| vTaskDelay(pdMS_TO_TICKS(10)); |
There was a problem hiding this comment.
P0: vTaskDelay() cannot be called before the FreeRTOS scheduler starts. Looking at main.c, lvgl_touchscreen_init() is called at line 200, but osKernelStart() doesn't happen until line 218. This will cause undefined behavior or a crash. Keep using HAL_Delay() for pre-scheduler initialization, or move the touchscreen initialization into a task.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CM7/Core/Src/lvgl_port_touch.c, line 35:
<comment>`vTaskDelay()` cannot be called before the FreeRTOS scheduler starts. Looking at `main.c`, `lvgl_touchscreen_init()` is called at line 200, but `osKernelStart()` doesn't happen until line 218. This will cause undefined behavior or a crash. Keep using `HAL_Delay()` for pre-scheduler initialization, or move the touchscreen initialization into a task.</comment>
<file context>
@@ -32,11 +32,11 @@ lvgl_touchscreen_init (void)
* here we just need to reset touchscreen controller */
HAL_GPIO_WritePin(CTP_RST_GPIO_Port, CTP_RST_Pin, GPIO_PIN_SET);
- HAL_Delay(10);
+ vTaskDelay(pdMS_TO_TICKS(10));
HAL_GPIO_WritePin(CTP_RST_GPIO_Port, CTP_RST_Pin, GPIO_PIN_RESET);
- HAL_Delay(10);
</file context>
| vTaskDelay(pdMS_TO_TICKS(10)); | |
| HAL_Delay(10); |
There was a problem hiding this comment.
6 issues found across 27 files
Prompt for AI agents (all 6 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c">
<violation number="1" location="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c:2022">
P1: Comparison `==` is used instead of assignment `=`. This statement does nothing and leaves `mp->sem` uninitialized when neither static nor dynamic allocation is configured, causing undefined behavior.</violation>
</file>
<file name="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.h">
<violation number="1" location="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.h:704">
P3: Documentation error: Comment says 'Memory Pool' but this function operates on a Message Queue. Consider changing to 'Get maximum message size in a Message Queue.'</violation>
</file>
<file name="Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h">
<violation number="1" location="Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h:75">
P3: Duplicate word "using using" in the documentation comment. Should be "using unitary".</violation>
<violation number="2" location="Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h:135">
P3: Typo "Tilmer" should be "Timer" in the documentation comment.</violation>
</file>
<file name="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os.h">
<violation number="1" location="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os.h:747">
P2: Message queue data array over-allocates memory by 4x. Using `uint32_t` as array type with `(queue_sz) * sizeof(type)` as element count results in 4x more memory than needed. Consider using `uint8_t` as the array type to match the intended byte size.</violation>
</file>
<file name="CM7/Core/Src/lvgl_port_touch.c">
<violation number="1" location="CM7/Core/Src/lvgl_port_touch.c:35">
P0: `vTaskDelay()` cannot be called before the FreeRTOS scheduler starts. Looking at `main.c`, `lvgl_touchscreen_init()` is called at line 200, but `osKernelStart()` doesn't happen until line 218. This will cause undefined behavior or a crash. Keep using `HAL_Delay()` for pre-scheduler initialization, or move the touchscreen initialization into a task.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| #elif (configSUPPORT_DYNAMIC_ALLOCATION == 1) | ||
| mp->sem = xSemaphoreCreateCounting (block_count, block_count); | ||
| #else | ||
| mp->sem == NULL; |
There was a problem hiding this comment.
P1: Comparison == is used instead of assignment =. This statement does nothing and leaves mp->sem uninitialized when neither static nor dynamic allocation is configured, causing undefined behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c, line 2022:
<comment>Comparison `==` is used instead of assignment `=`. This statement does nothing and leaves `mp->sem` uninitialized when neither static nor dynamic allocation is configured, causing undefined behavior.</comment>
<file context>
@@ -0,0 +1,2482 @@
+ #elif (configSUPPORT_DYNAMIC_ALLOCATION == 1)
+ mp->sem = xSemaphoreCreateCounting (block_count, block_count);
+ #else
+ mp->sem == NULL;
+ #endif
+
</file context>
| mp->sem == NULL; | |
| mp->sem = NULL; |
| /// \return maximum number of messages. | ||
| uint32_t osMessageQueueGetCapacity (osMessageQueueId_t mq_id); | ||
|
|
||
| /// Get maximum message size in a Memory Pool. |
There was a problem hiding this comment.
P3: Documentation error: Comment says 'Memory Pool' but this function operates on a Message Queue. Consider changing to 'Get maximum message size in a Message Queue.'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.h, line 704:
<comment>Documentation error: Comment says 'Memory Pool' but this function operates on a Message Queue. Consider changing to 'Get maximum message size in a Message Queue.'</comment>
<file context>
@@ -0,0 +1,734 @@
+/// \return maximum number of messages.
+uint32_t osMessageQueueGetCapacity (osMessageQueueId_t mq_id);
+
+/// Get maximum message size in a Memory Pool.
+/// \param[in] mq_id message queue ID obtained by \ref osMessageQueueNew.
+/// \return maximum message size in bytes.
</file context>
| /// Get maximum message size in a Memory Pool. | |
| /// Get maximum message size in a Message Queue. |
| * @{ | ||
| */ | ||
| #define LL_LPTIM_OPERATING_MODE_CONTINUOUS LPTIM_CR_CNTSTRT /*!<LP Timer starts in continuous mode*/ | ||
| #define LL_LPTIM_OPERATING_MODE_ONESHOT LPTIM_CR_SNGSTRT /*!<LP Tilmer starts in single mode*/ |
There was a problem hiding this comment.
P3: Typo "Tilmer" should be "Timer" in the documentation comment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h, line 135:
<comment>Typo "Tilmer" should be "Timer" in the documentation comment.</comment>
<file context>
@@ -0,0 +1,1514 @@
+ * @{
+ */
+#define LL_LPTIM_OPERATING_MODE_CONTINUOUS LPTIM_CR_CNTSTRT /*!<LP Timer starts in continuous mode*/
+#define LL_LPTIM_OPERATING_MODE_ONESHOT LPTIM_CR_SNGSTRT /*!<LP Tilmer starts in single mode*/
+/**
+ * @}
</file context>
| #define LL_LPTIM_OPERATING_MODE_ONESHOT LPTIM_CR_SNGSTRT /*!<LP Tilmer starts in single mode*/ | |
| #define LL_LPTIM_OPERATING_MODE_ONESHOT LPTIM_CR_SNGSTRT /*!<LP Timer starts in single mode*/ |
| uint32_t Prescaler; /*!< Specifies the prescaler division ratio. | ||
| This parameter can be a value of @ref LPTIM_LL_EC_PRESCALER. | ||
|
|
||
| This feature can be modified afterwards using using unitary |
There was a problem hiding this comment.
P3: Duplicate word "using using" in the documentation comment. Should be "using unitary".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_ll_lptim.h, line 75:
<comment>Duplicate word "using using" in the documentation comment. Should be "using unitary".</comment>
<file context>
@@ -0,0 +1,1514 @@
+ uint32_t Prescaler; /*!< Specifies the prescaler division ratio.
+ This parameter can be a value of @ref LPTIM_LL_EC_PRESCALER.
+
+ This feature can be modified afterwards using using unitary
+ function @ref LL_LPTIM_SetPrescaler().*/
+
</file context>
| #else // define the object | ||
| #define osMessageQDef(name, queue_sz, type) \ | ||
| static StaticQueue_t os_mq_cb_##name; \ | ||
| static uint32_t os_mq_data_##name[(queue_sz) * sizeof(type)]; \ |
There was a problem hiding this comment.
P2: Message queue data array over-allocates memory by 4x. Using uint32_t as array type with (queue_sz) * sizeof(type) as element count results in 4x more memory than needed. Consider using uint8_t as the array type to match the intended byte size.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os.h, line 747:
<comment>Message queue data array over-allocates memory by 4x. Using `uint32_t` as array type with `(queue_sz) * sizeof(type)` as element count results in 4x more memory than needed. Consider using `uint8_t` as the array type to match the intended byte size.</comment>
<file context>
@@ -0,0 +1,846 @@
+#else // define the object
+#define osMessageQDef(name, queue_sz, type) \
+static StaticQueue_t os_mq_cb_##name; \
+static uint32_t os_mq_data_##name[(queue_sz) * sizeof(type)]; \
+const osMessageQDef_t os_messageQ_def_##name = \
+{ (queue_sz), \
</file context>
| static uint32_t os_mq_data_##name[(queue_sz) * sizeof(type)]; \ | |
| static uint8_t os_mq_data_##name[(queue_sz) * sizeof(type)]; \ |
There was a problem hiding this comment.
2 issues found across 26 files
Prompt for AI agents (all 2 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="CM7/Core/Src/main.c">
<violation number="1" location="CM7/Core/Src/main.c:216">
P0: Incorrect CMSIS-RTOS V2 initialization order. `osKernelInitialize()` must be called BEFORE `MX_FREERTOS_Init()` because `MX_FREERTOS_Init()` creates threads using `osThreadNew()`, which requires the kernel to be initialized first.</violation>
</file>
<file name="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c">
<violation number="1" location="Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c:2022">
P1: Assignment operator typo: `mp->sem == NULL` should be `mp->sem = NULL`. Using `==` (comparison) instead of `=` (assignment) leaves `mp->sem` uninitialized, causing undefined behavior.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
|
||
| /* Call init function for freertos objects (in freertos.c) */ | ||
| MX_FREERTOS_Init(); | ||
| /* Init scheduler */ |
There was a problem hiding this comment.
P0: Incorrect CMSIS-RTOS V2 initialization order. osKernelInitialize() must be called BEFORE MX_FREERTOS_Init() because MX_FREERTOS_Init() creates threads using osThreadNew(), which requires the kernel to be initialized first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CM7/Core/Src/main.c, line 216:
<comment>Incorrect CMSIS-RTOS V2 initialization order. `osKernelInitialize()` must be called BEFORE `MX_FREERTOS_Init()` because `MX_FREERTOS_Init()` creates threads using `osThreadNew()`, which requires the kernel to be initialized first.</comment>
<file context>
@@ -189,29 +189,35 @@ Error_Handler();
- /* Call init function for freertos objects (in freertos.c) */
- MX_FREERTOS_Init();
+ /* Init scheduler */
+ osKernelInitialize();
</file context>
| #elif (configSUPPORT_DYNAMIC_ALLOCATION == 1) | ||
| mp->sem = xSemaphoreCreateCounting (block_count, block_count); | ||
| #else | ||
| mp->sem == NULL; |
There was a problem hiding this comment.
P1: Assignment operator typo: mp->sem == NULL should be mp->sem = NULL. Using == (comparison) instead of = (assignment) leaves mp->sem uninitialized, causing undefined behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Middlewares/Third_Party/FreeRTOS/Source/CMSIS_RTOS_V2/cmsis_os2.c, line 2022:
<comment>Assignment operator typo: `mp->sem == NULL` should be `mp->sem = NULL`. Using `==` (comparison) instead of `=` (assignment) leaves `mp->sem` uninitialized, causing undefined behavior.</comment>
<file context>
@@ -0,0 +1,2482 @@
+ #elif (configSUPPORT_DYNAMIC_ALLOCATION == 1)
+ mp->sem = xSemaphoreCreateCounting (block_count, block_count);
+ #else
+ mp->sem == NULL;
+ #endif
+
</file context>
| mp->sem == NULL; | |
| mp->sem = NULL; |
CTP_RST and CTP_INT pins are changed according to #3 issue.
FreeRTOS CMSIS interface ported from V1 to V2.
Summary by cubic
Migrated FreeRTOS CMSIS interface from V1 to V2 and fixed touchscreen reliability by correcting pin mapping and timing. This improves RTOS compatibility and stabilizes touch input on STM32H7.
Refactors
Bug Fixes
Written for commit 4683d7a. Summary will update automatically on new commits.