pw_kvs: Add tests for FlashPartition

Add test for FlashPartition::IsRegionErased case that can result in a false
negative.

Add FlashPartition::IsErased that checks if the entire partition is
erased.

Add additional tests for FlashPartition

Change-Id: I7f9a1da643b42bac89d0161d288a7ddcf1487649
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/13684
Commit-Queue: David Rogers <davidrogers@google.com>
Reviewed-by: Keir Mierle <keir@google.com>
diff --git a/pw_kvs/flash_memory.cc b/pw_kvs/flash_memory.cc
index c3871f6..aba923b 100644
--- a/pw_kvs/flash_memory.cc
+++ b/pw_kvs/flash_memory.cc
@@ -106,8 +106,9 @@
 }
 
 bool FlashPartition::AppearsErased(std::span<const byte> data) const {
+  byte erased_content = flash_.erased_memory_content();
   for (byte b : data) {
-    if (b != flash_.erased_memory_content()) {
+    if (b != erased_content) {
       return false;
     }
   }
diff --git a/pw_kvs/flash_partition_test.cc b/pw_kvs/flash_partition_test.cc
index df3d1c6..760f814 100644
--- a/pw_kvs/flash_partition_test.cc
+++ b/pw_kvs/flash_partition_test.cc
@@ -29,18 +29,17 @@
 
 constexpr size_t kTestIterations = PW_FLASH_TEST_ITERATIONS;
 
-constexpr size_t kTestDataSize = kMaxFlashAlignment;
+size_t error_count = 0;
 
 void WriteData(FlashPartition& partition, uint8_t fill_byte) {
-  uint8_t test_data[kTestDataSize];
+  uint8_t test_data[kMaxFlashAlignment];
   memset(test_data, fill_byte, sizeof(test_data));
 
-  ASSERT_GE(kTestDataSize, partition.alignment_bytes());
+  const size_t alignment = partition.alignment_bytes();
 
-  partition.Erase(0, partition.sector_count());
+  ASSERT_EQ(Status::OK, partition.Erase(0, partition.sector_count()));
 
-  const size_t chunks_per_sector =
-      partition.sector_size_bytes() / partition.alignment_bytes();
+  const size_t chunks_per_sector = partition.sector_size_bytes() / alignment;
 
   // Fill partition sector by sector. Fill the sector with an integer number
   // of alignment-size chunks. If the sector is not evenly divisible by
@@ -52,17 +51,16 @@
 
     for (size_t chunk_index = 0; chunk_index < chunks_per_sector;
          chunk_index++) {
-      StatusWithSize status = partition.Write(
-          address, as_bytes(std::span(test_data, partition.alignment_bytes())));
+      StatusWithSize status =
+          partition.Write(address, as_bytes(std::span(test_data, alignment)));
       ASSERT_EQ(Status::OK, status.status());
-      ASSERT_EQ(partition.alignment_bytes(), status.size());
-      address += partition.alignment_bytes();
+      ASSERT_EQ(alignment, status.size());
+      address += alignment;
     }
   }
 
   // Check the fill result. Use expect so the test doesn't bail on error.
   // Count the errors and print if any errors are found.
-  size_t error_count = 0;
   for (size_t sector_index = 0; sector_index < partition.sector_count();
        sector_index++) {
     FlashPartition::Address address =
@@ -71,23 +69,33 @@
     for (size_t chunk_index = 0; chunk_index < chunks_per_sector;
          chunk_index++) {
       memset(test_data, 0, sizeof(test_data));
-      StatusWithSize status =
-          partition.Read(address, partition.alignment_bytes(), test_data);
+      StatusWithSize status = partition.Read(address, alignment, test_data);
 
       EXPECT_EQ(Status::OK, status.status());
-      EXPECT_EQ(partition.alignment_bytes(), status.size());
-      if (!status.ok() || (partition.alignment_bytes() != status.size())) {
+      EXPECT_EQ(alignment, status.size());
+      if (!status.ok() || (alignment != status.size())) {
         error_count++;
+        PW_LOG_DEBUG("   Read Error [%s], %u of %u",
+                     status.status().str(),
+                     unsigned(status.size()),
+                     unsigned(alignment));
         continue;
       }
 
-      for (size_t i = 0; i < partition.alignment_bytes(); i++) {
+      for (size_t i = 0; i < alignment; i++) {
         if (test_data[i] != fill_byte) {
           error_count++;
+          PW_LOG_DEBUG(
+              "   Error %u, Read compare @ address %x, got 0x%02x, "
+              "expected 0x%02x",
+              unsigned(error_count),
+              unsigned(address + i),
+              unsigned(test_data[i]),
+              unsigned(fill_byte));
         }
       }
 
-      address += partition.alignment_bytes();
+      address += alignment;
     }
   }
 
@@ -102,13 +110,20 @@
 TEST(FlashPartitionTest, FillTest) {
   FlashPartition& test_partition = FlashTestPartition();
 
-  ASSERT_GE(kTestDataSize, test_partition.alignment_bytes());
+  ASSERT_GE(kMaxFlashAlignment, test_partition.alignment_bytes());
 
   for (size_t i = 0; i < kTestIterations; i++) {
+    PW_LOG_DEBUG("FillTest iteration %u, write '0'", unsigned(i));
     WriteData(test_partition, 0);
+    PW_LOG_DEBUG("FillTest iteration %u, write '0xff'", unsigned(i));
     WriteData(test_partition, 0xff);
+    PW_LOG_DEBUG("FillTest iteration %u, write '0x55'", unsigned(i));
     WriteData(test_partition, 0x55);
+    PW_LOG_DEBUG("FillTest iteration %u, write '0xa3'", unsigned(i));
     WriteData(test_partition, 0xa3);
+    PW_LOG_DEBUG("Completed iterations %u, Total errors %u",
+                 unsigned(i),
+                 unsigned(error_count));
   }
 }
 
@@ -116,10 +131,10 @@
   FlashPartition& test_partition = FlashTestPartition();
 
   static const uint8_t fill_byte = 0x55;
-  uint8_t test_data[kTestDataSize];
+  uint8_t test_data[kMaxFlashAlignment];
   memset(test_data, fill_byte, sizeof(test_data));
 
-  ASSERT_GE(kTestDataSize, test_partition.alignment_bytes());
+  ASSERT_GE(kMaxFlashAlignment, test_partition.alignment_bytes());
 
   const size_t block_size =
       std::min(sizeof(test_data), test_partition.sector_size_bytes());
@@ -140,18 +155,14 @@
 
   // Preset the flag to make sure the check actually sets it.
   bool is_erased = true;
-  ASSERT_EQ(Status::OK,
-            test_partition.IsRegionErased(
-                0, test_partition.size_bytes(), &is_erased));
+  ASSERT_EQ(Status::OK, test_partition.IsErased(&is_erased));
   ASSERT_EQ(false, is_erased);
 
   ASSERT_EQ(Status::OK, test_partition.Erase());
 
   // Preset the flag to make sure the check actually sets it.
   is_erased = false;
-  ASSERT_EQ(Status::OK,
-            test_partition.IsRegionErased(
-                0, test_partition.size_bytes(), &is_erased));
+  ASSERT_EQ(Status::OK, test_partition.IsErased(&is_erased));
   ASSERT_EQ(true, is_erased);
 
   // Read the first page of each sector and make sure it has been erased.
@@ -162,12 +173,67 @@
 
     StatusWithSize status =
         test_partition.Read(address, data_span.size_bytes(), data_span.data());
-    ASSERT_EQ(Status::OK, status.status());
-    ASSERT_EQ(data_span.size_bytes(), status.size());
+    EXPECT_EQ(Status::OK, status.status());
+    EXPECT_EQ(data_span.size_bytes(), status.size());
 
-    ASSERT_EQ(true, test_partition.AppearsErased(as_bytes(data_span)));
+    EXPECT_EQ(true, test_partition.AppearsErased(as_bytes(data_span)));
   }
 }
 
+TEST(FlashPartitionTest, AlignmentCheck) {
+  FlashPartition& test_partition = FlashTestPartition();
+  const size_t alignment = test_partition.alignment_bytes();
+  const size_t sector_size_bytes = test_partition.sector_size_bytes();
+
+  EXPECT_LE(alignment, kMaxFlashAlignment);
+  EXPECT_EQ(kMaxFlashAlignment % alignment, 0U);
+  EXPECT_LE(kMaxFlashAlignment, sector_size_bytes);
+  EXPECT_LE(sector_size_bytes % kMaxFlashAlignment, 0U);
+}
+
+TEST(FlashPartitionTest, IsErased) {
+  FlashPartition& test_partition = FlashTestPartition();
+  const size_t alignment = test_partition.alignment_bytes();
+
+  // Make sure the partition is big enough to do this test.
+  ASSERT_GE(test_partition.size_bytes(), 3 * kMaxFlashAlignment);
+
+  ASSERT_EQ(Status::OK, test_partition.Erase());
+
+  bool is_erased = true;
+  ASSERT_EQ(Status::OK, test_partition.IsErased(&is_erased));
+  ASSERT_EQ(true, is_erased);
+
+  static const uint8_t fill_byte = 0x55;
+  uint8_t test_data[kMaxFlashAlignment];
+  memset(test_data, fill_byte, sizeof(test_data));
+  auto data_span = std::span(test_data);
+
+  // Write the chunk with fill byte.
+  StatusWithSize status = test_partition.Write(alignment, as_bytes(data_span));
+  ASSERT_EQ(Status::OK, status.status());
+  ASSERT_EQ(data_span.size_bytes(), status.size());
+
+  EXPECT_EQ(Status::OK, test_partition.IsErased(&is_erased));
+  EXPECT_EQ(false, is_erased);
+
+  // Check the chunk that was written.
+  EXPECT_EQ(Status::OK,
+            test_partition.IsRegionErased(
+                alignment, data_span.size_bytes(), &is_erased));
+  EXPECT_EQ(false, is_erased);
+
+  // Check a region that starts erased but later has been written.
+  EXPECT_EQ(Status::OK,
+            test_partition.IsRegionErased(0, 2 * alignment, &is_erased));
+  EXPECT_EQ(false, is_erased);
+
+  // Check erased for a region smaller than kMaxFlashAlignment. This has been a
+  // bug in the past.
+  EXPECT_EQ(Status::OK,
+            test_partition.IsRegionErased(0, alignment, &is_erased));
+  EXPECT_EQ(true, is_erased);
+}
+
 }  // namespace
 }  // namespace pw::kvs::PartitionTest
diff --git a/pw_kvs/public/pw_kvs/flash_memory.h b/pw_kvs/public/pw_kvs/flash_memory.h
index 46be588..a42891f 100644
--- a/pw_kvs/public/pw_kvs/flash_memory.h
+++ b/pw_kvs/public/pw_kvs/flash_memory.h
@@ -211,7 +211,7 @@
   virtual StatusWithSize Write(Address address,
                                std::span<const std::byte> data);
 
-  // Check to see if chunk of flash memory is erased. Address and len need to
+  // Check to see if chunk of flash partition is erased. Address and len need to
   // be aligned with FlashMemory.
   // Returns: OK, on success.
   //          TIMEOUT, on timeout.
@@ -222,6 +222,12 @@
                                 size_t len,
                                 bool* is_erased);
 
+  // Check if the entire partition is erased.
+  // Returns: same as IsRegionErased().
+  Status IsErased(bool* is_erased) {
+    return IsRegionErased(0, this->size_bytes(), is_erased);
+  }
+
   // Checks to see if the data appears to be erased. No reads or writes occur;
   // the FlashPartition simply compares the data to
   // flash_.erased_memory_content().