From 47ff7106334599e144a03de97b25a5328044b5f8 Mon Sep 17 00:00:00 2001 From: Silent Date: Sat, 7 Sep 2024 17:42:59 +0200 Subject: [PATCH] FuriThread: Improve state callbacks State callbacks assumed they were invoked from the thread that changed its state, but this wasn't true for FuriThreadStateStarting in the past, and now it's not true for FuriThreadStateStopped either. Now it is safe to release the thread memory form the state callback once it switches to FuriThreadStateStopped. Therefore, pending deletion calls can be removed. --- applications/services/loader/loader.c | 8 ++- applications/services/region/region.c | 15 ++--- applications/services/rpc/rpc.c | 67 +++++++++---------- .../system/updater/util/update_task.c | 8 +-- furi/core/thread.c | 16 ++--- furi/core/thread.h | 6 +- targets/f18/api_symbols.csv | 2 +- targets/f7/api_symbols.csv | 2 +- 8 files changed, 58 insertions(+), 66 deletions(-) diff --git a/applications/services/loader/loader.c b/applications/services/loader/loader.c index 0f4cc4a0c24..f34b0e63b33 100644 --- a/applications/services/loader/loader.c +++ b/applications/services/loader/loader.c @@ -308,12 +308,14 @@ static void loader_applications_closed_callback(void* context) { furi_message_queue_put(loader->queue, &message, FuriWaitForever); } -static void loader_thread_state_callback(FuriThreadState thread_state, void* context) { +static void + loader_thread_state_callback(FuriThread* thread, FuriThreadState thread_state, void* context) { + UNUSED(thread); furi_assert(context); - Loader* loader = context; - if(thread_state == FuriThreadStateStopped) { + Loader* loader = context; + LoaderMessage message; message.type = LoaderMessageTypeAppClosed; furi_message_queue_put(loader->queue, &message, FuriWaitForever); diff --git a/applications/services/region/region.c b/applications/services/region/region.c index dffcc6b2d5e..bed676f9bd1 100644 --- a/applications/services/region/region.c +++ b/applications/services/region/region.c @@ -104,19 +104,12 @@ static int32_t region_load_file(void* context) { return 0; } -static void region_loader_pending_callback(void* context, uint32_t arg) { - UNUSED(arg); - - FuriThread* loader = context; - furi_thread_join(loader); - furi_thread_free(loader); -} - -static void region_loader_state_callback(FuriThreadState state, void* context) { +static void + region_loader_release_callback(FuriThread* thread, FuriThreadState state, void* context) { UNUSED(context); if(state == FuriThreadStateStopped) { - furi_timer_pending_callback(region_loader_pending_callback, furi_thread_get_current(), 0); + furi_thread_free(thread); } } @@ -126,7 +119,7 @@ static void region_storage_callback(const void* message, void* context) { if(event->type == StorageEventTypeCardMount) { FuriThread* loader = furi_thread_alloc_ex(NULL, 2048, region_load_file, NULL); - furi_thread_set_state_callback(loader, region_loader_state_callback); + furi_thread_set_state_callback(loader, region_loader_release_callback); furi_thread_start(loader); } } diff --git a/applications/services/rpc/rpc.c b/applications/services/rpc/rpc.c index 00ec2259c75..08a2c3f6ded 100644 --- a/applications/services/rpc/rpc.c +++ b/applications/services/rpc/rpc.c @@ -67,7 +67,7 @@ static RpcSystemCallbacks rpc_systems[] = { struct RpcSession { Rpc* rpc; - FuriThread* thread; + FuriThreadId thread_id; RpcHandlerDict_t handlers; FuriStreamBuffer* stream; @@ -172,7 +172,7 @@ size_t rpc_session_feed( size_t bytes_sent = furi_stream_buffer_send(session->stream, encoded_bytes, size, timeout); - furi_thread_flags_set(furi_thread_get_id(session->thread), RpcEvtNewData); + furi_thread_flags_set(session->thread_id, RpcEvtNewData); return bytes_sent; } @@ -220,7 +220,7 @@ bool rpc_pb_stream_read(pb_istream_t* istream, pb_byte_t* buf, size_t count) { break; } else { /* Save disconnect flag and continue reading buffer */ - furi_thread_flags_set(furi_thread_get_id(session->thread), RpcEvtDisconnect); + furi_thread_flags_set(session->thread_id, RpcEvtDisconnect); } } else if(flags & RpcEvtNewData) { // Just wake thread up @@ -347,35 +347,32 @@ static int32_t rpc_session_worker(void* context) { return 0; } -static void rpc_session_thread_pending_callback(void* context, uint32_t arg) { - UNUSED(arg); - RpcSession* session = (RpcSession*)context; +static void rpc_session_thread_release_callback( + FuriThread* thread, + FuriThreadState thread_state, + void* context) { + if(thread_state == FuriThreadStateStopped) { + RpcSession* session = (RpcSession*)context; - for(size_t i = 0; i < COUNT_OF(rpc_systems); ++i) { - if(rpc_systems[i].free) { - (rpc_systems[i].free)(session->system_contexts[i]); + for(size_t i = 0; i < COUNT_OF(rpc_systems); ++i) { + if(rpc_systems[i].free) { + (rpc_systems[i].free)(session->system_contexts[i]); + } } - } - free(session->system_contexts); - free(session->decoded_message); - RpcHandlerDict_clear(session->handlers); - furi_stream_buffer_free(session->stream); - - furi_mutex_acquire(session->callbacks_mutex, FuriWaitForever); - if(session->terminated_callback) { - session->terminated_callback(session->context); - } - furi_mutex_release(session->callbacks_mutex); - - furi_mutex_free(session->callbacks_mutex); - furi_thread_join(session->thread); - furi_thread_free(session->thread); - free(session); -} + free(session->system_contexts); + free(session->decoded_message); + RpcHandlerDict_clear(session->handlers); + furi_stream_buffer_free(session->stream); + + furi_mutex_acquire(session->callbacks_mutex, FuriWaitForever); + if(session->terminated_callback) { + session->terminated_callback(session->context); + } + furi_mutex_release(session->callbacks_mutex); -static void rpc_session_thread_state_callback(FuriThreadState thread_state, void* context) { - if(thread_state == FuriThreadStateStopped) { - furi_timer_pending_callback(rpc_session_thread_pending_callback, context, 0); + furi_mutex_free(session->callbacks_mutex); + furi_thread_free(thread); + free(session); } } @@ -407,12 +404,14 @@ RpcSession* rpc_session_open(Rpc* rpc, RpcOwner owner) { }; rpc_add_handler(session, PB_Main_stop_session_tag, &rpc_handler); - session->thread = furi_thread_alloc_ex("RpcSessionWorker", 3072, rpc_session_worker, session); + FuriThread* thread = + furi_thread_alloc_ex("RpcSessionWorker", 3072, rpc_session_worker, session); + session->thread_id = furi_thread_get_id(thread); - furi_thread_set_state_context(session->thread, session); - furi_thread_set_state_callback(session->thread, rpc_session_thread_state_callback); + furi_thread_set_state_context(thread, session); + furi_thread_set_state_callback(thread, rpc_session_thread_release_callback); - furi_thread_start(session->thread); + furi_thread_start(thread); return session; } @@ -424,7 +423,7 @@ void rpc_session_close(RpcSession* session) { rpc_session_set_send_bytes_callback(session, NULL); rpc_session_set_close_callback(session, NULL); rpc_session_set_buffer_is_empty_callback(session, NULL); - furi_thread_flags_set(furi_thread_get_id(session->thread), RpcEvtDisconnect); + furi_thread_flags_set(session->thread_id, RpcEvtDisconnect); } void rpc_on_system_start(void* p) { diff --git a/applications/system/updater/util/update_task.c b/applications/system/updater/util/update_task.c index cca488475ea..9db8339efc1 100644 --- a/applications/system/updater/util/update_task.c +++ b/applications/system/updater/util/update_task.c @@ -395,14 +395,15 @@ bool update_task_open_file(UpdateTask* update_task, FuriString* filename) { return open_success; } -static void update_task_worker_thread_cb(FuriThreadState state, void* context) { - UpdateTask* update_task = context; +static void + update_task_worker_thread_cb(FuriThread* thread, FuriThreadState state, void* context) { + UNUSED(context); if(state != FuriThreadStateStopped) { return; } - if(furi_thread_get_return_code(update_task->thread) == UPDATE_TASK_NOERR) { + if(furi_thread_get_return_code(thread) == UPDATE_TASK_NOERR) { furi_delay_ms(UPDATE_DELAY_OPERATION_OK); furi_hal_power_reset(); } @@ -427,7 +428,6 @@ UpdateTask* update_task_alloc(void) { furi_thread_alloc_ex("UpdateWorker", 5120, NULL, update_task); furi_thread_set_state_callback(thread, update_task_worker_thread_cb); - furi_thread_set_state_context(thread, update_task); #ifdef FURI_RAM_EXEC UNUSED(update_task_worker_backup_restore); furi_thread_set_callback(thread, update_task_worker_flash_writer); diff --git a/furi/core/thread.c b/furi/core/thread.c index 69c6b0f04e8..cb9b13ec74d 100644 --- a/furi/core/thread.c +++ b/furi/core/thread.c @@ -33,7 +33,7 @@ struct FuriThread { StaticTask_t container; StackType_t* stack_buffer; - FuriThreadState state; + volatile FuriThreadState state; int32_t ret; FuriThreadCallback callback; @@ -59,7 +59,6 @@ struct FuriThread { // this ensures that the size of this structure is minimal bool is_service; bool heap_trace_enabled; - volatile bool is_active; }; // IMPORTANT: container MUST be the FIRST struct member @@ -81,7 +80,7 @@ static void furi_thread_set_state(FuriThread* thread, FuriThreadState state) { furi_assert(thread); thread->state = state; if(thread->state_callback) { - thread->state_callback(state, thread->state_context); + thread->state_callback(thread, state, thread->state_context); } } @@ -121,7 +120,7 @@ static void furi_thread_body(void* context) { // flush stdout __furi_thread_stdout_flush(thread); - furi_thread_set_state(thread, FuriThreadStateStopped); + furi_thread_set_state(thread, FuriThreadStateStopping); vTaskDelete(NULL); furi_thread_catch(); @@ -202,7 +201,6 @@ void furi_thread_free(FuriThread* thread) { furi_check(thread->is_service == false); // Cannot free a non-joined thread furi_check(thread->state == FuriThreadStateStopped); - furi_check(!thread->is_active); furi_thread_set_name(thread, NULL); furi_thread_set_appid(thread, NULL); @@ -347,8 +345,6 @@ void furi_thread_start(FuriThread* thread) { uint32_t stack_depth = thread->stack_size / sizeof(StackType_t); UBaseType_t priority = thread->priority ? thread->priority : FuriThreadPriorityNormal; - thread->is_active = true; - furi_check( xTaskCreateStatic( furi_thread_body, @@ -366,7 +362,7 @@ void furi_thread_cleanup_tcb_event(TaskHandle_t task) { // clear thread local storage vTaskSetThreadLocalStoragePointer(task, 0, NULL); furi_check(thread == (FuriThread*)task); - thread->is_active = false; + furi_thread_set_state(thread, FuriThreadStateStopped); } } @@ -381,8 +377,8 @@ bool furi_thread_join(FuriThread* thread) { // // If your thread exited, but your app stuck here: some other thread uses // all cpu time, which delays kernel from releasing task handle - while(thread->is_active) { - furi_delay_ms(10); + while(thread->state != FuriThreadStateStopped) { + furi_delay_tick(2); } return true; diff --git a/furi/core/thread.h b/furi/core/thread.h index e8cdeaeafb6..d88ab0f2922 100644 --- a/furi/core/thread.h +++ b/furi/core/thread.h @@ -21,7 +21,8 @@ extern "C" { * Many of the FuriThread functions MUST ONLY be called when the thread is STOPPED. */ typedef enum { - FuriThreadStateStopped, /**< Thread is stopped */ + FuriThreadStateStopped, /**< Thread is stopped and is safe to release */ + FuriThreadStateStopping, /**< Thread is stopping */ FuriThreadStateStarting, /**< Thread is starting */ FuriThreadStateRunning, /**< Thread is running */ } FuriThreadState; @@ -81,10 +82,11 @@ typedef void (*FuriThreadStdoutWriteCallback)(const char* data, size_t size); * * The function to be used as a state callback MUST follow this signature. * + * @param[in] pointer to the FuriThread instance that changed the state * @param[in] state identifier of the state the thread has transitioned to * @param[in,out] context pointer to a user-specified object */ -typedef void (*FuriThreadStateCallback)(FuriThreadState state, void* context); +typedef void (*FuriThreadStateCallback)(FuriThread* thread, FuriThreadState state, void* context); /** * @brief Signal handler callback function pointer type. diff --git a/targets/f18/api_symbols.csv b/targets/f18/api_symbols.csv index 7f3e2f396d3..cb226a11fbf 100644 --- a/targets/f18/api_symbols.csv +++ b/targets/f18/api_symbols.csv @@ -1,5 +1,5 @@ entry,status,name,type,params -Version,+,72.4,, +Version,+,73.0,, Header,+,applications/services/bt/bt_service/bt.h,, Header,+,applications/services/bt/bt_service/bt_keys_storage.h,, Header,+,applications/services/cli/cli.h,, diff --git a/targets/f7/api_symbols.csv b/targets/f7/api_symbols.csv index c6f44f914be..7c75daf65ce 100644 --- a/targets/f7/api_symbols.csv +++ b/targets/f7/api_symbols.csv @@ -1,5 +1,5 @@ entry,status,name,type,params -Version,+,72.4,, +Version,+,73.0,, Header,+,applications/drivers/subghz/cc1101_ext/cc1101_ext_interconnect.h,, Header,+,applications/services/bt/bt_service/bt.h,, Header,+,applications/services/bt/bt_service/bt_keys_storage.h,,