Make w25n01g FLASH driver non-blocking for SPI #13555

SteveCEvans · 2024-04-19T21:44:52Z

Previously writes to the W25N01G FLASH device would block for ~160us with an H743 processor due to the code waiting for a sequence of transfers to complete. This is now reduced to under 4us and the FLASH writes then proceed in the background.

Note that QSPI operation is as before and will still be badly non-deterministic.

Impacted FCs are:

ALIENFLIGHTNGF7
AOCODARCH7DUAL
ARESF7
BAYCK_DOLPHIN
BAYCK_DOLPHIN_MINI
CLRACINGF7
FREEAXISF4
GREATMOUNTAINF435
IFLIGHT_BLITZ_F722
IFLIGHT_BLITZ_F7_PRO
IFLIGHT_F405_TWING
IFLIGHT_F722_TWING
IFLIGHT_F745_AIO
IFLIGHT_H7_TWING
JHEF405PRO
JHEH743_HD
KAKUTEH7MINI
KAKUTEH7V2
MAMBAH743
MAMBAH743_2022B
NEUTRONRCF435SE
NEUTRONRCF745
NEUTRONRCF7AIO
NEUTRONRCH743AIO
NEUTRONRCH7BT
SDMODELH7
SPRACINGH7EXTREME
SPRACINGH7NANO
SPRACINGH7ZERO
TAKERF722SE
TMOTORH743
TMOTORVELOXF7SE
VEKTORF722
ZEEZF7

Tested on a MAMBAH743.

github-actions · 2024-04-19T21:45:03Z

Do you want to test this code? You can flash it directly from Betaflight Configurator:

Simply put #13555 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

haslinghuis

Tested on SDMODELH7

BTFL_BLACKBOX_LOG_20240420_000417_SDMODELH7.zip

src/main/drivers/flash_w25n01g.c

ledvinap

Well ..

this will be trivial with RTOS

Performance benefit is obvious. But this can introduce very subtle bugs that will take ages to fix. It may be worth spending some time to make it cleaner.

One possible improvement is to modify segments to allow inline data storage, It will remove need for most static variables and help in other code too.

typedef struct busSegment_s {
    union {
        struct {
            // Transmit buffer
            union { 
                uint8_t *txData;
                uint8_t txInline[4]; 
            };
            // Receive buffer, or in the case of the final segment to
            union {
                 uint8_t *rxData;
                 uint8_t rxInline[4];  
            };
        } buffers;
        struct {
            // Link to the device associated with the next transfer
            const extDevice_t *dev;
            // Segments to process in the next transfer.
            volatile struct busSegment_s *segments;
        } link;
    } u;
    uint16_t len;
    uint16_t segmentFlags;
    busStatus_e (*callback)(uint32_t arg);
} busSegment_t;

#define SEGMENTF_NRGATECS 0x01
#define SEGMENTF_TXINLINE   0x02
#define SEGMENTF_RXINLINE   0x04

src/main/drivers/flash_w25n01g.c

ledvinap · 2024-04-20T08:55:23Z

src/main/drivers/flash_w25n01g.c

+        {.u.buffers = {readStatus, readyStatus}, sizeof(readStatus), true, w25n01g_callbackReady},
+        {.u.buffers = {writeEnable, NULL}, sizeof(writeEnable), true, w25n01g_callbackWriteEnable},
+        {.u.buffers = {progRandomProgDataLoad, NULL}, sizeof(progRandomProgDataLoad), false, NULL},
+        {.u.buffers = {NULL, NULL}, 0, true, NULL},


If possible, comment what will end up in given entry

OK, will do, but the callback shows how the read data is processed.

Problem is that here is only {NULL, NULL}, 0, true, NULL}. Something will be filled in, but in different place (lines with command referenced are fine).
And when reading actual sequence construction code, only indexes are used,
So there is no place to review full sequence that will be executed.

src/main/drivers/flash_w25n01g.c

SteveCEvans · 2024-04-20T12:35:18Z

Well ..

this will be trivial with RTOS

Performance benefit is obvious. But this can introduce very subtle bugs that will take ages to fix. It may be worth spending some time to make it cleaner.

One possible improvement is to modify segments to allow inline data storage, It will remove need for most static variables and help in other code too.

typedef struct busSegment_s {
    union {
        struct {
            // Transmit buffer
            union { 
                uint8_t *txData;
                uint8_t txInline[4]; 
            };
            // Receive buffer, or in the case of the final segment to
            union {
                 uint8_t *rxData;
                 uint8_t rxInline[4];  
            };
        } buffers;
        struct {
            // Link to the device associated with the next transfer
            const extDevice_t *dev;
            // Segments to process in the next transfer.
            volatile struct busSegment_s *segments;
        } link;
    } u;
    uint16_t len;
    uint16_t segmentFlags;
    busStatus_e (*callback)(uint32_t arg);
} busSegment_t;

#define SEGMENTF_NRGATECS 0x01
#define SEGMENTF_TXINLINE   0x02
#define SEGMENTF_RXINLINE   0x04

Well, it’s not an RTOS!

I’d rather have clearly named static variables than bring them as anonymous data inside a static segment structure. This keeps the segments as clean and simple as possible.

SteveCEvans · 2024-04-21T12:50:56Z

@ledvinap The code should be much more readable. I also considered the code paths, and proved them using a logic analyser and the initial flash is now removed simplifying the code further.

ledvinap

Great work, thanks. Now it is quite easy to follow.

BTW: flashfs write code is expecting pageProgramContinue to handle multiple buffers (at least in sync flush). But from quick look, it isn't flashfs main problem ...

ledvinap · 2024-04-21T12:44:15Z

src/main/drivers/flash_w25n01g.c

+
+    // Call transfer completion callback
+    if (fdevice->callback) {
+        fdevice->callback(fdevice->callbackArg);


io/flashfs.c is not interrupt-safe (mainly flashfsAdvanceTailInBuffer). Eventually, it should be fixed (IMO caching bufferTail in local variable before testing will be enough)

We only update bufferTail in flashfsAdvanceTailInBuffer (except following an erase) so I think this is safe. Can't think of a failure case, but perhaps I'm not thinking hard enough.

Problem is with reading it (flashfsGetDirtyDataBuffers is affected too).

bufferHead = 5; bufferTail = 9; FLASHFS_WRITE_BUFFER_SIZE = 10; static uint32_t flashfsTransmitBufferUsed(void) { if (bufferHead >= bufferTail) return bufferHead - bufferTail; // ISR update bufferTail, incrementing it by 1 bytes and wrapping around, bufferTail == 0 return FLASHFS_WRITE_BUFFER_SIZE - bufferTail + bufferHead; // 15 is returned }

In this case, fix is trivial. Function may return slightly old value, but that is expected:

static uint32_t flashfsTransmitBufferUsed(void) { unsigned tail = bufferTail; if (bufferHead >= tail) return bufferHead - tail; return FLASHFS_WRITE_BUFFER_SIZE - tail + bufferHead; }

The problem is not that obvious, because flashfsWriteByte is even more buggy and will silently wraparound buffer. And blackbox tries to prevent overflowing buffers, so this is not hit too often.

(I have learned to take these problems very seriously - bug happening once in a century (per device) is much worse than the one happening every day, ale least if you manage thousands of devices)

ledvinap · 2024-04-21T12:49:22Z

src/main/drivers/flash_w25n01g.c

+    uint32_t columnAddress;
+
+    if (bufferDirty) {
+        columnAddress = W25N01G_LINEAR_TO_COLUMN(programLoadAddress);


(you can reduce variable scope here)

Suggested change

columnAddress = W25N01G_LINEAR_TO_COLUMN(programLoadAddress);

uint32_t columnAddress = W25N01G_LINEAR_TO_COLUMN(programLoadAddress);

columnAddress is now used at line 703 and 712 so I expanded the scope.

((But that one and can have it's own scope. Sorry, this is too minor problem, don't worry about it))

ledvinap · 2024-04-21T12:57:54Z

src/main/drivers/flash_w25n01g.c

+        // Set the address and buffer details for the random data load
+        progRandomProgDataLoad[1] = (columnAddress >> 8) & 0xff;
+        progRandomProgDataLoad[2] = columnAddress & 0xff;
+        segmentsRandomDataLoad[3].u.buffers.txData = (uint8_t *)buffers[0];


Side note: both u and buffers can be anonymous in segment structure, segmentsRandomDataLoad[3].txData = will mask internal structure layout. Only disadvantage is that overlap with link is not that obvious.
But that may be my laziness when typing

src/main/drivers/flash_w25n01g.c

ledvinap · 2024-04-21T13:08:57Z

src/main/drivers/flash_w25n01g.c

+        programStartAddress = programLoadAddress;
+    } else {
+        // Callback on completion of data load
+        programSegment[3].callback = w25n01g_callbackWriteComplete;


I'd play it safe and clear link explicitly. BUS_ERROR (or future refactoring) may prevent link clearing code to work, and consequences can be ugly.

spiLinkSegments(NULL, programSegment, NULL);

The clearing is done automatically at

betaflight/src/main/drivers/bus_spi.c

Line 441 in f4d6a2c

nextSegment->u.link.dev = NULL;

This is needed because the linking is the mechanism for queuing requests and such linkage between segments from different drivers MUST be broken like this. This can't ever be refactored away.

(Caller may be required to clear last link of inserted chain. But as cycle prevention, ISR clearing is good idea.
It will get really interesting if one driver ends with BUS_ERROR - linked chains are not terminated and unlinked.)

IMO it is slightly better to break link explicitly here - normal links are created by driver chaining code, that code is responsible to undo it. But here the link was created explicitly, it is safer to undo it.

ledvinap · 2024-04-21T13:12:08Z

src/main/drivers/bus_spi.c

@@ -744,6 +744,19 @@ uint8_t spiGetExtDeviceCount(const extDevice_t *dev)
    return dev->bus->deviceCount;
 }

+// Link two segment lists
+// Note that there is no need to unlink segment lists as this is done automatically as they are processed
+void spiLinkSegments(const extDevice_t *dev, busSegment_t *firstSegment, busSegment_t *secondSegment)


Maybe change argument order to object, params... This function is not (directly) working on device, but is appending something to segment chain (*firstSegment)

Suggested change

void spiLinkSegments(const extDevice_t *dev, busSegment_t *firstSegment, busSegment_t *secondSegment)

void spiLinkSegments(busSegment_t *firstSegment, const extDevice_t *dev, busSegment_t *secondSegment)

I did it this way as the dev argument is always passed first in all other routines.

IMO the difference is that other functions have dev as object they are working on. It becomes obvious when you try passing NULL to terminate segments.

ledvinap · 2024-04-21T13:13:44Z

src/main/drivers/bus_spi.c

@@ -744,6 +744,19 @@ uint8_t spiGetExtDeviceCount(const extDevice_t *dev)
    return dev->bus->deviceCount;
 }

+// Link two segment lists
+// Note that there is no need to unlink segment lists as this is done automatically as they are processed


Except for BUS_ABORT ... see comment below

Ouch. Luckily nothing ever uses that case. I'll change the code to skip forward to the end of the segment list rather than just return. Thanks for this!

ledvinap · 2024-04-21T13:33:07Z

(@SteveCEvans : can you avoid force-pushing commits when not necessary, please? Incremental changes in commits are easier to follow)

nerdCopter · 2024-04-21T14:43:45Z

OMG i DID NOT MEAN TO PRESS MERGE, SO SORRY!!!!

SteveCEvans · 2024-04-21T14:49:01Z

Great work, thanks. Now it is quite easy to follow.

BTW: flashfs write code is expecting pageProgramContinue to handle multiple buffers (at least in sync flush). But from quick look, it isn't flashfs main problem ...

The m25p16_pageProgramContinue() version does handle multiple buffers. I could certainly stitch on the extras to w25n01g_pageProgramContinue() too. The original code didn't so I thought it best to constrain the scope of the changes and come back to this.

SteveCEvans · 2024-04-21T15:13:49Z

(@SteveCEvans : can you avoid force-pushing commits when not necessary, please? Incremental changes in commits are easier to follow)

Sorry, just trying to keep it tidy, but you're right that's best left until the merge into master.

SteveCEvans · 2024-04-21T15:14:26Z

OMG i DID NOT MEAN TO PRESS MERGE, SO SORRY!!!!

Ooooops. How do we rewind that so we can take this PR to its conclusion properly?

haslinghuis · 2024-04-21T15:17:54Z

Either we revert or continue in subsequent PR. Had tested the current code and it works.

ledvinap · 2024-04-21T15:18:09Z

Just live with it. If necessary, we can fasttrack bugfix PR.

)" This reverts commit f4d6a2c.

nerdCopter · 2024-04-21T15:20:42Z

i opened a revert PR, if you want. i again, deeply apologize
#13560

SteveCEvans · 2024-04-21T16:25:25Z

Great work, thanks. Now it is quite easy to follow.
BTW: flashfs write code is expecting pageProgramContinue to handle multiple buffers (at least in sync flush). But from quick look, it isn't flashfs main problem ...

The m25p16_pageProgramContinue() version does handle multiple buffers. I could certainly stitch on the extras to w25n01g_pageProgramContinue() too. The original code didn't so I thought it best to constrain the scope of the changes and come back to this.

I'm currently looking at this.

And I've made other commits since the merge.

SteveCEvans · 2024-04-21T16:26:09Z

I'll create a follow on PR then.

SteveCEvans · 2024-04-21T19:00:20Z

I found that writing multiple buffers at once resulted in the FLASH not keeping up and gaps appearing at 8kHz logging rate.

SteveCEvans · 2024-04-21T19:01:07Z

i opened a revert PR, if you want. i again, deeply apologize #13560

No worries. #13562 tidies things up.

SteveCEvans requested review from ledvinap, blckmn, haslinghuis, ctzsnooze, KarateBrot and sugaarK April 19, 2024 21:44

SteveCEvans self-assigned this Apr 19, 2024

haslinghuis added the RN: BUGFIX label Apr 19, 2024

haslinghuis added this to the 4.5 milestone Apr 19, 2024

blckmn approved these changes Apr 19, 2024

View reviewed changes

haslinghuis approved these changes Apr 19, 2024

View reviewed changes

haslinghuis reviewed Apr 19, 2024

View reviewed changes

src/main/drivers/flash_w25n01g.c Outdated Show resolved Hide resolved

haslinghuis reviewed Apr 19, 2024

View reviewed changes

src/main/drivers/flash_w25n01g.c Outdated Show resolved Hide resolved

SteveCEvans force-pushed the nonblocking_w25n01g branch from d726c96 to 55390be Compare April 20, 2024 07:29

ledvinap reviewed Apr 20, 2024

View reviewed changes

SteveCEvans force-pushed the nonblocking_w25n01g branch from 55390be to 595c40e Compare April 21, 2024 12:01

Make w25n01g FLASH driver non-blocking for SPI

0d8d6fa

SteveCEvans force-pushed the nonblocking_w25n01g branch from 595c40e to 0d8d6fa Compare April 21, 2024 12:50

ledvinap approved these changes Apr 21, 2024

View reviewed changes

This comment was marked as off-topic.

Sign in to view

nerdCopter merged commit f4d6a2c into betaflight:master Apr 21, 2024
23 checks passed

nerdCopter added a commit to nerdCopter/betaflight that referenced this pull request Apr 21, 2024

Revert "Make w25n01g FLASH driver non-blocking for SPI (betaflight#13555

a7d2e74

)" This reverts commit f4d6a2c.

SteveCEvans mentioned this pull request Apr 21, 2024

Nonblocking w25n01g code tidy up #13562

Merged

ledvinap mentioned this pull request Apr 22, 2024

bug - flash - SPRASINGH7NANO can't handle 100MHz #13571

Closed

hydra mentioned this pull request Apr 22, 2024

Revert "Make w25n01g FLASH driver non-blocking for SPI (#13555)" #13560

Closed

	columnAddress = W25N01G_LINEAR_TO_COLUMN(programLoadAddress);
	uint32_t columnAddress = W25N01G_LINEAR_TO_COLUMN(programLoadAddress);

	void spiLinkSegments(const extDevice_t dev, busSegment_t firstSegment, busSegment_t *secondSegment)
	void spiLinkSegments(busSegment_t firstSegment, const extDevice_t dev, busSegment_t *secondSegment)

Make w25n01g FLASH driver non-blocking for SPI #13555

Make w25n01g FLASH driver non-blocking for SPI #13555

Conversation

SteveCEvans commented Apr 19, 2024 • edited

github-actions bot commented Apr 19, 2024

haslinghuis left a comment

Choose a reason for hiding this comment

ledvinap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveCEvans commented Apr 20, 2024

SteveCEvans commented Apr 21, 2024

ledvinap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ledvinap commented Apr 21, 2024

This comment was marked as off-topic.

nerdCopter commented Apr 21, 2024 • edited

SteveCEvans commented Apr 21, 2024

SteveCEvans commented Apr 21, 2024

SteveCEvans commented Apr 21, 2024

haslinghuis commented Apr 21, 2024

ledvinap commented Apr 21, 2024

nerdCopter commented Apr 21, 2024

SteveCEvans commented Apr 21, 2024

SteveCEvans commented Apr 21, 2024

SteveCEvans commented Apr 21, 2024

SteveCEvans commented Apr 21, 2024 • edited

SteveCEvans commented Apr 19, 2024 •

edited

nerdCopter commented Apr 21, 2024 •

edited

SteveCEvans commented Apr 21, 2024 •

edited