refactor: remove uneccesary task{ENTER | EXIT}_CRITICAL()

[GuilhermeGiacomoSimoes]

refactor: remove uneccesary task{ENTER | EXIT}_CRITICAL()

Description

The call to the task{ENTER|EXIT}_CRITICAL() function is being made unnecessarily. It was probably placed to control access to the UBaseType_t variable, which the kernel is only accessed in atomic contexts. Therefore, the MUTEX to access this variable is not necessary since tasks in the atomic context do not have concurrency problems in accessing certain data

I would like to thanks @n9wxu, @svenbieg, @richard-damon to help me with this task.

Test Steps

For test in ATMega328p:

First I create this FreeRTOSConfig.h

#ifndef FREERTOS_CONFIG_H
#define FREERTOS_CONFIG_H

#define configUSE_PREEMPTION            1
#define configUSE_IDLE_HOOK             0
#define configUSE_TICK_HOOK             0
#define configCPU_CLOCK_HZ              ( 16000000UL )
#define configTICK_RATE_HZ              ( ( TickType_t ) 1000 )
#define configMAX_PRIORITIES            ( 4 )
#define configMINIMAL_STACK_SIZE        ( 85 )
#define configTOTAL_HEAP_SIZE           ( ( size_t ) ( 1024 ) )
#define configMAX_TASK_NAME_LEN         ( 8 )
#define configUSE_TRACE_FACILITY        0
#define configUSE_16_BIT_TICKS          1
#define configIDLE_SHOULD_YIELD         1
#define configUSE_MUTEXES               1
#define configQUEUE_REGISTRY_SIZE       0
#define configCHECK_FOR_STACK_OVERFLOW  0
#define configUSE_RECURSIVE_MUTEXES     1
#define configUSE_MALLOC_FAILED_HOOK    0
#define configUSE_APPLICATION_TASK_TAG  0
#define configUSE_COUNTING_SEMAPHORES   1
#define configSUPPORT_DYNAMIC_ALLOCATION 1
#define INCLUDE_uxTaskPriorityGet 1
#define configUSE_TIMERS 1
#define configTIMER_TASK_PRIORITY 1
#define configTIMER_QUEUE_LENGTH 1
#define configUSE_TIMERS 1
#define configTIMER_TASK_STACK_DEPTH 1

#define configUSE_PORT_OPTIMISED_TASK_SELECTION 0

#define configASSERT( x ) if( ( x ) == 0 ) { taskDISABLE_INTERRUPTS(); for( ;; ); }

#endif /* FREERTOS_CONFIG_H */

Then, I create this simple Makefile

MCU = atmega328p
F_CPU = 16000000UL
CC = avr-gcc
OBJCOPY = avr-objcopy
CFLAGS = -mmcu=$(MCU) -DF_CPU=$(F_CPU) -Wall -Os
LDFLAGS = -mmcu=$(MCU) -lm

SRC = main.c help.c FreeRTOS-Kernel/timers.c FreeRTOS-Kernel/portable/MemMang/heap_1.c FreeRTOS-Kernel/portable/GCC/ATMega323/port.c FreeRTOS-Kernel/list.c FreeRTOS-Kernel/queue.c FreeRTOS-Kernel/tasks.c

TARGET = main

all: $(TARGET).hex

$(TARGET).elf: $(SRC)
	$(CC) $(CFLAGS) -o $@ $(SRC)

%.hex: %.elf
	$(OBJCOPY) -O ihex -R .eeprom $< $@

clean:
	rm -f *.o *.elf *.hex

For help me with some things like print in serial port I create this help.c

#include <avr/interrupt.h>

#include "help.h"
#include <stdlib.h>
#include <stdio.h>

void serial_init(unsigned int ubrr) {
    UBRR0H = (unsigned char)(ubrr >> 8);
    UBRR0L = (unsigned char)ubrr;
    UCSR0B = (1 << RXEN0) | (1 << TXEN0);
    UCSR0C = (1 << UCSZ01) | (1 << UCSZ00);
}

static void USART_Transmit(unsigned char data) {
    while (!(UCSR0A & (1 << UDRE0)));
    UDR0 = data;
}

void serial_print(const char *str) {
    while (*str) {
        USART_Transmit(*str++);
    }
}

void to_str(unsigned int from, char *to) {
	sprintf(to, "%u", (unsigned int) from);
}

and this help.h

#ifndef __HELP_H__
#define __HELP_H__

void serial_init(unsigned int ubrr);
void serial_print(const char *str);

void to_str(unsigned int from, char *to);

#endif

for make a simple test about create a simple task, I create this main.c:

#define __AVR_ATmega328P__
#define F_CPU 16000000UL

#include "FreeRTOS-Kernel/Arduino_FreeRTOS.h"
#include "FreeRTOS-Kernel/include/task.h"

#include "help.h"

#define MYUBRR F_CPU/16/9599

void vTask1(void *pvParameters);

int main(void) {
    serial_init(MYUBRR);

	/// test a simple task create
	xTaskCreate(vTask1, "task1", 128, NULL, 1, NULL);

	vTaskStartScheduler();

	while(1); 
}

void vTask1(void *pvParameters) {
	serial_print("simple test about create a simple test...");
}

I compile: make and upload this for my Arduino:

avrdude -c arduino -P /dev/ttyUSB0 -b 115200 -p m328p -U flash:w:main.hex

I use the picocom for monitoring of serial port:

picocom -b 9600 /dev/ttyUSB0

main.c for test uxQueueMessagesWaiting() and uxQueueSpacesAvailable():

#define __AVR_ATmega328P__
#define F_CPU 16000000UL

#include <avr/io.h>
#include <util/delay.h>
#include <stdlib.h>

#include "FreeRTOS-Kernel/Arduino_FreeRTOS.h"
#include "FreeRTOS-Kernel/include/queue.h"

#include "help.h"

#define MYUBRR F_CPU/16/9599

QueueHandle_t xQueue;

int main(void) {
    serial_init(MYUBRR);

	char *str = (char*) malloc(sizeof(char));

  	xQueue = xQueueCreate(10, sizeof(int));

  	if (xQueue == NULL) {
  	  serial_print("fail\n");
  	  return 1;
  	}

  	serial_print("queue created\n");

  	serial_print("messages in the queue (expected 0): ");
	to_str(uxQueueMessagesWaiting(xQueue), str);
  	serial_print(str);  // should print 0
	serial_print("\n");

  	int valor = 42;
  	for (int i = 0; i < 3; i++) {
  	  xQueueSend(xQueue, &valor, 0);
  	}

  	serial_print("messeges in the queue after added 3 elements (expected 3): ");
	to_str(uxQueueMessagesWaiting(xQueue), str);
  	serial_print(str);  // sould print 3
	serial_print("\n");

  	int recebido;
  	xQueueReceive(xQueue, &recebido, 0);

  	serial_print("messages in the queue after remove 1 element (expected 2): ");
	to_str(uxQueueMessagesWaiting(xQueue), str);
  	serial_print(str);  // should print 2
	serial_print("\n");

	serial_print("uxQueueSpacesAvailable (expected 8): ");
	to_str(uxQueueSpacesAvailable(xQueue), str);
	serial_print(str);
	serial_print("\n");

	while(1); 
}

for test uxTaskPriorityGet() ans uxTaskBasePriorityGet();

#define __AVR_ATmega328P__
#define F_CPU 16000000UL

#include <stdlib.h>

#include "FreeRTOS-Kernel/Arduino_FreeRTOS.h"
#include "FreeRTOS-Kernel/include/task.h"

#include "help.h"

#define MYUBRR F_CPU/16/9599

void vTask1(void *pvParameters);

int main(void) {
    serial_init(MYUBRR);

	TaskHandle_t xHandle = NULL;

	/// test a simple task create
	xTaskCreate(vTask1, "task1", 128, NULL, 1, &xHandle);

	serial_print("task priority (expected 1): ");
	UBaseType_t res = uxTaskPriorityGet(xHandle);
	char *str = (char*) malloc(sizeof(char));
	to_str(res, str);
	serial_print(str);
	serial_print("\n");

	serial_print("task base priority (expected 1): ");
	UBaseType_t res_base = uxTaskBasePriorityGet(xHandle);
	to_str(res_base, str);
	serial_print(str);
	serial_print("\n");

	vTaskStartScheduler();
}

void vTask1(void *pvParameters) {
	serial_print("simple test about create a simple test...");
}

for test xTimerGetReloadMode() and xTimerIsTimerActive()

#define __AVR_ATmega328P__
#define F_CPU 16000000UL

#include <stdlib.h>

#include "FreeRTOS-Kernel/Arduino_FreeRTOS.h"
#include "FreeRTOS-Kernel/include/timers.h"

#include "help.h"

#define MYUBRR F_CPU/16/9599

TimerHandle_t periodicTimer;
TimerHandle_t oneShotTimer;

void vPeriodicTimerCallback(TimerHandle_t xTimer) {
  serial_print("temp added\n");
}

void vOneShotTimerCallback(TimerHandle_t xTimer) {
  serial_print("temp no periodic added\n");
}

int main(void) {
    serial_init(MYUBRR);

	periodicTimer = xTimerCreate("PeriodicTimer", pdMS_TO_TICKS(1000), pdTRUE, 0, vPeriodicTimerCallback);

  	if (periodicTimer == NULL) {
  	  serial_print("fail\n");
  	  return 1;
  	}

  	oneShotTimer = xTimerCreate("OneShotTimer", pdMS_TO_TICKS(1000), pdFALSE, 0, vOneShotTimerCallback);

  	if (oneShotTimer == NULL) {
  	  serial_print("fail\n");
  	  return 1;
  	}

  	xTimerStart(periodicTimer, 0);
  	xTimerStart(oneShotTimer, 0);

  	serial_print("Periodic timer recharge mode (expect - periodical): ");
	BaseType_t periodic = xTimerGetReloadMode(periodicTimer);
	char *str  = periodic == pdTRUE ? "periodical" : "not periodical";
  	serial_print(str);
  	serial_print("\n");

  	serial_print("Non-periodic timer recharge mode (expect - not periodical): ");
	BaseType_t nperiodic = xTimerGetReloadMode(oneShotTimer);
	char *nstr = periodic == pdFALSE ? "periodical" : "not periodical";
  	serial_print(nstr);
	serial_print("\n");


	char* astr = (char*) malloc(sizeof(char));

	serial_print("The timer is active? (expected 0): ");
	BaseType_t act = xTimerIsTimerActive(oneShotTimer);
	to_str(act, astr);
	serial_print(astr);
	serial_print("\n");
}

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

1059

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[aggarg]

uxQueueSpacesAvailable

This function reads 2 variables pxQueue->uxLength and pxQueue->uxMessagesWaiting - A context switch can happen after the first variable is read and the variables may get changed before the task is scheduled again. The function will end up returning incorrect result. Therefore, removing the critical section is not correct here.

xTimerGetReloadMode, xTimerGetReloadMode

These functions read pxTimer->ucStatus which is of type uint8_t and I do not think that we should assume that uint8_t read are atomic on all the platforms. Therefore, removing the critical section does not seem correct here.

[richard-damon]

uxQueueSpacesAvailable

This function reads 2 variables pxQueue->uxLength and pxQueue->uxMessagesWaiting - A context switch can happen after the first variable is read and the variables may get changed before the task is scheduled again. The function will end up returning incorrect result. Therefore, removing the critical section is not correct here.

uxlLength is a constant for a given queue, initialized on construction and never changed, thus its access doesn't need to be protected.

uxMesssagesWaiting can probably be read atomically, and not need to be protected, I believe we have a conditional critical section that could be used for that sort of action.

Yes, there is a fundamental race in the function that even its critical section can't handle, in that between the call to get the number of items and an operation to use them, the number of items could change, but there is nothing this function can do about that.

[aggarg]

uxlLength is a constant for a given queue, initialized on construction and never changed, thus its access doesn't need to be protected.

You are exactly right. Thank you for pointing that! Ignore my comment about this function.

[GuilhermeGiacomoSimoes]

uxMesssagesWaiting can probably be read atomically, and not need to be protected, I believe we have a conditional critical section that could be used for that sort of action.

You say that we have a conditional critial section. This Condifitional critical section have a check for atomic context?
For ex, if (context == atomic) { dont_call_mutex } else { call_mutex } ??

If uxMessagesWaiting can be access for a atomic context or a access for a unprivileged context, then this conditional critical section is neccessary.
But if uxMessagesWaiting is ever access for a atomic context, I don't can think why we need the critial section.

[aggarg]

@GuilhermeGiacomoSimoes Thank you for raising this PR. This PR relies on the fact that Read and Write of BaseType_t and UBaseType_t are atomic. We had a discussion internally and we think that it might be difficult to claim without looking at all the ports - especially the ones with extensions that increase the addressable memory space or the ones with different memory models (near, far, etc.).

Instead of doing this change in a way which impacts all the ports, we can introduce this gradually on a port-by-port basis. We can introduce a new set of macros, namely portBASE_TYPE_ENTER_CRITICAL and portBASE_TYPE_EXIT_CRITICAL, which default to taskENTER_CRITICAL and taskEXIT_CRITICAL:

#ifndef portBASE_TYPE_ENTER_CRITICAL
    #define portBASE_TYPE_ENTER_CRITICAL() taskENTER_CRITICAL()
#endif

#ifndef portBASE_TYPE_EXIT_CRITICAL
    #define portBASE_TYPE_EXIT_CRITICAL() taskEXIT_CRITICAL()
#endif

The above can be inserted here - https://github.com/FreeRTOS/FreeRTOS-Kernel/blob/main/include/portable.h#L87.

We can then change the functions in this PR to use these new macros instead. Once that is done, these new macros can be defined to nothing in portmacro.h for ports where Read and Write of BaseType_t and UBaseType_t are confirmed to be atomic:

#define portBASE_TYPE_ENTER_CRITICAL()
#define portBASE_TYPE_EXIT_CRITICAL()

What do you think about this?

[GuilhermeGiacomoSimoes]

Okay, I understand your concern, and really make sense.

Your suggestions make sense too .. Then, instead call taskENTER_CRITICAL(), we would should call the new portBASE_TYPE_ENTER_CRITICAL().

I will make this change and test this.
I would should open another PR ? or I can commit in this branch ?

[aggarg]

I would should open another PR ? or I can commit in this branch ?

I am good with whatever you prefer. Thank you for all the work!

[GuilhermeGiacomoSimoes]

@aggarg I make the changes that is suggested by you here. I test this rigorously.

For test this, you can make this simple change in portmacro.h:

diff --git a/portable/GCC/ATMega323/portmacro.h b/portable/GCC/ATMega323/portmacro.h
index 6ed5e4295..131819004 100644
--- a/portable/GCC/ATMega323/portmacro.h
+++ b/portable/GCC/ATMega323/portmacro.h
@@ -61,6 +61,8 @@
 #define portSTACK_TYPE           uint8_t
 #define portBASE_TYPE            char

+#include "help.h" /// this is the help.h that I mentionated in the PR description
+
 #define portPOINTER_SIZE_TYPE    uint16_t

 typedef portSTACK_TYPE   StackType_t;
@@ -78,13 +80,18 @@ typedef unsigned char    UBaseType_t;
 #endif
 /*-----------------------------------------------------------*/

+#define portBASE_TYPE_ENTER_CRITICAL()
+#define portBASE_TYPE_EXIT_CRITICAL()
+
 /* Critical section management. */
 #define portENTER_CRITICAL()                            \
+       serial_print("\nenter critical\n"); \
     asm volatile ( "in      __tmp_reg__, __SREG__" ::); \
     asm volatile ( "cli" ::);                           \
     asm volatile ( "push    __tmp_reg__" ::)

 #define portEXIT_CRITICAL()                   \
+       serial_print("\nexit critical\n"); \
     asm volatile ( "pop     __tmp_reg__" ::); \
     asm volatile ( "out     __SREG__, __tmp_reg__" ::)

You can see that I already create the empty macro portBASE_TYPE_ENTER_CRITIAL() .... Now, when I run the tests that I mentioned in PR description , you can see where the string "enter critical" should or don't should showed.

For ex, If you want run the test about uxQueueMessagesWaiting(), the string showing in console gonna be:
messages in the queue (expected 0): 0
Because, I declared the empty macros portBASE_TYPE_ENTER_CRITICAL(), then the portENTER_CRITICAL() don't is executed.

BUT, If you comment the empty macros:

/// #define portBASE_TYPE_ENTER_CRITICAL()
/// #define portBASE_TYPE_EXIT_CRITICAL()

Then, the portENTER_CRITIAL() should execute .. Then in your console, you can see this:

messages in the queue (expected 0): 
enter critical
exit critical
0

Because the prints that I set in portmacro should be executed.

If you need any explanation or help, you can found me on this email trintaeoitogc@gmail.com

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud