Update code to avoid comparisons between signed and unsigned integers.

Add verbose comments to show that the values changed from signed to
unsigned, for the current code, would always be positive values.

Add verbose comments to clarify the meaning of local variables.

Add verbose comments to show all three possible states at all key
computation points, for the higher cyclomatic complexity code block.

Author: henrygab

libraries/InternalFileSytem/src/flash/flash_cache.c

@@ -97,34 +97,105 @@ void flash_cache_flush (flash_cache_t* fc)
 
 void flash_cache_read (flash_cache_t* fc, void* dst, uint32_t addr, uint32_t count)
 {
+  // there is no check for overflow / wraparound for dst + count, addr + count.
+  // this might be a useful thing to add for at least debug builds.
+
   // overwrite with cache value if available
-  if ( (fc->cache_addr != FLASH_CACHE_INVALID_ADDR) &&
-       !(addr < fc->cache_addr && addr + count <= fc->cache_addr) &&
-       !(addr >= fc->cache_addr + FLASH_CACHE_SIZE) )
+  if ( (fc->cache_addr != FLASH_CACHE_INVALID_ADDR) &&               // cache is not valid
+       !(addr < fc->cache_addr && addr + count <= fc->cache_addr) && // starts before, ends before cache area
+       !(addr >= fc->cache_addr + FLASH_CACHE_SIZE) )                // starts after end of cache area
   {
-    int dst_off = fc->cache_addr - addr;
-    int src_off = 0;
-
-    if ( dst_off < 0 )
+    // This block is entered only when the read overlaps the cache area by at least one byte.
+    // If the read starts before the cache area, it's further guaranteed
+    //    that count is large enough to cause the read to enter
+    //    the cache area by at least 1 byte.
+    uint32_t dst_off = 0;
+    uint32_t src_off = 0;
+    if (addr < fc->cache_addr)
     {
-      src_off = -dst_off;
-      dst_off = 0;
+      dst_off = fc->cache_addr - addr;
+      // Read the bytes prior to the cache address
+      fc->read(dst, addr, dst_off);
     }
-
-    int cache_bytes = minof(FLASH_CACHE_SIZE-src_off, count - dst_off);
-
-    // start to cached
-    if ( dst_off ) fc->read(dst, addr, dst_off);
-
-    // cached
+    else
+    {
+      src_off = addr - fc->cache_addr;      
+    }
+    
+    // Thus, after the above code block executes:
+    // *** AT MOST ***, only one of src_off and dst_off are non-zero;
+    // (Both may be zero when the read starts at the start of the cache area.)
+    // dst_off corresponds to the number of bytes already read from PRIOR to the cache area.
+    // src_off corresponds to the byte offset to start reading at, from WITHIN the cache area.
+
+    // How many bytes to memcpy from flash area?
+    // Remember that, AT MOST, one of src_off and dst_off are non-zero.
+    // If src_off is non-zero, then dst_off is zero, representing that the
+    //   read starts inside the cache.  In this case:
+    //     PARAM1 := FLASH_CACHE_SIZE - src_off == maximum possible bytes to read from cache
+    //     PARAM2 := count
+    //   Thus, taking the minimum of the two gives the number of bytes to read from cache,
+    //     in the range [ 1 .. FLASH_CACHE_SIZE-src_off ].
+    // Else if dst_off is non-zero, then src_off is zero, representing that the
+    //   read started prior to the cache area.  In this case:
+    //     PARAM1 := FLASH_CACHE_SIZE == full size of the cache
+    //     PARAM2 := count - dst_off == total bytes requested, minus the count of those already read
+    //   Because the original request is guaranteed to overlap the cache, the range for
+    //     PARAM2 is ensured to be [ 1 .. count-1 ].
+    //   Thus, taking the minimum of the two gives the number of bytes to read from cache,
+    //     in the range [ 1 .. FLASH_CACHE_SIZE ]
+    // Else both src_off and dst_off are zero, representing that the read is starting
+    //   exactly aligned to the cache.
+    //     PARAM1 := FLASH_CACHE_SIZE
+    //     PARAM2 := count
+    //   Thus, taking the minimum of the two gives the number of bytes to read from cache,
+    //     in the range [ 1 .. FLASH_CACHE_SIZE ]
+    // 
+    // Therefore, in all cases, there is assurance that cache_bytes
+    // will be in the final range [1..FLASH_CACHE_SIZE].
+    uint32_t cache_bytes = minof(FLASH_CACHE_SIZE-src_off, count - dst_off);
+
+    // Use memcpy to read cached data into the buffer
+    // If src_off is non-zero, then dst_off is zero, representing that the
+    //   read starts inside the cache.  In this case:
+    //     PARAM1 := dst
+    //     PARAM2 := fc->cache_buf + src_off
+    //     PARAM3 := cache_bytes
+    //   Thus, all works as expected when starting in the midst of the cache.
+    // Else if dst_off is non-zero, then src_off is zero, representing that the
+    //   read started prior to the cache.  In this case:
+    //     PARAM1 := dst + dst_off == destination offset by number of bytes already read
+    //     PARAM2 := fc->cache_buf
+    //     PARAM3 := cache_bytes
+    //   Thus, all works as expected when starting prior to the cache.
+    // Else both src_off and dst_off are zero, representing that the read is starting
+    //   exactly aligned to the cache.
+    //     PARAM1 := dst
+    //     PARAM2 := fc->cache_buf
+    //     PARAM3 := cache_bytes
+    //   Thus, all works as expected when starting exactly at the cache boundary
+    // 
+    // Therefore, in all cases, there is assurance that cache_bytes
+    // will be in the final range [1..FLASH_CACHE_SIZE].
     memcpy(dst + dst_off, fc->cache_buf + src_off, cache_bytes);
 
-    // cached to end
-    int copied = dst_off + cache_bytes;
-    if ( copied < count ) fc->read(dst + copied, addr + copied, count - copied);
+    // Read any final bytes from flash
+    // As noted above, dst_off represents the count of bytes read prior to the cache
+    // while cache_bytes represents the count of bytes read from the cache;
+    // This code block is guaranteed to overlap the cache area by at least one byte.
+    // Thus, copied will correspond to the total bytes already copied,
+    // and is guaranteed to be in the range [ 1 .. count ].
+    uint32_t copied = dst_off + cache_bytes;
+    
+    // 
+    if ( copied < count )
+    {
+      fc->read(dst + copied, addr + copied, count - copied);
+    }
   }
   else
   {
+    // not using the cache, so just forward to read from flash
     fc->read(dst, addr, count);
   }
 }