feat: use cpp version of common drivers by wispl · Pull Request #5 · Rutgers-RPL/Hephaestus

wispl · 2026-08-16T21:52:46Z

Very very messy, there are a lot of things which I think can be done better.

wispl · 2026-08-17T23:25:07Z

This is temporary, and is a temporary way to test the cpp changes in Common in a quick and dirty way. Hence why I piggybacked off your cpp commits and stuff everything in a class.

I would like to move Tasks into Common personally, but this needs more design and discussion.

ncorrea210 · 2026-08-17T23:40:12Z

I agree with moving tasks into common, even if they are only used on one device. It just makes things easier to share. It may make sense though to have some kind of interface or functions that are not defined on the platform side that can be easily defined on the device side. For example, if we have a buzzer task, the pin and timer we want to use may change so that implementation may be on the device side.

ncorrea210 · 2026-08-24T00:17:31Z

make sure to commit the bumped submodule version, that is causing the build fail.

ncorrea210 · 2026-08-24T00:02:32Z

+#include "hal.h"
+
+/**
+ * @brief device init


really? 💀

ncorrea210 · 2026-08-24T00:12:28Z

+class TEST_CLASS {
+private:
+  CRC_HandleTypeDef &hcrc;
+
+  // Peripherals
+  Platform::SPI accel;
+  Platform::SPI gyro;
+  Platform::SPI bmp;
+  Platform::SPI flash;
+
+  // Sensors and Flash
+  Platform::GD5F1GQ5XE flash_dev;
+  Platform::BMP581 bmp581_dev;
+  Platform::BMI088 bmi088_dev;
+  std::array<Platform::Sensor*, NUM_SENSORS> sensors;
+
+  // Runtime State
+  SemaphoreHandle_t packet_mutex = xSemaphoreCreateMutex();
+  Platform::Packet packet = Platform::Packet();
+  volatile bool save_to_flash = false;
+  lfs_file_t packet_file;
+
+  int32_t fs_size = 0;
+  uint32_t boot_count = 0;
+  uint32_t file_size = 0;
+
+  // Task attributes
+  TaskHandle_t sensor_task_handle = NULL;
+  TaskHandle_t flash_task_handle = NULL;
+  // Tasks
+  static void sensor_task(void *argument) {
+    auto *self = static_cast<TEST_CLASS*>(argument);
+    self->sensor_loop();
+  }
+
+  static void flash_task(void *argument) {
+    auto *self = static_cast<TEST_CLASS*>(argument);
+    self->flash_loop();
+  }
+public:
+  TEST_CLASS(SPI_HandleTypeDef &hspi1, SPI_HandleTypeDef &hspi2, SPI_HandleTypeDef &hspi3, CRC_HandleTypeDef &hcrc)
+  : hcrc(hcrc),
+    accel(&hspi1, IMU2_ACC_CS_GPIO_Port, IMU2_ACC_CS_Pin),
+    gyro(&hspi1, IMU2_GYRO_CS_GPIO_Port, IMU2_GYRO_CS_Pin),
+    bmp(&hspi2,  BAR1_CS_GPIO_Port, BAR1_CS_Pin),
+    flash(&hspi3, FLASH_CS_GPIO_Port, FLASH_CS_Pin),
+
+    flash_dev(flash),
+    bmp581_dev(bmp),
+    bmi088_dev(accel, gyro),
+    sensors{&bmp581_dev, &bmi088_dev}
+  {
+    // Initialize sensors
+    for (auto& sensor : sensors) {
+      bool ready = 0;
+      for (int c = 0; c < 10; ++c) {
+        ready = sensor->init();
+        if (ready) break;
+        HAL_Delay(20000);
+      }
+      if (!ready) sensor = nullptr; 
+    }
+
+    // Initialize flash
+    for (int c = 0; c < 3; ++c) {
+      if (flash_dev.init()) {
+        fs_size = flash_dev.mount();
+        if (fs_size >= 0) {
+          save_to_flash = true;
+          boot_count = flash_dev.bootcount(false);
+          file_size = flash_dev.open(&packet_file, "packets");
+        }
+        break;
+      }
+      HAL_Delay(5000);
+    }
+    Platform::log("Flash: %u (size), %u (boot), %u (packet_size)\r\n", fs_size, boot_count, file_size);
+  }


I still don't really like this method of setting things up. I'd rather see tasks completely isolated rather than share a class for initialization. This is kinda the setup I have in mind:
In platform:
sensor task and init high level is written here. Shouldn't really have device specific code, can assume FreeRTOS and STM32 is used though.
In Hephaestus:
Any device specific functions needed for the task. This might be where we do stuff with specific pins for example, you use an LED for the sensor loop.

In the actual device_init function we would then have FlashTask::Init() which would handle the scheduling of the task. The idea here is to keep device-init.cpp clean, and to keep applications compartmentalized.

Lets get the cpp-migration merged first for this and common-drivers, then work on the task abstraction. Mahir is gonna yell at me if I stuff more abstractions in...

fair enough, that's fine with me

I agree with most of Nathan's proposal here, I also think tasks should be isolated units. However, I disagree with moving the Sensor Task into common. The sensors that each board is polling will be different, so a common Sensor Task will basically become a become a god class/object, not ideal.

How does that look though? If we made an individual sensor task for every sensor, we'd have a lot of boilerplate. Additionally we want to read all sensors at the same time to ensure the timestamp is accurate for everything. I think we can make a truly general sensor task by making it take the list of sensors on init and having a couple functions that are defined for each device? Some of those might be things like NotifyFlashTask and GetTimestamp.

The current sensor task just does this

for (auto *sensor : sensors) if (sensor != nullptr) sensor->read(packet);

It takes a list of the sensors it wants and just loop over it. It doesn't really care about what sensors are available, just what sensors is currently in the array

Of course it also does a bit of other stuff like semaphores, checksums, and notifying the flash task. Anyways, I did want more discussion on task abstractions which is why I wanted to punt this to another PR.

@ncorrea210 You misunderstood what I wrote. I was not saying to have a task for each sensor. Doesn't matter anyways since the way I was thinking the task would've been implemented was a bit foolish anyways, what Ivan just wrote makes sense so I can see how it can be in common,

ncorrea210 · 2026-08-24T00:13:37Z

+  void disable_flash() {
+    if (save_to_flash) {
+      save_to_flash = false;
+      vTaskNotifyGiveFromISR(flash_task_handle, NULL);
+    }
+  }


is this function guaranteed to only be called in an ISR context? It can be dangerous to use FromISR functions in a non-ISR context. See this.

Yes, this is guaranteed to only be called from ISR, similarly non-ISR functions should not be called in ISR-context so it goes both ways.

ncorrea210 · 2026-08-24T15:55:23Z

+    xTaskCreate(flash_task, "flash task", 512, this, 1, &flash_task_handle);
+    xTaskCreate(sensor_task, "sensor task", 256, this, 2, &sensor_task_handle);


Might be good to look into using xTaskCreateStatic so there aren't any heap allocations. Probably not super important though.

Note that this requires the cpp branch of CommonDrivers to be merged

cpp

7422dc3

ncorrea210 reviewed Aug 16, 2026

View reviewed changes

Comment thread Core/Src/device-init.cpp

wispl force-pushed the cpp-commondrivers branch 2 times, most recently from ce9e4a2 to ccc187e Compare August 23, 2026 23:55

ncorrea210 reviewed Aug 24, 2026

View reviewed changes

feat: use cpp

60fd547

Note that this requires the cpp branch of CommonDrivers to be merged

wispl force-pushed the cpp-commondrivers branch from ccc187e to 60fd547 Compare August 25, 2026 13:21

		xTaskCreate(flash_task, "flash task", 512, this, 1, &flash_task_handle);
		xTaskCreate(sensor_task, "sensor task", 256, this, 2, &sensor_task_handle);

Uh oh!

Conversation

wispl commented Aug 16, 2026

Uh oh!

Uh oh!

wispl commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncorrea210 commented Aug 17, 2026

Uh oh!

ncorrea210 commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncorrea210 Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dmanslick Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

3 participants

wispl commented Aug 17, 2026 •

edited

Loading

ncorrea210 commented Aug 24, 2026 •

edited

Loading

ncorrea210 Aug 24, 2026 •

edited

Loading

dmanslick Aug 24, 2026 •

edited

Loading