Initial new firmware commit by Mandeson · Pull Request #80 · grupacosmo/cosmorocket

Mandeson · 2025-11-28T12:28:14Z

No description provided.

…d comments for clarity

…ace as not implemented, add comments

… change pins

…ld BMP280 driver, add possibility to wait for the serial monitor to connect

Copilot

Pull request overview

This is a major firmware rewrite that replaces the rocket flight computer implementation with a new sensor data acquisition system. The PR removes the old flight control logic, LoRa communication, camera control, SD card storage, and flight state management, replacing them with a simplified system focused on reading data from barometer (BME280), accelerometer (LSM6DSO), and GPS sensors, then buffering the data using lock-free ring buffers.

Key Changes:

Removed MPU6050-based IMU and replaced with LSM6DSO32 accelerometer/gyroscope
Replaced BMP280 barometer with BME280 (adds humidity sensing)
Completely rewrote GPS handling to use raw UART with NMEA decoding instead of TinyGPS++ library
Implemented custom I2C driver using ESP-IDF APIs directly
Added lock-free SPSC ring buffer library for sensor data buffering
Removed all flight control logic, LoRa telemetry, camera control, ignition system, and persistent storage

Reviewed changes

Copilot reviewed 50 out of 52 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
boards/src/storage.cpp	New buffering system using lock-free ring buffers, periodically flushes sensor data to serial output
boards/src/i2c.cpp	New custom I2C driver implementation using ESP-IDF low-level APIs
boards/src/gps.cpp	Rewritten GPS module using UART event-driven processing with custom NMEA decoder
boards/src/barometer.cpp	New BME280 barometer driver with pressure, humidity, and temperature sensing
boards/src/accelerometer.cpp	New LSM6DSO accelerometer/gyroscope driver with FIFO buffering at 833Hz
boards/src/main.cpp	Simplified main loop that periodically reads sensors and posts to storage buffers
boards/lib/ultra-low-latency-ring-buffer/	Added lock-free SPSC ring buffer library for thread-safe sensor data buffering
boards/lib/nmea-decoder/	Added custom NMEA sentence parser for GPS data extraction
boards/platformio.ini	Updated build configuration for new hardware target (CosmoChip ESP32-S3) and dependencies
boards/include/board_config.h	Simplified board configuration with new I2C and GPS pin definitions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-30T22:28:11Z

boards/src/storage.cpp

+namespace storage {
+
+// 200ms delay between data flushes
+static constexpr inline int DATA_FLUSH_INTERVAL = 1000;


The comment says "200ms delay" but the constant is set to 1000ms (1 second). Either update the comment to say "1000ms delay" or "1 second delay", or change the constant value to 200 if 200ms was intended.

Copilot · 2025-11-30T22:28:11Z

boards/src/i2c.cpp

+            {
+                .clk_speed = FREQUENCY,
+            },
+        .clk_flags = I2C_SCLK_SRC_FLAG_FOR_NOMAL,


The typo in the constant name I2C_SCLK_SRC_FLAG_FOR_NOMAL appears to be from the ESP-IDF library itself (missing 'R' in NORMAL). However, if this constant doesn't exist in the actual library, this will cause a compilation error. Verify that this constant is correctly spelled according to the ESP-IDF version being used, or change it to the correct constant name like I2C_SCLK_SRC_FLAG_FOR_NOMAL (if that's the actual library constant) or potentially just 0 if the constant is unavailable.

Copilot · 2025-11-30T22:28:11Z

boards/src/main.cpp

-#else
-#include "lora-uart.h"
-#endif
+// In miliseconds


Typo in comment: "miliseconds" should be "milliseconds".

Copilot · 2025-11-30T22:28:11Z

boards/lib/ultra-low-latency-ring-buffer/ring_buffer.hpp

+        if (current_head == current_tail) return true;
+
+        return false;


The empty() function can be simplified. The explicit if statement and boolean return is redundant. Replace lines 58-60 with: return current_head == current_tail;

Copilot · 2025-11-30T22:28:12Z

boards/lib/ultra-low-latency-ring-buffer/ring_buffer.hpp

+        return true;
+    }
+
+    size_t Size() {


The Size() function uses PascalCase naming, which is inconsistent with the empty() and pop() functions that use lowercase. For consistency with C++ standard library conventions and the other methods in this class, rename Size() to size().

Copilot · 2025-11-30T22:28:12Z

boards/src/gps.cpp

-TinyGPSPlus tiny_gps;
-SemaphoreHandle_t gps_mutex;
+static void readLine() {
+    // Get posision of detected '\n' character


Typo in comment: "posision" should be "position".

Copilot · 2025-11-30T22:28:12Z

boards/src/gps.cpp

+    uart_set_pin(UART_BUS_NUMBER, board_config::GPS_UART_TX_PIN, board_config::GPS_UART_RX_PIN,
+                 UART_PIN_NO_CHANGE, UART_PIN_NO_CHANGE);
+
+    // Setup pattern recognition to detect '\n' as the and of NMEA line


Typo in comment: "and" should be "end" (referring to "the end of NMEA line").

Copilot · 2025-11-30T22:28:13Z

boards/include/board_config.h

+constexpr inline int GPS_BAUDRATE = 115200;

-#endif
+constexpr inline uint8_t LSM6DSO_ADDR = LSM6DSO_I2C_ADD_H;    // Alternetively LSM6DSO_I2C_ADD_L


Typo in comment: "Alternetively" should be "Alternatively".

Suggested change

constexpr inline uint8_t LSM6DSO_ADDR = LSM6DSO_I2C_ADD_H; // Alternetively LSM6DSO_I2C_ADD_L

constexpr inline uint8_t LSM6DSO_ADDR = LSM6DSO_I2C_ADD_H; // Alternatively LSM6DSO_I2C_ADD_L

Copilot · 2025-11-30T22:28:13Z

boards/src/i2c.cpp

+static bool performWriteTransaction(uint8_t addr, uint8_t reg, const uint8_t *buffer, uint16_t size,
+                                    i2c_cmd_handle_t command) {
+    if (i2c_master_start(command) != ESP_OK) return FAILURE;
+    if (i2c_master_write_byte(command, addr << 1, true) != ESP_OK) return FAILURE;
+    if (i2c_master_write_byte(command, reg, true) != ESP_OK) return FAILURE;
+    if (i2c_master_write(command, buffer, size, true) != ESP_OK) return FAILURE;
+    if (i2c_master_stop(command) != ESP_OK) return FAILURE;
+    if (i2c_master_cmd_begin(BUS_NUMBER, command, TIMEOUT) != ESP_OK) return FAILURE;
+    return SUCCESS;
+}
+
+Result write(uint8_t addr, uint8_t reg, const uint8_t *buffer, uint16_t size) {
+    if (size > MAX_SUPPORTED_TRANSFER_SIZE) {
+        Serial.println("I2C: Error: unsupported write size");
+        return FAILURE;
+    }
+
+    uint8_t command_buffer[I2C_LINK_RECOMMENDED_SIZE(2)] = {0};
+    i2c_cmd_handle_t command =
+        i2c_cmd_link_create_static(command_buffer, I2C_LINK_RECOMMENDED_SIZE(2));
+
+    if (performWriteTransaction(addr, reg, buffer, size, command) != SUCCESS) {


The function performWriteTransaction returns a bool but is being compared to SUCCESS (which is an enum value 0). While this works, it's inconsistent. The function should either return Result type to match the calling code, or the comparison should be changed to check the boolean return value directly (e.g., if (!performWriteTransaction(...))). Recommend changing the return type to Result for consistency.

Copilot · 2025-11-30T22:28:13Z

boards/lib/nmea-decoder/nmea_decoder.cpp

+    if (part[1][0] != ',') {
+        *outPtr++ = part[1][0];
+        *outPtr++ = part[1][1];
+        *outPtr++ = ':';
+        *outPtr++ = part[1][2];
+        *outPtr++ = part[1][3];
+        *outPtr++ = ':';
+        *outPtr++ = part[1][4];
+        *outPtr++ = part[1][5];
+        *outPtr++ = '.';
+        *outPtr++ = part[1][7];
+        *outPtr++ = part[1][8];
+    }
+
+    *outPtr++ = ',';
+
+    int frac;
+
+    if (part[2][0] != ',') {
+        // Latitude format: DDMM.mmmmm -> DD.ddddddd
+        frac = ((part[2][2] - '0') * 100'000'000LL + (part[2][3] - '0') * 10'000'000LL +
+                (part[2][5] - '0') * 1'000'000LL + (part[2][6] - '0') * 100'000LL +
+                (part[2][7] - '0') * 10'000LL + (part[2][8] - '0') * 1'000LL +
+                (part[2][9] - '0') * 100LL) /
+               60;
+        if (part[2][0] != '0') *outPtr++ = part[2][0];
+        *outPtr++ = part[2][1];
+        *outPtr++ = '.';
+        minutesToString(outPtr, frac);
+        *outPtr++ = part[3][0];
+    }
+
+    *outPtr++ = ',';
+
+    if (part[4][0] != ',') {
+        // Longitude format: DDDMM.mmmmm -> DDD.ddddddd
+        frac = ((part[4][3] - '0') * 100'000'000LL + (part[4][4] - '0') * 10'000'000LL +
+                (part[4][6] - '0') * 1'000'000LL + (part[4][7] - '0') * 100'000LL +
+                (part[4][8] - '0') * 10'000LL + (part[4][9] - '0') * 1'000LL +
+                (part[4][10] - '0') * 100LL) /
+               60;
+        if (part[4][0] != '0') *outPtr++ = part[4][0];
+        if (part[4][0] != '0' || part[4][1] != '0') *outPtr++ = part[4][1];
+        *outPtr++ = part[4][2];
+        *outPtr++ = '.';
+        minutesToString(outPtr, frac);
+        *outPtr++ = part[5][0];
+    }


The NMEA decoder assumes specific character positions are valid without bounds checking. For example, accessing part[1][7], part[1][8], part[2][9], etc. These accesses could read beyond the actual string data if the NMEA sentence is malformed or truncated. Add validation to ensure each part has sufficient length before accessing specific indices, or the parser may access invalid memory.

this part will be removed too

PatrykPaluch

Those comments are for discussion at Friday meeting. I wrote them very sparingly

PatrykPaluch · 2025-11-30T22:21:44Z

boards/include/barometer.h

+using Pressure = float;
+using Humidity = float;
+using Temperature = float;


i'm not a bing fan of aliasing simple types

PatrykPaluch · 2025-11-30T22:26:48Z

boards/include/common.h

@@ -0,0 +1,3 @@
+#pragma once
+
+enum Result { SUCCESS = 0, FAILURE };


i mean, it just bool, and if i remember correctly, we discussed to use single Result with all error codes defined in it

PatrykPaluch · 2025-11-30T22:30:24Z

boards/lib/nmea-decoder/nmea_decoder.cpp

+
+constexpr inline size_t SENTENCE_PARTS = 15;
+
+static void minutesToString(char *&outPtr, int frac) {


why *&, not just ** or std::string or std::span or anything

PatrykPaluch · 2025-11-30T22:31:36Z

boards/lib/nmea-decoder/nmea_decoder.cpp

+Result decode(std::array<char, MAX_OUTPUT_SIZE> &out,
+              const std::array<char, MAX_SENTENCE_SIZE> &nmea_sentence) {


this method is for longer discussion

PatrykPaluch · 2025-11-30T22:34:11Z

boards/lib/nmea-decoder/nmea_decoder.h

+static constexpr inline int MAX_SENTENCE_SIZE = 196;
+static constexpr inline int MAX_OUTPUT_SIZE = 50;
+
+enum Result { SUCCESS = 0, WRONG_HEADER, WRONG_DATA_FORMAT, BUFFER_OVERFLOW };


why not common.h Result

PatrykPaluch · 2025-11-30T22:43:38Z

boards/src/accelerometer.cpp

+    lsm6dso_fifo_tag_t tag;
+    uint16_t data_count;
+
+    if (g_init_error) return;


silent crash (comment in init())

PatrykPaluch · 2025-11-30T22:45:08Z

boards/src/barometer.cpp

+        init();
+        return FAILURE;
+    }
+    pressure = static_cast<float>(g_data.pressure);  // We don't need double precision


"We don't need double precision"

why not?

PatrykPaluch · 2025-11-30T22:47:10Z

boards/src/gps.cpp

+constexpr inline size_t UART_PATTERN_QUEUE_SIZE = 20;

-Data gps_data;
+QueueHandle_t g_uart_queue;


you are using many different queue implementation. Here QueueHandle_t, somewhere else is SPSCQueue and i think i saw yet another one different queue type

PatrykPaluch · 2025-11-30T22:50:14Z

boards/src/main.cpp

+    gps::init();
+    storage::init();
+    i2c::init();
+    barometer::init();
+    accelerometer::init();


again, those function can fail and there is no error handing

PatrykPaluch · 2025-11-30T22:51:34Z

boards/src/main.cpp

+    if (last_time == 0) last_time = esp_timer_get_time();
+
+    // Wait for the timer to measure given interval
+    if (xSemaphoreTake(g_main_loop_semaphore, 500) == pdTRUE) {


we need to double check every semaphore operation to make sure there is no race condition or we can try to spend some time on some other safer solution.

Im not sure what happen here with those semaphores in loop() and timers

Mandeson added 30 commits October 17, 2025 18:19

Remove old rocket code

994937e

Initial new firmware commit

39be789

Add C++17 build flags

6bf456f

Fix compile error, change code style, use declared style

972fc95

Add barometer

0bf5a89

Increase clang-format column limit

71e2697

Add I2C driver

c548f94

Use libdriver BMP280 driver

e8d5a3b

Remove unused libraries

63c04fb

Check I2C write size to prevent buffer overflow, add size constant an…

5b3be8d

…d comments for clarity

Make bmp280_interface_debug_print memory safe, mark SPI driver interf…

604e137

…ace as not implemented, add comments

Add comment informing about ignoring the bmp280_deinit return code

5f1591b

Fix inconsistent pointer alignment

0526853

Fix swapped SDL and SCL pins

c7cdd4a

Change .gitignore

de90c37

Add .clang-tidy and Makefile

c2eb8cd

Remove VSCode-specific config

8853f59

Change i2c return type from bool to enum

837e5db

Add support for CosmoChip ESP board

eaf2941

Add LSM6DSO32 sensor support, change BMP280 driver to support BME280,…

3eb43b9

… change pins

Make GitHub Actions init and update submodules

adc7c58

Move LSM6DSO32 driver dependency management to platformio.ini

94b73f5

Change main loop tick method to the precise ESP timer

5dc0ac2

Support BME280 using dedicated driver from the manufacturer, remove o…

ef0dd92

…ld BMP280 driver, add possibility to wait for the serial monitor to connect

Improve LSM6DSO32 error handling

c30be33

Improve main loop time stats

f9abf98

Make wait methods consistent

b248847

Improve main loop output formatting

eb629c5

Add GPS module

4586f22

Refactor code to use std::array instead of C-style arrays

0e24ab4

Mandeson and others added 6 commits November 14, 2025 16:56

Add storage module, store sensor and GPS data in thread-safe buffers

61009d5

Change I2C API being used to ESP-IDF driver

8c96019

Add NMEA decoder

252af13

Fix I2C write not actually sending any data

d60ee8f

Remove unused compile definition from CosmoChip.json

5412f0d

Add NMEA decoding

e10c851

Mandeson closed this Nov 28, 2025

Mandeson reopened this Nov 28, 2025

PatrykPaluch requested a review from Copilot November 30, 2025 22:23

Copilot started reviewing on behalf of PatrykPaluch November 30, 2025 22:24 View session

Copilot finished reviewing on behalf of PatrykPaluch November 30, 2025 22:26

Copilot AI reviewed Nov 30, 2025

View reviewed changes

PatrykPaluch requested changes Nov 30, 2025

View reviewed changes

Mandeson added 2 commits December 5, 2025 20:39

Enable clang-tidy

2a1a129

Clean up code, change error handling, add comments

aea3f6d

PatrykPaluch closed this Jan 2, 2026

	constexpr inline uint8_t LSM6DSO_ADDR = LSM6DSO_I2C_ADD_H; // Alternetively LSM6DSO_I2C_ADD_L
	constexpr inline uint8_t LSM6DSO_ADDR = LSM6DSO_I2C_ADD_H; // Alternatively LSM6DSO_I2C_ADD_L

		@@ -0,0 +1,3 @@
		#pragma once

		enum Result { SUCCESS = 0, FAILURE };


		constexpr inline size_t SENTENCE_PARTS = 15;

		static void minutesToString(char *&outPtr, int frac) {

		Result decode(std::array<char, MAX_OUTPUT_SIZE> &out,
		const std::array<char, MAX_SENTENCE_SIZE> &nmea_sentence) {

Conversation

Mandeson commented Nov 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 30, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PatrykPaluch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants