Fix OTA update

Image 0 did not work. We now get rid of bootloader_backup_irq_table and do this manually: We may not write to address 0 while an image is running. So for image 0 we write the lower 8 pages to the backup address. For all other images (ony image 1 currently) we write to *both*, the original address *and* the backup address. This is done because some addresses in the lower 8 pages *are* used at the original address and the bootloader doesn't (want to) know which addresses are which. There are more safeguards now: We refuse to write to the active or boot_next image (if boot_next is not boot_default). We mark the uploaded partition as not ok. Needs latest bootloader with commit ID a5771ae033b57.
2017-08-27 15:00:04 +02:00 · 2017-08-27 15:00:04 +02:00 · 0c3a9c6b5a
parent 32afc08622
commit 0c3a9c6b5a
4 changed files with 104 additions and 54 deletions
--- a/examples/osd/ota-update/res_bootloader.c
+++ b/examples/osd/ota-update/res_bootloader.c
@ -234,14 +234,27 @@ set_part_ok
    return 0;
 }

+static size_t
+get_part_ok
+    ( const char *name
+    , const char *uri
+    , const char *query
+    , char *buf
+    , size_t bsize
+    )
+{
+    int idx = get_query_partition (query);
+    if (idx < 0) {
+        return snprintf (buf, bsize, "Invalid: \"%s\" use part=N query", query);
+    }
+    return snprintf (buf, bsize, "%ld", bootloader_get_part_ok (idx));
+}
+
 GENERIC_RESOURCE
  ( part_ok
  , Set/Clear Partition OK flag
  , count
  , 0
  , set_part_ok
-  , NULL
+  , get_part_ok
  );
-
-// FIXME: Find out how to pass two parameters, for set/clr_part_ok and
-// for get_part_start
--- a/examples/osd/ota-update/res_upload_image.c
+++ b/examples/osd/ota-update/res_upload_image.c
@ -64,30 +64,59 @@ static uint8_t current_page [256];
 static uint32_t current_offset = 0;
 #define PAGESIZE (sizeof (current_page))

+/*
+ * Note that the current code relies on the fact that the bootloader
+ * used only supports two images. This may change in the future. We
+ * mainly need to relax some of the checks and use a different algorithm
+ * for computing imgidx, the index of the partition to be overwritten.
+ * If the bootloader supports more than two partitions at some point we
+ * may want the uploader to explicitly define the partition to be used.
+ */
+
 static void
-res_put_handler(void *request, void *response, uint8_t *buffer, uint16_t preferred_size, int32_t *offset)
+res_put_handler
+    ( void *request
+    , void *response
+    , uint8_t *buffer
+    , uint16_t preferred_size
+    , int32_t *offset
+    )
 {
  coap_packet_t *const packet = (coap_packet_t *)request;
  uint8_t *in_data = NULL;
  size_t len = 0;
  uint32_t partition_start = 0;
-  const uint32_t partition_size  = bootloader_get_part_size  ();
-
+  const uint32_t partition_size = bootloader_get_part_size  ();
+  uint32_t imgidx = 0;
  unsigned int ct = -1;

-  if(bootloader_get_boot_default()){
-	  partition_start = bootloader_get_part_start (0);
-  }else{
-	  partition_start = bootloader_get_part_start (1);	  
+  /* If the currently-booted partition is not the default partition we
+   * do not allow overwriting a partition: Neither the currently-booted
+   * one (this would crash) nor the only partition that is marked
+   * bootable. We also insist that boot_next == boot_default.
+   */
+  if (bootloader_get_boot_default () != bootloader_get_boot_next ()) {
+    REST.set_response_status (response, REST.status.BAD_REQUEST);
+    const char *error_msg = "Won't overwrite boot_next";
+    REST.set_response_payload (response, error_msg, strlen (error_msg));
+    return;
  }
+  if (bootloader_get_boot_default () != bootloader_get_active_part ()) {
+    REST.set_response_status (response, REST.status.BAD_REQUEST);
+    const char *error_msg = "Won't overwrite current";
+    REST.set_response_payload (response, error_msg, strlen (error_msg));
+    return;
+  }
+  imgidx = !bootloader_get_active_part ();
+  partition_start = bootloader_get_part_start (imgidx);

-  REST.get_header_content_type(request, &ct);
+  REST.get_header_content_type (request, &ct);

  /* Require content_type APPLICATION_OCTET_STREAM */
  if (ct != REST.type.APPLICATION_OCTET_STREAM) {
-    REST.set_response_status(response, REST.status.BAD_REQUEST);
+    REST.set_response_status (response, REST.status.BAD_REQUEST);
    const char *error_msg = "ContentType";
-    REST.set_response_payload(response, error_msg, strlen(error_msg));
+    REST.set_response_payload (response, error_msg, strlen (error_msg));
    return;
  }

@ -98,9 +127,9 @@ res_put_handler(void *request, void *response, uint8_t *buffer, uint16_t preferr
      packet->block1_offset, packet->block1_size, packet->block1_num,
      packet->block1_more, packet->size1));
  if (len == 0 || NULL == in_data) {
-    REST.set_response_status(response, REST.status.BAD_REQUEST);
+    REST.set_response_status (response, REST.status.BAD_REQUEST);
    const char *error_msg = "NoPayload";
-    REST.set_response_payload(response, error_msg, strlen(error_msg));
+    REST.set_response_payload (response, error_msg, strlen (error_msg));
    return;
  }

@ -110,15 +139,15 @@ res_put_handler(void *request, void *response, uint8_t *buffer, uint16_t preferr
  }

  if (packet->block1_offset > current_offset) {
-    REST.set_response_status(response, REST.status.REQUEST_ENTITY_INCOMPLETE);
+    REST.set_response_status (response, REST.status.REQUEST_ENTITY_INCOMPLETE);
    const char *error_msg = "OutOfSequence";
-    REST.set_response_payload(response, error_msg, strlen(error_msg));
+    REST.set_response_payload (response, error_msg, strlen (error_msg));
    return;
  }

  /* Old packet or retransmission, immediately confirm */
  if (packet->block1_offset && packet->block1_offset + len <= current_offset) {
-    REST.set_response_status(response, REST.status.CHANGED);
+    REST.set_response_status (response, REST.status.CHANGED);
    coap_set_header_block1
      (response, packet->block1_num, 0, packet->block1_size);
    return;
@ -126,67 +155,81 @@ res_put_handler(void *request, void *response, uint8_t *buffer, uint16_t preferr

  // FIXME: blocksize may be larger than our flash page size
  if (len > PAGESIZE) {
-    REST.set_response_status(response, REST.status.INTERNAL_SERVER_ERROR);
+    REST.set_response_status (response, REST.status.INTERNAL_SERVER_ERROR);
    const char *error_msg = "GRMPF: PageSize";
-    REST.set_response_payload(response, error_msg, strlen(error_msg));
+    REST.set_response_payload (response, error_msg, strlen (error_msg));
    return;
  }
  // FIXME: blocksize may be larger than our flash page size
  // So we should handle this case and repeatedly flash a block until the
  // received data is written.
  if (current_offset % PAGESIZE + len > PAGESIZE) {
-    REST.set_response_status(response, REST.status.INTERNAL_SERVER_ERROR);
+    REST.set_response_status (response, REST.status.INTERNAL_SERVER_ERROR);
    const char *error_msg = "GRMPF: blocksize";
-    REST.set_response_payload(response, error_msg, strlen(error_msg));
+    REST.set_response_payload (response, error_msg, strlen (error_msg));
    return;
  }
  // Should never happen, we test for < and > earlier.
  if (packet->block1_offset != current_offset) {
-    REST.set_response_status(response, REST.status.INTERNAL_SERVER_ERROR);
+    REST.set_response_status (response, REST.status.INTERNAL_SERVER_ERROR);
    const char *error_msg = "GRMPF: Offset";
-    REST.set_response_payload(response, error_msg, strlen(error_msg));
+    REST.set_response_payload (response, error_msg, strlen (error_msg));
    return;
  }

-  if(packet->block1_offset + len > partition_size) {
-    REST.set_response_status(response,
-                             REST.status.REQUEST_ENTITY_TOO_LARGE);
-    REST.set_response_payload(
-      response,
-      buffer,
-      sprintf((char *)buffer, "%luB max.", partition_size));
+  if (packet->block1_offset + len > partition_size) {
+    REST.set_response_status
+      (response, REST.status.REQUEST_ENTITY_TOO_LARGE);
+    REST.set_response_payload
+      (response, buffer, sprintf ((char *)buffer, "%luB max.", partition_size));
    return;
  }

  memcpy (current_page + current_offset % PAGESIZE, in_data, len);
+  /* Whenever an upload is started for a partition mark it as not ok */
+  if (current_offset == 0) {
+    PRINTF (("Clear partition_ok: %ld\n", imgidx));
+    bootloader_clr_part_ok (imgidx);
+  }
  current_offset += len;

  if (current_offset % PAGESIZE == 0) {
    uint32_t dst_address = partition_start + current_offset - PAGESIZE;
-    PRINTF (("Flashing: %lu to %lu\n", (uint32_t)PAGESIZE, dst_address));
+    /* Special case: Flash irq vectors to backup position */
+    if (current_offset - PAGESIZE < PART_IRQVEC_SIZE) {
+      /* Only for images not at position 0 write first PART_IRQVEC_SIZE
+       * bytes also to original position. For partition 0 it will be
+       * copied there anyway *and* we would crash if we wrote to the
+       * active memory!
+       */
+      if (partition_start != 0) {
+        PRINTF (("Flashing: %lx to %lx\n", (uint32_t)PAGESIZE, dst_address));
+        bootloader_write_page_to_flash (dst_address, PAGESIZE, current_page);
+      }
+      /* Note: The partition_size returned by the bootloader does *NOT*
+       * include the PART_IRQVEC_SIZE
+       */
+      dst_address = partition_start + partition_size
+                  + current_offset - PAGESIZE;
+    }
+    PRINTF (("Flashing: %lx to %lx\n", (uint32_t)PAGESIZE, dst_address));
    bootloader_write_page_to_flash (dst_address, PAGESIZE, current_page);
  } else if (!packet->block1_more) {
    uint32_t dst_address =
        partition_start + (current_offset / PAGESIZE) * PAGESIZE;
-    PRINTF (("Flashing: last page %lu to %lu\n", (uint32_t)PAGESIZE, dst_address));
+    PRINTF (("Flashing: last %lx to %lx\n", (uint32_t)PAGESIZE, dst_address));
    bootloader_write_page_to_flash (dst_address, PAGESIZE, current_page);
  }


  if (!packet->block1_more) {
    // we are finished
-    if(bootloader_get_boot_default()){
-		bootloader_backup_irq_table (0);
-		bootloader_set_boot_next (0);
-	}else{
-		bootloader_backup_irq_table (1);
-		bootloader_set_boot_next (1);		
-	}
+    bootloader_set_boot_next (imgidx);
    current_offset = 0;
  }

-  REST.set_response_status(response, REST.status.CHANGED);
-  coap_set_header_block1(response, packet->block1_num, 0, packet->block1_size);
+  REST.set_response_status (response, REST.status.CHANGED);
+  coap_set_header_block1 (response, packet->block1_num, 0, packet->block1_size);
 }

 RESOURCE(
--- a/platform/osd-merkur-256/Makefile.osd-merkur-256
+++ b/platform/osd-merkur-256/Makefile.osd-merkur-256
@ -88,7 +88,7 @@ BOOTLOADER_SET_PART_OK=0x0003ff94
 BOOTLOADER_CLR_PART_OK=0x0003ff98
 BOOTLOADER_SET_BOOT_DEFAULT=0x0003ff9c
 BOOTLOADER_SET_BOOT_NEXT=0x0003ffa0
-BOOTLOADER_BACKUP_IRQ_TABLE=0x0003ffa4
+BOOTLOADER_GET_PART_OK=0x0003ffa4
 BOOTLOADER_GET_BOOT_DEFAULT=0x0003ffa8
 BOOTLOADER_GET_BOOT_NEXT=0x0003ffac
 BOOTLOADER_GET_ACTIVE_PART=0x0003ffb0
@ -97,6 +97,8 @@ BOOTLOADER_PARTITION=0
 TEXT_SECTION_LENGTH=0x1ef00
 PART_IRQVEC_SIZE=$(shell echo $$((0x800)))

+CFLAGS += -DPART_IRQVEC_SIZE=$(PART_IRQVEC_SIZE)
+
 ifeq ($(BOOTLOADER_PARTITION),0)
 TEXT_SECTION_START=0x0
 LOW_PARTITIONS=--only-section=.customlowtext --only-section=.text
@ -119,7 +121,7 @@ LDFLAGS  += -Wl,--defsym,bootloader_get_mac=$(BOOTLOADER_GET_MAC) \
            -Wl,--defsym,_bootloader_clr_part_ok=$(BOOTLOADER_CLR_PART_OK) \
            -Wl,--defsym,_bootloader_set_boot_default=$(BOOTLOADER_SET_BOOT_DEFAULT) \
            -Wl,--defsym,_bootloader_set_boot_next=$(BOOTLOADER_SET_BOOT_NEXT) \
-            -Wl,--defsym,_bootloader_backup_irq_table=$(BOOTLOADER_BACKUP_IRQ_TABLE) \
+            -Wl,--defsym,bootloader_get_part_ok=$(BOOTLOADER_GET_PART_OK) \
            -Wl,--defsym,bootloader_get_boot_default=$(BOOTLOADER_GET_BOOT_DEFAULT) \
            -Wl,--defsym,bootloader_get_boot_next=$(BOOTLOADER_GET_BOOT_NEXT) \
            -Wl,--defsym,bootloader_get_active_part=$(BOOTLOADER_GET_ACTIVE_PART) \
--- a/platform/osd-merkur-256/bootloader_if.h
+++ b/platform/osd-merkur-256/bootloader_if.h
@ -8,13 +8,13 @@ extern uint32_t bootloader_get_part_start    (uint32_t part_index);
 extern uint32_t bootloader_get_boot_default  (void);
 extern uint32_t bootloader_get_boot_next     (void);
 extern uint32_t bootloader_get_active_part   (void);
+extern uint32_t bootloader_get_part_ok       (uint32_t part_index);

 /* These write to flash and need to turn off interrupts before start */
 extern void     _bootloader_set_part_ok      (uint32_t part_index);
 extern void     _bootloader_clr_part_ok      (uint32_t part_index);
 extern void     _bootloader_set_boot_default (uint32_t part_index);
 extern void     _bootloader_set_boot_next    (uint32_t part_index);
-extern void     _bootloader_backup_irq_table (uint32_t part_index);
 extern int      _bootloader_write_page_to_flash
    (uint32_t address, unsigned int size, unsigned char *p);

@ -52,14 +52,6 @@ static inline void bootloader_set_boot_next (uint32_t part_index)
    SREG = sreg;
 }

-static inline void bootloader_backup_irq_table (uint32_t part_index)
-{
-    uint8_t sreg = SREG;
-    cli ();
-    _bootloader_backup_irq_table (part_index);
-    SREG = sreg;
-}
-
 static inline int bootloader_write_page_to_flash 
    (uint32_t address, unsigned int size, unsigned char *p)
 {