Initial Rev 3 flash driver by ETSells · Pull Request #30 · SunDevilRocketry/driver

ETSells · 2026-05-05T19:57:54Z

Description

Implements the critical APIs needed to operate flash on Rev. 3.

Issue Link

Closes #5.

Child: SunDevilRocketry/Flight-Computer-Firmware#261

Depends on: SunDevilRocketry/mod#127 (can be reviewed separately though, just make sure this branch is checked out if you run any tests)

Testing

Passes existing unit tests
Unit tests modified
Integration test performed

N/A -- no testing possible until rev3 initial integration.

Other

N/A

Reviewer Checklist

Standards

Follows FCF Architectural Standards
Follows SDR Coding Standards
Code complexity/function Size is minimized
Code is testable
Code is readable and commented properly
License terms are respected

Error Handling

Potentially unsafe functions return a status code
Error returns properly handled

Memory

Stack allocated memory is scoped correctly
Heap allocated memory is avoided
Globally allocated memory is minimized except when necessary
Pointers are used correctly
Concurrency has been considered

Performance

Rate limiters are respected
Busy waiting is avoided
"Delay" calls are not used in performance sensitive code

ETSells · 2026-05-23T20:01:43Z

@NArmistead adding you for review bc this has stalled. if you're uncomfortable reviewing a driver lmk

NArmistead

Only thing from me is that there are a couple functions that would need a null check on the flash handle to be consistent with the other functions that have it. If it isn't necessary I'll approve.

NArmistead · 2026-05-30T03:55:34Z

 	FLASH_BLOCK_13   ,
 	FLASH_BLOCK_14   ,
-	FLASH_BLOCK_15    
+	FLASH_BLOCK_15   ,


nit: unnecessary comma

Can fix. If youre curious, this arose because I was originally going to extend the enum to support the number of blocks on the new chip, but theres hundreds of pages & the smallest block size.

I have to say: these kinds of inits made me wish C (and more languages in general) just had a way to create these kinds of types that are basically just ranges without crazy enums or (in the case of OOP languages like C++) a janky setup of operator overloading.

Is it time for SDR to contribute to the ISO C specification?

I think C2y (expected to be C29) will do something like this for switch statements! Syntax like:

case 1 ... case 99:

also inb4 the preprocessor lets you do for loops lol. turing complete c preprocessor when?

sometimes I kinda wish C had the C++ constexpr stuff

Agreed, guaranteed compile time evaluation would go hard asf.

NArmistead · 2026-05-30T04:19:35Z

+FLASH_STATUS flash_is_flash_busy
+    (
+    HFLASH_BUFFER* pflash_handle
+    ) 
+{
+if ( flash_get_status( pflash_handle ) != FLASH_OK )


should there be a check that pflash_handle is not NULL?

We can. There was a colton-era assumption that the flash handle would always be non-null, but we practice defensive programming and should null-check on new drivers.

NArmistead · 2026-05-30T04:28:59Z

+FLASH_STATUS flash_init 
+	(
+	HFLASH_BUFFER* pflash_handle
+	)
+{


should there be a check that pflash_handle is not NULL?

Null check in place.

NArmistead · 2026-05-31T19:11:24Z

+    if ( timeout_ms != 0xFFFFFFFFu )
+        {
+        if ( ( HAL_GetTick() - start ) > timeout_ms )


I think these two if statements can be consolidated into one

yeah why did i do that lol

NArmistead · 2026-05-31T19:28:28Z

+  * @file           : MX25L51245GZ2I-08G.c
+  * @brief          : Driver for the flash chip on FC rev 3.
+  ******************************************************************************
+  * @attention


should we be using @copyright?

I believe I inherited this from STM, but will revisit this when I get home. Either way @copyright is probably the correct tag for doxygen generation.

NArmistead · 2026-05-31T19:31:06Z

+    }
+
+/* Validate addresses */
+if( end_addr > FLASH_MAX_ADDR )


nit: space before parenthesis

266-750Balloons

Again, flash isn't my area of expertise, but I don't see anything problematic here.

266-750Balloons · 2026-05-31T22:39:56Z

 	FLASH_BLOCK_13   ,
 	FLASH_BLOCK_14   ,
-	FLASH_BLOCK_15    
+	FLASH_BLOCK_15   ,


I have to say: these kinds of inits made me wish C (and more languages in general) just had a way to create these kinds of types that are basically just ranges without crazy enums or (in the case of OOP languages like C++) a janky setup of operator overloading.

Is it time for SDR to contribute to the ISO C specification?

NArmistead

LGTM!

ETSells and others added 7 commits March 9, 2026 20:14

checkpoint

02b624f

Start of a flash driver

cbdb79b

Merge branch 'main' into feature/ES/r3-flash

7215016

Add "legacy" disclaimer to flash.

473eda0

Finish initialization routines

81e5a89

Possibly finished flash driver?

aa1740e

Final tweaks

5fa3056

ETSells mentioned this pull request May 5, 2026

Initial Rev 3 flash driver SunDevilRocketry/Flight-Computer-Firmware#261

Merged

19 tasks

ETSells requested review from 266-750Balloons and MasterUser43 May 5, 2026 19:58

ETSells requested a review from NArmistead May 23, 2026 20:00

NArmistead reviewed May 31, 2026

View reviewed changes

266-750Balloons approved these changes May 31, 2026

View reviewed changes

Better null-checks for safety

b9cef24

ETSells requested a review from NArmistead June 1, 2026 00:09

oopsie

60bef1e

NArmistead approved these changes Jun 1, 2026

View reviewed changes

ETSells merged commit e1bbfee into main Jun 2, 2026
6 checks passed

Uh oh!

Conversation

ETSells commented May 5, 2026 • edited by NArmistead Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue Link

Testing

Other

Reviewer Checklist

Standards

Error Handling

Memory

Performance

Uh oh!

ETSells commented May 23, 2026

Uh oh!

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

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!

266-750Balloons left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NArmistead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ETSells commented May 5, 2026 •

edited by NArmistead

Loading