Merge branch 'bugfix/always_disable_ints_in_critical_region' into 'master'

Always disable interrupts in a critical region

See merge request !821

components/freertos/include/freertos/portmacro.h

@@ -194,6 +194,10 @@ do not disable the interrupts (because they already are).
 
 This all assumes that interrupts are either entirely disabled or enabled. Interrupr priority levels
 will break this scheme.
+
+Remark: For the ESP32, portENTER_CRITICAL and portENTER_CRITICAL_ISR both alias vTaskEnterCritical, meaning
+that either function can be called both from ISR as well as task context. This is not standard FreeRTOS 
+behaviour; please keep this in mind if you need any compatibility with other FreeRTOS implementations.
 */
 void vPortCPUInitializeMutex(portMUX_TYPE *mux);
 #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG
@@ -203,8 +207,8 @@ void vTaskEnterCritical( portMUX_TYPE *mux, const char *function, int line );
 void vTaskExitCritical( portMUX_TYPE *mux, const char *function, int line );
 #define portENTER_CRITICAL(mux)        vTaskEnterCritical(mux, __FUNCTION__, __LINE__)
 #define portEXIT_CRITICAL(mux)         vTaskExitCritical(mux, __FUNCTION__, __LINE__)
-#define portENTER_CRITICAL_ISR(mux)    vPortCPUAcquireMutex(mux, __FUNCTION__, __LINE__)
-#define portEXIT_CRITICAL_ISR(mux)    vPortCPUReleaseMutex(mux, __FUNCTION__, __LINE__)
+#define portENTER_CRITICAL_ISR(mux)    vTaskEnterCritical(mux, __FUNCTION__, __LINE__)
+#define portEXIT_CRITICAL_ISR(mux)     vTaskExitCritical(mux, __FUNCTION__, __LINE__)
 #else
 void vTaskExitCritical( portMUX_TYPE *mux );
 void vTaskEnterCritical( portMUX_TYPE *mux );
@@ -212,13 +216,14 @@ void vPortCPUAcquireMutex(portMUX_TYPE *mux);
 portBASE_TYPE vPortCPUReleaseMutex(portMUX_TYPE *mux);
 #define portENTER_CRITICAL(mux)        vTaskEnterCritical(mux)
 #define portEXIT_CRITICAL(mux)         vTaskExitCritical(mux)
-#define portENTER_CRITICAL_ISR(mux)    vPortCPUAcquireMutex(mux)
-#define portEXIT_CRITICAL_ISR(mux)    vPortCPUReleaseMutex(mux)
+#define portENTER_CRITICAL_ISR(mux)    vTaskEnterCritical(mux)
+#define portEXIT_CRITICAL_ISR(mux)     vTaskExitCritical(mux)
 #endif
 
 // Cleaner and preferred solution allows nested interrupts disabling and restoring via local registers or stack.
 // They can be called from interrupts too.
-//NOT SMP-COMPATIBLE! Use only if all you want is to disable the interrupts locally!
+// WARNING: This ONLY disables interrupt on the current CPU, meaning they cannot be used as a replacement for the vTaskExitCritical spinlock
+// on a multicore system. Only use if disabling interrupts on the current CPU only is indeed what you want.
 static inline unsigned portENTER_CRITICAL_NESTED() { unsigned state = XTOS_SET_INTLEVEL(XCHAL_EXCM_LEVEL); portbenchmarkINTERRUPT_DISABLE(); return state; }
 #define portEXIT_CRITICAL_NESTED(state)   do { portbenchmarkINTERRUPT_RESTORE(state); XTOS_RESTORE_JUST_INTLEVEL(state); } while (0)
 

components/freertos/tasks.c

@@ -2676,11 +2676,8 @@ BaseType_t xSwitchRequired = pdFALSE;
 
 void vTaskSwitchContext( void )
 {
-	//Note: This can be called from interrupt context as well as from non-interrupt context (voluntary yield). The
-	//taskENTER_CRITICAL/taskEXIT_CRITICAL is modified to work in both scenarios for the ESP32, so we can freely use
-	//them here. However, in case of a voluntary yield, a nonvoluntary yield can still happen *during* the voluntary
-	//yield. Disabling interrupts using portENTER_CRITICAL_NESTED puts a stop to this and makes the rest of the code a 
-	//bit neater.
+	//Theoretically, this is only called from either the tick interrupt or the crosscore interrupt, so disabling
+	//interrupts shouldn't be necessary anymore. Still, for safety we'll leave it in for now.
 	int irqstate=portENTER_CRITICAL_NESTED();
 	tskTCB * pxTCB;
 	if( uxSchedulerSuspended[ xPortGetCoreID() ] != ( UBaseType_t ) pdFALSE )
@@ -2703,9 +2700,9 @@ void vTaskSwitchContext( void )
 				#endif
 
 				/* Add the amount of time the task has been running to the
-				accumulated	time so far.  The time the task started running was
+				accumulated time so far.  The time the task started running was
 				stored in ulTaskSwitchedInTime.  Note that there is no overflow
-				protection here	so count values are only valid until the timer
+				protection here so count values are only valid until the timer
 				overflows.  The guard against negative values is to protect
 				against suspect run time stat counter implementations - which
 				are provided by the application, not the kernel. */
@@ -2727,13 +2724,18 @@ void vTaskSwitchContext( void )
 		taskFIRST_CHECK_FOR_STACK_OVERFLOW();
 		taskSECOND_CHECK_FOR_STACK_OVERFLOW();
 
-		/* Select a new task to run using either the generic C or port
-		optimised asm code. */
-		/* ToDo: either get rid of port-changable task switching stuff, or put all this inside the
-		   taskSELECT_HIGHEST_PRIORITY_TASK macro, then replace this all with a taskSELECT_HIGHEST_PRIORITY_TASK(); 
-		   call */
+		/* Select a new task to run */
 		
-		taskENTER_CRITICAL_ISR(&xTaskQueueMutex);
+		/*
+		 We cannot do taskENTER_CRITICAL_ISR(&xTaskQueueMutex); here because it saves the interrupt context to the task tcb, and we're
+		 swapping that out here. Instead, we're going to do the work here ourselves. Because interrupts are already disabled, we only
+		 need to acquire the mutex.
+		*/
+#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG
+		vPortCPUAcquireMutex( &xTaskQueueMutex, __FUNCTION__, __LINE__ );
+#else
+		vPortCPUAcquireMutex( &xTaskQueueMutex );
+#endif
 		
 		unsigned portBASE_TYPE foundNonExecutingWaiter = pdFALSE, ableToSchedule = pdFALSE, resetListHead;
 		portBASE_TYPE uxDynamicTopReady = uxTopReadyPriority;
@@ -2818,12 +2820,17 @@ void vTaskSwitchContext( void )
 			}
 			--uxDynamicTopReady;
 		}
-		taskEXIT_CRITICAL_ISR(&xTaskQueueMutex);
-
-		/* ToDo: taskSELECT_HIGHEST_PRIORITY_TASK replacement code ends here. */
 
 		traceTASK_SWITCHED_IN();
 
+		//Exit critical region manually as well: release the mux now, interrupts will be re-enabled when we
+		//exit the function.
+#ifdef CONFIG_FREERTOS_PORTMUX_DEBUG
+		vPortCPUReleaseMutex( &xTaskQueueMutex, function, line );
+#else
+		vPortCPUReleaseMutex( &xTaskQueueMutex );
+#endif
+
 #if CONFIG_FREERTOS_WATCHPOINT_END_OF_STACK
 		vPortSetStackWatchpoint(pxCurrentTCB[xPortGetCoreID()]->pxStack);
 #endif
@@ -4060,7 +4067,10 @@ TCB_t *pxTCB;
 /* Gotcha (which seems to be deliberate in FreeRTOS, according to
 http://www.freertos.org/FreeRTOS_Support_Forum_Archive/December_2012/freertos_PIC32_Bug_-_vTaskEnterCritical_6400806.html
 ) is that calling vTaskEnterCritical followed by vTaskExitCritical will leave the interrupts DISABLED when the scheduler
-is not running.  Re-enabling the scheduler will re-enable the interrupts instead. */
+is not running.  Re-enabling the scheduler will re-enable the interrupts instead. 
+
+For ESP32 FreeRTOS, vTaskEnterCritical implements both portENTER_CRITICAL and portENTER_CRITICAL_ISR.
+*/
 
 #if ( portCRITICAL_NESTING_IN_TCB == 1 )
 
@@ -4103,7 +4113,9 @@ is not running.  Re-enabling the scheduler will re-enable the interrupts instead
 			protect against recursive calls if the assert function also uses a
 			critical section. */
 
-			/* DISABLED in the esp32 port - because of SMP, vTaskEnterCritical
+			/* DISABLED in the esp32 port - because of SMP, For ESP32 
+			FreeRTOS, vTaskEnterCritical implements both 
+			portENTER_CRITICAL and portENTER_CRITICAL_ISR. vTaskEnterCritical
 			has to be used in way more places than before, and some are called
 			both from ISR as well as non-ISR code, thus we re-organized 
 			vTaskEnterCritical to also work in ISRs. */
@@ -4124,6 +4136,10 @@ is not running.  Re-enabling the scheduler will re-enable the interrupts instead
 #endif /* portCRITICAL_NESTING_IN_TCB */
 /*-----------------------------------------------------------*/
 
+
+/*
+For ESP32 FreeRTOS, vTaskExitCritical implements both portEXIT_CRITICAL and portEXIT_CRITICAL_ISR.
+*/
 #if ( portCRITICAL_NESTING_IN_TCB == 1 )
 
 #ifdef CONFIG_FREERTOS_PORTMUX_DEBUG