NeoPixel Peripheral Memory Safety Improvements

DashPanel/Application/Src/dashutils.c

@@ -4,8 +4,8 @@
 #include "gr_neopixel.h"
 #include "main.h"
 
-NeopixelContext *NeoPixel_LED_Context;
-NeopixelContext *NeoPixel_Button_Context;
+NeopixelContext NeoPixel_LED_Context;
+NeopixelContext NeoPixel_Button_Context;
 
 static Neopixel_Color LED_colors[NUM_LEDS];
 static Neopixel_Color button_colors[NUM_BUTTONS];
@@ -43,8 +43,7 @@ void Neopixel_LEDWrite()
 	LED_colors[1] = (dashStatus.led_bits & 0x02) ? COLOR_RED : COLOR_OFF;
 	LED_colors[2] = (dashStatus.led_bits & 0x03) ? COLOR_RED : COLOR_OFF;
 
-	Neopixel_WriteAll(NeoPixel_LED_Context, LED_colors, sizeof(LED_colors));
-	// LOGOMATIC("LED Flashing\n");
+	Neopixel_WriteAll(&NeoPixel_LED_Context, LED_colors, sizeof(LED_colors));
 	return;
 }
 
@@ -80,6 +79,6 @@ void Neopixel_ButtonWrite()
 			break;
 	}
 
-	Neopixel_WriteAll(NeoPixel_Button_Context, button_colors, sizeof(button_colors));
+	Neopixel_WriteAll(&NeoPixel_Button_Context, button_colors, sizeof(button_colors));
 	return;
 }

DashPanel/Core/Inc/main.h

@@ -61,7 +61,7 @@ extern "C" {
 
 /* Exported macro ------------------------------------------------------------*/
 /* USER CODE BEGIN EM */
-
+#define GR_NEOPIXEL_MAX_LEDS (3)
 /* USER CODE END EM */
 
 /* Exported functions prototypes ---------------------------------------------*/

G4NEOTESTING/Core/Inc/main.h

@@ -62,7 +62,7 @@ extern "C" {
 
 /* Exported macro ------------------------------------------------------------*/
 /* USER CODE BEGIN EM */
-
+#define GR_NEOPIXEL_MAX_LEDS (60U)
 /* USER CODE END EM */
 
 /* Exported functions prototypes ---------------------------------------------*/

G4NEOTESTING/Core/Src/main.c

@@ -134,8 +134,8 @@ int main(void)
 
 	/* Initialize all configured peripherals */
 	/* USER CODE BEGIN 2 */
-	NeopixelContext *neopixel_context_1 = Neopixel_Setup(&neopixelConfig1);
-	NeopixelContext *neopixel_context_2 = Neopixel_Setup(&neopixelConfig2);
+	NeopixelContext neopixel_context_1 = Neopixel_Setup(&neopixelConfig1);
+	NeopixelContext neopixel_context_2 = Neopixel_Setup(&neopixelConfig2);
 
 	/* USER CODE END 2 */
 
@@ -147,8 +147,8 @@ int main(void)
 		/* USER CODE BEGIN 3 */
 		LL_mDelay(500);
 
-		Neopixel_WriteAll(neopixel_context_1, neopixelColors1, sizeof(neopixelColors1));
-		Neopixel_WriteAll(neopixel_context_2, neopixelColors2, sizeof(neopixelColors2));
+		Neopixel_WriteAll(&neopixel_context_1, neopixelColors1, sizeof(neopixelColors1));
+		Neopixel_WriteAll(&neopixel_context_2, neopixelColors2, sizeof(neopixelColors2));
 
 		for (uint32_t i = 0; i < NEOPIXEL_LED_COUNT; i++) {
 			neopixelColors1[i] = neopixelColors1[(i + 1) % NEOPIXEL_LED_COUNT];

Lib/FancyLayers-RENAME/NeoPixel/Inc/gr_neopixel.h

@@ -1,10 +1,16 @@
+#include <stdbool.h>
 #include <stdint.h>
 
 #include "main.h"
 
 #ifndef GR_NEOPIXEL_H
 #define GR_NEOPIXEL_H
 
+#ifndef GR_NEOPIXEL_MAX_LEDS
+#warning "GR_NEOPIXEL_MAX_LEDS is not defined: Define GR_NEOPIXEL_MAX_LEDS to the maximum number of LEDs you plan to use in your main.h to avoid this warning."
+#define GR_NEOPIXEL_MAX_LEDS (64)
+#endif
+
 /**
  * @brief Port used by Neopixel GPIO pins.
  * @note Same port must be used by all GPIO pins associated with one NeopixelConfig struct.
@@ -54,12 +60,18 @@ typedef enum {
 } Neopixel_SPI_BaudRatePrescaler;
 
 /**
- * @brief Context containing all necessary information for controlling a Neopixel strip.
- * @note Acquired by calling Neopixel_Setup with a NeopixelConfig struct.
- * @note This struct is opaque to users of the library, its contents should not be accessed directly. All interactions with the Neopixel strip should be done through the provided functions in this
- * library.
+ * @brief Encodes a 24-bit GRB color into the format required for Neopixel transmission.
  */
-typedef struct NeopixelContext NeopixelContext;
+typedef enum {
+	COLOR_OFF = (uint32_t)0x000000,
+	COLOR_RED = (uint32_t)0x03FC00,
+	COLOR_ORANGE = (uint32_t)0x80FF00,
+	COLOR_YELLOW = (uint32_t)0xFFFF00,
+	COLOR_GREEN = (uint32_t)0xFF0000,
+	COLOR_BLUE = (uint32_t)0x0000FF,
+	COLOR_PURPLE = (uint32_t)0x00FF7F,
+	COLOR_WHITE = (uint32_t)0xFFFFFF,
+} Neopixel_Color;
 
 /**
  * @brief Configuration struct for Neopixel control. This should be initialized and passed to Neopixel_Setup before using any other functions in this library.
@@ -74,27 +86,27 @@ typedef struct {
 } NeopixelConfig;
 
 /**
- * @brief Encodes a 24-bit GRB color into the format required for Neopixel transmission.
+ * @brief Internal context containing all necessary information for controlling a Neopixel strip.
+ * @note Acquired by calling Neopixel_Setup with a NeopixelConfig struct.
+ * @note This struct is opaque to users of the library, its contents should not be accessed directly. All interactions with the Neopixel strip should be done through the provided functions in this
+ * library.
  */
-typedef enum {
-	COLOR_OFF = (uint32_t)0x000000,
-	COLOR_RED = (uint32_t)0x03FC00,
-	COLOR_ORANGE = (uint32_t)0x80FF00,
-	COLOR_YELLOW = (uint32_t)0xFFFF00,
-	COLOR_GREEN = (uint32_t)0xFF0000,
-	COLOR_BLUE = (uint32_t)0x0000FF,
-	COLOR_PURPLE = (uint32_t)0x00FF7F,
-	COLOR_WHITE = (uint32_t)0xFFFFFF,
-} Neopixel_Color;
+typedef struct {
+	// Internal struct to hold configuration and state information, should not be accessed directly by users of the library
+	struct {
+		// Configuration provided at setup time, stored for internal use in library functions
+		NeopixelConfig config;
+		// Internal state to track whether setup was successful, used to prevent functions from being called with an uninitialized context
+		bool is_initialized;
+	} INTERNAL;
+} NeopixelContext;
 
 /**
- * @brief Initializes the Neopixel library with the provided configuration. This must be called before any other functions in this library.
- * @note This function will allocate memory for the NeopixelContext struct. There is no corresponding de-initialization function, this is not an issue in practice since this is only expected to be
- * called once at the beginning of the program.
- * @param neopixelConfiguration A pointer to a NeopixelConfig struct containing the desired configuration for the Neopixel library.
- * @return A pointer to a NeopixelContext struct that can be used for subsequent operations on the Neopixel strip.
+ * @brief Initialize GPIO and SPI for Neopixel based on neopixelConfiguration
+ * @param neopixelConfiguration A pointer to the NeopixelConfig containing all customizable parameters, must be initialized by user
+ * @return A struct containing all necessary information for controlling the Neopixel strip. This context should be passed to all other functions in this library when controlling the strip.
  */
-NeopixelContext *Neopixel_Setup(NeopixelConfig *neopixelConfiguration);
+NeopixelContext Neopixel_Setup(NeopixelConfig *neopixelConfiguration);
 
 /**
  * @brief Writes an array of colors to the Neopixel strip. The number of colors must match the number of Neopixels configured in Neopixel_Setup.

Lib/FancyLayers-RENAME/NeoPixel/Neopixel.md

@@ -1,34 +1,49 @@
+> [!WARNING]
+> This library is not safe to use inside of ISRs (but it should not be used there anyway)!
+
 # Neopixel Usage
 The Neopixel library is used for setting up the LEDs on the WS2812 Neopixel LED strip using an SPI MOSI pin.
 
-# Configuration
-Before initialization, a configuration struct of type NeopixelConfig needs to be created like so:
+## Configuration
 
+Write the maximum number of LEDs (in a single bus) you plan to use into your `main.h`
+```c
+#define GR_NEOPIXEL_MAX_LEDS (42)
 ```
-NeopixelConfig neopixelConfig1 = {.SPI_Instance = SPI1,
-				  .Neopixel_Count = NEOPIXEL_LED_COUNT,
+
+Before initialization, a configuration struct of type NeopixelConfig needs to be created like so:
+
+```c
+NeopixelConfig neopixelConfig = {.SPI_Instance = SPI1,
+				  .Neopixel_Count = 3,
 				  .MOSI_Pin = LL_GPIO_PIN_5,
 				  .GPIO_AlternateFunction = Neopixel_AF5,
 				  .GPIO_Port = Neopixel_GPIOB,
 				  .SPI_BaudRatePrescaler = Neopixel_SPI_PS64};
 ```
 
-## Configuration Parameters
-- SPI_Instance: The instance of SPI being used (SPI1, SPI2, SPI3)
-- Neopixel_Count: Number of LEDs
-- MOSI_Pin: Pin used for GPIO and SPI (can specify pin or use bitmask)
-- GPIO_AlternateFunction: For a GPIO pin to be directly connected to a peripheral. Use Neopixel_AF5 for SPI1 and SPI2, Neopixel_AF6 for SPI3.
-- GPIO_Port: Port that the MOSI pin belongs to. If using bitmask, all pins should belong to the same port.
-- SPI_BaudRatePrescaler: Sets SPI clock communication speed. The prescaler determines how much to divide an SCK tick by.
+### Configuration Parameters
+- `SPI_Instance`: The instance of SPI being used (`SPI1`, `SPI2`, `SPI3`)
+- `Neopixel_Count`: Number of LEDs, must be less than `GR_NEOPIXEL_MAX_LEDS`
+- `MOSI_Pin`: Pin used for GPIO and SPI (can specify pin or use bitmask)
+- `GPIO_AlternateFunction`: For a GPIO pin to be directly connected to a peripheral.
+- `GPIO_Port`: Port that the MOSI pin belongs to. If using bitmask, all pins should belong to the same port.
+- `SPI_BaudRatePrescaler`: Sets SPI clock communication speed. The prescaler determines how much to divide an SCK tick by.
 
-# Example initialization
-First, initialize the configured peripherals:
+## Example
+Zeroth, define in your header the `GR_NEOPIXEL_MAX_LEDS` value for your board in your `main.h`:
+```c
+#define GR_NEOPIXEL_MAX_LEDS (42)
 ```
-NeopixelContext *neopixel_context_1 = Neopixel_Setup(&neopixelConfig1);
+
+First, initialize the configured peripherals:
+```c
+NeopixelContext neopixel_context = {0};
+Neopixel_Setup(&neopixelConfig, &neopixel_context);
 ```
 
-To be able to write colors, you must have an array of colors of type Neopixel_Color and length equivalent to the number of LEDs. Each Neopixel_Color is 24 bits (8 for red, green, blue each). In this example, we call this array ```neopixelColors1```. Then, you can use:
+To be able to write colors, you must have an array of colors of type Neopixel_Color and length equivalent to the number of LEDs. Each Neopixel_Color is 24 bits (8 for red, green, blue each). In this example, we call this array ```neopixelColors```. Then, you can use:
 
-```
-Neopixel_WriteAll(neopixel_context_1, neopixelColors1, sizeof(neopixelColors1));
+```c
+Neopixel_WriteAll(neopixel_context, neopixelColors, sizeof(neopixelColors));
 ```

Lib/FancyLayers-RENAME/NeoPixel/Src/gr_neopixel.c

@@ -1,16 +1,17 @@
 #include "gr_neopixel.h"
 
-#include <malloc.h>
 #include <stdbool.h>
 #include <stdint.h>
 
 #include "Logomatic.h"
 #include "main.h"
 
-struct NeopixelContext {
-	// Configuration provided at setup
-	NeopixelConfig config;
-};
+/**
+ * @brief Buffer to hold the encoded color data for each LED. Each LED requires 24 bytes of data.
+ * @remarks This buffer is used to store the encoded color data before it is transmitted over SPI to the Neopixel strip.
+ * @note This buffer is used to avoid allocating large amounts of memory on the stack during transmission.
+ */
+static uint8_t neopixel_color_to_bits_conversion_buffer[GR_NEOPIXEL_MAX_LEDS][24]; // Buffer to hold the encoded color data for each LED, 24 bytes per LED
 
 /**
  * @brief Internal function to block execution until the SPI peripheral is no longer busy. This is used to ensure that we don't start a new transmission before the previous one has completed.
@@ -20,7 +21,40 @@ struct NeopixelContext {
  */
 static void Neopixel_BlockWhileBusy(NeopixelContext *context)
 {
-	while (LL_SPI_IsActiveFlag_BSY(context->config.SPI_Instance)) {}
+	if (context == NULL) {
+		LOGOMATIC("Context is NULL!\n");
+		return;
+	}
+
+	if (!context->INTERNAL.is_initialized) {
+		LOGOMATIC("Context is not initialized!\n");
+		return;
+	}
+
+	while (LL_SPI_IsActiveFlag_BSY(context->INTERNAL.config.SPI_Instance)) {}
+}
+
+/**
+ * @brief Internal function to block execution until the SPI peripheral is ready to transmit data. This is used to ensure that we don't write data to the SPI data register before the peripheral is
+ * ready, which could cause data corruption.
+ * @param context A pointer to the NeopixelContext containing the SPI instance to check.
+ * @return None
+ * @warning This function will block indefinitely if the SPI peripheral gets stuck in a transmitting state. This can happen if the SPI bus is not setup correctly or if there is a hardware issue with
+ * the SPI peripheral.
+ */
+static void Neopixel_BlockWhileTransmitting(NeopixelContext *context)
+{
+	if (context == NULL) {
+		LOGOMATIC("Context is NULL!\n");
+		return;
+	}
+
+	if (!context->INTERNAL.is_initialized) {
+		LOGOMATIC("Context is not initialized!\n");
+		return;
+	}
+
+	while (!LL_SPI_IsActiveFlag_TXE(context->INTERNAL.config.SPI_Instance)) {}
 }
 
 /**
@@ -36,16 +70,21 @@ void Neopixel_LatchStrip(NeopixelContext *context)
 	Neopixel_BlockWhileBusy(context);
 }
 
-/**
- * @brief Initialize GPIO and SPI for Neopixel based on neopixelConfiguration
- * @param neopixelConfiguration A pointer to the NeopixelConfig containing all customizable parameters, must be initialized by user
- * @return A pointer to NeopixelContext, which contains the set up from neopixelConfiguration
- */
-NeopixelContext *Neopixel_Setup(NeopixelConfig *neopixelConfiguration)
+NeopixelContext Neopixel_Setup(NeopixelConfig *neopixelConfiguration)
 {
 	if (neopixelConfiguration == NULL) {
 		LOGOMATIC("Neopixel configuration is NULL!\n");
-		return NULL;
+		return (NeopixelContext){0};
+	}
+
+	if (neopixelConfiguration->Neopixel_Count == 0) {
+		LOGOMATIC("Number of Neopixels cannot be 0!\n");
+		return (NeopixelContext){0};
+	}
+
+	if (neopixelConfiguration->Neopixel_Count > GR_NEOPIXEL_MAX_LEDS) {
+		LOGOMATIC("Number of Neopixels configured exceeds maximum supported!\n");
+		return (NeopixelContext){0};
 	}
 
 	// Enable clocks for GPIO depending on port used
@@ -68,7 +107,7 @@ NeopixelContext *Neopixel_Setup(NeopixelConfig *neopixelConfiguration)
 			LL_AHB2_GRP1_EnableClock(LL_AHB2_GRP1_PERIPH_GPIOD);
 			break;
 		default:
-			return NULL;
+			return (NeopixelContext){0};
 	}
 
 	// Set up and initialize GPIO
@@ -111,17 +150,8 @@ NeopixelContext *Neopixel_Setup(NeopixelConfig *neopixelConfiguration)
 	LL_SPI_EnableNSSPulseMgt(neopixelConfiguration->SPI_Instance);
 	LL_SPI_Enable(neopixelConfiguration->SPI_Instance);
 
-	// Config is internal, while context is what's passed into other functions
-	NeopixelContext *context = malloc(sizeof(NeopixelContext));
-
-	if (context == NULL) {
-		LOGOMATIC("Failed to allocate memory for Neopixel context!\n");
-		return NULL;
-	}
-
-	context->config = *neopixelConfiguration;
-
-	Neopixel_LatchStrip(context);
+	NeopixelContext context = {.INTERNAL = {.config = *neopixelConfiguration, .is_initialized = true}};
+	Neopixel_LatchStrip(&context);
 
 	return context;
 }
@@ -148,28 +178,35 @@ void Neopixel_WriteAll(NeopixelContext *context, const Neopixel_Color *colors, u
 		LOGOMATIC("Colors array is NULL!\n");
 		return;
 	}
+
 	if (sizeofColors == 0) {
 		LOGOMATIC("Size of colors array is 0!\n");
 		return;
 	}
 
-	if (context->config.Neopixel_Count * sizeof(Neopixel_Color) != sizeofColors) {
+	if (!context->INTERNAL.is_initialized) {
+		LOGOMATIC("Context is not initialized!\n");
+		return;
+	}
+
+	if (context->INTERNAL.config.Neopixel_Count * sizeof(Neopixel_Color) != sizeofColors) {
 		LOGOMATIC("Number of colors provided does not match number of Neopixels configured!\n");
-		LOGOMATIC("Expected %lu colors, got %lu colors\n", context->config.Neopixel_Count, sizeofColors / sizeof(Neopixel_Color));
-		assert_param(context->config.NumberOfNeopixels * sizeof(Neopixel_Color) == sizeofColors);
+		LOGOMATIC("\tExpected %lu colors, got %lu colors\n", context->INTERNAL.config.Neopixel_Count, sizeofColors / sizeof(Neopixel_Color));
+		assert_param(context->INTERNAL.config.NumberOfNeopixels * sizeof(Neopixel_Color) == sizeofColors);
 		return;
 	}
 
-	uint8_t neopixelTransmission[context->config.Neopixel_Count * 24];
-	for (uint32_t i = 0; i < context->config.Neopixel_Count; i++) {
-		Neopixel_EncodeColor(&neopixelTransmission[i * 24], colors[i]);
+	for (uint32_t i = 0; i < context->INTERNAL.config.Neopixel_Count; i++) {
+		Neopixel_EncodeColor(neopixel_color_to_bits_conversion_buffer[i], colors[i]);
 	}
 
 	Neopixel_BlockWhileBusy(context);
 
-	for (uint32_t i = 0; i < sizeof(neopixelTransmission); i++) {
-		while (!LL_SPI_IsActiveFlag_TXE(context->config.SPI_Instance)) {}
-		LL_SPI_TransmitData8(context->config.SPI_Instance, neopixelTransmission[i]);
+	for (uint32_t i = 0; i < context->INTERNAL.config.Neopixel_Count; i++) {
+		for (uint32_t j = 0; j < 24; j++) {
+			Neopixel_BlockWhileTransmitting(context);
+			LL_SPI_TransmitData8(context->INTERNAL.config.SPI_Instance, neopixel_color_to_bits_conversion_buffer[i][j]);
+		}
 	}
 
 	Neopixel_LatchStrip(context);

Lib/cmake/gcc-arm-none-eabi.cmake

@@ -20,7 +20,7 @@ set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY)
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} ${TARGET_FLAGS}")
 set(
 	CMAKE_C_FLAGS
-	"${CMAKE_C_FLAGS} -Wall -Wextra -Wpedantic -fdata-sections -ffunction-sections"
+	"${CMAKE_C_FLAGS} -Wall -Wextra -Wvla -Wpedantic -fdata-sections -ffunction-sections"
 )
 if(CMAKE_BUILD_TYPE MATCHES Debug)
 	set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O0 -g3")