eclipse-threadx / threadx

Eclipse ThreadX is an advanced real-time operating system (RTOS) designed specifically for deeply embedded applications.
https://github.com/eclipse-threadx/rtos-docs/blob/main/rtos-docs/threadx/index.md
MIT License
2.87k stars 782 forks source link

Cortex-M7 BASEPRI does not work properly #200

Closed MaJerle closed 1 year ago

MaJerle commented 2 years ago

I have noticed, when:

USBX device CDC mode stops working. Port for STM32 never gets properly transfer interrupt. Disabling TX_PORT_USE_BASEPRI leads to proper operation.

I'm still looking for an issue conclusion, but so far I noticed that in the port part, when disabling interrupts, previous BASEPRI value is stored in a variable and returned to user. Restoring them means write previous variable back.

But this is not the case in schedule assembly file that puts BASEPRI to TX_PORT_BASEPRI to lock irqs, and then forces BASEPRI back to 0 to unlock all.

Can this leads to an issue somehow?

CM7 port, considering TX_PORT_USE_BASEPRI is enabled (I removed ifdef code that is not used)

__attribute__( ( always_inline ) ) static inline unsigned int __get_interrupt_posture(void)
{
unsigned int posture;
    __asm__ volatile ("MRS  %0, BASEPRI ": "=r" (posture));
    return(posture);
}

__attribute__( ( always_inline ) ) static inline void __set_basepri_value(unsigned int basepri_value)
{
    __asm__ volatile ("MSR  BASEPRI,%0 ": : "r" (basepri_value));
}

__attribute__( ( always_inline ) ) static inline void __restore_interrupt(unsigned int int_posture)
{
    __set_basepri_value(int_posture);
}

__attribute__( ( always_inline ) ) static inline unsigned int __disable_interrupts(void)
{
unsigned int int_posture;

    int_posture = __get_interrupt_posture();
    __set_basepri_value(TX_PORT_BASEPRI);
    return(int_posture);
}

But in the scheduler, disable IRQ: https://github.com/azure-rtos/threadx/blob/54cda6ee9e68cd9701d759df13e3f6e41429b9bc/ports/cortex_m7/gnu/src/tx_thread_schedule.S#L136-L141

Re-enable IRQ https://github.com/azure-rtos/threadx/blob/54cda6ee9e68cd9701d759df13e3f6e41429b9bc/ports/cortex_m7/gnu/src/tx_thread_schedule.S#L145-L150

Should it store BASEPRI too?

goldscott commented 2 years ago

This looks like a bug only when the Execution Profile Kit (EPK) is enabled. Are you using the EPK? We will fix this in the next release.

EDIT: this doesn't appear to be a bug at all. Setting BASEPRI to 0 enables interrupts. We will look into this. Are you using a specific STM32 development board? If so, which board?

MaJerle commented 2 years ago

It is custom board with STM32H723 - USBX low-level port seems to be from MSFT straight. Part of X-CUBE-AZRTOS-H7 v2.1.0. Enabling BASEPRI fails to work properly and I still don't know why. None of the interrupts have irq in register lower than TX_PORT_BASEPRI, meaning all are lower-or-equal to this one, aside millisecond timer, but that one doesn't call any OS-related functions (just increases custom time, not at all linked to OS).

Tested also on H747-DK with example for USBX-Device_HID_CDC_ACM. same effect.

I do not use EPK.

goldscott commented 2 years ago

I see that BASEPRI is set to 0 later in the scheduler (https://github.com/azure-rtos/threadx/blob/54cda6ee9e68cd9701d759df13e3f6e41429b9bc/ports/cortex_m7/gnu/src/tx_thread_schedule.S#L208-L213)

Can you please contact ST directly about this issue. We will also investigate.

MaJerle commented 2 years ago

Question that remains - shall it be forced to zero or shall BASEPRI be stored first, then restored instead?

Priority-groups are set to 4 bits - so 16 levels.

goldscott commented 2 years ago

In the scheduler, BASEPRI is set to 0 before a thread is scheduled. This enables interrupts.

MaJerle commented 2 years ago

Found the problem - long night behind - BASEPRI is a masking register and therefore needs to have a proper mask set there, to determine level to block. I want to block all irqs with interrupt numerically higher or equal to 5.

TX_PORT_BASEPRI is therefore a mask value, not a logical number. If 4 priority bits are being used for interrupts, it means 16 levels, 0 - 15. To properly use TX_PORT_BASEPRI, value has to be set to (assuming CMSIS is used) BASEPRI = 5 << (8 - __NVIC_PRIO_BITS),

For reference, FreeRTOS uses 2 values, from FreeRTOSConfig.h file:

/* Lowest numerical interrupt that can still use OS functions */
#define configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY    5

/* configMAX_SYSCALL_INTERRUPT_PRIORITY must not be set to zero - this is used for BASEPRI setup */
#define configMAX_SYSCALL_INTERRUPT_PRIORITY    ( configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY << (8 - configPRIO_BITS) )

At least an explanation could be added in your tx_user for instance, that we expect mask, not irq level value.

goldscott commented 2 years ago

Hi @MaJerle - great find and thanks for letting us know! We will update our documentation. Please note that tx_user.h is for generic ThreadX defines. For something port-specific like BASEPRI, this is typically described in tx_port.h (though because TX_PORT_BASEPRI is used in assembly files, it will need to be defined on the command line or project options).

MaJerle commented 1 year ago

I have to come back to this point, because recently I realized that in one of the projects, using TX_PORT_BASEPRI with settings as explained above, device still doesn't work 100% properly, for some unknown reason. All interrupts are having numberically priority set to higher or equal to 5 so it should work, but USBX device CDC stops working under some special conditions that I cannot always reproduce.

Not using TX_PORT_BASEPRI, issue never appears

How was this tested on your side?

goldscott commented 1 year ago

Hi @xiaocq2001 - can you please help with the above issue?

goldscott commented 1 year ago

Hi @MaJerle - can you please contact ST about this issue.