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

[nerdCopter]

This reverts commit f4d6a2ce4344233284db50f89ef9389a1ac11166. (git revert -m 1 f4d6a2ce4)

[github-actions[bot]]

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

• Simply put #13560 (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!

[hydra]

IMHO, this PR is the right way to go as #13555 feels like a new feature, changes a lot and has already had unintended side effects, creates a mismatch between SPI and QSPI and feels incomplete and lacked sufficient testing on all the affected targets.

[haslinghuis]

• Affected targets did not work properly due to driver issues. So this is a bugfix and not a new feature
• There is a follow up in #13562
• Agree we need to pay attention to the QSPI side of things.

[SteveCEvans]

IMHO, this PR is the right way to go as #13555 feels like a new feature, changes a lot and has already had unintended side effects, creates a mismatch between SPI and QSPI and feels incomplete and lacked sufficient testing on all the affected targets.

Of course you’d say that as it doesn’t benefit your FCs, however #13555 makes 30 other FCs usable. You’re free to suggest any changes you’d like in a follow on PR.

[ledvinap]

IMHO, this PR is the right way to go as #13555 feels like a new feature, changes a lot and has already had unintended side effects, creates a mismatch between SPI and QSPI and feels incomplete and lacked sufficient testing on all the affected targets.

Of course you’d say that as it doesn’t benefit your FCs, however #13555 makes 30 other FCs usable. You’re free to suggest any changes you’d like in a follow on PR.

Actually it does brick H7NANO..

[ledvinap]

Of course you’d say that as it doesn’t benefit your FCs, however #13555 makes 30 other FCs usable. You’re free to suggest any changes you’d like in a follow on PR.

13555 introduced serious bug (overwrite of wrong union member), bricking all quad/octo SPI targets with w25n01g.

[SteveCEvans]

Of course you’d say that as it doesn’t benefit your FCs, however #13555 makes 30 other FCs usable. You’re free to suggest any changes you’d like in a follow on PR.

13555 introduced serious bug (overwrite of wrong union member), bricking all quad/octo SPI targets with w25n01g.

Details?

[nerdCopter]

and did https://github.com/betaflight/betaflight/pull/13562 fix or retain the breakages?

EDIT: found : https://github.com/betaflight/betaflight/pull/13562#discussion_r1579169258

[ledvinap]

@nerdCopter : H7EVO and H7RF are still broken

[SteveCEvans]

If USE_QUADSPI is defined then the old w25n01g_pageProgramBegin(), w25n01g_pageProgramContinue() and w25n01g_pageProgramFinish() are used, and the SPI clock is only set if it is undefined. Other than that w25n01g_callbackReady() had its modification of isProgramming removed. As it wasn't used by the SPI code, but the QUAD_SPI code still does maybe that's the issue.

@ledvinap Could you try the following patch.

diff --git a/src/main/drivers/flash_w25n01g.c b/src/main/drivers/flash_w25n01g.c
index 4cb93cd..b109871 100644
--- a/src/main/drivers/flash_w25n01g.c
+++ b/src/main/drivers/flash_w25n01g.c
@@ -792,6 +792,12 @@ void w25n01g_flush(flashDevice_t *fdevice)
         w25n01g_programExecute(fdevice, W25N01G_LINEAR_TO_PAGE(programStartAddress));

         bufferDirty = false;
+
+#ifdef USE_QUADSPI
+        isProgramming = true;
+    } else {
+        isProgramming = false;
+#endif // USE_QUADSPI
     }
 }

I don't believe the only other change to w25n01g_isReady() can be blamed.

[ledvinap]

@SteveCEvans : https://github.com/betaflight/betaflight/pull/13562#discussion_r1579169258

Problem is in w25n01g_identify - it is called from quadspi/octospi code when detecting memory chip. Interface is in quad/octo mode. But spiSetClkDivisor is called on fdevice, overwriting internal HAL structure. Board won't boot in this state.

https://github.com/SteveCEvans/betaflight/blob/12d171e80c8bbbe814d169a5b48ee22f112d4f98/src/main/drivers/flash_w25n01g.c#L356-L359

[SteveCEvans]

But that's only called if USE_QUADSPI isn't defined...

#ifndef USE_QUADSPI
    // Need to set clock speed for 8kHz logging support with SPI
    spiSetClkDivisor(fdevice->io.handle.dev, spiCalculateDivider(100000000));
#endif // USE_QUADSPI

Are you testing current master?

[ledvinap]

and USE_OCTOSPI? Also, technically, SPI_FLASH may probably be used even when USE_QUAD/USE_OCTO is configured.

The hack to fix it is trivial: if (fdevice->io.mode == FLASHIO_SPI) {. But IMO setting speed in detection routine this way is poor design.

w25n01g_deviceInit / w25n01g_configure is better place

[ledvinap]

Are you testing current master?

Actually, I prefer reading the code ;-)

[haslinghuis]

O damn of course - will also need USE_OCTOSPI.

[haslinghuis]

@ledvinap please check #13584

[hydra]

and USE_OCTOSPI? Also, technically, SPI_FLASH may probably be used even when USE_QUAD/USE_OCTO is configured.

that is correct, the H7EF (H730) has a single QuadSPI chip in memory mapped mode and an SPI flash for logging.

[hydra]

Of course you’d say that as it doesn’t benefit your FCs, however #13555 makes 30 other FCs usable. You’re free to suggest any changes you’d like in a follow on PR.

No, I say it for professional development reasons, accidental merges cause unexpected breakages which then has all the developers running around fire fighting and causes undue stress instead of operating in a nice relaxed 'change-test-review' cycle.

[hydra]

• Affected targets did not work properly due to driver issues. So this is a bugfix and not a new feature

ok noted.

@SteveCEvans / @haslinghuis The PR didn't link to any bug that it was attempting to fix, so it wasn't at all clear to me that there was a bug and that it was a bug fix, it just talked about making the spi flash drivers more deterministic, which felt like an improvement not a fix.

[ledvinap]

@hydra : #13555 would have been approved eventually, with the bug still present. And with similar consequences.