PartitionStream requires blockErase, other OTA-related improvements (S…

…mingHub#2729)

Leaving `blockErase` parameter at `false` when constructing a `PartitionStream` will result in corruption unless partition has already been erased. This is not always immediately apparent so can be difficult to diagnose.

- **Breaking change** Replacing `blockErase` with an enumerated `mode` parameter enforces read-only behaviour by default (new feature and a good thing), with write access more explicitly defined. It's a simple change to existing code and will ensure it gets manually checked to ensure correct behaviour.

- Update the `Basic_Ota` sample to check that an OTA update is not being attempted on the active running partition. This can happen with esp8266, for example, when running in temporary boot mode. (NB. Other checks may include active filing system partitions but that is application-specific.)

- Update notes on migrating from Sming 4.2 to include updating boot sector. (Learning points froim SmingHub#2727.)

- Remove redundant flashsize calculation from rboot and update version string to `Sming v1.5` (from v1.4.2). Note this should have been done in SmingHub#2258.

- Fix rboot README regarding use of slot 2, should correspond with subtype `ota_2` not `ota_1`.

Sming/Arch/Esp8266/Components/esp8266/startup.cpp

@@ -18,6 +18,7 @@
 extern void init();
 extern void cpp_core_initialize();
 
+// Normal entry point for user application code from SDK
 extern "C" void user_init(void)
 {
 	// Initialise hardware timers
@@ -39,12 +40,20 @@ extern "C" void user_init(void)
 
 	gdb_init();
 
+	/*
+	 * Load partition information.
+	 * Normally this is done in user_pre_init() but if building without WiFi
+	 * (via esp_no_wifi Component) then user_pre_init() is not called as none of the
+	 * SDK-related partitions are required.
+	 * Calling this a second time is a no-op.
+	 */
 	Storage::initialize();
 
 	init(); // User code init
 }
 
-extern "C" void ICACHE_FLASH_ATTR WEAK_ATTR user_pre_init(void)
+// SDK 3+ calls this method to configure partitions
+extern "C" void user_pre_init(void)
 {
 	Storage::initialize();
 

Sming/Arch/Esp8266/Components/esp_no_wifi/README.rst

@@ -24,6 +24,12 @@ The SDKnoWiFi implements the startup code and some system functions but contains
 which is provided by the SDK and in other parts of the Sming framework. We need to provide
 replacement functions to interoperate correctly with the remaining SDK code.
 
+Advantages
+----------
+
+- Reduces code image size when networking/WiFi is not required
+- Eliminates need for SDK-specific partitions (rf_cal, phy_init, sys_param)
+
 Process
 -------
 

Sming/Components/Storage/ota-migration.rst

@@ -14,8 +14,11 @@ This function is documented in the NON-OS-SDK guide and was introduced in versio
 It is called by the SDK before user_init() so nothing else in the framework has yet been initialised,
 including any C++ static initialisers.
 
-Sming uses this function to read the partition table into memory.
-Applications may override this function to perform any custom upgrade operations.
+Sming uses this function to read the partition table and pass the required information to the SDK
+(via :c:func:`partition_table_regist`).
+
+By overriding (``wrapping``) this function, applications have the opportunity to perform any
+additional checks or upgrades on the partition table before proceeding.
 
 Add to application's `component.mk`:
 
@@ -33,20 +36,43 @@ Add this to your application::
     {
     // Note: This file won't exist on initial build!
     IMPORT_FSTR(partitionTableData, PROJECT_DIR "/out/Esp8266/debug/firmware/partitions.bin")
+
+    // Optionally include updated bootloader
+    // IMPORT_FSTR(rbootData, PROJECT_DIR "/out/Esp8266/debug/firmware/rboot.bin")
     }
 
+    // Called by SDK
     extern "C" void __wrap_user_pre_init(void)
     {
+        // Make sure code is compiled with expected partition table offset - adjust as required
         static_assert(PARTITION_TABLE_OFFSET == 0x3fa000, "Bad PTO");
+
+        // Attempt to load the partition table
         Storage::initialize();
-            auto& flash = *Storage::spiFlash;
+
+        auto& flash = *Storage::spiFlash;
         if(!flash.partitions()) {
-            LOAD_FSTR(data, partitionTableData)
-            flash.erase_range(PARTITION_TABLE_OFFSET, flash.getBlockSize());
-            flash.write(PARTITION_TABLE_OFFSET, data, partitionTableData.size());
+            /*
+             * No partitions were found, which implies this device was previously
+             * running with a pre Sming 4.3 firmware.
+             */
+            // Write our partition table data to flash
+            {
+                LOAD_FSTR(data, partitionTableData)
+                flash.erase_range(PARTITION_TABLE_OFFSET, flash.getBlockSize());
+                flash.write(PARTITION_TABLE_OFFSET, data, partitionTableData.size());
+            }
+
+            // Load the newly written partition information
             flash.loadPartitions(PARTITION_TABLE_OFFSET);
+
+            // Optionally update bootloader
+            // LOAD_FSTR(data, rbootData)
+            // flash.erase_range(0, flash.getBlockSize());
+            // flash.write(0, data, rbootData.size());
         }
 
+        // Pass control back to Sming
         extern void __real_user_pre_init(void);
         __real_user_pre_init();
     }
@@ -64,6 +90,7 @@ An alternative method is to build the partition table layout in code, so there a
     // Support updating legacy devices without partition tables (Sming 4.2 and earlier)
     #ifdef ARCH_ESP8266
 
+    // Need low-level definition for the on-flash `esp_partition_info_t` structure
     #include <Storage/partition_info.h>
 
     extern "C" void __wrap_user_pre_init(void)
@@ -77,14 +104,15 @@ An alternative method is to build the partition table layout in code, so there a
             using FullType = Storage::Partition::FullType;
             using SubType = Storage::Partition::SubType;
     #define PT_ENTRY(name, fulltype, offset, size) \
-        { ESP_PARTITION_MAGIC, FullType(fulltype).type, FullType(fulltype).subtype, offset, size, name, 0 }
+        { Storage::ESP_PARTITION_MAGIC, FullType(fulltype).type, FullType(fulltype).subtype, offset, size, name, 0 }
 
+            // Amend this layout as required so it corresponds with your existing device
             static constexpr Storage::esp_partition_info_t partitionTableData[] PROGMEM{
                 PT_ENTRY("spiFlash", Storage::Device::Type::flash, 0, 0x400000),
                 PT_ENTRY("rom0", SubType::App::ota0, 0x2000, 0xf8000),
-                PT_ENTRY("rom1", SubType::App::ota1, 0x102000, 0xf8000),
-                PT_ENTRY("spiffs0", SubType::Data::spiffs, 0x200000, 0xc0000),
-                PT_ENTRY("spiffs1", SubType::Data::spiffs, 0x2c0000, 0xc0000),
+                PT_ENTRY("spiffs0", SubType::Data::spiffs, 0x100000, 0xc0000),
+                PT_ENTRY("rom1", SubType::App::ota1, 0x202000, 0xf8000),
+                PT_ENTRY("spiffs1", SubType::Data::spiffs, 0x300000, 0xc0000),
                 PT_ENTRY("rf_cal", SubType::Data::rfCal, 0x3fb000, 0x1000),
                 PT_ENTRY("phy_init", SubType::Data::phy, 0x3fc000, 0x1000),
                 PT_ENTRY("sys_param", SubType::Data::sysParam, 0x3fd000, 0x3000),
@@ -103,5 +131,6 @@ An alternative method is to build the partition table layout in code, so there a
 
     #endif // ARCH_ESP8266
 
+This example is based on a typical Sming 4.0 4MByte flash layout as for the ``Basic_rBoot`` sample application.
 
 The above examples are provided as templates and should be modified as required and tested thoroughly!

Sming/Components/Storage/src/PartitionStream.cpp

@@ -46,12 +46,16 @@ int PartitionStream::seekFrom(int offset, SeekOrigin origin)
 
 size_t PartitionStream::write(const uint8_t* data, size_t length)
 {
+	if(mode < Mode::Write) {
+		return 0;
+	}
+
 	auto len = std::min(size_t(size - writePos), length);
 	if(len == 0) {
 		return 0;
 	}
 
-	if(blockErase) {
+	if(mode == Mode::BlockErase) {
 		auto endPos = writePos + len;
 		if(endPos > erasePos) {
 			size_t blockSize = partition.getBlockSize();

Sming/Components/Storage/src/include/Storage/PartitionStream.h

@@ -15,6 +15,12 @@
 
 namespace Storage
 {
+enum class Mode {
+	ReadOnly,
+	Write,		///< Write but do not erase, region should be pre-erased
+	BlockErase, ///< Erase blocks as required before writing
+};
+
 /**
  * @brief Stream operating directory on a Storage partition
  *
@@ -33,9 +39,11 @@ class PartitionStream : public ReadWriteStream
 	 * @param blockErase Set to true to erase blocks before writing
 	 *
 	 * If blockErase is false then region must be pre-erased before writing.
+	 *
+	 * @deprecated Use `mode` parameter instead of `blockErase`
 	 */
-	PartitionStream(Partition partition, storage_size_t offset, size_t size, bool blockErase = false)
-		: partition(partition), startOffset(offset), size(size), blockErase(blockErase)
+	SMING_DEPRECATED PartitionStream(Partition partition, storage_size_t offset, size_t size, bool blockErase)
+		: PartitionStream(partition, offset, size, blockErase ? Mode::BlockErase : Mode::ReadOnly)
 	{
 	}
 
@@ -45,9 +53,38 @@ class PartitionStream : public ReadWriteStream
 	 * @param blockErase Set to true to erase blocks before writing
 	 *
 	 * If blockErase is false then partition must be pre-erased before writing.
+	 *
+	 * @deprecated Use `mode` parameter instead of `blockErase`
+	 */
+	SMING_DEPRECATED PartitionStream(Partition partition, bool blockErase)
+		: PartitionStream(partition, blockErase ? Mode::BlockErase : Mode::ReadOnly)
+	{
+	}
+
+	/**
+	 * @brief Access part of a partition using a stream
+	 * @param partition
+	 * @param offset Limit access to this starting offset
+	 * @param size Limit access to this number of bytes from starting offset
+	 * @param mode
+	 * @note When writing in Mode::BlockErase, block erasure is only performed at the
+	 * start of each block. Therefore if `offset` is not a block boundary then the corresponding
+	 * block will *not* be erased first.
+	 */
+	PartitionStream(Partition partition, storage_size_t offset, size_t size, Mode mode = Mode::ReadOnly)
+		: partition(partition), startOffset(offset), size(size), mode(mode)
+	{
+	}
+
+	/**
+	 * @brief Access entire partition using stream
+	 * @param partition
+	 * @param mode
+	 *
+	 * If blockErase is false then partition must be pre-erased before writing.
 	 */
-	PartitionStream(Partition partition, bool blockErase = false)
-		: partition(partition), startOffset{0}, size(partition.size()), blockErase(blockErase)
+	PartitionStream(Partition partition, Mode mode = Mode::ReadOnly)
+		: partition(partition), startOffset{0}, size(partition.size()), mode(mode)
 	{
 	}
 
@@ -74,7 +111,7 @@ class PartitionStream : public ReadWriteStream
 	uint32_t writePos{0};
 	uint32_t readPos{0};
 	uint32_t erasePos{0};
-	bool blockErase;
+	Mode mode;
 };
 
 } // namespace Storage

Sming/Components/rboot/README.rst

@@ -104,7 +104,7 @@ To enable slot 2, set these values:
                "address": "0x100000",
                "size": "512K",
                "type": "app",
-               "subtype": "ota_1"
+               "subtype": "ota_2"
             }
          }
       }

docs/source/information/flash.rst

@@ -21,16 +21,15 @@ A typical layout for a 4MByte device might look like this:
    (hex)    (if any)          (KB)   (if applicable)            
    =======  ===============   ====   =========================  ===================================================            
    000000                     1      rboot.bin                  Boot loader            
-   001000                     4                                 rBoot configuration
-   002000                     4                                 Partition table
-   003000                     4      esp_init_data_default.bin  PHY configuration data
-   004000                     12     blank.bin                  System parameter area
-   006000                     4      blank.bin                  RF Calibration data (Initialised to FFh)
-   006000                     4                                 Reserved
-   008000   ROM_0_ADDR               rom0.bin                   First ROM image            
-   100000   RBOOT_SPIFFS_0
-   208000   ROM_1_ADDR               rom1.bin                   Second ROM image            
+   001000                     4                                 rBoot configuration            
+   002000   ROM_0_ADDR               rom0.bin                   First ROM image            
+   102000   ROM_1_ADDR               rom1.bin                   Second ROM image            
+   200000   RBOOT_SPIFFS_0
    300000   RBOOT_SPIFFS_1
+   3FA000                     4                                 Partition table
+   3FB000                     4      blank.bin                  RF Calibration data (Initialised to FFh)
+   3FC000                     4      esp_init_data_default.bin  PHY configuration data            
+   3FD000                     12     blank.bin                  System parameter area
    =======  ===============   ====   =========================  ===================================================            
 
 

docs/source/upgrading/5.1-5.2.rst

@@ -0,0 +1,13 @@
+From v5.1 to v5.2
+=================
+
+.. highlight:: c++
+
+**Breaking change**
+
+The :cpp:class:`Storage::PartitionStream` constructors with ``blockErase`` parameter have been deprecated.
+The intended default behaviour is read-only, however previously this also allowed writing without block erase.
+This can result in corrupted flash contents where the flash has not been explicitly erased beforehand.
+
+The new constructors instead use a :cpp:enum:`Storage::Mode` so behaviour is more explicit.
+The default is read-only and writes will now be failed.

samples/Basic_Ota/app/application.cpp

@@ -54,6 +54,16 @@ void doUpgrade()
 	// select rom slot to flash
 	auto part = ota.getNextBootPartition();
 
+	/*
+	 * Applications should always include a sanity check to ensure partitions being updated are
+	 * not in use. This should always included the application partition but should also consider
+	 * filing system partitions, etc. which may be actively in use.
+	 */
+	if(part == ota.getRunningPartition()) {
+		Serial << F("May be running in temporary mode. Please reboot and try again.") << endl;
+		return;
+	}
+
 #ifndef RBOOT_TWO_ROMS
 	// flash rom to position indicated in the rBoot config rom table
 	otaUpdater->addItem(ROM_0_URL, part);
@@ -67,7 +77,8 @@ void doUpgrade()
 	auto spiffsPart = findSpiffsPartition(part);
 	if(spiffsPart) {
 		// use user supplied values (defaults for 4mb flash in hardware config)
-		otaUpdater->addItem(SPIFFS_URL, spiffsPart, new Storage::PartitionStream(spiffsPart));
+		otaUpdater->addItem(SPIFFS_URL, spiffsPart,
+							new Storage::PartitionStream(spiffsPart, Storage::Mode::BlockErase));
 	}
 
 	// request switch and reboot on success