From 1ad1139e591480ed2be7fc69ebf595f02a8f7d7c Mon Sep 17 00:00:00 2001
From: William Vinnicombe <william.vinnicombe@raspberrypi.com>
Date: Fri, 15 Nov 2024 13:29:10 +0000
Subject: [PATCH] Tidy up variable names, add assets on correct chars, and
 improve documentation

---
 README.md                       |  3 +++
 bootloaders/uart/CMakeLists.txt |  6 ++++++
 bootloaders/uart/uart_boot.c    | 27 +++++++++++++++------------
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/README.md b/README.md
index eeb3d85e1..1b3feeb07 100644
--- a/README.md
+++ b/README.md
@@ -41,6 +41,9 @@ App|Description
 [read_vsys](adc/read_vsys) | Demonstrates how to read VSYS to get the voltage of the power supply.
 
 ### Bootloaders (RP2350 Only)
+
+These examples all produce multiple UF2s - a bootloader UF2, and then a separate UF2 of the program that the bootloader will load and boot. To load them onto a device with empty flash, first load the bootloader UF2, then reset to BOOTSEL mode and load the program UF2. This ordering is required because the bootloaders contain embedded partition tables - see section 5.10.6 in the RP2350 datasheet for more details on those.
+
 App|Description
 ---|---
 [enc_bootloader](bootloaders/encrypted) | A bootloader which decrypts binaries from flash into SRAM. See the separate [README](bootloaders/encrypted/README.md) for more information
diff --git a/bootloaders/uart/CMakeLists.txt b/bootloaders/uart/CMakeLists.txt
index cc0e38caa..6c9561f12 100644
--- a/bootloaders/uart/CMakeLists.txt
+++ b/bootloaders/uart/CMakeLists.txt
@@ -5,7 +5,10 @@ add_executable(uart_boot
 # pull in common dependencies
 target_link_libraries(uart_boot pico_stdlib hardware_flash)
 
+# add partition table
 pico_embed_pt_in_binary(uart_boot ${CMAKE_CURRENT_LIST_DIR}/uart-pt.json)
+
+# create absolute UF2
 pico_set_uf2_family(uart_boot "absolute")
 
 # create map/bin/hex file etc.
@@ -15,6 +18,7 @@ pico_add_extra_outputs(uart_boot)
 example_auto_set_url(uart_boot)
 
 
+# Create separate binary to be loaded onto other device
 add_executable(uart_binary
     uart_binary.c
     )
@@ -23,6 +27,8 @@ add_executable(uart_binary
 target_link_libraries(uart_binary pico_stdlib)
 
 pico_set_binary_type(uart_binary no_flash)
+
+# package uf2 in flash
 pico_package_uf2_output(uart_binary 0x10000000)
 
 # create map/bin/hex/uf2 file etc.
diff --git a/bootloaders/uart/uart_boot.c b/bootloaders/uart/uart_boot.c
index 186bddd9f..97c72e95f 100644
--- a/bootloaders/uart/uart_boot.c
+++ b/bootloaders/uart/uart_boot.c
@@ -37,6 +37,7 @@ void uart_boot() {
 
         if (uart_is_readable_within_us(UART_ID, 1000)) {
             char in = uart_getc(UART_ID);
+            assert(in == 'n');
             printf("%c\n", in);
             break;
         } else {
@@ -61,18 +62,18 @@ void uart_boot() {
     assert(ret == 3);
 
     uint32_t location_and_permissions = buffer[1];
-    uint32_t saddr = XIP_BASE + ((location_and_permissions >> PICOBIN_PARTITION_LOCATION_FIRST_SECTOR_LSB) & 0x1fffu) * FLASH_SECTOR_SIZE;
-    uint32_t eaddr = XIP_BASE + (((location_and_permissions >> PICOBIN_PARTITION_LOCATION_LAST_SECTOR_LSB) & 0x1fffu) + 1) * FLASH_SECTOR_SIZE;
-    printf("Start %08x, end %08x\n", saddr, eaddr);
+    uint32_t start_addr = XIP_BASE + ((location_and_permissions & PICOBIN_PARTITION_LOCATION_FIRST_SECTOR_BITS) >> PICOBIN_PARTITION_LOCATION_FIRST_SECTOR_LSB) * FLASH_SECTOR_SIZE;
+    uint32_t end_addr = XIP_BASE + (((location_and_permissions & PICOBIN_PARTITION_LOCATION_LAST_SECTOR_BITS) >> PICOBIN_PARTITION_LOCATION_LAST_SECTOR_LSB) + 1) * FLASH_SECTOR_SIZE;
+    printf("Start %08x, end %08x\n", start_addr, end_addr);
 
     free(buffer);
 
     printf("Writing binary\n");
-    uint32_t tstart = time_us_32();
-    uint32_t caddr = saddr;
-    while (caddr < eaddr) {
+    uint32_t time_start = time_us_32();
+    uint32_t current_addr = start_addr;
+    while (current_addr < end_addr) {
         uart_putc_raw(UART_ID, 'w');
-        char *buf = (char*)caddr;
+        char *buf = (char*)current_addr;
         for (int i=0; i < 32; i++) {
             uart_putc_raw(UART_ID, buf[i]);
         }
@@ -84,11 +85,12 @@ void uart_boot() {
         }
         char in = uart_getc(UART_ID);
         printf("%c\n", in);
-        caddr += 32;
+        assert(in == 'w');
+        current_addr += 32;
     }
 
-    uint32_t tend = time_us_32();
-    printf("Write took %dus\n", tend - tstart);
+    uint32_t time_end = time_us_32();
+    printf("Write took %dus\n", time_end - time_start);
     printf("Write complete - executing\n");
     uart_putc_raw(UART_ID, 'x');
     if (!uart_is_readable_within_us(UART_ID, 500)) {
@@ -99,6 +101,7 @@ void uart_boot() {
     }
     char in = uart_getc(UART_ID);
     printf("%c\n", in);
+    assert(in == 'x');
 }
 
 
@@ -133,12 +136,12 @@ int main()
         if (i > 0) {
             printf(" ...Read done\n");
         }
-        char *ptr = memchr(buf, 'R', sizeof(buf));
+        char *ptr = memchr(buf, splash[0], sizeof(buf));
         if (ptr && strncmp(ptr, splash, sizeof(splash) - 1) == 0) {
             printf("Splash found\n");
             uart_boot();
         } else {
-            ptr = memchr(buf, 'H', sizeof(buf));
+            ptr = memchr(buf, hello[0], sizeof(buf));
             if (ptr && strncmp(ptr, hello, sizeof(hello) - 1) == 0) {
                 printf("Device is running\n");
             } else {