diff --git a/include/pbl/drivers/flash.h b/include/pbl/drivers/flash.h index 073e09f244..2d6ad34820 100644 --- a/include/pbl/drivers/flash.h +++ b/include/pbl/drivers/flash.h @@ -57,6 +57,13 @@ void flash_read_bytes(uint8_t* buffer, uint32_t start_addr, uint32_t buffer_size */ void flash_write_bytes(const uint8_t* buffer, uint32_t start_addr, uint32_t buffer_size); +/** + * The CPU-addressable pointer for a flash address. Only exists on platforms whose flash is + * permanently memory-mapped (region addresses include the XIP base, FLASH_REGION_BASE_ADDRESS), + * where flash bytes can be read directly instead of copied out with flash_read_bytes. + */ +const void *flash_memory_mapped_address(uint32_t flash_addr); + typedef void (*FlashOperationCompleteCb)(void *context, status_t result); /** diff --git a/include/pbl/services/imaging.h b/include/pbl/services/imaging.h index 09f25db69b..c19b7cfcc0 100644 --- a/include/pbl/services/imaging.h +++ b/include/pbl/services/imaging.h @@ -16,15 +16,26 @@ typedef struct CommSession CommSession; //! Generic image-fetch service. Consumers (album art, ...) ask the phone for an image and get the //! reassembled bitmap back through a registered handler. Watch-pull only, capability-gated. +//! On boards with memory-mapped flash the decoded image is stored in one of a small fixed pool of +//! flash slots and rendered straight from there, so no pixel-sized heap buffer is held; two +//! consumers can each display an image at once. //! Called on KernelMain when a requested image finishes transferring. `bitmap` is NULL when the //! phone reported it has no image (ImagingResponseFlagNoImage). Ownership of a non-NULL `bitmap` -//! (and its pixel/palette buffers) passes to the handler. `token` echoes the request. +//! (and its palette) passes to the handler. The pixels are heap memory to free only when +//! `info.is_bitmap_heap_allocated` is set; otherwise they live in a memory-mapped flash slot owned +//! by the service, which stays valid until the same consumer requests again or calls +//! imaging_release. `token` echoes the request. typedef void (*ImagingReceivedHandler)(uint8_t token, struct GBitmap *bitmap); //! Register the handler for an image type. One handler per type; overwrites any previous. void imaging_register_handler(ImagingImageType image_type, ImagingReceivedHandler handler); +//! Tell the service a consumer has stopped displaying its image and isn't replacing it (e.g. its +//! window closed), so the flash slot it held can be reused. Replacing an image needs no release — +//! a fresh request hands the slot over automatically. No-op where the slot pool isn't compiled in. +void imaging_release(ImagingImageType image_type); + //! True if the connected phone advertises image-fetch support and hasn't told us it can't serve //! this image type (see ImagingResponseFlagUnsupported). Latched state resets on reconnect. bool imaging_is_type_supported(ImagingImageType image_type); @@ -35,7 +46,8 @@ bool imaging_request_album_art(uint8_t token, ImagingFormat format, uint16_t wid const char *title, const char *artist); //! Ask the phone for the image it holds for timeline item `item_id`, at the given size/format. -//! No-op (and returns false) if unsupported. The matching handler is invoked when the transfer +//! No-op (and returns false) if unsupported or if the request could not be sent, in which case +//! nothing will be delivered. Otherwise the matching handler is invoked when the transfer //! completes. bool imaging_request_notification_image(uint8_t token, ImagingFormat format, uint16_t width, uint16_t height, const Uuid *item_id); diff --git a/src/fw/drivers/flash/flash_api.c b/src/fw/drivers/flash/flash_api.c index b260870ad3..f578d568b5 100644 --- a/src/fw/drivers/flash/flash_api.c +++ b/src/fw/drivers/flash/flash_api.c @@ -115,6 +115,12 @@ void flash_read_bytes(uint8_t* buffer, uint32_t start_addr, pbl_mutex_unlock(&s_flash_lock); } +#ifdef FLASH_REGION_BASE_ADDRESS +const void *flash_memory_mapped_address(uint32_t flash_addr) { + return (const void *)(uintptr_t)flash_addr; +} +#endif + #ifdef TEST_FLASH_LOCK_PROTECTION static bool s_assert_write_error = false; void flash_expect_program_failure(bool expect_failure) { diff --git a/src/fw/flash_region/flash_region_gd25q256e.h b/src/fw/flash_region/flash_region_gd25q256e.h index dedcf61bdb..d9953ab94c 100644 --- a/src/fw/flash_region/flash_region_gd25q256e.h +++ b/src/fw/flash_region/flash_region_gd25q256e.h @@ -26,9 +26,11 @@ MACRO(SAFE_FIRMWARE, 0x0090000 /* 576K */, arg) /* 0x12A20000 - 0x12AAFFFF */ \ MACRO(FILESYSTEM, 0x1490000 /* 21056K */, arg) /* 0x12AB0000 - 0x13F3FFFF */ \ MACRO(CD, 0x0080000 /* 512K */, arg) /* 0x12A40000 - 0x13FBFFFF */ \ - MACRO(RSVD1, 0x000F000 /* 60K */, arg) /* 0x13FC0000 - 0x13FCEFFF */ \ + MACRO(IMAGING_0, 0x000A000 /* 40K */, arg) /* 0x13FC0000 - 0x13FC9FFF */ \ + MACRO(RSVD1, 0x0005000 /* 20K */, arg) /* 0x13FCA000 - 0x13FCEFFF */ \ MACRO(DEBUG_DB, 0x0020000 /* 128K */, arg) /* 0x13FCF000 - 0x13FEEFFF */ \ - MACRO(RSVD2, 0x000C000 /* 48K */, arg) /* 0x13FEF000 - 0x13FFAFFF */ \ + MACRO(IMAGING_1, 0x000A000 /* 40K */, arg) /* 0x13FEF000 - 0x13FF8FFF */ \ + MACRO(RSVD2, 0x0002000 /* 8K */, arg) /* 0x13FF9000 - 0x13FFAFFF */ \ MACRO(MFG_RESULTS, 0x0001000 /* 4K */, arg) /* 0x13FFB000 - 0x13FFBFFF */ \ MACRO(MFG_BATTERY_STATE, 0x0001000 /* 4K */, arg) /* 0x13FFC000 - 0x13FFCFFF */ \ MACRO(TZINFO, 0x0001000 /* 4K */, arg) /* 0x13FFD000 - 0x13FFDFFF */ \ @@ -68,6 +70,14 @@ #define FLASH_REGION_CD_BEGIN FLASH_REGION_START_ADDR(CD) #define FLASH_REGION_CD_END FLASH_REGION_END_ADDR(CD) +// Two fixed slots for phone-supplied images (see services/imaging): each holds one decoded +// bitmap, written as it streams in and rendered directly through the memory-mapped flash +// window. Two slots so any two image consumers can display at once. +#define FLASH_REGION_IMAGING_0_BEGIN FLASH_REGION_START_ADDR(IMAGING_0) +#define FLASH_REGION_IMAGING_0_END FLASH_REGION_END_ADDR(IMAGING_0) +#define FLASH_REGION_IMAGING_1_BEGIN FLASH_REGION_START_ADDR(IMAGING_1) +#define FLASH_REGION_IMAGING_1_END FLASH_REGION_END_ADDR(IMAGING_1) + #define FLASH_REGION_DEBUG_DB_BEGIN FLASH_REGION_START_ADDR(DEBUG_DB) #define FLASH_REGION_DEBUG_DB_END FLASH_REGION_END_ADDR(DEBUG_DB) #define FLASH_DEBUG_DB_BLOCK_SIZE SUBSECTOR_SIZE_BYTES diff --git a/src/fw/flash_region/flash_region_qemu.h b/src/fw/flash_region/flash_region_qemu.h index e523ce1697..8b600c786c 100644 --- a/src/fw/flash_region/flash_region_qemu.h +++ b/src/fw/flash_region/flash_region_qemu.h @@ -25,9 +25,11 @@ MACRO(SAFE_FIRMWARE, 0x0090000 /* 576K */, arg) /* 0x10A20000 - 0x10AAFFFF */ \ MACRO(FILESYSTEM, 0x1490000 /* 21056K */, arg) /* 0x10AB0000 - 0x11F3FFFF */ \ MACRO(CD, 0x0080000 /* 512K */, arg) /* 0x11F40000 - 0x11FBFFFF */ \ - MACRO(RSVD1, 0x000F000 /* 60K */, arg) /* 0x11FC0000 - 0x11FCEFFF */ \ + MACRO(IMAGING_0, 0x000A000 /* 40K */, arg) /* 0x11FC0000 - 0x11FC9FFF */ \ + MACRO(RSVD1, 0x0005000 /* 20K */, arg) /* 0x11FCA000 - 0x11FCEFFF */ \ MACRO(DEBUG_DB, 0x0020000 /* 128K */, arg) /* 0x11FCF000 - 0x11FEEFFF */ \ - MACRO(RSVD2, 0x000C000 /* 48K */, arg) /* 0x11FEF000 - 0x11FFAFFF */ \ + MACRO(IMAGING_1, 0x000A000 /* 40K */, arg) /* 0x11FEF000 - 0x11FF8FFF */ \ + MACRO(RSVD2, 0x0002000 /* 8K */, arg) /* 0x11FF9000 - 0x11FFAFFF */ \ MACRO(MFG_RESULTS, 0x0001000 /* 4K */, arg) /* 0x11FFB000 - 0x11FFBFFF */ \ MACRO(MFG_BATTERY_STATE, 0x0001000 /* 4K */, arg) /* 0x11FFC000 - 0x11FFCFFF */ \ MACRO(TZINFO, 0x0001000 /* 4K */, arg) /* 0x11FFD000 - 0x11FFDFFF */ \ @@ -66,6 +68,14 @@ // CD region is defined by flash_region.h based on CONFIG_PBLBOOT +// Two fixed slots for phone-supplied images (see services/imaging): each holds one decoded +// bitmap, written as it streams in and rendered directly through the memory-mapped flash +// window. Two slots so any two image consumers can display at once. +#define FLASH_REGION_IMAGING_0_BEGIN FLASH_REGION_START_ADDR(IMAGING_0) +#define FLASH_REGION_IMAGING_0_END FLASH_REGION_END_ADDR(IMAGING_0) +#define FLASH_REGION_IMAGING_1_BEGIN FLASH_REGION_START_ADDR(IMAGING_1) +#define FLASH_REGION_IMAGING_1_END FLASH_REGION_END_ADDR(IMAGING_1) + #define FLASH_REGION_DEBUG_DB_BEGIN FLASH_REGION_START_ADDR(DEBUG_DB) #define FLASH_REGION_DEBUG_DB_END FLASH_REGION_END_ADDR(DEBUG_DB) #define FLASH_DEBUG_DB_BLOCK_SIZE SUBSECTOR_SIZE_BYTES diff --git a/src/fw/popups/notifications/notification_window.c b/src/fw/popups/notifications/notification_window.c index 30b058ec14..a134695174 100644 --- a/src/fw/popups/notifications/notification_window.c +++ b/src/fw/popups/notifications/notification_window.c @@ -1151,6 +1151,10 @@ static void prv_window_unload(Window *window) { swap_layer_deinit(&data->swap_layer); notification_image_clear(); +#if NOTIFICATION_IMAGE_SUPPORTED + // The card is gone, so let the imaging service reuse the flash slot its image was in. + imaging_release(ImagingImageTypeNotification); +#endif status_bar_layer_deinit(&data->status_layer); notifications_presented_list_deinit(prv_handle_presented_notif_deinit, NULL); gbitmap_deinit(&data->dnd_icon); @@ -1195,8 +1199,11 @@ static void prv_maybe_request_notification_image(LayoutLayer *layout, TimelineIt !notification_image_claim(&item->header.id, &token)) { return; } - imaging_request_notification_image(token, ImagingFormat4BitPalette, size.w, size.h, - &item->header.id); + if (!imaging_request_notification_image(token, ImagingFormat4BitPalette, size.w, size.h, + &item->header.id)) { + // Nothing is coming, so resolve the slot rather than leave the card on its placeholder. + notification_image_store(token, NULL); + } } #endif diff --git a/src/fw/services/imaging/imaging.c b/src/fw/services/imaging/imaging.c index 6730f795e7..9d8ce688f5 100644 --- a/src/fw/services/imaging/imaging.c +++ b/src/fw/services/imaging/imaging.c @@ -4,27 +4,33 @@ #include "pbl/services/imaging.h" #include "applib/graphics/gtypes.h" +#include "flash_region/flash_region.h" #include "kernel/pbl_malloc.h" #include "pbl/kernel/mutex.h" #include "pbl/services/comm_session/session.h" #include "pbl/util/math.h" #include "pbl/util/size.h" +#include "system/status_codes.h" +#include +#include +#include #include static const uint16_t IMAGING_ENDPOINT = 0x35; // Full-screen 4-bpp on the largest supported display (260x260) is ~34 KB. Cap generously and reject -// anything larger to bound kernel-heap use against a malformed or hostile phone. +// anything larger to bound buffer use against a malformed or hostile phone. #define IMAGING_MAX_BYTES (40 * 1024) #define IMAGING_MAX_DIM (300) #define IMAGING_PALETTE_ENTRIES (16) static ImagingReceivedHandler s_handlers[ImagingImageTypeCount]; -//! Guards the latch state below: requests come in on the requesting task (e.g. the Music app) -//! while responses are handled on KernelMain. The reassembly state (s_rx) is deliberately not -//! covered — it is only ever touched on KernelMain (endpoint receiver and comm-session events). +//! Guards the latch state and the slot pool below: requests come in on the requesting task (e.g. +//! the Music app) while responses are handled on KernelMain. The reassembly state (s_rx) is +//! deliberately not covered — it is only ever touched on KernelMain (endpoint receiver and +//! comm-session events). static PBL_MUTEX_DEFINE(s_lock); // Image types the phone told us it can't serve (ImagingResponseFlagUnsupported), so we stop asking. @@ -42,10 +48,210 @@ static struct { uint16_t row_size_bytes; uint32_t total_bytes; uint32_t received_bytes; + bool flash; //!< Pixels go to flash slot `slot`; `pixels` stays NULL. + int slot; uint8_t *pixels; GColor *palette; // NULL for non-palette formats } s_rx; +// Image store: two fixed slots in the dedicated, memory-mapped IMAGING flash region. Each slot +// holds one decoded bitmap, written as it streams in and rendered straight from flash — no +// pixel-sized heap buffer. Slots are assigned to whichever image types are displaying (not wired to +// a specific consumer), so any two consumers can hold an image at once; a third uses the heap. +// +// A slot lives only while its owner displays it — a display store, not a history cache (the +// consumers already dedupe their own fetches), and imaging is the region's only writer, so stored +// pixels need no integrity check. All access goes through the helpers below, which stub out where +// the region isn't carved so the request/reassembly paths stay free of conditionals. +#ifdef FLASH_REGION_IMAGING_0_BEGIN +#define IMAGING_FLASH_SLOTS 1 +#define IMAGING_NUM_SLOTS (2) +_Static_assert(FLASH_REGION_IMAGING_0_END - FLASH_REGION_IMAGING_0_BEGIN >= IMAGING_MAX_BYTES && + FLASH_REGION_IMAGING_1_END - FLASH_REGION_IMAGING_1_BEGIN >= IMAGING_MAX_BYTES, + "each imaging slot must fit any accepted image"); + +typedef enum { + SlotIdle, //!< Not in a load pipeline (blank, or holding a delivered image if `live`). + SlotErasing, //!< Erase in flight ahead of a transfer. + SlotReady, //!< Blank, waiting for the transfer's first chunk. + SlotWriting, //!< The transfer behind `load_token` is streaming in. +} SlotState; + +static struct ImagingSlot { + uint32_t addr; + uint32_t size; + SlotState state; + uint8_t load_token; //!< Request token being loaded (Erasing/Ready/Writing). + uint8_t owner; //!< ImagingImageType displaying this slot (valid while `live`). + bool live; //!< A consumer is displaying this slot's image; don't reuse it. + bool cancel_pending; //!< The reservation was dropped mid-erase; go Idle when the erase finishes. +} s_slots[IMAGING_NUM_SLOTS] = { + { .addr = FLASH_REGION_IMAGING_0_BEGIN, .size = FLASH_REGION_IMAGING_0_END - + FLASH_REGION_IMAGING_0_BEGIN }, + { .addr = FLASH_REGION_IMAGING_1_BEGIN, .size = FLASH_REGION_IMAGING_1_END - + FLASH_REGION_IMAGING_1_BEGIN }, +}; + +static void prv_slot_erase_done(void *context, status_t result) { + const int i = (int)(uintptr_t)context; + pbl_mutex_lock(&s_lock, PBL_FOREVER); + if (s_slots[i].state == SlotErasing) { + // A slot dropped mid-erase (its transfer went to the heap) must not become a Ready reservation + // no one will ever claim — that would orphan it and, once both slots orphan, force every image + // onto the heap. Take it back to Idle instead. + s_slots[i].state = (s_slots[i].cancel_pending || !PASSED(result)) ? SlotIdle : SlotReady; + } + s_slots[i].cancel_pending = false; + pbl_mutex_unlock(&s_lock); +} + +//! Drop a pipeline slot safely: an in-flight erase can't just be set Idle (a fresh reserve could +//! pick the slot and race the erase's completion), so mark it for reclaim when the erase lands; +//! anything else is reusable immediately. +static void prv_slot_drop(int i) { + if (s_slots[i].state == SlotErasing) { + s_slots[i].cancel_pending = true; + } else { + s_slots[i].state = SlotIdle; + } +} + +//! Reserve a slot for the transfer behind `token` and start erasing it, so it is blank by the time +//! the phone's first chunk arrives. Prefers a slot nobody is displaying (leaving the requester's +//! current image up until the new one is ready); failing that reuses the requester's own slot in +//! place; failing that (both slots busy with other consumers) leaves nothing reserved, and the +//! response falls back to the heap. +static void prv_slot_reserve(ImagingImageType type, uint8_t token) { + pbl_mutex_lock(&s_lock, PBL_FOREVER); + int target = -1; + for (int i = 0; i < IMAGING_NUM_SLOTS; ++i) { + if (s_slots[i].state == SlotIdle && !s_slots[i].live) { + target = i; + break; + } + } + if (target < 0) { + for (int i = 0; i < IMAGING_NUM_SLOTS; ++i) { + if (s_slots[i].state == SlotIdle && s_slots[i].live && s_slots[i].owner == type) { + target = i; + break; + } + } + } + if (target < 0) { + pbl_mutex_unlock(&s_lock); + return; + } + s_slots[target].state = SlotErasing; + s_slots[target].load_token = token; + const uint32_t addr = s_slots[target].addr; + const uint32_t size = s_slots[target].size; + pbl_mutex_unlock(&s_lock); + flash_erase_optimal_range(addr, addr, addr + size, addr + size, prv_slot_erase_done, + (void *)(uintptr_t)target); +} + +//! Claim the reserved-and-erased slot for transfer `token`. Returns the slot, or -1 to buffer on +//! the heap (no slot was free when the request went out, or the erase hasn't finished). On a miss, +//! drops this token's reservation so a slot whose erase lost the race to the response doesn't +//! orphan. +static int prv_slot_claim(uint8_t token) { + int slot = -1; + pbl_mutex_lock(&s_lock, PBL_FOREVER); + for (int i = 0; i < IMAGING_NUM_SLOTS; ++i) { + if (s_slots[i].load_token != token || s_slots[i].state == SlotIdle) { + continue; + } + if (s_slots[i].state == SlotReady) { + s_slots[i].state = SlotWriting; + slot = i; + } else { + prv_slot_drop(i); // reserved but not ready in time; this transfer takes the heap + } + break; + } + pbl_mutex_unlock(&s_lock); + return slot; +} + +//! Write one chunk into a claimed slot. False if a newer request has reclaimed it, meaning this +//! transfer is dead and the caller should abandon it. +static bool prv_slot_write(int slot, uint32_t offset, const uint8_t *data, uint16_t len, + uint8_t token) { + pbl_mutex_lock(&s_lock, PBL_FOREVER); + const bool ours = (s_slots[slot].state == SlotWriting && s_slots[slot].load_token == token); + const uint32_t addr = s_slots[slot].addr; + pbl_mutex_unlock(&s_lock); + if (!ours) { + return false; + } + flash_write_bytes(data, addr + offset, len); + return true; +} + +//! Finish a slot load: hand `type`'s display over to it, release whichever slot `type` was showing +//! before (flicker-free when the two differ), and return the memory-mapped pixel pointer. +static const void *prv_slot_commit(int slot, uint8_t type) { + pbl_mutex_lock(&s_lock, PBL_FOREVER); + s_slots[slot].state = SlotIdle; + s_slots[slot].owner = type; + for (int j = 0; j < IMAGING_NUM_SLOTS; ++j) { + if (j != slot && s_slots[j].state == SlotIdle && s_slots[j].live && s_slots[j].owner == type) { + s_slots[j].live = false; + } + } + s_slots[slot].live = true; + const void *addr = flash_memory_mapped_address(s_slots[slot].addr); + pbl_mutex_unlock(&s_lock); + return addr; +} + +//! Abandon the slot reserved for transfer `token` when the request resolves without filling it +//! (the phone had no image, the type is unsupported, or the response was malformed). Leaves any +//! already-delivered image (an Idle+live slot) untouched. +static void prv_slot_cancel(uint8_t token) { + pbl_mutex_lock(&s_lock, PBL_FOREVER); + for (int i = 0; i < IMAGING_NUM_SLOTS; ++i) { + if (s_slots[i].state != SlotIdle && s_slots[i].load_token == token) { + prv_slot_drop(i); + } + } + pbl_mutex_unlock(&s_lock); +} + +//! Drop `type`'s claim on its slot: called when a consumer stops displaying its image outright +//! (rather than replacing it), so the slot can be reused. +static void prv_slot_release(uint8_t type) { + pbl_mutex_lock(&s_lock, PBL_FOREVER); + for (int i = 0; i < IMAGING_NUM_SLOTS; ++i) { + if (s_slots[i].state == SlotIdle && s_slots[i].live && s_slots[i].owner == type) { + s_slots[i].live = false; + } + } + pbl_mutex_unlock(&s_lock); +} + +//! Abandon any in-flight load (e.g. on disconnect); delivered images stay displayable. +static void prv_slots_abandon_loads(void) { + pbl_mutex_lock(&s_lock, PBL_FOREVER); + for (int i = 0; i < IMAGING_NUM_SLOTS; ++i) { + if (s_slots[i].state != SlotIdle) { + prv_slot_drop(i); + } + } + pbl_mutex_unlock(&s_lock); +} +#else // No IMAGING flash region: everything runs through the heap path. +static void prv_slot_reserve(ImagingImageType type, uint8_t token) {} +static int prv_slot_claim(uint8_t token) { return -1; } +static bool prv_slot_write(int slot, uint32_t offset, const uint8_t *data, uint16_t len, + uint8_t token) { return false; } +static const void *prv_slot_commit(int slot, uint8_t type) { return NULL; } +static void prv_slot_cancel(uint8_t token) {} +static void prv_slot_release(uint8_t type) {} +static void prv_slots_abandon_loads(void) {} +#endif // FLASH_REGION_IMAGING_0_BEGIN + static void prv_rx_reset(void) { kernel_free(s_rx.pixels); kernel_free(s_rx.palette); @@ -58,6 +264,10 @@ void imaging_register_handler(ImagingImageType image_type, ImagingReceivedHandle } } +void imaging_release(ImagingImageType image_type) { + prv_slot_release(image_type); +} + //! The type a response answers, from the top nibble of its flags byte. Zero — which is what a phone //! that doesn't set those bits sends — is album art. A value we don't know is left out of range so //! it routes nowhere rather than to the wrong consumer. @@ -70,7 +280,9 @@ static void prv_deliver(uint8_t token, uint8_t type, GBitmap *bitmap) { if (handler) { handler(token, bitmap); } else if (bitmap) { - kernel_free(bitmap->addr); + if (bitmap->info.is_bitmap_heap_allocated) { + kernel_free(bitmap->addr); + } kernel_free(bitmap->palette); kernel_free(bitmap); } @@ -99,12 +311,23 @@ bool imaging_is_type_supported(ImagingImageType image_type) { return !latched; } +//! Reserve a slot (if any) then send `payload`; shared tail of the two request builders. +static bool prv_send_request(ImagingImageType type, uint8_t token, const uint8_t *payload, + size_t length) { + CommSession *session = comm_session_get_system_session(); + if (!session) { + return false; + } + prv_slot_reserve(type, token); + return comm_session_send_data(session, IMAGING_ENDPOINT, payload, length, + COMM_SESSION_DEFAULT_TIMEOUT); +} + bool imaging_request_album_art(uint8_t token, ImagingFormat format, uint16_t width, uint16_t height, const char *title, const char *artist) { if (!imaging_is_type_supported(ImagingImageTypeAlbumArt)) { return false; } - CommSession *session = comm_session_get_system_session(); const size_t title_len = title ? MIN(strlen(title), 255) : 0; const size_t artist_len = artist ? MIN(strlen(artist), 255) : 0; uint8_t payload[sizeof(ImagingRequestHeader) + 2 + 255 + 255]; @@ -123,9 +346,7 @@ bool imaging_request_album_art(uint8_t token, ImagingFormat format, uint16_t wid memcpy(cursor, artist, artist_len); cursor += artist_len; - comm_session_send_data(session, IMAGING_ENDPOINT, payload, cursor - payload, - COMM_SESSION_DEFAULT_TIMEOUT); - return true; + return prv_send_request(ImagingImageTypeAlbumArt, token, payload, cursor - payload); } bool imaging_request_notification_image(uint8_t token, ImagingFormat format, uint16_t width, @@ -133,7 +354,6 @@ bool imaging_request_notification_image(uint8_t token, ImagingFormat format, uin if (!item_id || !imaging_is_type_supported(ImagingImageTypeNotification)) { return false; } - CommSession *session = comm_session_get_system_session(); uint8_t payload[sizeof(ImagingRequestHeader) + UUID_SIZE]; ImagingRequestHeader *hdr = (ImagingRequestHeader *)payload; hdr->cmd = ImagingCmdIDRequest; @@ -144,9 +364,7 @@ bool imaging_request_notification_image(uint8_t token, ImagingFormat format, uin hdr->height = height; memcpy(payload + sizeof(*hdr), item_id, UUID_SIZE); - comm_session_send_data(session, IMAGING_ENDPOINT, payload, sizeof(payload), - COMM_SESSION_DEFAULT_TIMEOUT); - return true; + return prv_send_request(ImagingImageTypeNotification, token, payload, sizeof(payload)); } static uint16_t prv_gbitmap_format_for(ImagingFormat format, GBitmapFormat *out) { @@ -185,12 +403,14 @@ void imaging_protocol_msg_callback(CommSession *session, const uint8_t *msg, siz s_unsupported_types |= (1u << type); pbl_mutex_unlock(&s_lock); } + prv_slot_cancel(hdr->token); prv_rx_reset(); prv_deliver(hdr->token, type, NULL); return; } if (hdr->flags & ImagingResponseFlagNoImage) { + prv_slot_cancel(hdr->token); prv_rx_reset(); prv_deliver(hdr->token, type, NULL); return; @@ -199,6 +419,8 @@ void imaging_protocol_msg_callback(CommSession *session, const uint8_t *msg, siz if (hdr->flags & ImagingResponseFlagFirst) { prv_rx_reset(); if (cursor + 6 > msg_end) { + PBL_LOG_WRN("Imaging: truncated image header (%zu B)", length); + prv_slot_cancel(hdr->token); return; } const uint16_t width = cursor[0] | (cursor[1] << 8); @@ -210,25 +432,46 @@ void imaging_protocol_msg_callback(CommSession *session, const uint8_t *msg, siz const uint16_t max_palette = prv_gbitmap_format_for(format, &gformat); if (width == 0 || height == 0 || width > IMAGING_MAX_DIM || height > IMAGING_MAX_DIM || palette_count > max_palette || (max_palette > 0 && palette_count == 0)) { + PBL_LOG_WRN("Imaging: bad image header %ux%u fmt %u palette %u", width, height, format, + palette_count); + prv_slot_cancel(hdr->token); return; } if (cursor + palette_count > msg_end) { + PBL_LOG_WRN("Imaging: truncated palette (%u entries)", palette_count); + prv_slot_cancel(hdr->token); return; } const uint16_t row_size = gbitmap_format_get_row_size_bytes(width, gformat); const uint32_t total = (uint32_t)row_size * height; if (total == 0 || total > IMAGING_MAX_BYTES) { + PBL_LOG_WRN("Imaging: %ux%u image needs %"PRIu32" B, over the %u B cap", width, height, total, + IMAGING_MAX_BYTES); + prv_slot_cancel(hdr->token); return; } - s_rx.pixels = kernel_zalloc(total); - if (!s_rx.pixels) { - prv_rx_reset(); - return; + const int slot = prv_slot_claim(hdr->token); + if (slot >= 0) { + s_rx.flash = true; + s_rx.slot = slot; + } else { + // INFO, not DBG: a fresh image that can't get a slot is the notable case, and it fires at + // most once per image so it isn't spammy. + PBL_LOG_INFO("Imaging: no flash slot free, buffering type %u image on the heap", type); + s_rx.pixels = kernel_zalloc(total); + if (!s_rx.pixels) { + PBL_LOG_WRN("Imaging: out of memory for %ux%u image (%"PRIu32" B)", width, height, total); + prv_rx_reset(); + prv_deliver(hdr->token, type, NULL); + return; + } } if (max_palette > 0) { s_rx.palette = kernel_zalloc(IMAGING_PALETTE_ENTRIES * sizeof(GColor)); if (!s_rx.palette) { + PBL_LOG_WRN("Imaging: out of memory for image palette"); prv_rx_reset(); + prv_deliver(hdr->token, type, NULL); return; } for (uint8_t i = 0; i < palette_count; ++i) { @@ -257,29 +500,51 @@ void imaging_protocol_msg_callback(CommSession *session, const uint8_t *msg, siz const size_t avail = (cursor <= msg_end) ? (size_t)(msg_end - cursor) : 0; if (hdr->offset != s_rx.received_bytes || hdr->chunk_len != avail || (uint32_t)hdr->offset + hdr->chunk_len > s_rx.total_bytes) { + PBL_LOG_WRN("Imaging: bad chunk at %"PRIu32" (len %u, avail %zu, have %"PRIu32"/%"PRIu32")", + hdr->offset, hdr->chunk_len, avail, s_rx.received_bytes, s_rx.total_bytes); + prv_slot_cancel(hdr->token); prv_rx_reset(); return; } - memcpy(s_rx.pixels + hdr->offset, cursor, hdr->chunk_len); + if (s_rx.flash) { + if (!prv_slot_write(s_rx.slot, hdr->offset, cursor, hdr->chunk_len, hdr->token)) { + // A newer request reclaimed the slot; this transfer is dead. + prv_rx_reset(); + return; + } + } else { + memcpy(s_rx.pixels + hdr->offset, cursor, hdr->chunk_len); + } s_rx.received_bytes += hdr->chunk_len; if (hdr->flags & ImagingResponseFlagLast) { if (s_rx.received_bytes != s_rx.total_bytes) { + PBL_LOG_WRN("Imaging: short image, got %"PRIu32" of %"PRIu32" B", s_rx.received_bytes, + s_rx.total_bytes); + prv_slot_cancel(hdr->token); prv_rx_reset(); return; } GBitmap *bmp = kernel_zalloc(sizeof(GBitmap)); if (!bmp) { + PBL_LOG_WRN("Imaging: out of memory for GBitmap"); prv_rx_reset(); + prv_deliver(hdr->token, type, NULL); return; } - bmp->addr = s_rx.pixels; + if (s_rx.flash) { + bmp->addr = (void *)prv_slot_commit(s_rx.slot, type); + } else { + bmp->addr = s_rx.pixels; + bmp->info.is_bitmap_heap_allocated = true; + } bmp->row_size_bytes = s_rx.row_size_bytes; bmp->info.format = s_rx.format; bmp->info.version = GBITMAP_VERSION_CURRENT; bmp->bounds = (GRect) { { 0, 0 }, { s_rx.width, s_rx.height } }; bmp->palette = s_rx.palette; - // Ownership of the pixel and palette buffers moves into the bitmap. + bmp->info.is_palette_heap_allocated = (s_rx.palette != NULL); + // Ownership of the buffers moves into the bitmap (slot pixels stay put, flagged not-heap). const uint8_t token = s_rx.token; s_rx.pixels = NULL; s_rx.palette = NULL; @@ -296,8 +561,10 @@ void imaging_handle_comm_session_event(const PebbleCommSessionEvent *event) { // hold its pixel buffer until the next one starts. This runs on KernelMain, the same task as // the endpoint receiver, so touching s_rx is safe. Also clear the unsupported-type latch here // rather than relying solely on the pointer comparison in prv_type_latched_unsupported: a - // future session could be allocated at the address of the freed one. + // future session could be allocated at the address of the freed one. Delivered slot images + // survive — they stay displayable while disconnected — but abandon any in-flight load. prv_rx_reset(); + prv_slots_abandon_loads(); pbl_mutex_lock(&s_lock, PBL_FOREVER); s_latched_session = NULL; s_unsupported_types = 0; diff --git a/src/fw/services/music/service.c b/src/fw/services/music/service.c index 76d7335d97..afa9f66b6f 100644 --- a/src/fw/services/music/service.c +++ b/src/fw/services/music/service.c @@ -125,7 +125,9 @@ static bool prv_str_differs(const char *dest, const char *src, size_t src_length //! Free the currently-stored album art. Caller must hold the mutex. static void prv_free_album_art_locked(void) { if (s_music_ctx.album_art) { - kernel_free(s_music_ctx.album_art->addr); + if (s_music_ctx.album_art->info.is_bitmap_heap_allocated) { + kernel_free(s_music_ctx.album_art->addr); + } kernel_free(s_music_ctx.album_art->palette); kernel_free(s_music_ctx.album_art); s_music_ctx.album_art = NULL; diff --git a/src/fw/services/notifications/notification_image.c b/src/fw/services/notifications/notification_image.c index c59c778655..5dd0e9b008 100644 --- a/src/fw/services/notifications/notification_image.c +++ b/src/fw/services/notifications/notification_image.c @@ -21,7 +21,10 @@ static bool s_pending; //!< Waiting on a response for s_token. static void prv_free(GBitmap *bitmap) { if (bitmap) { - kernel_free(bitmap->addr); + // Flash-slot-backed pixels (see services/imaging) are not heap memory. + if (bitmap->info.is_bitmap_heap_allocated) { + kernel_free(bitmap->addr); + } kernel_free(bitmap->palette); kernel_free(bitmap); } diff --git a/tests/fakes/fake_spi_flash.c b/tests/fakes/fake_spi_flash.c index 54fdb52951..99cc1803b6 100644 --- a/tests/fakes/fake_spi_flash.c +++ b/tests/fakes/fake_spi_flash.c @@ -125,6 +125,13 @@ void fake_spi_flash_force_future_failure(int after_n_bytes, jmp_buf *retire_to) s_state.jmp_on_failure = retire_to; } +const void *flash_memory_mapped_address(uint32_t flash_addr) { + cl_assert(s_state.storage); + cl_assert(flash_addr >= s_state.offset); + cl_assert(flash_addr < s_state.offset + s_state.length); + return s_state.storage + (flash_addr - s_state.offset); +} + void flash_read_bytes(uint8_t* buffer, uint32_t start_addr, uint32_t buffer_size) { cl_assert(start_addr >= s_state.offset); cl_assert(start_addr + buffer_size <= s_state.offset + s_state.length); diff --git a/tests/fw/services/CMakeLists.txt b/tests/fw/services/CMakeLists.txt index ad22a9f1b8..c38138adc2 100644 --- a/tests/fw/services/CMakeLists.txt +++ b/tests/fw/services/CMakeLists.txt @@ -97,7 +97,10 @@ pbl_clar_test(test_music_endpoint pbl_clar_test(test_imaging SOURCES tests/fakes/fake_session.c + tests/fakes/fake_spi_flash.c src/fw/services/imaging/imaging.c + PLATFORMS obelix + DEFINES DUMA_DISABLED ) pbl_clar_test(test_compass_cal diff --git a/tests/fw/services/test_imaging.c b/tests/fw/services/test_imaging.c index 49b32dac2b..348e5ab095 100644 --- a/tests/fw/services/test_imaging.c +++ b/tests/fw/services/test_imaging.c @@ -15,8 +15,12 @@ /////////////////////////////////////////////////////////// #include "fake_session.h" +#include "fake_spi_flash.h" #include "fake_system_task.h" +#include "flash_region/flash_region.h" +#include + #include "stubs_bt_lock.h" #include "stubs_hexdump.h" #include "stubs_logging.h" @@ -43,6 +47,43 @@ uint16_t gbitmap_format_get_row_size_bytes(int16_t width, GBitmapFormat format) } } +// The slot erase completes synchronously against the fake flash, so by the time a request returns +// the slot is ready and the transfer takes the flash path. Tests that want the heap fallback +// receive a response without a matching request first (no slot reserved). +// +// Set s_defer_erase to model real hardware's async erase: the erase is performed but its completion +// callback is withheld until prv_fire_deferred_erase(), so a response can arrive while the slot is +// still "erasing" (the race that used to orphan a slot). +static bool s_defer_erase; +#define MAX_DEFERRED_ERASES (4) +static FlashOperationCompleteCb s_deferred_cb[MAX_DEFERRED_ERASES]; +static void *s_deferred_ctx[MAX_DEFERRED_ERASES]; +static int s_deferred_count; + +void flash_erase_optimal_range(uint32_t min_start, uint32_t max_start, uint32_t min_end, + uint32_t max_end, FlashOperationCompleteCb on_complete, + void *context) { + for (uint32_t addr = min_start; addr < max_end; addr += SUBSECTOR_SIZE_BYTES) { + flash_erase_subsector_blocking(addr); + } + if (s_defer_erase) { + cl_assert(s_deferred_count < MAX_DEFERRED_ERASES); + s_deferred_cb[s_deferred_count] = on_complete; + s_deferred_ctx[s_deferred_count] = context; + s_deferred_count++; + } else { + on_complete(context, S_SUCCESS); + } +} + +static void prv_fire_deferred_erases(void) { + const int n = s_deferred_count; + s_deferred_count = 0; + for (int i = 0; i < n; ++i) { + s_deferred_cb[i](s_deferred_ctx[i], S_SUCCESS); + } +} + // Delivery capture /////////////////////////////////////////////////////////// @@ -52,7 +93,9 @@ static GBitmap *s_last_bitmap; static void prv_free_last_bitmap(void) { if (s_last_bitmap) { - kernel_free(s_last_bitmap->addr); + if (s_last_bitmap->info.is_bitmap_heap_allocated) { + kernel_free(s_last_bitmap->addr); + } kernel_free(s_last_bitmap->palette); kernel_free(s_last_bitmap); s_last_bitmap = NULL; @@ -135,7 +178,13 @@ static void prv_receive_valid_image(uint8_t token) { static Transport *s_transport; +// The two image slots live in the IMAGING flash region; back it with fake flash. The two slots are +// non-contiguous (carved from separate RSVD areas), so init a window spanning both. void test_imaging__initialize(void) { + s_defer_erase = false; + s_deferred_count = 0; + fake_spi_flash_init(FLASH_REGION_IMAGING_0_BEGIN, + FLASH_REGION_IMAGING_1_END - FLASH_REGION_IMAGING_0_BEGIN); fake_comm_session_init(); s_transport = fake_transport_create(TransportDestinationSystem, NULL, NULL); fake_transport_set_connected(s_transport, true); @@ -145,17 +194,21 @@ void test_imaging__initialize(void) { s_notif_deliveries = 0; s_last_token = 0; s_last_bitmap = NULL; - // Reset any latched state left over from a previous test + // The slot pool is static and persists across tests: abandon any in-flight load (session close) + // and release both types' held slots so every test starts with an empty pool. const PebbleCommSessionEvent closed_event = { .is_open = false, .is_system = true, }; imaging_handle_comm_session_event(&closed_event); + imaging_release(ImagingImageTypeAlbumArt); + imaging_release(ImagingImageTypeNotification); } void test_imaging__cleanup(void) { prv_free_last_bitmap(); fake_comm_session_cleanup(); + fake_spi_flash_cleanup(); } void test_imaging__single_chunk_image(void) { @@ -352,6 +405,157 @@ void test_imaging__notification_request_payload_format(void) { fake_transport_assert_sent(s_transport, 0, 0x35, expected, sizeof(expected)); } +static const Uuid s_notif_id = UuidMake(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16); + +//! The well-formed 4x2 image, typed as a notification image. +static void prv_receive_valid_notification_image(uint8_t token) { + uint8_t buf[64]; + prv_receive(buf, prv_build_response( + buf, token, + prv_typed(ImagingImageTypeNotification, + ImagingResponseFlagFirst | ImagingResponseFlagLast), + 0, sizeof(s_pixels), 4, 2, ImagingFormat4BitPalette, + s_palette, sizeof(s_palette), s_pixels, sizeof(s_pixels))); +} + +//! True if `addr` points inside either fake-flash imaging slot. +static bool prv_addr_in_slots(const void *addr) { + const uint8_t *p = addr; + return (p >= flash_memory_mapped_address(FLASH_REGION_IMAGING_0_BEGIN) && + p < flash_memory_mapped_address(FLASH_REGION_IMAGING_0_END - 1)) || + (p >= flash_memory_mapped_address(FLASH_REGION_IMAGING_1_BEGIN) && + p < flash_memory_mapped_address(FLASH_REGION_IMAGING_1_END - 1)); +} + +void test_imaging__notification_image_stored_in_slot(void) { + cl_assert(imaging_request_notification_image(9, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + prv_receive_valid_notification_image(9); + cl_assert_equal_i(s_notif_deliveries, 1); + cl_assert(s_last_bitmap != NULL); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); + cl_assert(prv_addr_in_slots(s_last_bitmap->addr)); + cl_assert(memcmp(s_last_bitmap->addr, s_pixels, sizeof(s_pixels)) == 0); + cl_assert_equal_i(((GColor *)s_last_bitmap->palette)[1].argb, s_palette[1]); +} + +void test_imaging__heap_fallback_without_reservation(void) { + // No request first, so no slot was reserved: buffered on the heap. + prv_receive_valid_notification_image(9); + cl_assert_equal_i(s_notif_deliveries, 1); + cl_assert(s_last_bitmap != NULL); + cl_assert(s_last_bitmap->info.is_bitmap_heap_allocated); + cl_assert(memcmp(s_last_bitmap->addr, s_pixels, sizeof(s_pixels)) == 0); +} + +void test_imaging__two_consumers_hold_slots_at_once(void) { + // Album art takes one slot... + cl_assert(imaging_request_album_art(7, ImagingFormat4BitPalette, 166, 166, "Song", "Band")); + prv_receive_valid_image(7); + cl_assert_equal_i(s_deliveries, 1); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); + const void *art_addr = s_last_bitmap->addr; + + // ...a notification image takes the other, concurrently, in a different slot. + cl_assert(imaging_request_notification_image(9, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + prv_receive_valid_notification_image(9); + cl_assert_equal_i(s_notif_deliveries, 1); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); + cl_assert(prv_addr_in_slots(s_last_bitmap->addr)); + cl_assert(s_last_bitmap->addr != art_addr); +} + +void test_imaging__replacing_own_image_stays_flash_backed(void) { + // Only notifications active: one slot free. First image loads. + cl_assert(imaging_request_notification_image(9, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + prv_receive_valid_notification_image(9); + cl_assert_equal_i(s_notif_deliveries, 1); + + // A new notification replaces it. With a free slot it loads flicker-free into the other slot; + // either way it stays flash-backed and delivers. + const Uuid other = UuidMake(2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2); + cl_assert(imaging_request_notification_image(10, ImagingFormat4BitPalette, 180, 136, &other)); + prv_receive_valid_notification_image(10); + cl_assert_equal_i(s_notif_deliveries, 2); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); + cl_assert(prv_addr_in_slots(s_last_bitmap->addr)); + + // After releasing, the slots are all reusable again. + imaging_release(ImagingImageTypeNotification); + cl_assert(imaging_request_notification_image(11, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + prv_receive_valid_notification_image(11); + cl_assert_equal_i(s_notif_deliveries, 3); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); +} + +void test_imaging__superseded_transfer_is_abandoned(void) { + cl_assert(imaging_request_notification_image(9, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + // Transfer 9 starts streaming into its slot... + uint8_t buf[64]; + size_t len = prv_build_response( + buf, 9, prv_typed(ImagingImageTypeNotification, ImagingResponseFlagFirst), 0, 2, + 4, 2, ImagingFormat4BitPalette, s_palette, sizeof(s_palette), s_pixels, 2); + prv_receive(buf, len); + // ...but album art (a different consumer) fetches and completes into the other slot. + cl_assert(imaging_request_album_art(7, ImagingFormat4BitPalette, 166, 166, "Song", "Band")); + prv_receive_valid_image(7); + cl_assert_equal_i(s_deliveries, 1); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); + // The abandoned notification transfer resets s_rx; its stale last chunk delivers nothing. + cl_assert_equal_i(s_notif_deliveries, 0); +} + +void test_imaging__no_image_frees_the_reserved_slot(void) { + // Reserve a slot, then the phone reports no image: the slot must be freed, not leaked. + cl_assert(imaging_request_notification_image(9, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + uint8_t buf[32]; + prv_receive(buf, prv_build_response( + buf, 9, prv_typed(ImagingImageTypeNotification, ImagingResponseFlagNoImage), + 0, 0, 0, 0, 0, NULL, 0, NULL, 0)); + cl_assert_equal_i(s_notif_deliveries, 1); + cl_assert(s_last_bitmap == NULL); + + // Both slots are free again, so two fresh images both land in flash (not the heap). + cl_assert(imaging_request_album_art(7, ImagingFormat4BitPalette, 166, 166, "Song", "Band")); + prv_receive_valid_image(7); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); + cl_assert(imaging_request_notification_image(10, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + prv_receive_valid_notification_image(10); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); +} + +void test_imaging__erase_race_does_not_orphan_slots(void) { + // Model real hardware: the erase is async, so a fast response can arrive before the slot is + // erased. Both slots race and both images fall back to the heap. + s_defer_erase = true; + cl_assert(imaging_request_notification_image(9, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + prv_receive_valid_notification_image(9); + cl_assert(s_last_bitmap->info.is_bitmap_heap_allocated); + cl_assert(imaging_request_album_art(7, ImagingFormat4BitPalette, 166, 166, "Song", "Band")); + prv_receive_valid_image(7); + cl_assert(s_last_bitmap->info.is_bitmap_heap_allocated); + + // Both erases now complete. The dropped reservations must return to Idle, not become orphaned + // Ready slots that no response will ever claim (which would force every later image to the heap). + prv_fire_deferred_erases(); + + // With the erase synchronous again, a fresh image must land in a flash slot — proving the two + // slots were reclaimed rather than leaked. + s_defer_erase = false; + cl_assert(imaging_request_notification_image(11, ImagingFormat4BitPalette, 180, 136, + &s_notif_id)); + prv_receive_valid_notification_image(11); + cl_assert(!s_last_bitmap->info.is_bitmap_heap_allocated); + cl_assert(prv_addr_in_slots(s_last_bitmap->addr)); +} + void test_imaging__response_routed_by_type(void) { uint8_t buf[64]; const size_t len = prv_build_response( diff --git a/tests/stubs/stubs_imaging.h b/tests/stubs/stubs_imaging.h index 3a0c47ca52..b0db649319 100644 --- a/tests/stubs/stubs_imaging.h +++ b/tests/stubs/stubs_imaging.h @@ -8,6 +8,8 @@ void WEAK imaging_register_handler(ImagingImageType image_type, ImagingReceivedHandler handler) {} +void WEAK imaging_release(ImagingImageType image_type) {} + bool WEAK imaging_is_type_supported(ImagingImageType image_type) { return false; }