Merge 'Port hardware locks to new mutex API'

Move hw-locks to new mutex API in multiple steps to keep diffs readable.  The
users of hw-locks are **not** ported to the new-semantics; this needs to be
done as a follow up.  One patch is included, porting the MAX30001 driver in
commit 6da4644e ("chore(max30001): Port to new mutex and hw-lock APIs").

See merge request !356

epicardium/modules/bhi.c

@@ -135,7 +135,7 @@ int epic_bhi160_enable_sensor(
 		return -ENODEV;
 	}
 
-	result = hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100));
+	result = hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY);
 	if (result < 0) {
 		return result;
 	}
@@ -192,7 +192,7 @@ int epic_bhi160_disable_sensor(enum bhi160_sensor_type sensor_type)
 		return -ENODEV;
 	}
 
-	result = hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100));
+	result = hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY);
 	if (result < 0) {
 		return result;
 	}
@@ -347,7 +347,7 @@ static int bhi160_fetch_fifo(void)
 	/* Number of bytes left in BHI160's FIFO buffer */
 	uint16_t bytes_left_in_fifo = 1;
 
-	result = hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100));
+	result = hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY);
 	if (result < 0) {
 		return result;
 	}
@@ -433,7 +433,7 @@ void vBhi160Task(void *pvParameters)
 	 */
 	vTaskDelay(pdMS_TO_TICKS(3));
 
-	int lockret = hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100));
+	int lockret = hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY);
 	if (lockret < 0) {
 		LOG_CRIT("bhi160", "Failed to acquire I2C lock!");
 		vTaskDelay(portMAX_DELAY);
@@ -469,7 +469,7 @@ void vBhi160Task(void *pvParameters)
 	/* Wait for first interrupt */
 	hwlock_release(HWLOCK_I2C);
 	ulTaskNotifyTake(pdTRUE, pdMS_TO_TICKS(100));
-	lockret = hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100));
+	lockret = hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY);
 	if (lockret < 0) {
 		LOG_CRIT("bhi160", "Failed to acquire I2C lock!");
 		vTaskDelay(portMAX_DELAY);

epicardium/modules/bme680.c

@@ -43,7 +43,7 @@ int epic_bme680_init()
 		return 0;
 	}
 
-	if (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100)) < 0) {
+	if (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 		return -EBUSY;
 	}
 
@@ -110,7 +110,7 @@ int epic_bme680_deinit()
 		return 0;
 	}
 
-	if (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100)) < 0) {
+	if (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 		return -EBUSY;
 	}
 
@@ -133,7 +133,7 @@ int epic_bme680_read_sensors(struct bme680_sensor_data *data)
 		return -EINVAL;
 	}
 
-	if (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100)) < 0) {
+	if (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 		return -EBUSY;
 	}
 
@@ -152,7 +152,7 @@ int epic_bme680_read_sensors(struct bme680_sensor_data *data)
 	 */
 	hwlock_release(HWLOCK_I2C);
 	vTaskDelay(pdMS_TO_TICKS(profile_dur));
-	if (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100)) < 0) {
+	if (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 		return -EBUSY;
 	}
 

epicardium/modules/buttons.c

@@ -17,7 +17,7 @@ uint8_t epic_buttons_read(uint8_t mask)
 {
 	uint8_t ret = 0;
 	if (portexpander_detected() && (mask & 0x7)) {
-		if (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(100)) < 0) {
+		if (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 			LOG_ERR("buttons", "Can't acquire I2C bus");
 			return 0;
 		}

epicardium/modules/gpio.c

@@ -137,7 +137,7 @@ int epic_gpio_read_pin(uint8_t pin)
 	} else if (cfg->func == GPIO_FUNC_IN) {
 		return GPIO_InGet(cfg) != 0;
 	} else if (cfg->func == GPIO_FUNC_ALT1) {
-		int rc = hwlock_acquire(HWLOCK_ADC, pdMS_TO_TICKS(10));
+		int rc = hwlock_acquire_timeout(HWLOCK_ADC, portMAX_DELAY);
 		if (!rc) {
 			ADC_StartConvert(s_adc_channels[pin], 0, 0);
 			uint16_t value;

epicardium/modules/hw-lock.c

 #include "modules/log.h"
 #include "modules/modules.h"
+#include "modules/mutex.h"
 
 #include "FreeRTOS.h"
 #include "task.h"
-#include "semphr.h"
 
 #include <errno.h>
 
-static StaticSemaphore_t hwlock_mutex_data[_HWLOCK_MAX];
-static SemaphoreHandle_t hwlock_mutex[_HWLOCK_MAX];
-/* Which task is holding the lock */
-static TaskHandle_t hwlock_tasks[_HWLOCK_MAX];
+static struct mutex hwlock_mutex[_HWLOCK_MAX] = { { 0 } };
 
 void hwlock_init(void)
 {
 	for (int i = 0; i < _HWLOCK_MAX; i++) {
-		hwlock_mutex[i] =
-			xSemaphoreCreateMutexStatic(&hwlock_mutex_data[i]);
+		/*
+		 * TODO: mutex_create() names these all these mutexes
+		 *       "&hwlock_mutex[i]" which is not helpful at all.  We
+		 *       should somehow rename them to the actual hwlock names.
+		 */
+		mutex_create(&hwlock_mutex[i]);
 	}
 }
 
-int hwlock_acquire(enum hwlock_periph p, TickType_t wait)
+void hwlock_acquire(enum hwlock_periph p)
 {
-	if (p >= _HWLOCK_MAX) {
-		return -EINVAL;
+	assert(p < _HWLOCK_MAX);
+	mutex_lock(&hwlock_mutex[p]);
 }
-	TaskHandle_t task = xTaskGetCurrentTaskHandle();
 
-	if (xSemaphoreTake(hwlock_mutex[p], wait) != pdTRUE) {
-		/* Don't print warnings for 0 wait acquires */
-		if (wait == 0) {
+int hwlock_acquire_nonblock(enum hwlock_periph p)
+{
+	assert(p < _HWLOCK_MAX);
+	if (mutex_trylock(&hwlock_mutex[p])) {
+		return 0;
+	} else {
 		return -EBUSY;
 	}
+}
+
+int hwlock_acquire_timeout(enum hwlock_periph p, TickType_t wait)
+{
+	assert(p < _HWLOCK_MAX);
 
+	/*
+	 * Check for code still defining a timeout.  It will be ignored in the
+	 * new implementation but a warning is emitted so someone hopefully
+	 * eventually fixes the offending code ...
+	 *
+	 * At some point, the signature of this function should be changed.
+	 * Alternatively it could be split into two, similar to the mutex API.
+	 */
+	if (wait != 0 && wait != portMAX_DELAY) {
 		LOG_WARN(
 			"hwlock",
-			"Lock %u is busy! Held by \"%s\" and attempted to accquire by \"%s\"",
+			"Attempting to lock %d with a timeout (from %p).",
 			p,
-			pcTaskGetName(hwlock_tasks[p]),
-			pcTaskGetName(task)
-		);
-		LOG_DEBUG(
-			"hwlock",
-			"...attempted to lock from pc %p",
 			__builtin_return_address(0)
 		);
-		return -EBUSY;
 	}
 
-	hwlock_tasks[p] = task;
-
+	/* Non-blocking lock attempt */
+	if (wait == 0) {
+		if (mutex_trylock(&hwlock_mutex[p]) == true) {
 			return 0;
+		} else {
+			return -EBUSY;
 		}
-
-int hwlock_release(enum hwlock_periph p)
-{
-	int ret = 0;
-
-	if (p >= _HWLOCK_MAX) {
-		return -EINVAL;
-	}
-
-	if (hwlock_tasks[p] != xTaskGetCurrentTaskHandle()) {
-		LOG_ERR("hwlock",
-			"Lock %u is released by task \"%s\" while it was acquired by \"%s\"",
-			p,
-			pcTaskGetName(NULL),
-			pcTaskGetName(hwlock_tasks[p]));
-		ret = -EACCES;
 	}
 
-	if (xSemaphoreGive(hwlock_mutex[p]) != pdTRUE) {
-		LOG_ERR("hwlock", "Lock %u not released correctly.", p);
-		ret = -EINVAL;
+	mutex_lock(&hwlock_mutex[p]);
+	return 0;
 }
 
-	return ret;
+void hwlock_release(enum hwlock_periph p)
+{
+	assert(p < _HWLOCK_MAX);
+	mutex_unlock(&hwlock_mutex[p]);
 }

epicardium/modules/leds.c

@@ -13,7 +13,7 @@
 
 static void do_update()
 {
-	while (hwlock_acquire(HWLOCK_LED, pdMS_TO_TICKS(1)) < 0) {
+	while (hwlock_acquire_timeout(HWLOCK_LED, portMAX_DELAY) < 0) {
 		vTaskDelay(pdMS_TO_TICKS(1));
 	}
 
@@ -96,7 +96,7 @@ void epic_leds_dim_top(uint8_t value)
 {
 	leds_set_dim_top(value);
 	if (personal_state_enabled() == 0) {
-		while (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(1)) < 0) {
+		while (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 			vTaskDelay(pdMS_TO_TICKS(1));
 		}
 
@@ -110,7 +110,7 @@ void epic_leds_dim_bottom(uint8_t value)
 {
 	leds_set_dim_bottom(value);
 	if (personal_state_enabled() == 0) {
-		while (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(1)) < 0) {
+		while (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 			vTaskDelay(pdMS_TO_TICKS(1));
 		}
 
@@ -122,7 +122,7 @@ void epic_leds_dim_bottom(uint8_t value)
 
 void epic_leds_set_rocket(int led, uint8_t value)
 {
-	while (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(1)) < 0) {
+	while (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 		vTaskDelay(pdMS_TO_TICKS(1));
 	}
 
@@ -135,7 +135,7 @@ void epic_leds_set_rocket(int led, uint8_t value)
 int epic_leds_get_rocket(int led)
 {
 	int ret = 0;
-	while (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(1)) < 0) {
+	while (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 		vTaskDelay(pdMS_TO_TICKS(1));
 	}
 
@@ -146,7 +146,7 @@ int epic_leds_get_rocket(int led)
 
 void epic_set_flashlight(bool power)
 {
-	while (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(1)) < 0) {
+	while (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 		vTaskDelay(pdMS_TO_TICKS(1));
 	}
 
@@ -162,7 +162,7 @@ void epic_leds_update(void)
 
 void epic_leds_set_powersave(bool eco)
 {
-	while (hwlock_acquire(HWLOCK_I2C, pdMS_TO_TICKS(1)) < 0) {
+	while (hwlock_acquire_timeout(HWLOCK_I2C, portMAX_DELAY) < 0) {
 		vTaskDelay(pdMS_TO_TICKS(1));
 	}
 

epicardium/modules/light_sensor.c

@@ -29,7 +29,7 @@ static int light_sensor_init()
 
 uint16_t epic_light_sensor_read()
 {
-	if (hwlock_acquire(HWLOCK_ADC, pdMS_TO_TICKS(1000)) != 0) {
+	if (hwlock_acquire_timeout(HWLOCK_ADC, portMAX_DELAY) != 0) {
 		return 0;
 	}
 
@@ -42,7 +42,7 @@ uint16_t epic_light_sensor_read()
 
 static void readAdcCallback()
 {
-	if (hwlock_acquire(HWLOCK_ADC, 0) != 0) {
+	if (hwlock_acquire_timeout(HWLOCK_ADC, 0) != 0) {
 		/* Can't do much about this here ... Retry next time */
 		return;
 	}
@@ -57,7 +57,7 @@ int epic_light_sensor_run()
 {
 	int ret = 0;
 
-	if (hwlock_acquire(HWLOCK_ADC, pdMS_TO_TICKS(500)) != 0) {
+	if (hwlock_acquire_timeout(HWLOCK_ADC, portMAX_DELAY) != 0) {
 		return -EBUSY;
 	}
 

epicardium/modules/max30001.c

@@ -9,7 +9,6 @@
 
 #include "FreeRTOS.h"
 #include "task.h"
-#include "semphr.h"
 #include "queue.h"
 
 #include "api/interrupt-sender.h"
@@ -17,9 +16,7 @@
 #include "modules/log.h"
 #include "modules/modules.h"
 #include "modules/stream.h"
-
-/* Ticks to wait when trying to acquire lock */
-#define LOCK_WAIT pdMS_TO_TICKS(MAX30001_MUTEX_WAIT_MS)
+#include "modules/mutex.h"
 
 /* Interrupt Pin */
 static const gpio_cfg_t max30001_interrupt_pin = {
@@ -36,8 +33,7 @@ static const gpio_cfg_t analog_switch = {
 static TaskHandle_t max30001_task_id = NULL;
 
 /* MAX30001 Mutex */
-static StaticSemaphore_t max30001_mutex_data;
-static SemaphoreHandle_t max30001_mutex = NULL;
+static struct mutex max30001_mutex = { 0 };
 
 /* Stream */
 static struct stream_info max30001_stream;
@@ -54,15 +50,8 @@ int epic_max30001_enable_sensor(struct max30001_sensor_config *config)
 {
 	int result = 0;
 
-	result = hwlock_acquire(HWLOCK_SPI_ECG, pdMS_TO_TICKS(100));
-	if (result < 0) {
-		return result;
-	}
-
-	if (xSemaphoreTake(max30001_mutex, LOCK_WAIT) != pdTRUE) {
-		result = -EBUSY;
-		goto out_free_spi;
-	}
+	mutex_lock(&max30001_mutex);
+	hwlock_acquire(HWLOCK_SPI_ECG);
 
 	struct stream_info *stream = &max30001_stream;
 	;
@@ -97,9 +86,8 @@ int epic_max30001_enable_sensor(struct max30001_sensor_config *config)
 	result                 = SD_MAX30001_ECG;
 
 out_free_both:
-	xSemaphoreGive(max30001_mutex);
-out_free_spi:
 	hwlock_release(HWLOCK_SPI_ECG);
+	mutex_unlock(&max30001_mutex);
 	return result;
 }
 
@@ -107,15 +95,8 @@ int epic_max30001_disable_sensor(void)
 {
 	int result = 0;
 
-	result = hwlock_acquire(HWLOCK_SPI_ECG, pdMS_TO_TICKS(100));
-	if (result < 0) {
-		return result;
-	}
-
-	if (xSemaphoreTake(max30001_mutex, LOCK_WAIT) != pdTRUE) {
-		result = -EBUSY;
-		goto out_free_spi;
-	}
+	mutex_lock(&max30001_mutex);
+	hwlock_acquire(HWLOCK_SPI_ECG);
 
 	struct stream_info *stream = &max30001_stream;
 	result                     = stream_deregister(SD_MAX30001_ECG, stream);
@@ -134,9 +115,8 @@ int epic_max30001_disable_sensor(void)
 
 	result = 0;
 out_free_both:
-	xSemaphoreGive(max30001_mutex);
-out_free_spi:
 	hwlock_release(HWLOCK_SPI_ECG);
+	mutex_unlock(&max30001_mutex);
 	return result;
 }
 
@@ -298,15 +278,8 @@ static int max30001_fetch_fifo(void)
 {
 	int result = 0;
 
-	result = hwlock_acquire(HWLOCK_SPI_ECG, pdMS_TO_TICKS(100));
-	if (result < 0) {
-		return result;
-	}
-
-	if (xSemaphoreTake(max30001_mutex, LOCK_WAIT) != pdTRUE) {
-		result = -EBUSY;
-		goto out_free_spi;
-	}
+	mutex_lock(&max30001_mutex);
+	hwlock_acquire(HWLOCK_SPI_ECG);
 
 	uint32_t ecgFIFO, readECGSamples, ETAG[32], status;
 	int16_t ecgSample[32];
@@ -344,9 +317,8 @@ static int max30001_fetch_fifo(void)
 		max30001_handle_samples(ecgSample, readECGSamples);
 	}
 
-	xSemaphoreGive(max30001_mutex);
-out_free_spi:
 	hwlock_release(HWLOCK_SPI_ECG);
+	mutex_unlock(&max30001_mutex);
 	return result;
 }
 
@@ -369,24 +341,15 @@ static void max300001_interrupt_callback(void *_)
 
 void max30001_mutex_init(void)
 {
-	max30001_mutex = xSemaphoreCreateMutexStatic(&max30001_mutex_data);
+	mutex_create(&max30001_mutex);
 }
 
 void vMAX30001Task(void *pvParameters)
 {
 	max30001_task_id = xTaskGetCurrentTaskHandle();
 
-	int lockret = hwlock_acquire(HWLOCK_SPI_ECG, pdMS_TO_TICKS(100));
-	if (lockret < 0) {
-		LOG_CRIT("max30001", "Failed to acquire SPI lock!");
-		vTaskDelay(portMAX_DELAY);
-	}
-
-	/* Take Mutex during initialization, just in case */
-	if (xSemaphoreTake(max30001_mutex, 0) != pdTRUE) {
-		LOG_CRIT("max30001", "Failed to acquire MAX30001 mutex!");
-		vTaskDelay(portMAX_DELAY);
-	}
+	mutex_lock(&max30001_mutex);
+	hwlock_acquire(HWLOCK_SPI_ECG);
 
 	/* Install interrupt callback */
 	GPIO_Config(&max30001_interrupt_pin);
@@ -407,20 +370,15 @@ void vMAX30001Task(void *pvParameters)
 	GPIO_Config(&analog_switch);
 	GPIO_OutClr(&analog_switch); // Wrist
 
-	xSemaphoreGive(max30001_mutex);
 	hwlock_release(HWLOCK_SPI_ECG);
+	mutex_unlock(&max30001_mutex);
 
 	/* ----------------------------------------- */
 
 	while (1) {
 		if (max30001_sensor_active) {
 			int ret = max30001_fetch_fifo();
-			if (ret == -EBUSY) {
-				LOG_WARN(
-					"max30001", "Could not acquire mutex?"
-				);
-				continue;
-			} else if (ret < 0) {
+			if (ret < 0) {
 				LOG_ERR("max30001", "Unknown error: %d", -ret);
 			}
 		}

epicardium/modules/modules.h

@@ -97,8 +97,10 @@ enum hwlock_periph {
 	_HWLOCK_MAX,
 };
 
-int hwlock_acquire(enum hwlock_periph p, TickType_t wait);
-int hwlock_release(enum hwlock_periph p);
+int hwlock_acquire_timeout(enum hwlock_periph p, TickType_t wait);
+void hwlock_acquire(enum hwlock_periph p);
+int hwlock_acquire_nonblock(enum hwlock_periph p);
+void hwlock_release(enum hwlock_periph p);
 
 /* ---------- Display ------------------------------------------------------ */
 /* Forces an unlock of the display. Only to be used in Epicardium */
@@ -110,12 +112,10 @@ void disp_forcelock();
 void vBhi160Task(void *pvParameters);
 
 /* ---------- MAX30001 ----------------------------------------------------- */
-#define MAX30001_MUTEX_WAIT_MS          50
 void vMAX30001Task(void *pvParameters);
 void max30001_mutex_init(void);
 
 /* ---------- GPIO --------------------------------------------------------- */
-#define MAX30001_MUTEX_WAIT_MS          50
 extern gpio_cfg_t gpio_configs[];
 
 

epicardium/modules/personal_state.c

@@ -31,7 +31,7 @@ int epic_personal_state_set(uint8_t state, bool persistent)
 	personal_state_persistent = persistent;
 
 	if (was_enabled && !_personal_state_enabled) {
-		while (hwlock_acquire(HWLOCK_LED, pdMS_TO_TICKS(1)) < 0) {
+		while (hwlock_acquire_timeout(HWLOCK_LED, portMAX_DELAY) < 0) {
 			vTaskDelay(pdMS_TO_TICKS(1));
 		}
 
@@ -60,8 +60,8 @@ void vLedTask(void *pvParameters)
 	const int led_animation_rate = 1000 / 25; /* 25Hz -> 40ms*/
 	while (1) {
 		if (_personal_state_enabled) {
-			while (hwlock_acquire(HWLOCK_LED, pdMS_TO_TICKS(1)) <
-			       0) {
+			while (hwlock_acquire_timeout(
+				       HWLOCK_LED, portMAX_DELAY) < 0) {
 				vTaskDelay(pdMS_TO_TICKS(1));
 			}
 

epicardium/modules/pmic.c

@@ -18,7 +18,7 @@
 #include <stdio.h>
 #include <string.h>
 
-#define LOCK_WAIT pdMS_TO_TICKS(1000)
+#define LOCK_WAIT portMAX_DELAY
 
 /* Task ID for the pmic handler */
 static TaskHandle_t pmic_task_id = NULL;
@@ -53,12 +53,12 @@ int pmic_read_amux(enum pmic_amux_signal sig, float *result)
 		return -EINVAL;
 	}
 
-	int adc_ret = hwlock_acquire(HWLOCK_ADC, LOCK_WAIT);
+	int adc_ret = hwlock_acquire_timeout(HWLOCK_ADC, LOCK_WAIT);
 	if (adc_ret < 0) {
 		ret = adc_ret;
 		goto done;
 	}
-	i2c_ret = hwlock_acquire(HWLOCK_I2C, LOCK_WAIT);
+	i2c_ret = hwlock_acquire_timeout(HWLOCK_I2C, LOCK_WAIT);
 	if (i2c_ret < 0) {
 		ret = i2c_ret;
 		goto done;
@@ -75,7 +75,7 @@ int pmic_read_amux(enum pmic_amux_signal sig, float *result)
 	hwlock_release(HWLOCK_I2C);
 
 	vTaskDelay(pdMS_TO_TICKS(5));
-	i2c_ret = hwlock_acquire(HWLOCK_I2C, LOCK_WAIT);
+	i2c_ret = hwlock_acquire_timeout(HWLOCK_I2C, LOCK_WAIT);
 	if (i2c_ret < 0) {
 		ret = i2c_ret;
 		goto done;
@@ -138,7 +138,7 @@ done:
  */
 static uint8_t pmic_poll_interrupts(void)
 {
-	while (hwlock_acquire(HWLOCK_I2C, LOCK_WAIT) < 0) {
+	while (hwlock_acquire_timeout(HWLOCK_I2C, LOCK_WAIT) < 0) {
 		LOG_WARN("pmic", "Failed to acquire I2C. Retrying ...");
 		vTaskDelay(pdMS_TO_TICKS(100));
 	}