From f65c02c5e07ed7f4417b2da255792528b85ac286 Mon Sep 17 00:00:00 2001 From: Bruce Luckcuck Date: Thu, 28 May 2020 12:43:13 -0400 Subject: [PATCH] Blackbox flush SD card sector cache after writing header and before logging starts The normal "flush" for SD card only queues a cache sector for writing and the actual sync to the media happens asynchronously. During this period the cache entry is not available until the write completes sometime later. So as the blackbox header fields were written they end up consuming the majority of the cache. A "flush" was made before actual logging starts, but the async writes were not completing fast enough to ensure available cache sectors for the actual logging. This resulted in the cache getting overwritten and corrupting the header. Changed to wait until the sector cache completes writing to the media before starting the actual logging. This ensures that the logging has ample cache sectors. Changes only affect SD card logging. --- src/main/blackbox/blackbox.c | 28 +++++++++++++++++++--------- src/main/blackbox/blackbox_io.c | 28 +++++++++++++++++++++++++--- src/main/blackbox/blackbox_io.h | 2 ++ src/main/io/asyncfatfs/asyncfatfs.c | 24 +++++++++++++++++++----- src/main/io/asyncfatfs/asyncfatfs.h | 1 + 5 files changed, 66 insertions(+), 17 deletions(-) diff --git a/src/main/blackbox/blackbox.c b/src/main/blackbox/blackbox.c index 33fe7f08c3..3fd7cbf638 100644 --- a/src/main/blackbox/blackbox.c +++ b/src/main/blackbox/blackbox.c @@ -42,17 +42,13 @@ #include "common/time.h" #include "common/utils.h" +#include "config/config.h" #include "config/feature.h" -#include "pg/pg.h" -#include "pg/pg_ids.h" -#include "pg/motor.h" -#include "pg/rx.h" #include "drivers/compass/compass.h" #include "drivers/sensor.h" #include "drivers/time.h" -#include "config/config.h" #include "fc/board_info.h" #include "fc/controlrate_profile.h" #include "fc/rc.h" @@ -70,6 +66,11 @@ #include "io/gps.h" #include "io/serial.h" +#include "pg/pg.h" +#include "pg/pg_ids.h" +#include "pg/motor.h" +#include "pg/rx.h" + #include "rx/rx.h" #include "sensors/acceleration.h" @@ -281,6 +282,7 @@ typedef enum BlackboxState { BLACKBOX_STATE_SEND_GPS_G_HEADER, BLACKBOX_STATE_SEND_SLOW_HEADER, BLACKBOX_STATE_SEND_SYSINFO, + BLACKBOX_STATE_CACHE_FLUSH, BLACKBOX_STATE_PAUSED, BLACKBOX_STATE_RUNNING, BLACKBOX_STATE_SHUTTING_DOWN, @@ -1607,6 +1609,8 @@ STATIC_UNIT_TESTED void blackboxLogIteration(timeUs_t currentTimeUs) */ void blackboxUpdate(timeUs_t currentTimeUs) { + static BlackboxState cacheFlushNextState; + switch (blackboxState) { case BLACKBOX_STATE_STOPPED: if (ARMING_FLAG(ARMED)) { @@ -1680,7 +1684,8 @@ void blackboxUpdate(timeUs_t currentTimeUs) //On entry of this state, xmitState.headerIndex is 0 and xmitState.u.fieldIndex is -1 if (!sendFieldDefinition('S', 0, blackboxSlowFields, blackboxSlowFields + 1, ARRAYLEN(blackboxSlowFields), NULL, NULL)) { - blackboxSetState(BLACKBOX_STATE_SEND_SYSINFO); + cacheFlushNextState = BLACKBOX_STATE_SEND_SYSINFO; + blackboxSetState(BLACKBOX_STATE_CACHE_FLUSH); } break; case BLACKBOX_STATE_SEND_SYSINFO: @@ -1694,9 +1699,14 @@ void blackboxUpdate(timeUs_t currentTimeUs) * (overflowing circular buffers causes all data to be discarded, so the first few logged iterations * could wipe out the end of the header if we weren't careful) */ - if (blackboxDeviceFlushForce()) { - blackboxSetState(BLACKBOX_STATE_RUNNING); - } + cacheFlushNextState = BLACKBOX_STATE_RUNNING; + blackboxSetState(BLACKBOX_STATE_CACHE_FLUSH); + } + break; + case BLACKBOX_STATE_CACHE_FLUSH: + // Flush the cache and wait until all possible entries have been written to the media + if (blackboxDeviceFlushForceComplete()) { + blackboxSetState(cacheFlushNextState); } break; case BLACKBOX_STATE_PAUSED: diff --git a/src/main/blackbox/blackbox_io.c b/src/main/blackbox/blackbox_io.c index da8886475d..b3515091bb 100644 --- a/src/main/blackbox/blackbox_io.c +++ b/src/main/blackbox/blackbox_io.c @@ -242,9 +242,11 @@ bool blackboxDeviceFlushForce(void) #ifdef USE_SDCARD case BLACKBOX_DEVICE_SDCARD: - /* SD card will flush itself without us calling it, but we need to call flush manually in order to check - * if it's done yet or not! - */ + // SD card will flush itself without us calling it, but we need to call flush manually in order to check + // if it's done yet or not! + // However the "flush" only queues one dirty sector each time and the process is asynchronous. So after + // the last dirty sector is queued the flush returns true even though the sector may not actually have + // been physically written to the SD card yet. return afatfs_flush(); #endif // USE_SDCARD @@ -253,6 +255,26 @@ bool blackboxDeviceFlushForce(void) } } +// Flush the blackbox device and only return true if sync is actually complete. +// Primarily to ensure the async operations of SD card sector writes complete thus freeing the cache entries. +bool blackboxDeviceFlushForceComplete(void) +{ + switch (blackboxConfig()->device) { +#ifdef USE_SDCARD + case BLACKBOX_DEVICE_SDCARD: + if (afatfs_sectorCacheInSync()) { + return true; + } else { + blackboxDeviceFlushForce(); + return false; + } +#endif // USE_SDCARD + + default: + return blackboxDeviceFlushForce(); + } +} + /** * Attempt to open the logging device. Returns true if successful. */ diff --git a/src/main/blackbox/blackbox_io.h b/src/main/blackbox/blackbox_io.h index 52af9d5d2f..ba3ec1cc18 100644 --- a/src/main/blackbox/blackbox_io.h +++ b/src/main/blackbox/blackbox_io.h @@ -46,6 +46,8 @@ int blackboxWriteString(const char *s); void blackboxDeviceFlush(void); bool blackboxDeviceFlushForce(void); +bool blackboxDeviceFlushForceComplete(void); + bool blackboxDeviceOpen(void); void blackboxDeviceClose(void); diff --git a/src/main/io/asyncfatfs/asyncfatfs.c b/src/main/io/asyncfatfs/asyncfatfs.c index 871ad1382e..3b4b2db9e8 100644 --- a/src/main/io/asyncfatfs/asyncfatfs.c +++ b/src/main/io/asyncfatfs/asyncfatfs.c @@ -44,21 +44,23 @@ #include #endif -#include "asyncfatfs.h" - -#include "fat_standard.h" -#include "drivers/sdcard.h" #include "common/maths.h" #include "common/time.h" #include "common/utils.h" +#include "drivers/sdcard.h" + +#include "fat_standard.h" + +#include "asyncfatfs.h" + #ifdef AFATFS_DEBUG #define ONLY_EXPOSE_FOR_TESTING #else #define ONLY_EXPOSE_FOR_TESTING static #endif -#define AFATFS_NUM_CACHE_SECTORS 10 +#define AFATFS_NUM_CACHE_SECTORS 11 // FAT filesystems are allowed to differ from these parameters, but we choose not to support those weird filesystems: #define AFATFS_SECTOR_SIZE 512 @@ -736,6 +738,18 @@ static void afatfs_cacheFlushSector(int cacheIndex) } } +// Check whether every sector in the cache that can be flushed has been synchronized +bool afatfs_sectorCacheInSync(void) +{ + for (int i = 0; i < AFATFS_NUM_CACHE_SECTORS; i++) { + if ((afatfs.cacheDescriptor[i].state == AFATFS_CACHE_STATE_WRITING) || + ((afatfs.cacheDescriptor[i].state == AFATFS_CACHE_STATE_DIRTY) && !afatfs.cacheDescriptor[i].locked)) { + return false; + } + } + return true; +} + /** * Find a sector in the cache which corresponds to the given physical sector index, or NULL if the sector isn't * cached. Note that the cached sector could be in any state including completely empty. diff --git a/src/main/io/asyncfatfs/asyncfatfs.h b/src/main/io/asyncfatfs/asyncfatfs.h index 99750f1e66..67906719e2 100644 --- a/src/main/io/asyncfatfs/asyncfatfs.h +++ b/src/main/io/asyncfatfs/asyncfatfs.h @@ -93,3 +93,4 @@ bool afatfs_isFull(void); afatfsFilesystemState_e afatfs_getFilesystemState(void); afatfsError_e afatfs_getLastError(void); +bool afatfs_sectorCacheInSync(void);