feat: use c++ by wispl · Pull Request #7 · Rutgers-RPL/CommonDrivers

wispl · 2026-08-16T20:38:46Z

Discussed internally, this is a rather large change. Really sorry about that. Couple of changes

use c++ classes to simplify and make things more readable, we can discuss optimizations later
cmake magic to conditionally compile in spi, i2c, uart, and co. files only when they are enabled in cubemx
swapped to gtest + fff, no more dependence on ruby

There are some interesting stuff for the Protocol abstract class, feel free to criticize and discuss. One cool benefit of this rewrite is that we no longer have #ifdefs anywhere outside of hal.h.

Resolves #6

wispl · 2026-08-16T20:39:38Z

Don't merge yet, this needs a lot more testing.

ncorrea210

Haven't looked through it all 100% of the way, these are some initial thoughts though. Main thing is that private functions (private to the .cpp file) should probably be static, and I am not sure about this using Span = ... stuff.

ncorrea210 · 2026-08-16T20:55:59Z

+/// Span ---
+/// A quick primer on span, it just a regular C buffer but it also
+/// includes the size, really convenient, we will be using this a lot
+using Span = std::span<uint8_t>;
+using ConstSpan = std::span<const uint8_t>;


Why not directly use std::span?

This enforces byte buffers, also needs a bit less typing.

I guess. Not sure if I am a big fan though. @dmanslick thoughts?

I am fine with removing it, though that does mean we have to do do std::span<uint8_t>(...) instead.

ncorrea210 · 2026-08-16T21:05:06Z

overall though it looks nice, am enjoying the cpp

dmanslick · 2026-08-17T04:08:06Z

I don't think I understand the purpose of this "Platform" namespace. If it is going to be used everywhere, what is the benefit of the namespace besides knowing that these classes are from the common drivers repo? And if that's its purpose, I think calling the namespace "Common" is better suited. In any case, doesn't it make more sense for sensors to be in their own "Sensors" namespace, protocols to be in their own "Protocols" namespace, etc?

wispl · 2026-08-17T23:30:21Z

It is a namespace collision thing, but right now it is more of a convention. Libraries generally add a namespace to all their exports so it is something like library_name::module::item.

I am fine with removing it, or calling it something else. Technically your sensor example would be Platform::Sensors::BMP581. I just slapped everything under a Platform cause I don't see too much of a reason for sub namespaces yet.

ncorrea210 · 2026-08-21T23:41:03Z

+        (addr & 0x0000FF)          // third byte
+    };
+    protocol->write(ConstSpan(cmd1), {}, AddressSize::Byte3);
+    HAL_Delay(2);  // maximum delay for reading to cache


Is this safe in the context of a task? If this isn't in the context of a task then it is fine, but in the context of a task we are blocking for 2ms when we could be running a different task had we called vTaskDelay instead. If we don't want to allow a context switch while this runs we can wrap the call with a critical section.

ncorrea210 · 2026-08-21T23:42:50Z

+        (addr & 0x0000FF)         // it operates on whiole blocks
+    };
+    protocol->write(ConstSpan(cmd), {}, AddressSize::Byte3);
+    HAL_Delay(12); // maximum delay required for erases


yeah this looks quite alarming if we run this in any task (if it does run in a task). We completely miss deadlines on sensor tasks 6 times over (assuming we run those at 200 Hz)

ncorrea210 · 2026-08-21T23:43:25Z

+        return false;
+    uint8_t cmd2[] = {GD5F_SET_FEATURE, 0xA0, 0x00};
+    protocol.write(ConstSpan(cmd2), {}, AddressSize::Byte2);
+    HAL_Delay(5000);


not in task so fine (leaving for myself)

ncorrea210 · 2026-08-21T23:44:12Z

+}
+
+void bosch_delay(uint32_t period, void* intf_ptr) {
+    HAL_Delay(ceil(static_cast<double>(period) / (1000.0)));


if used in task, probably replace with vTaskDelay

ncorrea210 · 2026-08-21T23:46:45Z

This is getting pretty close to a good starting point in my mind. Still not sure I love the file structure but that is more of a nit-pick for now. I am more of a fan of putting the headers and source in the same folder for each component so we can do the cmake for each induvidually (could be nice later if we choose to pull in only specific things rather than all common drivers), but I will defer decisions like that to @wispl and @dmanslick .

Discussed internally, this is a rather large change.

Using a similar cmake trick to conditionally compile in peripheral code, we can also use it to pull in the right header file automatically. Also reorganize imports while we are at it.

Note that tests in this case means unit tests using the drivers, not the DEBUG build. That will still have debug statements available. We want to compile these out for performance and storage reasons.

ncorrea210

Probably good to go after checking out the usage of vTaskDelay in init.

ncorrea210 · 2026-08-24T11:58:17Z

+        return false;
+    uint8_t cmd2[] = {GD5F_SET_FEATURE, 0xA0, 0x00};
+    protocol.write(ConstSpan(cmd2), {}, AddressSize::Byte2);
+    Delay(5000);


Won't init be called before starting the scheduler? Does vTaskDelay work in that instance?

I moved the device_init to inside the StartDefaultTask, so the scheduler would be called already. STM32 already does that for their MX_USB_DEVICE_Init function which we depend on, so moving our init there as well is a good idea.

Also things also seem to behave a bit better (in regards to timing) when moving stuff into the task for some reason.

That's fine then, I guess I missed that. I see that we have an infinite loop in there, might as well just delete the task though after initialization. Doesn't really matter for now though.

ncorrea210

I'll approve it, however you should talk to Dhruv before merging I think

dmanslick · 2026-08-24T19:13:24Z

Can we change the namespace to Common? We can do this after merging too, I just like that better.

wispl force-pushed the cpp-migration branch 2 times, most recently from 5008545 to de65eb2 Compare August 16, 2026 20:42

ncorrea210 reviewed Aug 16, 2026

View reviewed changes

wispl force-pushed the cpp-migration branch from de65eb2 to 87ab97a Compare August 20, 2026 18:40

ncorrea210 reviewed Aug 21, 2026

View reviewed changes

ncorrea210 requested a review from dmanslick August 21, 2026 23:47

feat: use c++

db6bafa

Discussed internally, this is a rather large change.

wispl force-pushed the cpp-migration branch from 87ab97a to db6bafa Compare August 23, 2026 23:54

wispl added 3 commits August 23, 2026 19:55

feat: add logging

9cf181c

feat: auto detect hal header file

feb3eaa

Using a similar cmake trick to conditionally compile in peripheral code, we can also use it to pull in the right header file automatically. Also reorganize imports while we are at it.

feat: use vTaskDelay when using RTOS

c33d48c

wispl force-pushed the cpp-migration branch from b3f3be1 to dda2077 Compare August 24, 2026 05:15

feat: compile out log statements in release and tests

e63e69c

Note that tests in this case means unit tests using the drivers, not the DEBUG build. That will still have debug statements available. We want to compile these out for performance and storage reasons.

wispl force-pushed the cpp-migration branch from dda2077 to e63e69c Compare August 24, 2026 05:24

ncorrea210 reviewed Aug 24, 2026

View reviewed changes

ncorrea210 approved these changes Aug 24, 2026

View reviewed changes

Uh oh!

Conversation

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

Uh oh!

Uh oh!

wispl commented Aug 16, 2026

Uh oh!

ncorrea210 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

ncorrea210 commented Aug 16, 2026

Uh oh!

dmanslick commented Aug 17, 2026

Uh oh!

wispl commented Aug 17, 2026

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!

ncorrea210 commented Aug 21, 2026

Uh oh!

ncorrea210 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!

ncorrea210 left a comment • 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!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wispl commented Aug 16, 2026 •

edited by ncorrea210

Loading

ncorrea210 left a comment •

edited

Loading

dmanslick commented Aug 24, 2026 •

edited

Loading