My first code ever in C, how do you rate it?

[TryThings]

I know I've used some overkill features like interrupts, but I wanted to experiment.
Initially I hoped that the glitch filter would help me against the bounce of my button, but in the end I had to find an additional workaround, otherwise it often happened that the trigger would fire twice.

The purpose of this project is to explore different features of the ESP32 combined with Espressif's IDE to have two LEDs blink alternatively each second.

So far it works good, except for some occasional double triggers even the DEBOUNCE_MS value set to 50, 70 or 150ms.
I'll probably have to set it even higher, but one day I'd like to find a better solution for this issue, maybe by dropping any attempt to tie interrupts to physical input and just use a while() cycle.


Here is the main code:

Code: Select all

#include <stdio.h>
#include "driver/gpio_filter.h"
#include "freertos/FreeRTOS.h"
#include "freertos/idf_additions.h"
#include "freertos/task.h"
#include "driver/gpio.h"
#include "hal/gpio_types.h"
#include "BlinkCycleFunctions.h"


extern bool is_cycle_running;

void app_main(void)
{
	//create the binary semaphore then check success status
	printf("Generating semaphore..\n");
	BUTTON_semaphore_handle = xSemaphoreCreateBinary();
	
	if(BUTTON_semaphore_handle == NULL){
		printf("Failed to create the semaphore, the program is closed.\n");
		return;
	}
	else{
		printf("Success.\n");
	}
	
	//reset the GPIOs before a fresh session
	printf("Resetting GPIOs for the new session...\n");
	gpio_reset_pin(LED1);
	gpio_reset_pin(LED2);
	gpio_reset_pin(BUTTON);	
	
	//set the LED pins with a "compound literal" struct
	printf("Setting GPIOs...\n");
	gpio_config(&(gpio_config_t){
		.pin_bit_mask = ((1ULL << (int)LED1) | (1ULL << (int)LED2)),
		.mode = GPIO_MODE_OUTPUT,
		.pull_up_en = GPIO_PULLUP_DISABLE,
		.pull_down_en = GPIO_PULLDOWN_DISABLE,
		.intr_type = GPIO_INTR_DISABLE
	});
	
	//set the button interrupt pin with a "compound literal" struct
	gpio_config(&(gpio_config_t){
		.pin_bit_mask = (1ULL << (int)BUTTON),
		.mode = GPIO_MODE_INPUT,
		.pull_up_en = GPIO_PULLUP_ENABLE,
		.pull_down_en = GPIO_PULLDOWN_DISABLE,
		.intr_type = GPIO_INTR_NEGEDGE
	});
	
	//set pin BUTTON to have glitch filter
	gpio_glitch_filter_handle_t BUTTON_glitch_filter_handle;
	gpio_new_pin_glitch_filter(&(gpio_pin_glitch_filter_config_t){
		.clk_src = GLITCH_FILTER_CLK_SRC_DEFAULT,
		.gpio_num = BUTTON
		}, &BUTTON_glitch_filter_handle);
	
	//enable the interrupt
	printf("Enabling the interrupt...\n");
	gpio_intr_enable(BUTTON);
	
	//install ISR (interrupt handler service) with default flag "0"	
	printf("Installing the ISR service...\n");
	gpio_install_isr_service(0);
	
	//add my GPIO to the ISR attaching it to the function it triggers
	printf("Attaching the interrupt to the function...\n");
	gpio_isr_handler_add(BUTTON, BUTTON_interrupt_function, NULL);
	
	
	
	//declare task, attached to previously declared handler
	printf("Preparing the start-stop function task...\n");
	
	xTaskCreate(	
		blink_manager_task,
		"blink_manager",
		2048,
		NULL,
		1,
		&blink_manager_handle
	);
	
	printf("Preparing the blinking cycle task...\n");
	xTaskCreate(
		blink_cycle_task, 
		"blink_cycle", 
		2048, 
		NULL, 
		1, 
		&blink_cycle_handle
	);
}

and the header:

Code: Select all

#ifndef BLINKCYCLEFUNCTIONS_H_
#define BLINKCYCLEFUNCTIONS_H_

#include <stdio.h>
#include "freertos/FreeRTOS.h"
#include "freertos/projdefs.h"
#include "freertos/task.h"
#include "driver/gpio.h"
#include "soc/gpio_num.h"
#include "freertos/semphr.h"


#define LED1 GPIO_NUM_20
#define LED2 GPIO_NUM_21
#define BUTTON GPIO_NUM_10

//milliseconds to wait while the input bounces
#define DEBOUNCE_MS 70

TaskHandle_t blink_cycle_handle = NULL;		//declare handle for blink cycle task
TaskHandle_t blink_manager_handle = NULL;	//declare handle for blink manager task

SemaphoreHandle_t BUTTON_semaphore_handle;	//declare the handle for the semaphore between the interrupt and blink manager task

bool is_cycle_running = 0;	//flag to keep track if the cycle is running or not

//function to blink LEDs
void blink_cycle_task(void * pvParameters){	
	
    printf("Task ready to launch\n");
	vTaskSuspend(NULL); //start task in suspended mode, won't start until the button is pushed for the first time
	
	while (true) {   //blink cycle
		
		gpio_set_level(LED1, 1);		//pin 20 on
		vTaskDelay(pdMS_TO_TICKS(1000));	//delay 1sec
		gpio_set_level(LED1,0);		//pin 20 off
		
		gpio_set_level(LED2, 1);		//pin 21 on
		vTaskDelay(pdMS_TO_TICKS(1000));	//delay 1sec
		gpio_set_level(LED2,0);		//pin 21 off
 		}
};
	
//function that's called when the button interrupt is pressed, resumes or suspends the task based on the flag's status
void BUTTON_interrupt_function(void *arg){
	
	xSemaphoreGiveFromISR(BUTTON_semaphore_handle, NULL);
};

//this task takes in the semaphore from the button interrupt and manages the blinking cycle
void blink_manager_task(void * pvParameters){

  while(1){
		xSemaphoreTake(BUTTON_semaphore_handle, portMAX_DELAY); //xSemaphoreTake suspends the task indefinitely until the semaphore is given
		
		if (is_cycle_running == 0){
			vTaskResume(blink_cycle_handle);
			is_cycle_running = 1;
			printf("Start\n");
			xSemaphoreTake(BUTTON_semaphore_handle, pdMS_TO_TICKS(DEBOUNCE_MS));	//if the bounce of the button raised the semaphore again, it is absorbed
			
		}
		else{
			vTaskSuspend(blink_cycle_handle);
			is_cycle_running = 0;
			printf("Stop\n");
			xSemaphoreTake(BUTTON_semaphore_handle, pdMS_TO_TICKS(DEBOUNCE_MS));	//if the bounce of the button raised the semaphore again, it is absorbed
		}
	}
};


#endif /* BLINKCYCLEFUNCTIONS_H_ */

IMG_20260707_194134.jpg (597.77 KiB) Viewed 119 times

[Sprite]

- For things that you'd expect to always succeed (outside of a bug in the code) you can also use assert() or ESP_CHECK() to test it. It's shorter than a manual if(fail) printf("failed") solution.
- gpio_reset_pin is superfluous, gpio_config already has the same functionality under the hood (iirc gpio_reset_pin calls gpio_config with some default values)
- You're enabling the interrupt before attaching the ISR service. That means there's a small period of time where pressing the button will lead to a panic (ISR called but none installed). You probably want to switch those statements around.
- I did not know the compound literal trick. Gotta remember that, that's useful.
- You can use #ifdef HEADER_NAME_H guards, but nowaday it's more common to replace those #IFDEF/#DEFINE/#ENDIF lines with a single '#pragma once'. No namespace collisions possible and it's a lot shorter.
- Your header is a mess. General rule of thumb: Never put variable declarations in there unless they're 'extern' (and if you feel the need to do that, think really really hard if it's needed), never put function bodies in there unless they're 'static' (and again, think really hard if you need to do that). Reason is that multiple C files can include the same H file, and remember: including in C means the preprocessor effectively copy-pastes the content of the H file into the C file. That means you'll have multiple variables or functions with the same name, making the linker fail.

This is personal, but I'd take a look at your naming scheme as well. E.g. you're calling something BUTTON. It's a definition/constant, so capitals are traditional, that's good. But what does BUTTON contain? As it is right now, it seems to contain the physical button. Probably better to be more precise, and call it e.g. BUTTON_GPIO_NO: it contains the number of the GPIO the button is connected to. You then seem to take the fact that BUTTON is written in all caps and propagate that to e.g. BUTTON_interrupt_function. That has nothing to do with the fact that BUTTON is a constant, though, so there's no need to write that in caps. You seem to a use lower-case underscore style for functions, so that would be button_interrupt_function instead (and notice how you are being properly descriptive in what it is in this name: the function called on the interrupt for the button)

Your C file generally is decent, but I would read up a bit more on what headers (.h files) do if I were you. They're the interfaces to your code and generally only contain the function declarations (not the definitions, in other words, there's no actual code in your header file) as well as any type definitions (structs, typedefs etc) needed to interface with that code, and if you really need to, also extern variable declarations. There are exceptions (e.g. static inline functions for quick accessors or utility functions) but if you're a beginner I'd forget those exist for now.

[TryThings]

- For things that you'd expect to always succeed (outside of a bug in the code) you can also use assert() or ESP_CHECK() to test it. It's shorter than a manual if(fail) printf("failed") solution.
- gpio_reset_pin is superfluous, gpio_config already has the same functionality under the hood (iirc gpio_reset_pin calls gpio_config with some default values)...

Hello, thank you very much for the detailed feedback, I'm working to implement all the improvements you've recommended.

In the meantime, I'm having issues with the #define macros, since I've split

Code: Select all

BlinkCycleFunctions.c

and

Code: Select all

BlinkCycleFunctions.h

but now my main.c doesn't recognize stuff like BUTTON, LED1, LED2 etc. anymore.

What should I do? re-#define-ing all these also in main.c seems not the right way.

[TryThings]

- For things that you'd expect to always succeed (outside of a bug in the code) you can also use assert() or ESP_CHECK() to test it. It's shorter than a manual if(fail) printf("failed") solution.
- gpio_reset_pin is superfluous, gpio_config already has the same functionality under the hood (iirc gpio_reset_pin calls gpio_config with some default values)
- You're enabling the interrupt before attaching the ISR service. That means there's a small period of time where pressing the button will lead to a panic (ISR called but none installed). You probably want to switch those statements around.
- I did not know...

I have an UPDATE.

I have separated the declaration and definition files (.h and .c) for my functions.
Initially I had issues when I kept all the declaration for semaphores and handles in the .h files, as the compiler recognized them as being defined twice although I even had the #ifndef guards etc.

So I've placed them in the .c file and re-declared them again under main.c using the "extern" specifier.

Here is the updated main.c:

Code: Select all

#include <stdio.h>
#include "driver/gpio_filter.h"
#include "freertos/FreeRTOS.h"
#include "freertos/idf_additions.h"
#include "freertos/task.h"
#include "driver/gpio.h"
#include "hal/gpio_types.h"
#include "BlinkCycleFunctions.h"


extern TaskHandle_t blink_cycle_handle;
extern TaskHandle_t blink_manager_handle;
extern SemaphoreHandle_t BUTTON_semaphore_handle;

extern bool is_cycle_running;

void app_main(void)
{
	//create the binary semaphore then check success status
	printf("Generating semaphore..\n");
	BUTTON_semaphore_handle = xSemaphoreCreateBinary();
	
	if(BUTTON_semaphore_handle == NULL){
		printf("Failed to create the semaphore, the program is closed.\n");
		return;
	}
	else{
		printf("Success.\n");
	}
	
	//reset the GPIOs before a fresh session
	printf("Resetting GPIOs for the new session...\n");
	gpio_reset_pin(LED1);
	gpio_reset_pin(LED2);
	gpio_reset_pin(BUTTON);	
	
	//set the LED pins with a "compound literal" struct
	printf("Setting GPIOs...\n");
	gpio_config(&(gpio_config_t){
		.pin_bit_mask = ((1ULL << (int)LED1) | (1ULL << (int)LED2)),
		.mode = GPIO_MODE_OUTPUT,
		.pull_up_en = GPIO_PULLUP_DISABLE,
		.pull_down_en = GPIO_PULLDOWN_DISABLE,
		.intr_type = GPIO_INTR_DISABLE
	});
	
	//set the button interrupt pin with a "compound literal" struct
	gpio_config(&(gpio_config_t){
		.pin_bit_mask = (1ULL << (int)BUTTON),
		.mode = GPIO_MODE_INPUT,
		.pull_up_en = GPIO_PULLUP_ENABLE,
		.pull_down_en = GPIO_PULLDOWN_DISABLE,
		.intr_type = GPIO_INTR_NEGEDGE
	});
	
	//set pin BUTTON to have glitch filter
	gpio_glitch_filter_handle_t BUTTON_glitch_filter_handle;
	gpio_new_pin_glitch_filter(&(gpio_pin_glitch_filter_config_t){
		.clk_src = GLITCH_FILTER_CLK_SRC_DEFAULT,
		.gpio_num = BUTTON
		}, &BUTTON_glitch_filter_handle);
	
	//install ISR (interrupt handler service) with default flag "0"	
	printf("Installing the ISR service...\n");
	gpio_install_isr_service(0);
	
	//add my GPIO to the ISR attaching it to the function it triggers
	printf("Attaching the interrupt to the function...\n");
	gpio_isr_handler_add(BUTTON, BUTTON_interrupt_function, NULL);
	
	//enable the interrupt
	printf("Enabling the interrupt...\n");
	gpio_intr_enable(BUTTON);
	
	//declare task, attached to previously declared handler
	printf("Preparing the start-stop function task...\n");
	
	xTaskCreate(	
		blink_manager_task,
		"blink_manager",
		2048,
		NULL,
		1,
		&blink_manager_handle
	);
	
	printf("Preparing the blinking cycle task...\n");
	xTaskCreate(
		blink_cycle_task, 
		"blink_cycle", 
		2048, 
		NULL, 
		1, 
		&blink_cycle_handle
	);
}

Then the updated BlinkCycleFunctions.h:

Code: Select all

#ifndef BLINKCYCLEFUNCTIONS_H_
#define BLINKCYCLEFUNCTIONS_H_


#define LED1 GPIO_NUM_20
#define LED2 GPIO_NUM_21
#define BUTTON GPIO_NUM_10

//milliseconds to wait while the input bounces
#define DEBOUNCE_MS 150

void blink_cycle_task(void * pvParameters); //Functions to blink LEDs

void BUTTON_interrupt_function(void *arg);  //function that's called when the button interrupt is pressed, resumes or suspends the task based on the flag's status

void blink_manager_task(void * pvParameters); 

#endif /* BLINKCYCLEFUNCTIONS_H_ */

Plus the new BlinkCycleFunctions.c:

Code: Select all

#include <stdio.h>
#include "freertos/FreeRTOS.h"
#include "freertos/projdefs.h"
#include "freertos/task.h"
#include "driver/gpio.h"
#include "soc/gpio_num.h"
#include "freertos/semphr.h"
#include "BlinkCycleFunctions.h"

TaskHandle_t blink_cycle_handle = NULL;		//declare handle for blink cycle task
TaskHandle_t blink_manager_handle = NULL;	//declare handle for blink manager task

SemaphoreHandle_t BUTTON_semaphore_handle;	//declare the handle for the semaphore between the interrupt and blink manager task

bool is_cycle_running = 0;	//flag to keep track if the cycle is running or not


 //function to blink LEDs
 void blink_cycle_task(void * pvParameters){	
 	
     printf("Task ready to launch\n");
 	 vTaskSuspend(NULL); //start task in suspended mode, won't start until the button is pushed for the first time
 	
 	while (true) {   //blink cycle
 		
 		gpio_set_level(LED1, 1);		//pin 20 on
 		vTaskDelay(pdMS_TO_TICKS(1000));	//delay 1sec
 		gpio_set_level(LED1,0);		//pin 20 off
 		
 		gpio_set_level(LED2, 1);		//pin 21 on
 		vTaskDelay(pdMS_TO_TICKS(1000));	//delay 1sec
 		gpio_set_level(LED2,0);		//pin 21 off
  		}
 };
 	
 //function that's called when the button interrupt is pressed, resumes or suspends the task based on the flag's status
 void BUTTON_interrupt_function(void *arg){
 	
 	xSemaphoreGiveFromISR(BUTTON_semaphore_handle, NULL);
 };

 //this task takes in the semaphore from the button interrupt and manages the blinking cycle
 void blink_manager_task(void * pvParameters){

   while(1){
 		xSemaphoreTake(BUTTON_semaphore_handle, portMAX_DELAY); //xSemaphoreTake suspends the task indefinitely until the semaphore is given
 		
 		if (is_cycle_running == 0){
 			vTaskResume(blink_cycle_handle);
 			is_cycle_running = 1;
 			printf("Start\n");
 			xSemaphoreTake(BUTTON_semaphore_handle, pdMS_TO_TICKS(DEBOUNCE_MS));	//if the bounce of the button raised the semaphore again, it is absorbed
 			
 		}
 		else{
 			vTaskSuspend(blink_cycle_handle);
 			is_cycle_running = 0;
 			printf("Stop\n");
 			xSemaphoreTake(BUTTON_semaphore_handle, pdMS_TO_TICKS(DEBOUNCE_MS));	//if the bounce of the button raised the semaphore again, it is absorbed
 		}
 	}
 };

[Sprite]

That looks a lot better. One more remark, but it's more 'advanced': it's not necessarily useful now but it'll help you when your projects get bigger. You ideally want to separate the interface from the implementation, and you're not really doing that. What I mean by that is that in the .h file, you describe functions that map to *what* you want to do, without 'leaking through' *how* the .c code does it.

What I mean is that at this particular point, you're 'leaking' that the C file uses a task in order to blink the LED (and external code needs to initialize that task). Now, if you want to change this out to use e.g. an ESP-Timer, or maybe even a full hardware implementation, you can't just change the .c file: you need to track down every single instance in your code where you're turning the blinking on and off and change that to whatever the new way is, as it doesn't use a task anymore.

Instead, you could have a BlinkCycleFunctions.h file exposing the following interface:

Code: Select all

void blink_cycle_init();
void blink_cycle_enable_blink();
void blink_cycle_disable_blink();

You'd then move all the implementation details into the .c file:

- blink_cycle_init would call the xTaskCreate() function to start the blink_cycle_task task. As blink_cycle_task is not used anymore outside of BlinkCycleFunctions.c, you don't need to include it in the .h file.
- blink_cycle_enable_blink()/blink_cycle_disable_blink() (or you could put that into one function, with an argument stating if you want to enable/disable blinking) will do the vTaskSuspend/vTaskResume. As nothing else needs blink_cycle_handle, you can move it from your main.c into your BlinkCycleFunctions.c. No need to also have it in the header, as as nothing else needs it.
- You could even put the GPIO initialization in blink_cycle_start if you wanted to, that's up to you.

The thing is that organizing it in this way (a header that has functions for the effect you need the code to have, rather than for what the code does) means you can swap out implementations easily. For instance, I think you can use the LEDC controller to blink a LED entirely without CPU involvement, If your API looks like I described above, it's just a change of the BlinkCycleFunctions.c content, the rest stays exactly the same.

[TryThings]

That looks a lot better. One more remark, but it's more 'advanced': it's not necessarily useful now but it'll help you when your projects get bigger. You ideally want to...

I think I understand what you mean, this is basically what Espressif did with their API no?
For example allowing me to use the xTaskCreate function once I include the relative header, without however showing me what's inside the .c file or how does xTaskCreate do its magic.

I'm currently working to rework my code in such a manner, I'm almost finished but there are some problems arising and I'm also implementing some new stuff, nothing too crazy though.

I'm sending the new code once it's ready

[TryThings]

That looks a lot better. One more remark, but it's more 'advanced': it's not necessarily useful now but it'll help you when your projects get bigger. You ideally want to separate the interface from the implementation, and you're not really doing that. What I mean by that is that in the .h file, you describe...

Hello again,

I have re-written the code like how you recommended, but now there is a weird bug that sends the board in a panic loop.

I've tired to fix the issue myself for a long time but I can't get my head around it...

main.c

Code: Select all

#include <stdio.h>
#include "driver/gpio.h"
#include "driver/gpio_filter.h"
#include "freertos/FreeRTOS.h"
#include "freertos/idf_additions.h"
#include "freertos/projdefs.h"
#include "freertos/task.h"
#include "hal/gpio_types.h"
#include "BlinkCycleFunctions.h"
#include "portmacro.h"
#include "soc/gpio_num.h"


//example of user's implementation
SemaphoreHandle_t enabler_button_semaphore;

struct enabler_args{
	
	cycle_tasks_handle_t *cycle_handle;
	gpio_num_t LED;
};


void enabler_interrupt_function(void *pvParameters){
		
		xSemaphoreGiveFromISR(enabler_button_semaphore, NULL);
	};	

	
void enabler_function(void *pvParameters){
	
	struct enabler_args *args = pvParameters;
	
	bool status = 0; //0 is disabled, 1 is enabled
	
	while(1){
		
		xSemaphoreTake(enabler_button_semaphore, portMAX_DELAY);	//wait until the interrupt is triggered
		
		if(status == 1){
			
			blink_cycle_disable(args->cycle_handle);
			status = 0;
			gpio_set_level(args->LED, 0);
			printf("Cycles are disabled\n");
			xSemaphoreTake(enabler_button_semaphore, pdMS_TO_TICKS(250));
		}
		
		else{
			
			blink_cycle_enable(args->cycle_handle);
			status = 1;
			gpio_set_level(args->LED, 1);
			printf("Cycles are enabled\n");
			xSemaphoreTake(enabler_button_semaphore, pdMS_TO_TICKS(250));
		}
	}
};



void app_main(void)
{
	
	#define LED1 GPIO_NUM_20
	#define LED2 GPIO_NUM_21
	#define BUTTON GPIO_NUM_10
	#define DEBOUNCE_MS 250

	#define ENABLER_BUTTON GPIO_NUM_0
	#define ENABLE_STATUS_LED GPIO_NUM_1
	
	/*
	install ISR (interrupt handler service) with default flag "0"	
	it's a system-level call so I've decided to keep it in main.c, out of my init function
	*/
	
	printf("Installing the ISR service...\n");
	gpio_install_isr_service(0);
	
	
	//I'm not planning to include the following pins into the other .c file, as these are supposed to be an example of the user's implementation
	printf("Setting initial GPIOs...\n");
	
	gpio_config(&(gpio_config_t){
		.pin_bit_mask = (1ULL << (int)ENABLER_BUTTON),
		.mode = GPIO_MODE_INPUT,
		.pull_up_en = GPIO_PULLUP_ENABLE,
		.pull_down_en = GPIO_PULLDOWN_DISABLE,
		.intr_type = GPIO_INTR_NEGEDGE
	});
	

	gpio_config(&(gpio_config_t){
		.pin_bit_mask = (1ULL << (int)ENABLE_STATUS_LED),
		.mode = GPIO_MODE_OUTPUT,
		.pull_up_en = GPIO_PULLUP_DISABLE,
		.pull_down_en = GPIO_PULLDOWN_DISABLE,
		.intr_type = GPIO_INTR_DISABLE
	});
	
	printf("Done\n");
	
	gpio_glitch_filter_handle_t enabler_glitch_filter_handle;
	
	
	gpio_new_pin_glitch_filter(&(gpio_pin_glitch_filter_config_t){
		.clk_src = GLITCH_FILTER_CLK_SRC_DEFAULT,
		.gpio_num = ENABLER_BUTTON
		}, &enabler_glitch_filter_handle);
		
	
		
	gpio_isr_handler_add(ENABLER_BUTTON, enabler_interrupt_function, NULL);
	gpio_intr_enable(ENABLER_BUTTON);
	
	
	cycle_tasks_handle_t cycle_task;		
	
	struct enabler_args EnArgs = {.cycle_handle = &cycle_task, .LED = ENABLE_STATUS_LED};
	
	
	//initialize all the necessary PINs
	blink_cycle_init(LED1, LED2, BUTTON, DEBOUNCE_MS, &cycle_task);
	
	
	//start the task which will enable of disable the cycles (multiple cycles support available in the future)
	TaskHandle_t enabler_task_handle = NULL;
	
	xTaskCreate(
		enabler_function,
		"enabler_function",
		2048,
		&EnArgs,   //can be (I think) replaced with an array of cycle handles in case I have two or more separate cycles
		1,
		&enabler_task_handle
	);
	
}

then BlinkCycleFunctions.h:

Code: Select all

#ifndef BLINKCYCLEFUNCTIONS_H_
#define BLINKCYCLEFUNCTIONS_H_

#include <stdio.h>
#include "freertos/FreeRTOS.h"
#include "freertos/projdefs.h"
#include "freertos/task.h"
#include "driver/gpio.h"
#include "soc/gpio_num.h"
#include "freertos/semphr.h"


//type definition of an anonymous struct containing all the stuff I need, named cycle_task_handle, so I don't have to type "struct" every time when using the handle
typedef struct {
	
	TaskHandle_t cycle_handle;
	TaskHandle_t manager_handle;
	SemaphoreHandle_t button_semaphore_handle;	//declare the handle for the semaphore between the interrupt and blink manager task
	gpio_num_t LED1;
	gpio_num_t LED2;
	gpio_num_t BUTTON;
	int DEBOUNCE_MS;
	bool is_cycle_running;	//flag to keep track if the cycle is running or not
	bool is_cycle_enabled;	//flag to keep track of the enable/disabled state specifically for the blink cycle
	
} cycle_tasks_handle_t;

//function to initialize a pair of LEDs plus a button with its interrupt, the order is: first LED, second LED, button
void blink_cycle_init(gpio_num_t, gpio_num_t, gpio_num_t, int, cycle_tasks_handle_t *);

void blink_cycle_enable(cycle_tasks_handle_t *);

void blink_cycle_disable(cycle_tasks_handle_t *);

/*
void blink_cycle_task(void * pvParameters); //Functions to blink LEDs

void BUTTON_interrupt_function(void *arg);  //function that's called when the button interrupt is pressed, resumes or suspends the task based on the flag's status

void blink_manager_task(void * pvParameters); 
*/

#endif /* BLINKCYCLEFUNCTIONS_H_ */

and finally BlinkCycleFunctions.c:

Code: Select all

#include <stdio.h>
#include "freertos/FreeRTOS.h"
#include "freertos/projdefs.h"
#include "freertos/task.h"
#include "driver/gpio.h"
#include "soc/gpio_num.h"
#include "freertos/semphr.h"
#include "BlinkCycleFunctions.h"
#include "driver/gpio_filter.h"




//function to give the semaphore to the cycle manager
void BUTTON_interrupt_function(void *arg){
	
	cycle_tasks_handle_t *handle = (cycle_tasks_handle_t *) arg;
	
	xSemaphoreGiveFromISR(handle->button_semaphore_handle, NULL);
};


//function to blink LEDs
void blink_cycle_task(void *pvParameters){	

	cycle_tasks_handle_t *handle = pvParameters;
	
	printf("Cycle task ready to launch\n");
	vTaskSuspend(NULL); //start task in suspended mode, won't start until the button is pushed for the first time
 	
 	while (true) {   //blink cycle		
 		gpio_set_level(handle->LED1, 1);		//LED 1 on
 		vTaskDelay(pdMS_TO_TICKS(1000));	//delay 1sec
 		gpio_set_level(handle->LED1,0);		//LED 1 off
 		
 		gpio_set_level(handle->LED2, 1);		//LED 2 on
 		vTaskDelay(pdMS_TO_TICKS(1000));	//delay 1sec
 		gpio_set_level(handle->LED2,0);		//LED 2 off
  		}
};

 
 //function to start/stop the blink cycle, receives the semaphore from the interrupt
void blink_manager_task(void *pvParameters){

	cycle_tasks_handle_t *handle = pvParameters;

	printf("Blink manager ready to be enabled\n");
	
	vTaskSuspend(NULL);  //start in suspended mode
	
	while(1){
 		xSemaphoreTake(handle->button_semaphore_handle, portMAX_DELAY); //xSemaphoreTake suspends the task indefinitely until the semaphore is given
 		
 		if (handle->is_cycle_running == 0){
 			vTaskResume(handle->cycle_handle);
 			handle->is_cycle_running = 1;
 			printf("Start\n");
 			xSemaphoreTake(handle->button_semaphore_handle, pdMS_TO_TICKS(handle->DEBOUNCE_MS));	//if the bounce of the button raised the semaphore again, it is absorbed
 			
 		}
 		else{
 			vTaskSuspend(handle->cycle_handle);
 			handle->is_cycle_running = 0;
 			printf("Stop\n");
 			xSemaphoreTake(handle->button_semaphore_handle, pdMS_TO_TICKS(handle->DEBOUNCE_MS));	//if the bounce of the button raised the semaphore again, it is absorbed
 		}
 	}
};
 
 

//function to pass to main.c in order to initialize the handles, tasks, pins etc...
void blink_cycle_init(gpio_num_t LED1_func, gpio_num_t LED2_func, gpio_num_t BUTTON_func, int DEBOUNCE_MS_func, cycle_tasks_handle_t *handle){ // writing "void *variable" works too and I guess I could pass to pass straight ints too that way, but I like this method more

	//Saving the data set by the user into the handle	
	handle->LED1 = LED1_func;
	handle->LED2 = LED2_func;
	handle->BUTTON = BUTTON_func;
	handle->DEBOUNCE_MS = DEBOUNCE_MS_func;	
	
	handle->cycle_handle = NULL;
	handle->manager_handle = NULL;
	
	//create the binary semaphore then check success status
	printf("Generating cycle's semaphore...\n");
	handle->button_semaphore_handle = xSemaphoreCreateBinary();
	
	if(handle->button_semaphore_handle == NULL){
		printf("Failed to create the semaphore, the program is closed.\n");
		return;
	}
	else{
		printf("Success.\n");
	}
	
	/*set the LED pins with a "compound literal" struct
	
		From now on i could've kept using LED1_func, LED2_func and BUTTON_func,
		but I've decided to instead replace them with the struct pointers in case
		I wanted to add new features in the future, like being able to edit the pins
		while the code is running.
		It's an useless feature, but the whole code is useless anyway and it's just meant
		as an exercise to try out the ESP32's features
	*/
	
	printf("Setting cycle's GPIOs...\n");
	gpio_config(&(gpio_config_t){
		.pin_bit_mask = ((1ULL << (int)handle->LED1) | (1ULL << (int)handle->LED2)),
		.mode = GPIO_MODE_OUTPUT,
		.pull_up_en = GPIO_PULLUP_DISABLE,
		.pull_down_en = GPIO_PULLDOWN_DISABLE,
		.intr_type = GPIO_INTR_DISABLE
	});
	
	
	//set the button interrupt pin with a "compound literal" struct
	gpio_config(&(gpio_config_t){
		.pin_bit_mask = (1ULL << (int)handle->BUTTON),
		.mode = GPIO_MODE_INPUT,
		.pull_up_en = GPIO_PULLUP_ENABLE,
		.pull_down_en = GPIO_PULLDOWN_DISABLE,
		.intr_type = GPIO_INTR_NEGEDGE
	});
	
	
	//declare the handle for the button's glitch filter, it won't be touched ever again after init so no point including it in the handle
	gpio_glitch_filter_handle_t button_glitch_filter_handle;
	
	//set pin BUTTON to have glitch filter
	gpio_new_pin_glitch_filter(&(gpio_pin_glitch_filter_config_t){
		.clk_src = GLITCH_FILTER_CLK_SRC_DEFAULT,
		.gpio_num = handle->BUTTON
		}, &button_glitch_filter_handle);
	
	
	//add my GPIO to the ISR attaching it to the function it triggers
	printf("Attaching the interrupt to the function...\n");
	gpio_isr_handler_add(handle->BUTTON, BUTTON_interrupt_function, &handle);
	
	//enable the interrupt
	printf("Enabling the interrupt...\n");
	gpio_intr_enable((int)handle->BUTTON);
	
	
	handle->is_cycle_running = 0;	//sets the flag to zero, because the blinking task starts in suspended mode
	handle->is_cycle_enabled = 0;	//sets flag to zero because the tasks have to wait the enabling function
	
	
	//start the two leading functions (in suspended mode), until the blink_cycle_enable will be called
	printf("Creating tasks...\n");
	
	xTaskCreate(
		blink_cycle_task,
		"blink_cycle",
		2048,
		handle,
		1,
		&handle->cycle_handle
	);
	
	xTaskCreate(
		blink_manager_task,
		"blink_manager",
		2048,
		handle,
		1,
		&handle->manager_handle
	);
	
	printf("Blink and Manager tasks created\n");
	
}


/*
Down here I've initially planned to make the cycle be affected by enable/disable too
but now I've changed idea, it's pointless since the cycle itself is already paused/resumed by the button.
The enable/disable will then only allow/impede the user to touch the cycle manager button
*/


void blink_cycle_enable(cycle_tasks_handle_t * handle){
	
	/*
	if(handle->is_cycle_running == 1){
		
		//the cycle could be "enabled", but only starts if the button press has allowed it
		vTaskResume(handle->cycle_handle);
		
	}
	*/
	
	vTaskResume(handle->manager_handle);
	
	printf("The functionality is now enabled\n");
	
	
 };
 	
 
void blink_cycle_disable(cycle_tasks_handle_t * handle){
	
	vTaskSuspend(handle->manager_handle);
	
	/*
	//always suspend the cycle too, but the status flag is untouched, so the cycle is ready to be re-enabled
	vTaskSuspend(handle->cycle_handle);
	*/
}

This is the error I'm getting, and I don't really know what to do with it:

Code: Select all

I (252) main_task: Calling app_main()
Installing the ISR service...
Setting initial GPIOs...
Done
Generating cycle's semaphore...
Success.
Setting cycle's GPIOs...
Attaching the interrupt to the function...
Enabling the interrupt...
Creating tasks...
Cycle task ready to launch
Blink manager ready to be enabled
Blink and Manager tasks created

assert failed: xQueueSemaphoreTake queue.c:1709 (( pxQueue ))
Core  0 register dump:
MEPC    : 0x40383580  RA      : 0x4038353e  SP      : 0x3fc923c0  GP      : 0x3fc8c600  
--- 0x40383580: panic_abort at /home/anatoliy/.espressif/v5.5.3/esp-idf/components/esp_system/panic.c:491
--- 0x4038353e: esp_vApplicationTickHook at /home/anatoliy/.espressif/v5.5.3/esp-idf/components/esp_system/freertos_hooks.c:31
TP      : 0x3fc92560  T0      : 0x37363534  T1      : 0x7271706f  T2      : 0x33323130  
S0/FP   : 0x0000008a  S1      : 0x00000001  A0      : 0x3fc923fc  A1      : 0x3fc8cb59  
A2      : 0x00000001  A3      : 0x00000029  A4      : 0x00000001  A5      : 0x3fc8e000  
A6      : 0x7a797877  A7      : 0x76757473  S2      : 0x00000009  S3      : 0x3fc9250d  
S4      : 0x3fc8cb58  S5      : 0x00000000  S6      : 0x00000000  S7      : 0x00000000  
S8      : 0x00000000  S9      : 0x00000000  S10     : 0x00000000  S11     : 0x00000000  
T3      : 0x6e6d6c6b  T4      : 0x6a696867  T5      : 0x66656463  T6      : 0x62613938  
MSTATUS : 0x00001881  MTVEC   : 0x40380001  MCAUSE  : 0x00000002  MTVAL   : 0x00000000  
--- 0x40380001: _vector_table at /home/anatoliy/.espressif/v5.5.3/esp-idf/components/riscv/vectors_intc.S:54
MHARTID : 0x00000000  

Inititally I thought there was an issue wiht my semaphores, and it probably is, but I've tinkered with the debug tool and although I don't understand the stuff going on inside it, it's clear that the value of my semaphore is changing, it's not NULL.
Thus I don't know why I'm getting this error, could there be an issue with my pointers to the struct?

[Sprite]

Is that all the backtrace the panic is giving you? Normally it gives you a nice stack dump leading to the call path that triggered it.

Regardless, there is something NULL in one of your semaphores. Usually, something being NULL means A. it hasn't been initialized yet, or B. it has been free'ed and zero'ed. Given that you don't free semaphores in your code, I'd take a deep hard look at all semaphores you declare in your code (I can see two) and check if A. they're initialized at all, and if so, B if they're initialized before they are used.

[TryThings]

Is that all the backtrace the panic is giving you? Normally it gives you a nice stack dump leading to the call path that triggered it.

Regardless, there is something NULL in one of your semaphores. Usually, something being NULL means A. it hasn't been initialized yet, or B. it has been free'ed and zero'ed. Given that you don't free semaphores in your code, I'd take a deep hard look at all semaphores you declare in your code (I can see two) and check if A. they're initialized at all, and if so, B if they're initialized before they are used.

The error log was way longer so I only included that first part.

Anyway, I've (forgive me) used AI to help me debug, as I'm still inexpert and many things can pass unnoticed.
The AI pointed out how, before I could press any buttons, the app_main() returns, like in the picture.

Screenshot_20260803_193222.png (17.82 KiB) Viewed 26 times

The AI also told me how the app_main() always returns once finished, leaving the tasks running by themselves, this also means that any variable with scope in app_main() gets cleared once it returns, is that correct?
Anyway, the AI told me that this is easily fixed by adding

Code: Select all

static

before the variables I need, namely the handlers and other structs that contains all my pointers and values.

Is this something that's commonly done by developers, a good habit/solution? Can I keep doing things like this or will it cause trouble when I get more advanced?.

Next, the above solved only a part of the issues, another bug I found was caused by the fact I forgot to run xSemaphoreCreateBinary() for my second semaphore, the one that enables the manager function, that was an easy fix.

Lastly, I need your explanation here.
The last bug before stuff started running smoothly was caused by the fact I passed an address for the handle instead of the handle itself here:

Code: Select all

gpio_isr_handler_add(handle->BUTTON, BUTTON_interrupt_function, handle);    //here I had "&handle" before I corrected it with "handle"

The code started working correctly after I removed the &.
I don't understand however why I'm not having this issue with:

Code: Select all

	xTaskCreate(
		blink_cycle_task,
		"blink_cycle",
		2048,
		handle,
		1,
		&handle->cycle_handle   //here I use "&" too to pass an address instead of a copy of the handle-struct
	);

Reading the API reference guides, I can see how both the handler_add and the TaskCreate take a pointer-to-something as the last argument, so why does one need "&" while the other causes panic if I add it?

[Sprite]

The AI also told me how the app_main() always returns once finished, leaving the tasks running by themselves, this also means that any variable with scope in app_main() gets cleared once it returns, is that correct?

Not cleared. Those variables are part of the stack, and when a task exits that stack is handed back to the heap allocator, to be used for whatever decides to allocate a chunk of memory and happens to get it next. While it's possible that it subsequently gets cleared (e.g. using calloc), it's just as likely that it immediately gets overwritten with non-zero data. I'd generally not expect a clean NULL dereference ift that was the case.

Anyway, the AI told me that this is easily fixed by adding

Code: Select all

static

before the variables I need, namely the handlers and other structs that contains all my pointers and values.

Is this something that's commonly done by developers, a good habit/solution? Can I keep doing things like this or will it cause trouble when I get more advanced?.

It depends where the actual data as used by the drivers or subsystems or whatever lives. If the variable you declare contains the actual data as used, you need to keep that (and e.g. declaring it static works), but if it just contains a pointer or handle, you can loose it without issues as nothing needs the pointer or handle. Generally, you can distinguish the two by how you'd call a non-initialization function:

Code: Select all

some_var t;
initialize_thing(&t);
do_thing_with_thing(t, 123);

Whatever t points at (or whatever handle t contains) is passed on into the api, but not the memory location of t itself. This means t is a pointer or handle, you can let it go out of scope safely. Compare to:

Code: Select all

some_var t;
initialize_thing(&t);
do_thing_with_thing(&t, 123);

A pointer to t gets passed to all functions, meaning the memory at t is actively used by the API. Letting t go out of scope = stuff might break.

Declaring it as static is one method, but it's a bit iffy. It works for a function that only executes once, like app_main(), but as soon as you use it in a function that can get called multiple times, the static value will get overwritten. It'd be more common to still make it static but take it out of the function: that way you can't confuse it with a non-static stack variable (as it's not declared in a function anymore) but it won't pollute the global namespace (if you have another global with the same name in a different C file, they don't bite eachother). Another advantage is that you don't loose access to the thing alltogether: a different function in the same C file can still use it. To illustrate: Instead of this:

Code: Select all

void app_main() {
	static some_type my_variable;
	do_something(&my_variable);
}

do this:

Code: Select all

static some_type my_variable;
void app_main() {
	do_something(&my_variable);
}

Lastly, I need your explanation here.
The last bug before stuff started running smoothly was caused by the fact I passed an address for the handle instead of the handle itself here:

Code: Select all

gpio_isr_handler_add(handle->BUTTON, BUTTON_interrupt_function, handle);    //here I had "&handle" before I corrected it with "handle"

The code started working correctly after I removed the &.
I don't understand however why I'm not having this issue with:

Code: Select all

	xTaskCreate(
		blink_cycle_task,
		"blink_cycle",
		2048,
		handle,
		1,
		&handle->cycle_handle   //here I use "&" too to pass an address instead of a copy of the handle-struct
	);

Reading the API reference guides, I can see how both the handler_add and the TaskCreate take a pointer-to-something as the last argument, so why does one need "&" while the other causes panic if I add it?

Yep, that's C pointer madness at its worst, because both functions are typed to accept 'void*' meaning you can stash any pointer in there and the compiler will happily accept it without being able to warn you if you do it wrong. The mental image I have with APIs like this is that the 'void*' argument is effectively a way to send a 32-bit value, usually a pointer, over to the other side (e.g. from the function that creates the task to the task function, or from the function that installs the ISR to the ISR function. You can even use it as such:

Code: Select all

void my_task(void* arg) {
	printf("Number is %d\n", (int)arg);
}
void app_main() {
	xTaskCreate(my_task, ...., (void*)123456);
}

will happily print out 123456.

Read it like this, and you'll see that your gpio_isr_handler_add() is passing the value of 'handle' over, while the xTaskCreate is passing the address where handle->cycle_handle is stored over.

Now, none of those two have to be wrong per se, it depends on what you want to do, but you need to handle the thing properly on the other side. If it's an address, you can dereference it (or feed it into a function that expects an address), if it's a value you should treat it as such.

Code: Select all

void my_function(...., void *arg) {
	my_handle_t *handle=(my_handle_t*)arg; //will work if arg is a pointer to a handle
	my_handle_t handle=*((my_handle_t)arg); //might also work if arg is a pointer to a handle, crashes if it's the handle itself
	my_handle_t handle=arg; //works if arg is the handle itself, fails if it's a pointer
}

When you normally should use what again differs on what you're sending. If it's something that can be contained in a 32-bit field, so a pointer or a handle, you can send it over directly. If it's something larger, like a struct, you need to send a pointer to it and make sure that the underlying memory doesn't go out of scope.