vcp, cli: Handle Tx/Rx events before Connect/Disconnect + extra fixes (#4181)

* cli_vcp: handle tx/rx before connext/disconnect

* cli_vcp: disable trace

* cli_perf: advanced error reporting

* cli_vcp: reset tx flag directly in event handler

* fix formatting

* cli_vcp: make tx flag volatile

* storage_settings: fix scene ids

* cli_shell: add safety check to set_prompt

* cli_registry: move from bptree to dict, fix memory leak

* cli_vcp: go back to message queue for event signaling

* loader: move BeforeLoad event even earlier

* fix formatting

.gitignore

@@ -67,3 +67,7 @@ PVS-Studio.log
 
 # JS packages
 node_modules/
+
+# cli_perf script output in case of errors
+/block.bin
+/return_block.bin

applications/services/cli/cli_vcp.c

@@ -30,27 +30,23 @@ typedef struct {
 } CliVcpMessage;
 
 typedef enum {
-    CliVcpInternalEventConnected = (1 << 0),
+    CliVcpInternalEventConnected,
-    CliVcpInternalEventDisconnected = (1 << 1),
+    CliVcpInternalEventDisconnected,
-    CliVcpInternalEventTxDone = (1 << 2),
+    CliVcpInternalEventTxDone,
-    CliVcpInternalEventRx = (1 << 3),
+    CliVcpInternalEventRx,
 } CliVcpInternalEvent;
 
-#define CliVcpInternalEventAll                                                                    \
-    (CliVcpInternalEventConnected | CliVcpInternalEventDisconnected | CliVcpInternalEventTxDone | \
-     CliVcpInternalEventRx)
-
 struct CliVcp {
     FuriEventLoop* event_loop;
     FuriMessageQueue* message_queue; // <! external messages
-    FuriThreadId thread_id;
+    FuriMessageQueue* internal_evt_queue;
 
     bool is_enabled, is_connected;
     FuriHalUsbInterface* previous_interface;
 
     PipeSide* own_pipe;
     PipeSide* shell_pipe;
-    bool is_currently_transmitting;
+    volatile bool is_currently_transmitting;
     size_t previous_tx_length;
 
     CliRegistry* main_registry;
@@ -101,11 +97,12 @@ static void cli_vcp_maybe_receive_data(CliVcp* cli_vcp) {
 // =============
 
 static void cli_vcp_signal_internal_event(CliVcp* cli_vcp, CliVcpInternalEvent event) {
-    furi_thread_flags_set(cli_vcp->thread_id, event);
+    furi_check(furi_message_queue_put(cli_vcp->internal_evt_queue, &event, 0) == FuriStatusOk);
 }
 
 static void cli_vcp_cdc_tx_done(void* context) {
     CliVcp* cli_vcp = context;
+    cli_vcp->is_currently_transmitting = false;
     cli_vcp_signal_internal_event(cli_vcp, CliVcpInternalEventTxDone);
 }
 
@@ -190,12 +187,25 @@ static void cli_vcp_message_received(FuriEventLoopObject* object, void* context)
 /**
  * Processes messages arriving from CDC event callbacks
  */
-static void cli_vcp_internal_event_happened(void* context) {
+static void cli_vcp_internal_event_happened(FuriEventLoopObject* object, void* context) {
     CliVcp* cli_vcp = context;
-    CliVcpInternalEvent event = furi_thread_flags_wait(CliVcpInternalEventAll, FuriFlagWaitAny, 0);
+    CliVcpInternalEvent event;
-    furi_check(!(event & FuriFlagError));
+    furi_check(furi_message_queue_get(object, &event, 0) == FuriStatusOk);
 
-    if(event & CliVcpInternalEventDisconnected) {
+    switch(event) {
+    case CliVcpInternalEventRx: {
+        VCP_TRACE(TAG, "Rx");
+        cli_vcp_maybe_receive_data(cli_vcp);
+        break;
+    }
+
+    case CliVcpInternalEventTxDone: {
+        VCP_TRACE(TAG, "TxDone");
+        cli_vcp_maybe_send_data(cli_vcp);
+        break;
+    }
+
+    case CliVcpInternalEventDisconnected: {
         if(!cli_vcp->is_connected) return;
         FURI_LOG_D(TAG, "Disconnected");
         cli_vcp->is_connected = false;
@@ -204,9 +214,10 @@ static void cli_vcp_internal_event_happened(void* context) {
         pipe_detach_from_event_loop(cli_vcp->own_pipe);
         pipe_free(cli_vcp->own_pipe);
         cli_vcp->own_pipe = NULL;
+        break;
     }
 
-    if(event & CliVcpInternalEventConnected) {
+    case CliVcpInternalEventConnected: {
         if(cli_vcp->is_connected) return;
         FURI_LOG_D(TAG, "Connected");
         cli_vcp->is_connected = true;
@@ -233,17 +244,8 @@ static void cli_vcp_internal_event_happened(void* context) {
         cli_vcp->shell = cli_shell_alloc(
             cli_main_motd, NULL, cli_vcp->shell_pipe, cli_vcp->main_registry, &cli_main_ext_config);
         cli_shell_start(cli_vcp->shell);
+        break;
     }
-
-    if(event & CliVcpInternalEventRx) {
-        VCP_TRACE(TAG, "Rx");
-        cli_vcp_maybe_receive_data(cli_vcp);
-    }
-
-    if(event & CliVcpInternalEventTxDone) {
-        VCP_TRACE(TAG, "TxDone");
-        cli_vcp->is_currently_transmitting = false;
-        cli_vcp_maybe_send_data(cli_vcp);
     }
 }
 
@@ -253,7 +255,6 @@ static void cli_vcp_internal_event_happened(void* context) {
 
 static CliVcp* cli_vcp_alloc(void) {
     CliVcp* cli_vcp = malloc(sizeof(CliVcp));
-    cli_vcp->thread_id = furi_thread_get_current_id();
 
     cli_vcp->event_loop = furi_event_loop_alloc();
 
@@ -265,8 +266,14 @@ static CliVcp* cli_vcp_alloc(void) {
         cli_vcp_message_received,
         cli_vcp);
 
-    furi_event_loop_subscribe_thread_flags(
+    cli_vcp->internal_evt_queue =
-        cli_vcp->event_loop, cli_vcp_internal_event_happened, cli_vcp);
+        furi_message_queue_alloc(VCP_MESSAGE_Q_LEN, sizeof(CliVcpInternalEvent));
+    furi_event_loop_subscribe_message_queue(
+        cli_vcp->event_loop,
+        cli_vcp->internal_evt_queue,
+        FuriEventLoopEventIn,
+        cli_vcp_internal_event_happened,
+        cli_vcp);
 
     cli_vcp->main_registry = furi_record_open(RECORD_CLI);
 

applications/services/loader/loader.c

@@ -418,9 +418,6 @@ static void loader_start_internal_app(
     const FlipperInternalApplication* app,
     const char* args) {
     FURI_LOG_I(TAG, "Starting %s", app->name);
-    LoaderEvent event;
-    event.type = LoaderEventTypeApplicationBeforeLoad;
-    furi_pubsub_publish(loader->pubsub, &event);
 
     // store args
     furi_assert(loader->app.args == NULL);
@@ -508,10 +505,6 @@ static LoaderMessageLoaderStatusResult loader_start_external_app(
     result.value = loader_make_success_status(error_message);
     result.error = LoaderStatusErrorUnknown;
 
-    LoaderEvent event;
-    event.type = LoaderEventTypeApplicationBeforeLoad;
-    furi_pubsub_publish(loader->pubsub, &event);
-
     do {
         loader->app.fap = flipper_application_alloc(storage, firmware_api_interface);
         size_t start = furi_get_tick();
@@ -566,6 +559,7 @@ static LoaderMessageLoaderStatusResult loader_start_external_app(
     if(result.value != LoaderStatusOk) {
         flipper_application_free(loader->app.fap);
         loader->app.fap = NULL;
+        LoaderEvent event;
         event.type = LoaderEventTypeApplicationLoadFailed;
         furi_pubsub_publish(loader->pubsub, &event);
     }
@@ -615,6 +609,10 @@ static LoaderMessageLoaderStatusResult loader_do_start_by_name(
     status.value = loader_make_success_status(error_message);
     status.error = LoaderStatusErrorUnknown;
 
+    LoaderEvent event;
+    event.type = LoaderEventTypeApplicationBeforeLoad;
+    furi_pubsub_publish(loader->pubsub, &event);
+
     do {
         // check lock
         if(loader_do_is_locked(loader)) {

applications/settings/storage_settings/storage_settings.c

@@ -5,8 +5,8 @@ const SubmenuSettingsHelperDescriptor descriptor_template = {
     .options_cnt = 6,
     .options =
         {
-            {.name = "About Internal Storage", .scene_id = StorageSettingsSDInfo},
+            {.name = "About Internal Storage", .scene_id = StorageSettingsInternalInfo},
-            {.name = "About SD Card", .scene_id = StorageSettingsInternalInfo},
+            {.name = "About SD Card", .scene_id = StorageSettingsSDInfo},
             {.name = "Unmount SD Card", .scene_id = StorageSettingsUnmountConfirm},
             {.name = "Format SD Card", .scene_id = StorageSettingsFormatConfirm},
             {.name = "Benchmark SD Card", .scene_id = StorageSettingsBenchmarkConfirm},

lib/toolbox/cli/cli_registry.c

@@ -3,16 +3,16 @@
 #include <toolbox/pipe.h>
 #include <storage/storage.h>
 
-#define TAG "cli"
+#define TAG "CliRegistry"
 
 struct CliRegistry {
-    CliCommandTree_t commands;
+    CliCommandDict_t commands;
     FuriMutex* mutex;
 };
 
 CliRegistry* cli_registry_alloc(void) {
     CliRegistry* registry = malloc(sizeof(CliRegistry));
-    CliCommandTree_init(registry->commands);
+    CliCommandDict_init(registry->commands);
     registry->mutex = furi_mutex_alloc(FuriMutexTypeRecursive);
     return registry;
 }
@@ -20,7 +20,7 @@ CliRegistry* cli_registry_alloc(void) {
 void cli_registry_free(CliRegistry* registry) {
     furi_check(furi_mutex_acquire(registry->mutex, FuriWaitForever) == FuriStatusOk);
     furi_mutex_free(registry->mutex);
-    CliCommandTree_clear(registry->commands);
+    CliCommandDict_clear(registry->commands);
     free(registry);
 }
 
@@ -61,7 +61,7 @@ void cli_registry_add_command_ex(
     };
 
     furi_check(furi_mutex_acquire(registry->mutex, FuriWaitForever) == FuriStatusOk);
-    CliCommandTree_set_at(registry->commands, name_str, command);
+    CliCommandDict_set_at(registry->commands, name_str, command);
     furi_check(furi_mutex_release(registry->mutex) == FuriStatusOk);
 
     furi_string_free(name_str);
@@ -79,7 +79,7 @@ void cli_registry_delete_command(CliRegistry* registry, const char* name) {
     } while(name_replace != FURI_STRING_FAILURE);
 
     furi_check(furi_mutex_acquire(registry->mutex, FuriWaitForever) == FuriStatusOk);
-    CliCommandTree_erase(registry->commands, name_str);
+    CliCommandDict_erase(registry->commands, name_str);
     furi_check(furi_mutex_release(registry->mutex) == FuriStatusOk);
 
     furi_string_free(name_str);
@@ -91,7 +91,7 @@ bool cli_registry_get_command(
     CliRegistryCommand* result) {
     furi_assert(registry);
     furi_check(furi_mutex_acquire(registry->mutex, FuriWaitForever) == FuriStatusOk);
-    CliRegistryCommand* data = CliCommandTree_get(registry->commands, command);
+    CliRegistryCommand* data = CliCommandDict_get(registry->commands, command);
     if(data) *result = *data;
 
     furi_check(furi_mutex_release(registry->mutex) == FuriStatusOk);
@@ -103,16 +103,14 @@ void cli_registry_remove_external_commands(CliRegistry* registry) {
     furi_check(registry);
     furi_check(furi_mutex_acquire(registry->mutex, FuriWaitForever) == FuriStatusOk);
 
-    // FIXME FL-3977: memory leak somewhere within this function
+    CliCommandDict_t internal_cmds;
-
+    CliCommandDict_init(internal_cmds);
-    CliCommandTree_t internal_cmds;
-    CliCommandTree_init(internal_cmds);
     for
-        M_EACH(item, registry->commands, CliCommandTree_t) {
+        M_EACH(item, registry->commands, CliCommandDict_t) {
-            if(!(item->value_ptr->flags & CliCommandFlagExternal))
+            if(!(item->value.flags & CliCommandFlagExternal))
-                CliCommandTree_set_at(internal_cmds, *item->key_ptr, *item->value_ptr);
+                CliCommandDict_set_at(internal_cmds, item->key, item->value);
         }
-    CliCommandTree_move(registry->commands, internal_cmds);
+    CliCommandDict_move(registry->commands, internal_cmds);
 
     furi_check(furi_mutex_release(registry->mutex) == FuriStatusOk);
 }
@@ -148,7 +146,7 @@ void cli_registry_reload_external_commands(
                 .execute_callback = NULL,
                 .flags = CliCommandFlagExternal,
             };
-            CliCommandTree_set_at(registry->commands, plugin_name, command);
+            CliCommandDict_set_at(registry->commands, plugin_name, command);
         }
 
         furi_string_free(plugin_name);
@@ -172,7 +170,7 @@ void cli_registry_unlock(CliRegistry* registry) {
     furi_mutex_release(registry->mutex);
 }
 
-CliCommandTree_t* cli_registry_get_commands(CliRegistry* registry) {
+CliCommandDict_t* cli_registry_get_commands(CliRegistry* registry) {
     furi_assert(registry);
     return &registry->commands;
 }

lib/toolbox/cli/cli_registry_i.h

@@ -6,7 +6,7 @@
 #pragma once
 
 #include <furi.h>
-#include <m-bptree.h>
+#include <m-dict.h>
 #include "cli_registry.h"
 
 #ifdef __cplusplus
@@ -22,19 +22,9 @@ typedef struct {
     size_t stack_depth;
 } CliRegistryCommand;
 
-#define CLI_COMMANDS_TREE_RANK 4
+DICT_DEF2(CliCommandDict, FuriString*, FURI_STRING_OPLIST, CliRegistryCommand, M_POD_OPLIST);
 
-// -V:BPTREE_DEF2:1103
+#define M_OPL_CliCommandDict_t() DICT_OPLIST(CliCommandDict, FURI_STRING_OPLIST, M_POD_OPLIST)
-// -V:BPTREE_DEF2:524
-BPTREE_DEF2(
-    CliCommandTree,
-    CLI_COMMANDS_TREE_RANK,
-    FuriString*,
-    FURI_STRING_OPLIST,
-    CliRegistryCommand,
-    M_POD_OPLIST);
-
-#define M_OPL_CliCommandTree_t() BPTREE_OPLIST2(CliCommandTree, FURI_STRING_OPLIST, M_POD_OPLIST)
 
 bool cli_registry_get_command(
     CliRegistry* registry,
@@ -48,7 +38,7 @@ void cli_registry_unlock(CliRegistry* registry);
 /**
  * @warning Surround calls to this function with `cli_registry_[un]lock`
  */
-CliCommandTree_t* cli_registry_get_commands(CliRegistry* registry);
+CliCommandDict_t* cli_registry_get_commands(CliRegistry* registry);
 
 #ifdef __cplusplus
 }

lib/toolbox/cli/shell/cli_shell.c

@@ -103,15 +103,15 @@ void cli_command_help(PipeSide* pipe, FuriString* args, void* context) {
 
     printf("Available commands:\r\n" ANSI_FG_GREEN);
     cli_registry_lock(registry);
-    CliCommandTree_t* commands = cli_registry_get_commands(registry);
+    CliCommandDict_t* commands = cli_registry_get_commands(registry);
-    size_t commands_count = CliCommandTree_size(*commands);
+    size_t commands_count = CliCommandDict_size(*commands);
 
-    CliCommandTree_it_t iterator;
+    CliCommandDict_it_t iterator;
-    CliCommandTree_it(iterator, *commands);
+    CliCommandDict_it(iterator, *commands);
     for(size_t i = 0; i < commands_count; i++) {
-        const CliCommandTree_itref_t* item = CliCommandTree_cref(iterator);
+        const CliCommandDict_itref_t* item = CliCommandDict_cref(iterator);
-        printf("%-30s", furi_string_get_cstr(*item->key_ptr));
+        printf("%-30s", furi_string_get_cstr(item->key));
-        CliCommandTree_next(iterator);
+        CliCommandDict_next(iterator);
 
         if(i % columns == columns - 1) printf("\r\n");
     }
@@ -474,5 +474,6 @@ void cli_shell_join(CliShell* shell) {
 
 void cli_shell_set_prompt(CliShell* shell, const char* prompt) {
     furi_check(shell);
+    furi_check(furi_thread_get_state(shell->thread) == FuriThreadStateStopped);
     shell->prompt = prompt;
 }

lib/toolbox/cli/shell/cli_shell_completions.c

@@ -111,10 +111,10 @@ void cli_shell_completions_fill_variants(CliShellCompletions* completions) {
     if(segment.type == CliShellCompletionSegmentTypeCommand) {
         CliRegistry* registry = completions->registry;
         cli_registry_lock(registry);
-        CliCommandTree_t* commands = cli_registry_get_commands(registry);
+        CliCommandDict_t* commands = cli_registry_get_commands(registry);
         for
-            M_EACH(registered_command, *commands, CliCommandTree_t) {
+            M_EACH(registered_command, *commands, CliCommandDict_t) {
-                FuriString* command_name = *registered_command->key_ptr;
+                FuriString* command_name = registered_command->key;
                 if(furi_string_start_with(command_name, input)) {
                     CommandCompletions_push_back(completions->variants, command_name);
                 }

scripts/serial_cli_perf.py

@@ -32,16 +32,27 @@ def main():
 
     bytes_to_send = args.length
     block_size = 1024
+    success = True
     while bytes_to_send:
         actual_size = min(block_size, bytes_to_send)
         # can't use 0x03 because that's ASCII ETX, or Ctrl+C
-        block = bytes([randint(4, 255) for _ in range(actual_size)])
+        # block = bytes([randint(4, 255) for _ in range(actual_size)])
+        block = bytes([4 + (i // 64) for i in range(actual_size)])
 
         port.write(block)
         return_block = port.read(actual_size)
 
         if return_block != block:
-            logger.error("Incorrect block received")
+            with open("block.bin", "wb") as f:
+                f.write(block)
+            with open("return_block.bin", "wb") as f:
+                f.write(return_block)
+
+            logger.error(
+                "Incorrect block received. Saved to `block.bin' and `return_block.bin'."
+            )
+            logger.error(f"{bytes_to_send} bytes left. Aborting.")
+            success = False
             break
 
         bytes_to_send -= actual_size
@@ -49,7 +60,8 @@ def main():
     end_time = time()
     delta = end_time - start_time
     speed = args.length / delta
-    print(f"Speed: {speed/1024:.2f} KiB/s")
+    if success:
+        print(f"Speed: {speed/1024:.2f} KiB/s")
 
     port.write(b"\x03")  # Ctrl+C
     port.close()