Fix/erase sector timing

.gitignore

@@ -7,3 +7,4 @@
 .DS_Store
 build/
 *.csv
+.codex

AGENTS.md

@@ -105,8 +105,9 @@ python -m http.server --directory build/doxygen 8000
 
 1. Prefer small, focused patches.
 2. If behavior changes, update or add tests in `test/` to cover it.
-3. Preserve public API names unless the task explicitly asks for a breaking change.
-4. Keep docs in sync when changing module behavior:
+3. If you are patching a bug, add a regression test that fails without the fix and passes with it.
+4. Preserve public API names unless the task explicitly asks for a breaking change.
+5. Keep docs in sync when changing module behavior:
    - top-level `README.md` for high-level behavior,
    - `docs/*.md` for detailed formats/protocols.
 

include/data_handling/DataSaverSPI.h

@@ -6,6 +6,7 @@
 #include "data_handling/DataSaver.h"
 #include <array>
 #include <cstdlib>
+#include <limits>
 
 
 constexpr uint32_t kMetadataStartAddress = 0x000000;  // Start writing metadata at the beginning of flash
@@ -69,12 +70,16 @@ class DataSaverSPI : public IDataSaver {
      * @param timestamp_ms Timestamp in milliseconds to record.
      * @note When to use: emitted internally when gaps exceed
      *       timestampInterval_ms_, or explicitly in tests.
+     * @return int 0 on success, 1 when writes are blocked by post-launch state,
+     *         and -1 on write/buffer error.
      */
     int saveTimestamp(uint32_t timestamp_ms);
 
     /**
      * @brief Initialize the flash chip and metadata.
      * @note When to use: call during setup before any saveDataPoint usage.
+     * @return true when flash is initialized and writable for logging.
+     * @return false when initialization fails or chip is already in post-launch mode.
      */
     virtual bool begin() override;
 
@@ -116,6 +121,7 @@ class DataSaverSPI : public IDataSaver {
      * 
      * The rocket may not be recovered for several hours, this prevents the cool launch data
      * from being overwitten with boring laying-on-the-ground data.
+     * @param launchTimestamp_ms Timestamp at which launch was detected.
      */
     void launchDetected(uint32_t launchTimestamp_ms);
 
@@ -125,6 +131,7 @@ class DataSaverSPI : public IDataSaver {
      * @param ignoreEmptyPages  Skip pages that appear unwritten.
      * @note When to use: post-flight data retrieval before erasing or
      *       redeploying the flash chip.
+     * @return void
      */
     void dumpData(Stream &serial, bool ignoreEmptyPages);
 
@@ -146,6 +153,7 @@ class DataSaverSPI : public IDataSaver {
 
     /**
      * @brief Returns the last timestamp that was actually written to flash.
+     * @return Last persisted timestamp in milliseconds.
      */
     uint32_t getLastTimestamp() const {
         return lastTimestamp_ms_;
@@ -154,15 +162,24 @@ class DataSaverSPI : public IDataSaver {
     /**
      * @brief Returns the last DataPoint that was written (not necessarily
      *        including timestamp, just the data chunk).
+     * @return Copy of the last cached data point.
      */
     DataPoint getLastDataPoint() const {
         return lastDataPoint_;
     }
 
+    /**
+     * @brief Returns the launch-protected address computed during launch detection.
+     * @return Address in flash used as post-launch protection boundary.
+     */
     uint32_t getLaunchWriteAddress() const {
         return launchWriteAddress_;
     }
 
+    /**
+     * @brief Returns the next flash address where a full page will be written.
+     * @return Next write address in flash.
+     */
     uint32_t getNextWriteAddress() const {
         return nextWriteAddress_;
     }
@@ -178,6 +195,7 @@ class DataSaverSPI : public IDataSaver {
     /**
      * @brief Returns whether the flash chip is in post-launch mode
      *       without updating the post-launch mode flag or reading from flash.
+     * @return true when in post-launch mode; otherwise false.
      */
     bool quickGetPostLaunchMode() {
         return this->postLaunchMode_;
@@ -211,11 +229,18 @@ class DataSaverSPI : public IDataSaver {
     /**
      * @brief Helper to write a block of bytes to flash at the current
      *        write address and advance that pointer.
+     * @param data Pointer to bytes to write.
+     * @param length Number of bytes to write.
+     * @return true on successful write and pointer advance; otherwise false.
      */
     bool writeToFlash(const uint8_t* data, size_t length);
 
     /**
      * @brief Helper to read a block of bytes from flash (updates read pointer externally).
+     * @param readAddress Input/output flash address. Advanced by `length` on success.
+     * @param buffer Output byte buffer.
+     * @param length Number of bytes to read.
+     * @return true on successful read and pointer advance; otherwise false.
      */
     bool readFromFlash(uint32_t& readAddress, uint8_t* buffer, size_t length);
 
@@ -228,6 +253,7 @@ class DataSaverSPI : public IDataSaver {
     /**
      * @brief Returns the current buffer index
      * Useful for testing
+     * @return Number of bytes currently buffered.
      */
     size_t getBufferIndex() const {
         return bufferIndex_;
@@ -236,20 +262,74 @@ class DataSaverSPI : public IDataSaver {
     /**
      * @brief Returns the current buffer flush count
      * Useful for testing
+     * @return Number of successful page flushes.
      */
     uint32_t getBufferFlushes() const {
         return bufferFlushes_;
     }
 
+    /**
+     * @brief Returns whether writes have been stopped by post-launch protection.
+     * @return true if chip-full post-launch protection latch is set; otherwise false.
+     */
     bool getIsChipFullDueToPostLaunchProtection() const {
         return isChipFullDueToPostLaunchProtection_;
     }
 
+    /**
+     * @brief Returns whether startup detected existing post-launch mode metadata.
+     * @return true if rebooted in post-launch mode; otherwise false.
+     */
     bool getRebootedInPostLaunchMode() const {
         return rebootedInPostLaunchMode_;
     }
 
+#ifdef UNIT_TEST
+    /**
+     * @brief Test-only helper to override internal post-launch state.
+     * @param nextWriteAddress_in Next write address to inject.
+     * @param launchWriteAddress_in Launch-protected address to inject.
+     * @param postLaunchMode_in Post-launch mode flag to inject.
+     */
+    void setPostLaunchStateForTest(uint32_t nextWriteAddress_in,
+                                   uint32_t launchWriteAddress_in,
+                                   bool postLaunchMode_in) {
+        nextWriteAddress_ = nextWriteAddress_in;
+        launchWriteAddress_ = launchWriteAddress_in;
+        postLaunchMode_ = postLaunchMode_in;
+        isChipFullDueToPostLaunchProtection_ = false;
+    }
+#endif
+
 private:
+    /**
+     * @brief Normalizes an address into the data region when crossing flash end.
+     * @param address Candidate flash address.
+     * @return `address` when in range, otherwise `kDataStartAddress`.
+     */
+    uint32_t normalizeDataAddress(uint32_t address) const;
+
+    /**
+     * @brief Checks if a sector is protected by post-launch rules.
+     * @param sectorNumber Sector index to evaluate.
+     * @return true when sector contains `launchWriteAddress_` and post-launch mode is active.
+     * @return false otherwise.
+     */
+    bool isProtectedLaunchSector(uint32_t sectorNumber) const;
+
+    /**
+     * @brief Erases a sector unless blocked by post-launch protection.
+     * @param sectorNumber Sector index to erase.
+     * @return int 0 on successful erase, -1 when blocked or flash erase fails.
+     */
+    int eraseSectorIfAllowed(uint32_t sectorNumber);
+
+    /**
+     * @brief Checks if the upcoming write window would violate post-launch protection.
+     * @return true when writing must stop and chip-full protection is latched.
+     * @return false when write may proceed.
+     */
+    bool shouldStopForPostLaunchWindow();
 
     /**
      * @brief Flushes the buffer to flash.
@@ -272,10 +352,20 @@ class DataSaverSPI : public IDataSaver {
 
     // Overloaded functions to add data to the buffer from a Record_t or TimestampRecord_t
     // More efficient than callling addDataToBuffer with each part of the record
+    /**
+     * @brief Adds a 5-byte record payload to the page buffer.
+     * @param record Pointer to packed record bytes.
+     * @return int 0 on success; -1 on flush/buffer error.
+     */
     int addRecordToBuffer(Record_t * record) {
         return addDataToBuffer(reinterpret_cast<const uint8_t*>(record), 5);
     }
 
+    /**
+     * @brief Adds a 5-byte timestamp record payload to the page buffer.
+     * @param record Pointer to packed timestamp record bytes.
+     * @return int 0 on success; -1 on flush/buffer error.
+     */
     int addRecordToBuffer(TimestampRecord_t * record) {
         return addDataToBuffer(reinterpret_cast<const uint8_t*>(record), 5);
     }
@@ -287,6 +377,10 @@ class DataSaverSPI : public IDataSaver {
     // If the flight computer boots and is already in post launch mode, do not write to flash.
     // Calling clearPostLaunchMode() will allow writing to flash again after a reboot.
     bool rebootedInPostLaunchMode_ = false;
+
+    // Tracks which sector has already been pre-erased for the next boundary write.
+    // UINT32_MAX means "no prepared sector".
+    uint32_t preparedSectorNumber_ = std::numeric_limits<uint32_t>::max();
 };
 
 #endif // DATA_SAVER_SPI_H

src/data_handling/DataSaverSPI.cpp

@@ -68,36 +68,96 @@ int DataSaverSPI::addDataToBuffer(const uint8_t* data, size_t length) {
     return 0;
 }
 
+uint32_t DataSaverSPI::normalizeDataAddress(uint32_t address) const {
+    if (address >= flash_->size()) {
+        return kDataStartAddress;
+    }
+    return address;
+}
+
+bool DataSaverSPI::isProtectedLaunchSector(uint32_t sectorNumber) const {
+    if (!postLaunchMode_) {
+        return false;
+    }
+    return sectorNumber == launchWriteAddress_ / SFLASH_SECTOR_SIZE;
+}
+
+int DataSaverSPI::eraseSectorIfAllowed(uint32_t sectorNumber) {
+    if (isProtectedLaunchSector(sectorNumber)) {
+        isChipFullDueToPostLaunchProtection_ = true; // We can't erase the next sector to allow for more writes.
+        return -1;
+    }
+    if (!flash_->eraseSector(sectorNumber)) {
+        return -1;
+    }
+    preparedSectorNumber_ = sectorNumber;
+    return 0;
+}
+
+bool DataSaverSPI::shouldStopForPostLaunchWindow() {
+    if (!postLaunchMode_) {
+        return false;
+    }
+    if (nextWriteAddress_ > launchWriteAddress_) {
+        return false;
+    }
+    if (nextWriteAddress_ + kBufferSize_bytes * 2 <= launchWriteAddress_) {
+        return false;
+    }
+    isChipFullDueToPostLaunchProtection_ = true;
+    return true;
+}
+
 // Write the entire buffer to flash.
 int DataSaverSPI::flushBuffer() {
     if (bufferIndex_ == 0) {
         return 1;  // Nothing to flush
     }
 
-    // Check if we need to wrap around
-    if (nextWriteAddress_ + bufferIndex_ > flash_->size()) {
-        // Wrap around
+    // If the next write would go past the end of the flash, wrap around to the beginning of the data section.
+    if (nextWriteAddress_ + kBufferSize_bytes > flash_->size()) {
         nextWriteAddress_ = kDataStartAddress;
-    }   
+    }
 
-    // Check that we haven't wrapped around to the launch address while in post-launch mode
-    if (postLaunchMode_ && nextWriteAddress_ <= launchWriteAddress_ && nextWriteAddress_ + kBufferSize_bytes * 2 > launchWriteAddress_) {
+    if (shouldStopForPostLaunchWindow()) {
         isChipFullDueToPostLaunchProtection_ = true;
-        return -1; // Indicate no write due to post-launch protection
+        return -1;
     }
 
-    if (nextWriteAddress_ % SFLASH_SECTOR_SIZE == 0) {
-        if (!flash_->eraseSector(nextWriteAddress_ / SFLASH_SECTOR_SIZE)) {
-            return -1;
+    // Fallback erase for first entry into a sector. In steady-state this sector
+    // should already be prepared by the previous flush.
+    if (nextWriteAddress_ % SFLASH_SECTOR_SIZE == 0U) {
+        uint32_t const currentSectorNumber = nextWriteAddress_ / SFLASH_SECTOR_SIZE;
+        if (preparedSectorNumber_ != currentSectorNumber) {
+            if (eraseSectorIfAllowed(currentSectorNumber) < 0) {
+                return -1;
+            }
         }
     }
 
-    // Write 1 page of data
+    // Write 1 page of data.
     if (!flash_->writeBuffer(nextWriteAddress_, buffer_, kBufferSize_bytes)) {
         return -1;
     }
 
-    nextWriteAddress_ += kBufferSize_bytes;  // keep it aligned to the buffer size or page size
+    nextWriteAddress_ = normalizeDataAddress(nextWriteAddress_ + kBufferSize_bytes);
+
+    // Pre-erase one step earlier: after writing the previous page, erase the
+    // sector that the next page will start in. This lets erase latency overlap
+    // with buffer fill time.
+    if (nextWriteAddress_ % SFLASH_SECTOR_SIZE == 0U) {
+        uint32_t const nextSectorNumber = nextWriteAddress_ / SFLASH_SECTOR_SIZE;
+
+        // If the next sector, is protected, skip the erase and still return 0
+        if (!isProtectedLaunchSector(nextSectorNumber)) {
+
+            // Try to erase the next sector and return -1 on failure
+            if (eraseSectorIfAllowed(nextSectorNumber) < 0) {
+                return -1;
+            }
+        }
+    }
+
     bufferIndex_ = 0; // Reset the buffer
     bufferFlushes_++;
     return 0;
@@ -235,6 +295,7 @@ void DataSaverSPI::clearInternalState() {
     launchWriteAddress_ = 0;
     bufferFlushes_ = 0;
     isChipFullDueToPostLaunchProtection_ = false;
+    preparedSectorNumber_ = std::numeric_limits<uint32_t>::max();
 }
 
 void DataSaverSPI::eraseAllData() {