ESP32 Forum

ESP32 Official Forum
https://esp32.com/

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

https://esp32.com/viewtopic.php?t=48385

Page 1 of 1

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

Posted: Tue Jul 07, 2026 6:31 pm
by 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
IMG_20260707_194134.jpg (597.77 KiB) Viewed 54 times

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

Posted: Tue Jul 07, 2026 11:54 pm
by 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.

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

Posted: Sat Jul 11, 2026 10:05 am
by 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.

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

Posted: Sat Jul 11, 2026 6:06 pm
by 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
 		}
 	}
 };

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

Posted: Sun Jul 12, 2026 5:42 am
by 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.
All times are UTC
Page 1 of 1
Powered by phpBB® Forum Software © phpBB Limited
https://www.phpbb.com/