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.