Missing vSemaphoreDelete() in arduino-esp32/cores/esp32/

[mrengineer7777]

Board

Any

Device Description

NA

Hardware Configuration

NS

Version

latest master

IDE Name

PlatformIO

Operating System

Win10

Flash frequency

80

PSRAM enabled

no

Upload speed

115200

Description

This is a proposed bugfix and feature improvement. In #6425 it was discovered that esp32-ha-spi.c never releases the Mutex created in spiStartBus(). Looking at the other libraries in arduino-esp32/cores/esp32/, I can see that the same issue occurs in:
esp32-hal-i2c.c
esp32-hal-i2c-slave.c
esp32-hal-spi.c
esp32-hal-uart.c
esp32-hap-cpu.c [There isn't a matching function to initApbChangeCallback() to release the Mutex]

Everything except the CPU file uses the Mutex when CONFIG_DISABLE_HAL_LOCKS==false. The code is littered with #if !CONFIG_DISABLE_HAL_LOCKS, which makes it difficult to read and maintain. While I could edit each file and add the missing vSemaphoreDelete() calls, I propose creating a unified set of defines in esp32-hal.h, and possibly a couple supporting function calls. Then we can use the new defines in the core libraries to simplify HAL locks.

I'd be happy to create a PR if this would help.

Here's what I have so far. Keep in mind this is conceptual. Code has not been compiled or tested.

In esp32-hal.h:

#if !CONFIG_DISABLE_HAL_LOCKS
	#define HAL_LOCKS_CREATEVAR(varname)                       xSemaphoreHandle varname = NULL
	#define HAL_LOCKS_INIT(varname)                            HalLocksInit(&varname)
	#define HAL_LOCKS_LOCK(varname, timeout, infiniteretries)  HalLocksLock(&varname, timeout, infiniteretries)
	#define HAL_LOCKS_UNLOCK(varname)                          HalLocksUnlock(&varname)
	#define HAL_LOCKS_DEINIT(varname)                          HalLocksDeinit(&varname)

#else
	#define HAL_LOCKS_CREATEVAR(varname)
	#define HAL_LOCKS_INIT(varname)                            //Passing a missing variable name should work as it expands to nothing. TODO TEST!!
	#define HAL_LOCKS_LOCK(varname, timeout, infiniteretries)
	#define HAL_LOCKS_UNLOCK(varname)
	#define HAL_LOCKS_DEINIT(varname)
#endif

Not sure where to put this:

bool HalLocksInit(xSemaphoreHandle *hnd) {
	if(*hnd != NULL) {
		return true;			//Handle already exists
	}
	*hnd = xSemaphoreCreateMutex();
	return (*hnd != NULL);

}

bool HalLocksLock(xSemaphoreHandle *hnd, TickType_t timeout_ticks, bool InfiniteRetries) {	//So far all code uses portMAX_DELAY for the timeout, so we could remove the timeout parameter.
	if(*hnd == NULL) {
		return false;
	}
	
	BaseType_t result = xSemaphoreTake(*hnd, timeout_ticks);
	while(InfiniteRetries && (result != pdTRUE)) {
		result = xSemaphoreTake(*hnd, timeout_ticks);
	}
	return result == pdTRUE;
}

//Could return the result of xSemaphoreGive(), but I didn't find any code that uses it.
void HalLocksUnlock(xSemaphoreHandle *hnd) {
	if(*hnd == NULL) {
		return;
	}
	xSemaphoreGive(*hnd);
}

void HalLocksDeinit(xSemaphoreHandle *hnd) {
	if(*hnd == NULL) {
		return;
	}
	vSemaphoreDelete(*hnd);
	*hnd = NULL;							//Clear handle so it can't be released again
}

USAGE:

HAL_LOCKS_CREATEVAR(_hal_uart_lock);	//Note we don't have to include "#if !CONFIG_DISABLE_HAL_LOCKS" here.
HAL_LOCKS_INIT(_hal_uart_lock);
HAL_LOCKS_LOCK(_hal_uart_lock, portMAX_DELAY, true);
HAL_LOCKS_UNLOCK(_hal_uart_lock);
HAL_LOCKS_DEINIT(_hal_uart_lock);

Compare this to esp32-hal-uart.c:

#if !CONFIG_DISABLE_HAL_LOCKS
    xSemaphoreHandle lock;
#endif

#if CONFIG_DISABLE_HAL_LOCKS
#define UART_MUTEX_LOCK()
#define UART_MUTEX_UNLOCK()
...
#else
#define UART_MUTEX_LOCK()    do {} while (xSemaphoreTake(uart->lock, portMAX_DELAY) != pdPASS)
#define UART_MUTEX_UNLOCK()  xSemaphoreGive(uart->lock)
...
#endif
...
uart_t* uartBegin(...)
{
    ...
#if !CONFIG_DISABLE_HAL_LOCKS
    if(uart->lock == NULL) {
        uart->lock = xSemaphoreCreateMutex();
        if(uart->lock == NULL) {
            return NULL;
        }
    }
#endif

    UART_MUTEX_LOCK();
   ...
    UART_MUTEX_UNLOCK();
    ...
}
...
void uartEnd(uart_t* uart)
{
    if(uart == NULL) {
        return;
    }

    UART_MUTEX_LOCK();
    uart_driver_delete(uart->num);
    UART_MUTEX_UNLOCK();
   // [[ SHOULD RELEASE MUTEX HERE  ]]
}

Sketch

NA

Debug Message

NA

Other Steps to Reproduce

No response

I have checked existing issues, online documentation and the Troubleshooting Guide

• I confirm I have checked existing issues, online documentation and Troubleshooting guide.

Edit: renamed HalLocksCreateMutex() to HalLocksInit().

[mrengineer7777]

I would love to hear some feedback from the devs, or anybody, if this is a good addition to this project.

[VojtechBartoska]

Hi @mrengineer7777, thanks for suggestion. What do you think @me-no-dev?

[mrengineer7777]

Bump. Still willing to do the work on this if it helps.

[VojtechBartoska]

thanks for reminder @mrengineer7777! I'm adding this feature request to the roadmap and we will evaluate this.

[Parsaabasi]

Hello,

Due to the overwhelming volume of issues currently being addressed, we have decided to close the previously received tickets. If you still require assistance or if the issue persists, please don't hesitate to reopen the ticket.

Thanks.