From 5417ff201cccd25feaaed9bb3b528d8a46577a77 Mon Sep 17 00:00:00 2001 From: Steve Penna Date: Tue, 8 Sep 2026 20:15:11 -0400 Subject: [PATCH 1/3] drivers/flash: add memory-mapped address accessor Expose flash_memory_mapped_address(), the CPU-addressable pointer for a flash address on platforms whose external flash is permanently memory-mapped (region addresses include the XIP base). Callers can then read flash bytes directly instead of copying them out, which the imaging service uses to render phone-supplied images straight from flash. Also provide the accessor in the SPI-flash test fake. Signed-off-by: Steve Penna Co-Authored-By: Claude Opus 4.8 --- include/pbl/drivers/flash.h | 7 +++++++ src/fw/drivers/flash/flash_api.c | 6 ++++++ tests/fakes/fake_spi_flash.c | 7 +++++++ 3 files changed, 20 insertions(+) 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/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/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); From d051b3b7e4dd64d6f1bc506e0abf298e2756a5c9 Mon Sep 17 00:00:00 2001 From: Steve Penna Date: Tue, 8 Sep 2026 20:15:42 -0400 Subject: [PATCH 2/3] flash_region: carve two IMAGING slots from reserved space Add IMAGING_0 and IMAGING_1, two 40 KiB slots taken from the previously reserved RSVD1/RSVD2 areas on the gd25q256e (emery/gabbro) and QEMU layouts. The imaging service stores decoded phone images here and renders them through the memory-mapped flash window. Every other region keeps its address, so the layout stays compatible with deployed devices. Signed-off-by: Steve Penna Co-Authored-By: Claude Opus 4.8 --- src/fw/flash_region/flash_region_gd25q256e.h | 14 ++++++++++++-- src/fw/flash_region/flash_region_qemu.h | 14 ++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) 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 From 9444f062e94325212f2b4d25958e7e7d645247a5 Mon Sep 17 00:00:00 2001 From: Steve Penna Date: Tue, 8 Sep 2026 20:15:54 -0400 Subject: [PATCH 3/3] services/imaging: store phone images in a flash slot pool Decoded album-art and notification images were reassembled into a pixel-sized kernel-heap buffer (~34 KiB on the largest displays), which a busy or fragmented heap could refuse, leaving the card on a grey placeholder. Stream each image instead into one of two fixed slots in the dedicated IMAGING flash region and render it directly through the memory-mapped window, so no pixel-sized heap allocation is needed. Slots are assigned to whichever image types are displaying rather than wired to a specific consumer, so any two consumers can hold an image at once; a third, or a transfer whose erase loses the race to the response, falls back to the heap. The reservation's erase overlaps the phone round trip, and a dropped reservation is reclaimed rather than left to orphan its slot. Delivered bitmaps now carry is_bitmap_heap_allocated so the consumers only free pixels that actually live on the heap, and imaging_release() lets a consumer hand its slot back when its window closes. Failure paths (no image, malformed response) log and, where relevant, free the reserved slot. On boards without the region the whole pool compiles out and images use the heap as before. Signed-off-by: Steve Penna Co-Authored-By: Claude Opus 4.8 --- include/pbl/services/imaging.h | 16 +- .../notifications/notification_window.c | 11 +- src/fw/services/imaging/imaging.c | 309 ++++++++++++++++-- src/fw/services/music/service.c | 4 +- .../notifications/notification_image.c | 5 +- tests/fw/services/CMakeLists.txt | 3 + tests/fw/services/test_imaging.c | 208 +++++++++++- tests/stubs/stubs_imaging.h | 2 + 8 files changed, 529 insertions(+), 29 deletions(-) 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/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/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; }