[BUG] Port MPLAB PIC32MZ-EF, Timer1 is 16bits

[KammutierSpule]

Describe the bug
On the Port MPLAB PIC32MZ-EF, the Timer1 is 16bit

Target

• MPLAB PIC32MZ-EF

To Reproduce

• using configPERIPHERAL_CLOCK_HZ = 100 000 000 and configTICK_RATE_HZ = 100, the ulCompareMatch on vApplicationSetupTickTimerInterrupt will overflow. While using the current portTIMER_PRESCALE and portPRESCALE_BITS settings on the source code.

Expected behavior

• Expect that the vApplicationSetupTickTimerInterrupt asserts the ulCompareMatch to fit on a 16bit register.

[aggarg]

Thank you for raising this. If I understand correctly, you want to add the following assert:

    /* PR1 is 16-bit. Ensure that the configPERIPHERAL_CLOCK_HZ and
     * configTICK_RATE_HZ are defined such that ulCompareMatch value would fit
     * in 16-bits. */
    configASSERT( ( ulCompareMatch & 0xFFFF0000 ) == 0 );

If yes, we can add that. Would you like to raise a PR for that?

[KammutierSpule]

I have some concerns. I'm new to the FreeRTOS "business" environment and how this port targets should work.
In fact, I got this "bug" from Microchip code generator, so it may be a Microchip side bug?

However, there is info about using different timers for tick, but no warnings (or assert) about choose the right values for Timer1.

I tested the configASSERT,

the problem is that, again, on this port implementation, the timer configuration is performed before the scheduler start (makes sense), but the port implementation (generated/suggested by Microchip) of the configASSERT is:

void vAssertCalled( const char * pcFile, unsigned long ulLine )
{
   taskENTER_CRITICAL();
   {
      // your code
   }
   taskEXIT_CRITICAL();
}

at the moment the assert happens, the taskENTER_CRITICAL asserts because scheduler is not working.
Eventually, this will lead to a stack overflow and crash anyway.

What do you suggest?

Keep the configASSERT on timer? Change the vAssertCalled ? Is possible to check if scheduler is already running?

[aggarg]

the timer configuration is performed before the scheduler start (makes sense)

The timer configuration happens in the function vApplicationSetupTickTimerInterrupt which is called when starting the scheduler.

the taskENTER_CRITICAL asserts because scheduler is not working.

On this port, the taskENTER_CRITICAL ends up calling vTaskEnterCritical and I am not sure why would it assert. Can you share the assert that you are hitting?

[KammutierSpule]

I did some more investigation, please check the code below for <--- FAILS!

void vAssertCalled( const char * pcFile, unsigned long ulLine )
{
   taskENTER_CRITICAL();   <--- FAILS HERE!
   {
      // your code
   }
   taskEXIT_CRITICAL();
}

    void vTaskEnterCritical( void )
    {
        traceENTER_vTaskEnterCritical();

        portDISABLE_INTERRUPTS();

        if( xSchedulerRunning != pdFALSE )
        {
            ( pxCurrentTCB->uxCriticalNesting )++;

            /* This is not the interrupt safe version of the enter critical
             * function so  assert() if it is being called from an interrupt
             * context.  Only API functions that end in "FromISR" can be used in an
             * interrupt.  Only assert if the critical nesting count is 1 to
             * protect against recursive calls if the assert function also uses a
             * critical section. */
            if( pxCurrentTCB->uxCriticalNesting == 1U )
            {
                portASSERT_IF_IN_ISR();           <--- FAILS!

Then

extern volatile UBaseType_t uxInterruptNesting;
#define portASSERT_IF_IN_ISR() configASSERT( uxInterruptNesting == 0 )        <--- FAILS!

because:

/* Records the interrupt nesting depth.  This is initialised to one as it is
decremented to 0 when the first task starts. */
volatile UBaseType_t uxInterruptNesting = 0x01;

[aggarg]

Thank you for explaining. I understand that now. configASSERT and vAssertCalled are defined by the application and it should be okay to update it. Can you try updating vAssertCalled to the following:

void vAssertCalled( const char * pcFile, unsigned long ulLine )
{
   portDISABLE_INTERRUPTS();
   {
      // your code
   }
   portENABLE_INTERRUPTS();
}

If the above definition of vAssertCalled came from the Microchip generated code, I do not think it is worth adding this assert as it will break the default generated applications.

[KammutierSpule]

portDISABLE_INTERRUPTS/portENABLE_INTERRUPTS should work but depends on the implementation (eg: may dump to a USART controlled by some ISR buffer). So it is implementation specific (and I think it is ok).

Since vAssertCalled is on user (Microchip) side, I think should be ok on FreeRTOS project side to add what is reasonable.

Even with the current vAssertCalled, it will crash me on a stack overflow, which if I backtrack with debug, I will find the configASSERT( ( ulCompareMatch & 0xFFFF0000 ) == 0 );, which will help me to fix the bug.

[aggarg]

Even with the current vAssertCalled, it will crash me on a stack overflow, which if I backtrack with debug, I will find the configASSERT( ( ulCompareMatch & 0xFFFF0000 ) == 0 );, which will help me to fix the bug.

You are right. I missed that the assert will only fire when configPERIPHERAL_CLOCK_HZ and/or configTICK_RATE_HZ is mis-configured. In other words, the default generated applications would continue to work. Would you like to raise a PR?