From cf0cc4fa8d413f1313129b6074c43c6cfd139b39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=82=E3=81=8F?= Date: Sat, 6 Jul 2024 11:44:25 +0100 Subject: [PATCH 1/5] Furi: count ISR time. Cli: show ISR time in top. (#3751) * Furi: count ISR time. Cli: show ISR time in top. * hal: interrupt: macros for interrupt accounting; split FURI_ALWAYS_STATIC_INLINE -> FURI_ALWAYS_INLINE static Co-authored-by: hedger Co-authored-by: hedger --- applications/services/cli/cli_commands.c | 3 +- furi/core/common_defines.h | 4 +- furi/core/thread_list.c | 16 ++++++- furi/core/thread_list.h | 8 ++++ targets/f18/api_symbols.csv | 4 +- targets/f7/api_symbols.csv | 4 +- targets/f7/furi_hal/furi_hal_gpio.c | 2 +- targets/f7/furi_hal/furi_hal_interrupt.c | 53 ++++++++++++++++++------ targets/f7/furi_hal/furi_hal_interrupt.h | 6 +++ 9 files changed, 81 insertions(+), 19 deletions(-) diff --git a/applications/services/cli/cli_commands.c b/applications/services/cli/cli_commands.c index ea13bb8b7..d6fa395c8 100644 --- a/applications/services/cli/cli_commands.c +++ b/applications/services/cli/cli_commands.c @@ -399,8 +399,9 @@ static void cli_command_top(Cli* cli, FuriString* args, void* context) { uint32_t uptime = tick / furi_kernel_get_tick_frequency(); printf( - "Threads: %zu, Uptime: %luh%lum%lus\r\n", + "Threads: %zu, ISR Time: %0.2f%%, Uptime: %luh%lum%lus\r\n", furi_thread_list_size(thread_list), + (double)furi_thread_list_get_isr_time(thread_list), uptime / 60 / 60, uptime / 60 % 60, uptime % 60); diff --git a/furi/core/common_defines.h b/furi/core/common_defines.h index 0f6230c19..20883332b 100644 --- a/furi/core/common_defines.h +++ b/furi/core/common_defines.h @@ -29,8 +29,8 @@ extern "C" { #define FURI_PACKED __attribute__((packed)) #endif -#ifndef FURI_ALWAYS_STATIC_INLINE -#define FURI_ALWAYS_STATIC_INLINE __attribute__((always_inline)) static inline +#ifndef FURI_ALWAYS_INLINE +#define FURI_ALWAYS_INLINE __attribute__((always_inline)) inline #endif #ifndef FURI_IS_IRQ_MASKED diff --git a/furi/core/thread_list.c b/furi/core/thread_list.c index 65ee11ad3..86759b66b 100644 --- a/furi/core/thread_list.c +++ b/furi/core/thread_list.c @@ -1,6 +1,8 @@ #include "thread_list.h" #include "check.h" +#include + #include #include @@ -23,6 +25,8 @@ struct FuriThreadList { FuriThreadListItemDict_t search; uint32_t runtime_previous; uint32_t runtime_current; + uint32_t isr_previous; + uint32_t isr_current; }; FuriThreadList* furi_thread_list_alloc(void) { @@ -85,7 +89,10 @@ void furi_thread_list_process(FuriThreadList* instance, uint32_t runtime, uint32 instance->runtime_previous = instance->runtime_current; instance->runtime_current = runtime; - uint32_t runtime_counter = instance->runtime_current - instance->runtime_previous; + instance->isr_previous = instance->isr_current; + instance->isr_current = furi_hal_interrupt_get_time_in_isr_total(); + + const uint32_t runtime_counter = instance->runtime_current - instance->runtime_previous; FuriThreadListItemArray_it_t it; FuriThreadListItemArray_it(it, instance->items); @@ -108,3 +115,10 @@ void furi_thread_list_process(FuriThreadList* instance, uint32_t runtime, uint32 } } } + +float furi_thread_list_get_isr_time(FuriThreadList* instance) { + const uint32_t runtime_counter = instance->runtime_current - instance->runtime_previous; + const uint32_t isr_counter = instance->isr_current - instance->isr_previous; + + return (float)isr_counter / (float)runtime_counter; +} \ No newline at end of file diff --git a/furi/core/thread_list.h b/furi/core/thread_list.h index bf15e4032..d01aa24a0 100644 --- a/furi/core/thread_list.h +++ b/furi/core/thread_list.h @@ -76,6 +76,14 @@ FuriThreadListItem* furi_thread_list_get_or_insert(FuriThreadList* instance, Fur */ void furi_thread_list_process(FuriThreadList* instance, uint32_t runtime, uint32_t tick); +/** Get percent of time spent in ISR + * + * @param instance The instance + * + * @return percent of time spent in ISR + */ +float furi_thread_list_get_isr_time(FuriThreadList* instance); + #ifdef __cplusplus } #endif diff --git a/targets/f18/api_symbols.csv b/targets/f18/api_symbols.csv index 23e098a91..ae561c98c 100644 --- a/targets/f18/api_symbols.csv +++ b/targets/f18/api_symbols.csv @@ -1,5 +1,5 @@ entry,status,name,type,params -Version,+,68.0,, +Version,+,68.1,, Header,+,applications/services/bt/bt_service/bt.h,, Header,+,applications/services/bt/bt_service/bt_keys_storage.h,, Header,+,applications/services/cli/cli.h,, @@ -1288,6 +1288,7 @@ Function,+,furi_hal_info_get_api_version,void,"uint16_t*, uint16_t*" Function,-,furi_hal_init,void, Function,-,furi_hal_init_early,void, Function,+,furi_hal_interrupt_get_name,const char*,uint8_t +Function,+,furi_hal_interrupt_get_time_in_isr_total,uint32_t, Function,-,furi_hal_interrupt_init,void, Function,+,furi_hal_interrupt_set_isr,void,"FuriHalInterruptId, FuriHalInterruptISR, void*" Function,+,furi_hal_interrupt_set_isr_ex,void,"FuriHalInterruptId, FuriHalInterruptPriority, FuriHalInterruptISR, void*" @@ -1633,6 +1634,7 @@ Function,+,furi_thread_join,_Bool,FuriThread* Function,+,furi_thread_list_alloc,FuriThreadList*, Function,+,furi_thread_list_free,void,FuriThreadList* Function,+,furi_thread_list_get_at,FuriThreadListItem*,"FuriThreadList*, size_t" +Function,+,furi_thread_list_get_isr_time,float,FuriThreadList* Function,+,furi_thread_list_get_or_insert,FuriThreadListItem*,"FuriThreadList*, FuriThread*" Function,+,furi_thread_list_process,void,"FuriThreadList*, uint32_t, uint32_t" Function,+,furi_thread_list_size,size_t,FuriThreadList* diff --git a/targets/f7/api_symbols.csv b/targets/f7/api_symbols.csv index 6cccce1e6..f4c7a84e1 100644 --- a/targets/f7/api_symbols.csv +++ b/targets/f7/api_symbols.csv @@ -1,5 +1,5 @@ entry,status,name,type,params -Version,+,68.0,, +Version,+,68.1,, 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,, @@ -1413,6 +1413,7 @@ Function,+,furi_hal_infrared_set_tx_output,void,FuriHalInfraredTxPin Function,-,furi_hal_init,void, Function,-,furi_hal_init_early,void, Function,+,furi_hal_interrupt_get_name,const char*,uint8_t +Function,+,furi_hal_interrupt_get_time_in_isr_total,uint32_t, Function,-,furi_hal_interrupt_init,void, Function,+,furi_hal_interrupt_set_isr,void,"FuriHalInterruptId, FuriHalInterruptISR, void*" Function,+,furi_hal_interrupt_set_isr_ex,void,"FuriHalInterruptId, FuriHalInterruptPriority, FuriHalInterruptISR, void*" @@ -1847,6 +1848,7 @@ Function,+,furi_thread_join,_Bool,FuriThread* Function,+,furi_thread_list_alloc,FuriThreadList*, Function,+,furi_thread_list_free,void,FuriThreadList* Function,+,furi_thread_list_get_at,FuriThreadListItem*,"FuriThreadList*, size_t" +Function,+,furi_thread_list_get_isr_time,float,FuriThreadList* Function,+,furi_thread_list_get_or_insert,FuriThreadListItem*,"FuriThreadList*, FuriThread*" Function,+,furi_thread_list_process,void,"FuriThreadList*, uint32_t, uint32_t" Function,+,furi_thread_list_size,size_t,FuriThreadList* diff --git a/targets/f7/furi_hal/furi_hal_gpio.c b/targets/f7/furi_hal/furi_hal_gpio.c index ec3e725c5..c68c1ffd4 100644 --- a/targets/f7/furi_hal/furi_hal_gpio.c +++ b/targets/f7/furi_hal/furi_hal_gpio.c @@ -249,7 +249,7 @@ void furi_hal_gpio_remove_int_callback(const GpioPin* gpio) { FURI_CRITICAL_EXIT(); } -FURI_ALWAYS_STATIC_INLINE void furi_hal_gpio_int_call(uint16_t pin_num) { +FURI_ALWAYS_INLINE static void furi_hal_gpio_int_call(uint16_t pin_num) { if(gpio_interrupt[pin_num].callback) { gpio_interrupt[pin_num].callback(gpio_interrupt[pin_num].context); } diff --git a/targets/f7/furi_hal/furi_hal_interrupt.c b/targets/f7/furi_hal/furi_hal_interrupt.c index 15a9a819f..f366cff16 100644 --- a/targets/f7/furi_hal/furi_hal_interrupt.c +++ b/targets/f7/furi_hal/furi_hal_interrupt.c @@ -13,12 +13,22 @@ #define FURI_HAL_INTERRUPT_DEFAULT_PRIORITY (configLIBRARY_MAX_SYSCALL_INTERRUPT_PRIORITY + 5) +#define FURI_HAL_INTERRUPT_ACCOUNT_START() const uint32_t _isr_start = DWT->CYCCNT; +#define FURI_HAL_INTERRUPT_ACCOUNT_END() \ + const uint32_t _time_in_isr = DWT->CYCCNT - _isr_start; \ + furi_hal_interrupt.counter_time_in_isr_total += _time_in_isr; + typedef struct { FuriHalInterruptISR isr; void* context; } FuriHalInterruptISRPair; -FuriHalInterruptISRPair furi_hal_interrupt_isr[FuriHalInterruptIdMax] = {0}; +typedef struct { + FuriHalInterruptISRPair isr[FuriHalInterruptIdMax]; + uint32_t counter_time_in_isr_total; +} FuriHalIterrupt; + +static FuriHalIterrupt furi_hal_interrupt = {}; const IRQn_Type furi_hal_interrupt_irqn[FuriHalInterruptIdMax] = { // TIM1, TIM16, TIM17 @@ -67,12 +77,16 @@ const IRQn_Type furi_hal_interrupt_irqn[FuriHalInterruptIdMax] = { [FuriHalInterruptIdLpUart1] = LPUART1_IRQn, }; -FURI_ALWAYS_STATIC_INLINE void furi_hal_interrupt_call(FuriHalInterruptId index) { - furi_check(furi_hal_interrupt_isr[index].isr); - furi_hal_interrupt_isr[index].isr(furi_hal_interrupt_isr[index].context); +FURI_ALWAYS_INLINE static void furi_hal_interrupt_call(FuriHalInterruptId index) { + const FuriHalInterruptISRPair* isr_descr = &furi_hal_interrupt.isr[index]; + furi_check(isr_descr->isr); + + FURI_HAL_INTERRUPT_ACCOUNT_START(); + isr_descr->isr(isr_descr->context); + FURI_HAL_INTERRUPT_ACCOUNT_END(); } -FURI_ALWAYS_STATIC_INLINE void +FURI_ALWAYS_INLINE static void furi_hal_interrupt_enable(FuriHalInterruptId index, uint16_t priority) { NVIC_SetPriority( furi_hal_interrupt_irqn[index], @@ -80,19 +94,19 @@ FURI_ALWAYS_STATIC_INLINE void NVIC_EnableIRQ(furi_hal_interrupt_irqn[index]); } -FURI_ALWAYS_STATIC_INLINE void furi_hal_interrupt_clear_pending(FuriHalInterruptId index) { +FURI_ALWAYS_INLINE static void furi_hal_interrupt_clear_pending(FuriHalInterruptId index) { NVIC_ClearPendingIRQ(furi_hal_interrupt_irqn[index]); } -FURI_ALWAYS_STATIC_INLINE void furi_hal_interrupt_get_pending(FuriHalInterruptId index) { +FURI_ALWAYS_INLINE static void furi_hal_interrupt_get_pending(FuriHalInterruptId index) { NVIC_GetPendingIRQ(furi_hal_interrupt_irqn[index]); } -FURI_ALWAYS_STATIC_INLINE void furi_hal_interrupt_set_pending(FuriHalInterruptId index) { +FURI_ALWAYS_INLINE static void furi_hal_interrupt_set_pending(FuriHalInterruptId index) { NVIC_SetPendingIRQ(furi_hal_interrupt_irqn[index]); } -FURI_ALWAYS_STATIC_INLINE void furi_hal_interrupt_disable(FuriHalInterruptId index) { +FURI_ALWAYS_INLINE static void furi_hal_interrupt_disable(FuriHalInterruptId index) { NVIC_DisableIRQ(furi_hal_interrupt_irqn[index]); } @@ -137,17 +151,18 @@ void furi_hal_interrupt_set_isr_ex( uint16_t real_priority = FURI_HAL_INTERRUPT_DEFAULT_PRIORITY - priority; + FuriHalInterruptISRPair* isr_descr = &furi_hal_interrupt.isr[index]; if(isr) { // Pre ISR set - furi_check(furi_hal_interrupt_isr[index].isr == NULL); + furi_check(isr_descr->isr == NULL); } else { // Pre ISR clear furi_hal_interrupt_disable(index); furi_hal_interrupt_clear_pending(index); } - furi_hal_interrupt_isr[index].isr = isr; - furi_hal_interrupt_isr[index].context = context; + isr_descr->isr = isr; + isr_descr->context = context; __DMB(); if(isr) { @@ -304,27 +319,37 @@ extern void HW_IPCC_Tx_Handler(void); extern void HW_IPCC_Rx_Handler(void); void SysTick_Handler(void) { + FURI_HAL_INTERRUPT_ACCOUNT_START(); furi_hal_os_tick(); + FURI_HAL_INTERRUPT_ACCOUNT_END(); } void USB_LP_IRQHandler(void) { #ifndef FURI_RAM_EXEC + FURI_HAL_INTERRUPT_ACCOUNT_START(); usbd_poll(&udev); + FURI_HAL_INTERRUPT_ACCOUNT_END(); #endif } void USB_HP_IRQHandler(void) { //-V524 #ifndef FURI_RAM_EXEC + FURI_HAL_INTERRUPT_ACCOUNT_START(); usbd_poll(&udev); + FURI_HAL_INTERRUPT_ACCOUNT_END(); #endif } void IPCC_C1_TX_IRQHandler(void) { + FURI_HAL_INTERRUPT_ACCOUNT_START(); HW_IPCC_Tx_Handler(); + FURI_HAL_INTERRUPT_ACCOUNT_END(); } void IPCC_C1_RX_IRQHandler(void) { + FURI_HAL_INTERRUPT_ACCOUNT_START(); HW_IPCC_Rx_Handler(); + FURI_HAL_INTERRUPT_ACCOUNT_END(); } void FPU_IRQHandler(void) { @@ -499,3 +524,7 @@ const char* furi_hal_interrupt_get_name(uint8_t exception_number) { return NULL; } } + +uint32_t furi_hal_interrupt_get_time_in_isr_total(void) { + return furi_hal_interrupt.counter_time_in_isr_total; +} \ No newline at end of file diff --git a/targets/f7/furi_hal/furi_hal_interrupt.h b/targets/f7/furi_hal/furi_hal_interrupt.h index c06ec23d2..2326d3c0a 100644 --- a/targets/f7/furi_hal/furi_hal_interrupt.h +++ b/targets/f7/furi_hal/furi_hal_interrupt.h @@ -118,6 +118,12 @@ void furi_hal_interrupt_set_isr_ex( */ const char* furi_hal_interrupt_get_name(uint8_t exception_number); +/** Get total time(in CPU clocks) spent in ISR + * + * @return total time in CPU clocks + */ +uint32_t furi_hal_interrupt_get_time_in_isr_total(void); + #ifdef __cplusplus } #endif From 248e926a82a7a2ac2610128f6ab1282675431cd0 Mon Sep 17 00:00:00 2001 From: Konstantin Volkov <72250702+doomwastaken@users.noreply.github.com> Date: Sat, 6 Jul 2024 13:49:56 +0300 Subject: [PATCH 2/5] Updating bench flipper search algorithm (#3742) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * initial commit, changed flipper search function and unit test runner script * increased timeouts due to new unpacking delays * python formatting fixes * fixed flipper name Co-authored-by: doomwastaken Co-authored-by: あく --- .github/workflows/unit_tests.yml | 11 ++- .github/workflows/updater_test.yml | 10 +-- scripts/flipper/utils/cdc.py | 1 - scripts/testing/await_flipper.py | 61 -------------- scripts/testing/units.py | 87 ------------------- scripts/testops.py | 129 +++++++++++++++++++++++++++++ 6 files changed, 138 insertions(+), 161 deletions(-) delete mode 100755 scripts/testing/await_flipper.py delete mode 100755 scripts/testing/units.py create mode 100644 scripts/testops.py diff --git a/.github/workflows/unit_tests.yml b/.github/workflows/unit_tests.yml index d35ca0c17..d37337452 100644 --- a/.github/workflows/unit_tests.yml +++ b/.github/workflows/unit_tests.yml @@ -1,5 +1,4 @@ name: 'Unit tests' - on: pull_request: @@ -25,7 +24,7 @@ jobs: - name: 'Get flipper from device manager (mock)' id: device run: | - echo "flipper=/dev/ttyACM0" >> $GITHUB_OUTPUT + echo "flipper=auto" >> $GITHUB_OUTPUT - name: 'Flash unit tests firmware' id: flashing @@ -40,7 +39,7 @@ jobs: timeout-minutes: 5 run: | source scripts/toolchain/fbtenv.sh - python3 scripts/testing/await_flipper.py ${{steps.device.outputs.flipper}} + python3 scripts/testops.py -p=${{steps.device.outputs.flipper}} -t=120 await_flipper python3 scripts/storage.py -p ${{steps.device.outputs.flipper}} format_ext - name: 'Copy assets and unit data, reboot and wait for flipper' @@ -49,11 +48,11 @@ jobs: timeout-minutes: 7 run: | source scripts/toolchain/fbtenv.sh - python3 scripts/testing/await_flipper.py ${{steps.device.outputs.flipper}} + python3 scripts/testops.py -p=${{steps.device.outputs.flipper}} -t=15 await_flipper rm -rf build/latest/resources/dolphin python3 scripts/storage.py -p ${{steps.device.outputs.flipper}} -f send build/latest/resources /ext python3 scripts/power.py -p ${{steps.device.outputs.flipper}} reboot - python3 scripts/testing/await_flipper.py ${{steps.device.outputs.flipper}} + python3 scripts/testops.py -p=${{steps.device.outputs.flipper}} -t=15 await_flipper - name: 'Run units and validate results' id: run_units @@ -61,7 +60,7 @@ jobs: timeout-minutes: 7 run: | source scripts/toolchain/fbtenv.sh - python3 scripts/testing/units.py ${{steps.device.outputs.flipper}} + python3 scripts/testops.py run_units -p ${{steps.device.outputs.flipper}} - name: 'Check GDB output' if: failure() && steps.flashing.outcome == 'success' diff --git a/.github/workflows/updater_test.yml b/.github/workflows/updater_test.yml index b14b618a6..45bc53c38 100644 --- a/.github/workflows/updater_test.yml +++ b/.github/workflows/updater_test.yml @@ -1,8 +1,6 @@ name: 'Updater test' - on: pull_request: - env: TARGETS: f7 DEFAULT_TARGET: f7 @@ -26,7 +24,7 @@ jobs: - name: 'Get flipper from device manager (mock)' id: device run: | - echo "flipper=Rekigyn" >> $GITHUB_OUTPUT + echo "flipper=auto" >> $GITHUB_OUTPUT echo "stlink=0F020D026415303030303032" >> $GITHUB_OUTPUT - name: 'Flashing target firmware' @@ -35,7 +33,7 @@ jobs: run: | source scripts/toolchain/fbtenv.sh ./fbt flash_usb_full PORT=${{steps.device.outputs.flipper}} FORCE=1 - python3 scripts/testing/await_flipper.py ${{steps.device.outputs.flipper}} + python3 scripts/testops.py -p=${{steps.device.outputs.flipper}} -t=120 await_flipper - name: 'Validating updater' id: second_full_flash @@ -44,7 +42,7 @@ jobs: run: | source scripts/toolchain/fbtenv.sh ./fbt flash_usb PORT=${{steps.device.outputs.flipper}} FORCE=1 - python3 scripts/testing/await_flipper.py ${{steps.device.outputs.flipper}} + python3 scripts/testops.py -p=${{steps.device.outputs.flipper}} -t=120 await_flipper - name: 'Get last release tag' id: release_tag @@ -71,5 +69,5 @@ jobs: if: failure() run: | source scripts/toolchain/fbtenv.sh - python3 scripts/testing/await_flipper.py ${{steps.device.outputs.flipper}} + python3 scripts/testops.py -p=${{steps.device.outputs.flipper}} -t=120 await_flipper python3 scripts/storage.py -p ${{steps.device.outputs.flipper}} format_ext diff --git a/scripts/flipper/utils/cdc.py b/scripts/flipper/utils/cdc.py index 956408859..ee1125f77 100644 --- a/scripts/flipper/utils/cdc.py +++ b/scripts/flipper/utils/cdc.py @@ -15,4 +15,3 @@ def resolve_port(logger, portname: str = "auto"): logger.error("Failed to find connected Flipper") elif len(flippers) > 1: logger.error("More than one Flipper is attached") - logger.error("Failed to guess which port to use") diff --git a/scripts/testing/await_flipper.py b/scripts/testing/await_flipper.py deleted file mode 100755 index f8dffeb66..000000000 --- a/scripts/testing/await_flipper.py +++ /dev/null @@ -1,61 +0,0 @@ -#!/usr/bin/env python3 -import logging -import os -import sys -import time - - -def flp_serial_by_name(flp_name): - if sys.platform == "darwin": # MacOS - flp_serial = "/dev/cu.usbmodemflip_" + flp_name + "1" - logging.info(f"Darwin, looking for {flp_serial}") - elif sys.platform == "linux": # Linux - flp_serial = ( - "/dev/serial/by-id/usb-Flipper_Devices_Inc._Flipper_" - + flp_name - + "_flip_" - + flp_name - + "-if00" - ) - logging.info(f"linux, looking for {flp_serial}") - - if os.path.exists(flp_serial): - return flp_serial - else: - logging.info(f"Couldn't find {flp_name} on this attempt.") - if os.path.exists(flp_name): - return flp_name - else: - return "" - - -UPDATE_TIMEOUT = 30 * 4 # 4 minutes - - -def main(): - flipper_name = sys.argv[1] - elapsed = 0 - flipper = flp_serial_by_name(flipper_name) - logging.basicConfig( - format="%(asctime)s %(levelname)-8s %(message)s", - level=logging.INFO, - datefmt="%Y-%m-%d %H:%M:%S", - ) - logging.info(f"Waiting for Flipper {flipper_name} to be ready...") - - while flipper == "" and elapsed < UPDATE_TIMEOUT: - elapsed += 1 - time.sleep(1) - flipper = flp_serial_by_name(flipper_name) - - if flipper == "": - logging.error("Flipper not found!") - exit(1) - - logging.info(f"Found Flipper at {flipper}") - - sys.exit(0) - - -if __name__ == "__main__": - main() diff --git a/scripts/testing/units.py b/scripts/testing/units.py deleted file mode 100755 index db302e9da..000000000 --- a/scripts/testing/units.py +++ /dev/null @@ -1,87 +0,0 @@ -#!/usr/bin/env python3 -import logging -import re -import sys - -import serial -from await_flipper import flp_serial_by_name - - -def main(): - logging.basicConfig( - format="%(asctime)s %(levelname)-8s %(message)s", - level=logging.INFO, - datefmt="%Y-%m-%d %H:%M:%S", - ) - logging.info("Trying to run units on flipper") - flp_serial = flp_serial_by_name(sys.argv[1]) - - if flp_serial == "": - logging.error("Flipper not found!") - sys.exit(1) - - with serial.Serial(flp_serial, timeout=150) as flipper: - logging.info(f"Found Flipper at {flp_serial}") - flipper.baudrate = 230400 - flipper.flushOutput() - flipper.flushInput() - - flipper.read_until(b">: ").decode("utf-8") - flipper.write(b"unit_tests\r") - data = flipper.read_until(b">: ").decode("utf-8") - - lines = data.split("\r\n") - - tests_re = r"Failed tests: \d{0,}" - time_re = r"Consumed: \d{0,}" - leak_re = r"Leaked: \d{0,}" - status_re = r"Status: \w{3,}" - - tests_pattern = re.compile(tests_re) - time_pattern = re.compile(time_re) - leak_pattern = re.compile(leak_re) - status_pattern = re.compile(status_re) - - tests, time, leak, status = None, None, None, None - total = 0 - - for line in lines: - logging.info(line) - if "()" in line: - total += 1 - - if not tests: - tests = re.match(tests_pattern, line) - if not time: - time = re.match(time_pattern, line) - if not leak: - leak = re.match(leak_pattern, line) - if not status: - status = re.match(status_pattern, line) - - if None in (tests, time, leak, status): - logging.error(f"Failed to parse output: {leak} {time} {leak} {status}") - sys.exit(1) - - leak = int(re.findall(r"[- ]\d+", leak.group(0))[0]) - status = re.findall(r"\w+", status.group(0))[1] - tests = int(re.findall(r"\d+", tests.group(0))[0]) - time = int(re.findall(r"\d+", time.group(0))[0]) - - if tests > 0 or status != "PASSED": - logging.error(f"Got {tests} failed tests.") - logging.error(f"Leaked (not failing on this stat): {leak}") - logging.error(f"Status: {status}") - logging.error(f"Time: {time/1000} seconds") - sys.exit(1) - - logging.info(f"Leaked (not failing on this stat): {leak}") - logging.info( - f"Tests ran successfully! Time elapsed {time/1000} seconds. Passed {total} tests." - ) - - sys.exit(0) - - -if __name__ == "__main__": - main() diff --git a/scripts/testops.py b/scripts/testops.py new file mode 100644 index 000000000..bf02feaad --- /dev/null +++ b/scripts/testops.py @@ -0,0 +1,129 @@ +#!/usr/bin/env python3 + +import re +import sys +import time +from typing import Optional + +from flipper.app import App +from flipper.storage import FlipperStorage +from flipper.utils.cdc import resolve_port + + +class Main(App): + # this is basic use without sub-commands, simply to reboot flipper / power it off, not meant as a full CLI wrapper + def init(self): + self.parser.add_argument("-p", "--port", help="CDC Port", default="auto") + self.parser.add_argument( + "-t", "--timeout", help="Timeout in seconds", type=int, default=10 + ) + + self.subparsers = self.parser.add_subparsers(help="sub-command help") + + self.parser_await_flipper = self.subparsers.add_parser( + "await_flipper", help="Wait for Flipper to connect or reconnect" + ) + self.parser_await_flipper.set_defaults(func=self.await_flipper) + + self.parser_run_units = self.subparsers.add_parser( + "run_units", help="Run unit tests and post result" + ) + self.parser_run_units.set_defaults(func=self.run_units) + + def _get_flipper(self, retry_count: Optional[int] = 1): + port = None + self.logger.info(f"Attempting to find flipper with {retry_count} attempts.") + + for i in range(retry_count): + self.logger.info(f"Attempt to find flipper #{i}.") + + if port := resolve_port(self.logger, self.args.port): + self.logger.info(f"Found flipper at {port}") + break + time.sleep(1) + + if not port: + self.logger.info(f"Failed to find flipper {port}") + return None + + flipper = FlipperStorage(port) + flipper.start() + return flipper + + def await_flipper(self): + if not (flipper := self._get_flipper(retry_count=self.args.timeout)): + return 1 + + self.logger.info("Flipper started") + flipper.stop() + return 0 + + def run_units(self): + if not (flipper := self._get_flipper(retry_count=10)): + return 1 + + self.logger.info("Running unit tests") + flipper.send("unit_tests" + "\r") + self.logger.info("Waiting for unit tests to complete") + data = flipper.read.until(">: ") + self.logger.info("Parsing result") + + lines = data.decode().split("\r\n") + + tests_re = r"Failed tests: \d{0,}" + time_re = r"Consumed: \d{0,}" + leak_re = r"Leaked: \d{0,}" + status_re = r"Status: \w{3,}" + + tests_pattern = re.compile(tests_re) + time_pattern = re.compile(time_re) + leak_pattern = re.compile(leak_re) + status_pattern = re.compile(status_re) + + tests, elapsed_time, leak, status = None, None, None, None + total = 0 + + for line in lines: + self.logger.info(line) + if "()" in line: + total += 1 + + if not tests: + tests = re.match(tests_pattern, line) + if not elapsed_time: + elapsed_time = re.match(time_pattern, line) + if not leak: + leak = re.match(leak_pattern, line) + if not status: + status = re.match(status_pattern, line) + + if None in (tests, elapsed_time, leak, status): + self.logger.error( + f"Failed to parse output: {tests} {elapsed_time} {leak} {status}" + ) + sys.exit(1) + + leak = int(re.findall(r"[- ]\d+", leak.group(0))[0]) + status = re.findall(r"\w+", status.group(0))[1] + tests = int(re.findall(r"\d+", tests.group(0))[0]) + elapsed_time = int(re.findall(r"\d+", elapsed_time.group(0))[0]) + + if tests > 0 or status != "PASSED": + self.logger.error(f"Got {tests} failed tests.") + self.logger.error(f"Leaked (not failing on this stat): {leak}") + self.logger.error(f"Status: {status}") + self.logger.error(f"Time: {elapsed_time/1000} seconds") + flipper.stop() + return 1 + + self.logger.info(f"Leaked (not failing on this stat): {leak}") + self.logger.info( + f"Tests ran successfully! Time elapsed {elapsed_time/1000} seconds. Passed {total} tests." + ) + + flipper.stop() + return 0 + + +if __name__ == "__main__": + Main()() From 1510d8773b3da0774ecd9d8c9081139d5814dbbc Mon Sep 17 00:00:00 2001 From: Filipe Paz Rodrigues Date: Sat, 6 Jul 2024 06:08:44 -0500 Subject: [PATCH 3/5] CCID: Improve request and response data handling (#3741) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * CCID: Improve request and response data handling - Add iso7816_set_response function: serves a helpers to set SW1 and SW2 values - improved iso7816_read_response_apdu by correctly parsing Lc and Le values - add client script to make testing easier * lint and rename * Format * Review changes: pragma once, typedef * Move command/response data and datalen into respective structures * Remove conditional for Lc=0 * Fix comment: Le * Make PVS happy and fix spelling Co-authored-by: あく --- applications/debug/ccid_test/ccid_test_app.c | 137 +++++++++++++----- .../debug/ccid_test/client/ccid_client.py | 116 +++++++++++++++ .../debug/ccid_test/client/requirements.txt | 2 + applications/debug/ccid_test/iso7816_atr.h | 5 +- .../debug/ccid_test/iso7816_callbacks.c | 55 +++---- .../debug/ccid_test/iso7816_callbacks.h | 15 +- .../debug/ccid_test/iso7816_response.c | 8 + .../debug/ccid_test/iso7816_response.h | 12 ++ .../debug/ccid_test/iso7816_t0_apdu.c | 64 ++++++-- .../debug/ccid_test/iso7816_t0_apdu.h | 40 ++--- targets/f7/furi_hal/furi_hal_usb_ccid.c | 3 +- targets/furi_hal_include/furi_hal_usb_ccid.h | 2 + 12 files changed, 341 insertions(+), 118 deletions(-) create mode 100644 applications/debug/ccid_test/client/ccid_client.py create mode 100644 applications/debug/ccid_test/client/requirements.txt create mode 100644 applications/debug/ccid_test/iso7816_response.c create mode 100644 applications/debug/ccid_test/iso7816_response.h diff --git a/applications/debug/ccid_test/ccid_test_app.c b/applications/debug/ccid_test/ccid_test_app.c index 6993901d2..be6f631f4 100644 --- a/applications/debug/ccid_test/ccid_test_app.c +++ b/applications/debug/ccid_test/ccid_test_app.c @@ -9,6 +9,7 @@ #include "iso7816_callbacks.h" #include "iso7816_t0_apdu.h" #include "iso7816_atr.h" +#include "iso7816_response.h" typedef enum { EventTypeInput, @@ -118,6 +119,76 @@ static const CcidCallbacks ccid_cb = { ccid_xfr_datablock_callback, }; +//Instruction 1: returns an OK response unconditionally +//APDU example: 0x01:0x01:0x00:0x00 +//response: SW1=0x90, SW2=0x00 +void handle_instruction_01(ISO7816_Response_APDU* responseAPDU) { + responseAPDU->DataLen = 0; + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_OK); +} + +//Instruction 2: expect command with no body, replies wit with a body with two bytes +//APDU example: 0x01:0x02:0x00:0x00:0x02 +//response: 'bc' (0x62, 0x63) SW1=0x90, SW2=0x00 +void handle_instruction_02( + uint8_t p1, + uint8_t p2, + uint8_t lc, + uint8_t le, + ISO7816_Response_APDU* responseAPDU) { + if(p1 == 0 && p2 == 0 && lc == 0 && le >= 2) { + responseAPDU->Data[0] = 0x62; + responseAPDU->Data[1] = 0x63; + + responseAPDU->DataLen = 2; + + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_OK); + } else if(p1 != 0 || p2 != 0) { + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_WRONG_PARAMETERS_P1_P2); + } else { + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_WRONG_LENGTH); + } +} + +//Instruction 3: sends a command with a body with two bytes, receives a response with no bytes +//APDU example: 0x01:0x03:0x00:0x00:0x02:CA:FE +//response SW1=0x90, SW2=0x00 +void handle_instruction_03(uint8_t p1, uint8_t p2, uint8_t lc, ISO7816_Response_APDU* responseAPDU) { + if(p1 == 0 && p2 == 0 && lc == 2) { + responseAPDU->DataLen = 0; + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_OK); + } else if(p1 != 0 || p2 != 0) { + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_WRONG_PARAMETERS_P1_P2); + } else { + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_WRONG_LENGTH); + } +} + +//instruction 4: sends a command with a body with 'n' bytes, receives a response with 'n' bytes +//APDU example: 0x01:0x04:0x00:0x00:0x04:0x01:0x02:0x03:0x04:0x04 +//receives (0x01, 0x02, 0x03, 0x04) SW1=0x90, SW2=0x00 +void handle_instruction_04( + uint8_t p1, + uint8_t p2, + uint8_t lc, + uint8_t le, + const uint8_t* commandApduDataBuffer, + ISO7816_Response_APDU* responseAPDU) { + if(p1 == 0 && p2 == 0 && lc > 0 && le > 0 && le >= lc) { + for(uint16_t i = 0; i < lc; i++) { + responseAPDU->Data[i] = commandApduDataBuffer[i]; + } + + responseAPDU->DataLen = lc; + + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_OK); + } else if(p1 != 0 || p2 != 0) { + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_WRONG_PARAMETERS_P1_P2); + } else { + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_WRONG_LENGTH); + } +} + void iso7816_answer_to_reset(Iso7816Atr* atr) { //minimum valid ATR: https://smartcard-atr.apdu.fr/parse?ATR=3B+00 atr->TS = 0x3B; @@ -125,48 +196,38 @@ void iso7816_answer_to_reset(Iso7816Atr* atr) { } void iso7816_process_command( - const struct ISO7816_Command_APDU* commandAPDU, - struct ISO7816_Response_APDU* responseAPDU, - const uint8_t* commandApduDataBuffer, - uint8_t commandApduDataBufferLen, - uint8_t* responseApduDataBuffer, - uint8_t* responseApduDataBufferLen) { + const ISO7816_Command_APDU* commandAPDU, + ISO7816_Response_APDU* responseAPDU) { //example 1: sends a command with no body, receives a response with no body - //sends APDU 0x01:0x02:0x00:0x00 + //sends APDU 0x01:0x01:0x00:0x00 //receives SW1=0x90, SW2=0x00 - if(commandAPDU->CLA == 0x01 && commandAPDU->INS == 0x01) { - responseAPDU->SW1 = 0x90; - responseAPDU->SW2 = 0x00; - } - //example 2: sends a command with no body, receives a response with a body with two bytes - //sends APDU 0x01:0x02:0x00:0x00 - //receives 'bc' (0x62, 0x63) SW1=0x80, SW2=0x10 - else if(commandAPDU->CLA == 0x01 && commandAPDU->INS == 0x02) { - responseApduDataBuffer[0] = 0x62; - responseApduDataBuffer[1] = 0x63; - *responseApduDataBufferLen = 2; - - responseAPDU->SW1 = 0x90; - responseAPDU->SW2 = 0x00; - } - //example 3: ends a command with a body with two bytes, receives a response with a body with two bytes - //sends APDU 0x01:0x03:0x00:0x00:0x02:CA:FE - //receives (0xCA, 0xFE) SW1=0x90, SW2=0x02 - else if( - commandAPDU->CLA == 0x01 && commandAPDU->INS == 0x03 && commandApduDataBufferLen == 2 && - commandAPDU->Lc == 2) { - //echo command body to response body - responseApduDataBuffer[0] = commandApduDataBuffer[0]; - responseApduDataBuffer[1] = commandApduDataBuffer[1]; - - *responseApduDataBufferLen = 2; - - responseAPDU->SW1 = 0x90; - responseAPDU->SW2 = 0x00; + if(commandAPDU->CLA == 0x01) { + switch(commandAPDU->INS) { + case 0x01: + handle_instruction_01(responseAPDU); + break; + case 0x02: + handle_instruction_02( + commandAPDU->P1, commandAPDU->P2, commandAPDU->Lc, commandAPDU->Le, responseAPDU); + break; + case 0x03: + handle_instruction_03(commandAPDU->P1, commandAPDU->P2, commandAPDU->Lc, responseAPDU); + break; + case 0x04: + handle_instruction_04( + commandAPDU->P1, + commandAPDU->P2, + commandAPDU->Lc, + commandAPDU->Le, + commandAPDU->Data, + responseAPDU); + break; + default: + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_INSTRUCTION_NOT_SUPPORTED); + } } else { - responseAPDU->SW1 = 0x6A; - responseAPDU->SW2 = 0x00; + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_CLASS_NOT_SUPPORTED); } } diff --git a/applications/debug/ccid_test/client/ccid_client.py b/applications/debug/ccid_test/client/ccid_client.py new file mode 100644 index 000000000..5f43deb87 --- /dev/null +++ b/applications/debug/ccid_test/client/ccid_client.py @@ -0,0 +1,116 @@ +# pylint: disable=missing-module-docstring, too-many-arguments, consider-using-f-string, missing-function-docstring +from smartcard.System import readers + + +def test_apdu(connection, test_name, apdu, expected_sw1, expected_sw2, expected_data): + print("Running test: [%s]" % test_name) + data, sw1, sw2 = connection.transmit(apdu) + + failed = [] + + if sw1 != expected_sw1: + failed.append("SW1: Expected %x, actual %x" % (expected_sw1, sw1)) + + if sw2 != expected_sw2: + failed.append("SW2: Expected %x, actual %x" % (expected_sw2, sw2)) + + if len(data) != len(expected_data): + failed.append( + "Data: Sizes differ: Expected %x, actual %x" + % (len(expected_data), len(data)) + ) + print(data) + elif len(data) > 0: + data_matches = True + for i, _ in enumerate(data): + if data[i] != expected_data[i]: + data_matches = False + + if not data_matches: + failed.append("Data: Expected %s, actual %s" % (expected_data, data)) + + if len(failed) > 0: + print("Test failed: ") + for failure in failed: + print("- %s" % failure) + else: + print("Test passed!") + + +def main(): + r = readers() + print("Found following smartcard readers: ") + + for i, sc in enumerate(r): + print("[%d] %s" % (i, sc)) + + print("Select the smartcard reader you want to run tests against:") + + reader_index = int(input()) + + if reader_index < len(r): + connection = r[reader_index].createConnection() + + connection.connect() + + test_apdu( + connection, + "INS 0x01: No Lc, no Data, No Le. Expect no data in return", + [0x01, 0x01, 0x00, 0x00], + 0x90, + 0x00, + [], + ) + + test_apdu( + connection, + "INS 0x02: No Lc, no Data, Le=2. Expect 2 byte data in return", + [0x01, 0x02, 0x00, 0x00, 0x02], + 0x90, + 0x00, + [0x62, 0x63], + ) + + test_apdu( + connection, + "INS 0x03: Lc=2, data=[0xCA, 0xFE], No Le. Expect no data in return", + [0x01, 0x03, 0x00, 0x00, 0x02, 0xCA, 0xFE], + 0x90, + 0x00, + [], + ) + + test_apdu( + connection, + "INS 0x04: Lc=2, data=[0xCA, 0xFE], Le=2. Expect 1 byte data in return", + [0x01, 0x04, 0x00, 0x00, 0x02, 0xCA, 0xFE, 0x02], + 0x90, + 0x00, + [0xCA, 0xFE], + ) + + small_apdu = list(range(0, 0x0F)) + + test_apdu( + connection, + "INS 0x04: Lc=0x0F, data=small_apdu, Le=0x0F. Expect 14 bytes data in return", + [0x01, 0x04, 0x00, 0x00, 0x0F] + small_apdu + [0x0F], + 0x90, + 0x00, + small_apdu, + ) + + max_apdu = list(range(0, 0x30)) + + test_apdu( + connection, + "INS 0x04: Lc=0x30, data=max_apdu, Le=0x30. Expect 0x30 bytes data in return", + [0x01, 0x04, 0x00, 0x00, 0x30] + max_apdu + [0x30], + 0x90, + 0x00, + max_apdu, + ) + + +if __name__ == "__main__": + main() diff --git a/applications/debug/ccid_test/client/requirements.txt b/applications/debug/ccid_test/client/requirements.txt new file mode 100644 index 000000000..fe0a8e539 --- /dev/null +++ b/applications/debug/ccid_test/client/requirements.txt @@ -0,0 +1,2 @@ +pyscard +# or sudo apt install python3-pyscard \ No newline at end of file diff --git a/applications/debug/ccid_test/iso7816_atr.h b/applications/debug/ccid_test/iso7816_atr.h index 050457f8c..215ec60ee 100644 --- a/applications/debug/ccid_test/iso7816_atr.h +++ b/applications/debug/ccid_test/iso7816_atr.h @@ -1,9 +1,6 @@ -#ifndef _ISO7816_ATR_H_ -#define _ISO7816_ATR_H_ +#pragma once typedef struct { uint8_t TS; uint8_t T0; } Iso7816Atr; - -#endif //_ISO7816_ATR_H_ diff --git a/applications/debug/ccid_test/iso7816_callbacks.c b/applications/debug/ccid_test/iso7816_callbacks.c index 1a66fa775..6c1bb106a 100644 --- a/applications/debug/ccid_test/iso7816_callbacks.c +++ b/applications/debug/ccid_test/iso7816_callbacks.c @@ -1,17 +1,21 @@ // transforms low level calls such as XFRCallback or ICC Power on to a structured one // an application can register these calls and listen for the callbacks defined in Iso7816Callbacks +#include +#include +#include +#include + #include "iso7816_t0_apdu.h" #include "iso7816_atr.h" #include "iso7816_callbacks.h" -#include -#include -#include - -#define ISO7816_RESPONSE_BUFFER_SIZE 255 +#include "iso7816_response.h" static Iso7816Callbacks* callbacks = NULL; +static uint8_t commandApduBuffer[sizeof(ISO7816_Command_APDU) + CCID_SHORT_APDU_SIZE]; +static uint8_t responseApduBuffer[sizeof(ISO7816_Response_APDU) + CCID_SHORT_APDU_SIZE]; + void iso7816_set_callbacks(Iso7816Callbacks* cb) { callbacks = cb; } @@ -36,41 +40,26 @@ void iso7816_xfr_datablock_callback( uint32_t pcToReaderDataBlockLen, uint8_t* readerToPcDataBlock, uint32_t* readerToPcDataBlockLen) { - struct ISO7816_Response_APDU responseAPDU; - uint8_t responseApduDataBuffer[ISO7816_RESPONSE_BUFFER_SIZE]; - uint8_t responseApduDataBufferLen = 0; + ISO7816_Response_APDU* responseAPDU = (ISO7816_Response_APDU*)&responseApduBuffer; if(callbacks != NULL) { - struct ISO7816_Command_APDU commandAPDU; + ISO7816_Command_APDU* commandAPDU = (ISO7816_Command_APDU*)&commandApduBuffer; - const uint8_t* commandApduDataBuffer = NULL; - uint8_t commandApduDataBufferLen = 0; + uint8_t result = + iso7816_read_command_apdu(commandAPDU, pcToReaderDataBlock, pcToReaderDataBlockLen); - iso7816_read_command_apdu(&commandAPDU, pcToReaderDataBlock, pcToReaderDataBlockLen); + if(result == ISO7816_READ_COMMAND_APDU_OK) { + callbacks->iso7816_process_command(commandAPDU, responseAPDU); - if(commandAPDU.Lc > 0) { - commandApduDataBufferLen = commandAPDU.Lc; - commandApduDataBuffer = &pcToReaderDataBlock[5]; + furi_assert(responseAPDU->DataLen < CCID_SHORT_APDU_SIZE); + } else if(result == ISO7816_READ_COMMAND_APDU_ERROR_WRONG_LE) { + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_WRONG_LE); + } else if(result == ISO7816_READ_COMMAND_APDU_ERROR_WRONG_LENGTH) { + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_WRONG_LENGTH); } - - callbacks->iso7816_process_command( - &commandAPDU, - &responseAPDU, - commandApduDataBuffer, - commandApduDataBufferLen, - responseApduDataBuffer, - &responseApduDataBufferLen); - } else { - //class not supported - responseAPDU.SW1 = 0x6E; - responseAPDU.SW2 = 0x00; + iso7816_set_response(responseAPDU, ISO7816_RESPONSE_INTERNAL_EXCEPTION); } - iso7816_write_response_apdu( - &responseAPDU, - readerToPcDataBlock, - readerToPcDataBlockLen, - responseApduDataBuffer, - responseApduDataBufferLen); + iso7816_write_response_apdu(responseAPDU, readerToPcDataBlock, readerToPcDataBlockLen); } diff --git a/applications/debug/ccid_test/iso7816_callbacks.h b/applications/debug/ccid_test/iso7816_callbacks.h index 3d337d23a..7288b021a 100644 --- a/applications/debug/ccid_test/iso7816_callbacks.h +++ b/applications/debug/ccid_test/iso7816_callbacks.h @@ -1,5 +1,4 @@ -#ifndef _ISO7816_CALLBACKS_H_ -#define _ISO7816_CALLBACKS_H_ +#pragma once #include #include "iso7816_atr.h" @@ -8,12 +7,8 @@ typedef struct { void (*iso7816_answer_to_reset)(Iso7816Atr* atr); void (*iso7816_process_command)( - const struct ISO7816_Command_APDU* command, - struct ISO7816_Response_APDU* response, - const uint8_t* commandApduDataBuffer, - uint8_t commandApduDataBufferLen, - uint8_t* responseApduDataBuffer, - uint8_t* responseApduDataBufferLen); + const ISO7816_Command_APDU* command, + ISO7816_Response_APDU* response); } Iso7816Callbacks; void iso7816_set_callbacks(Iso7816Callbacks* cb); @@ -23,6 +18,4 @@ void iso7816_xfr_datablock_callback( const uint8_t* dataBlock, uint32_t dataBlockLen, uint8_t* responseDataBlock, - uint32_t* responseDataBlockLen); - -#endif //_ISO7816_CALLBACKS_H_ \ No newline at end of file + uint32_t* responseDataBlockLen); \ No newline at end of file diff --git a/applications/debug/ccid_test/iso7816_response.c b/applications/debug/ccid_test/iso7816_response.c new file mode 100644 index 000000000..3a65486b6 --- /dev/null +++ b/applications/debug/ccid_test/iso7816_response.c @@ -0,0 +1,8 @@ +#include +#include "iso7816_t0_apdu.h" +#include "iso7816_response.h" + +void iso7816_set_response(ISO7816_Response_APDU* responseAPDU, uint16_t responseCode) { + responseAPDU->SW1 = (responseCode >> (8 * 1)) & 0xff; + responseAPDU->SW2 = (responseCode >> (8 * 0)) & 0xff; +} \ No newline at end of file diff --git a/applications/debug/ccid_test/iso7816_response.h b/applications/debug/ccid_test/iso7816_response.h new file mode 100644 index 000000000..7c2e74257 --- /dev/null +++ b/applications/debug/ccid_test/iso7816_response.h @@ -0,0 +1,12 @@ +#pragma once + +#define ISO7816_RESPONSE_OK 0x9000 + +#define ISO7816_RESPONSE_WRONG_LENGTH 0x6700 +#define ISO7816_RESPONSE_WRONG_PARAMETERS_P1_P2 0x6A00 +#define ISO7816_RESPONSE_WRONG_LE 0x6C00 +#define ISO7816_RESPONSE_INSTRUCTION_NOT_SUPPORTED 0x6D00 +#define ISO7816_RESPONSE_CLASS_NOT_SUPPORTED 0x6E00 +#define ISO7816_RESPONSE_INTERNAL_EXCEPTION 0x6F00 + +void iso7816_set_response(ISO7816_Response_APDU* responseAPDU, uint16_t responseCode); \ No newline at end of file diff --git a/applications/debug/ccid_test/iso7816_t0_apdu.c b/applications/debug/ccid_test/iso7816_t0_apdu.c index 5fb695af1..916983229 100644 --- a/applications/debug/ccid_test/iso7816_t0_apdu.c +++ b/applications/debug/ccid_test/iso7816_t0_apdu.c @@ -2,37 +2,73 @@ #include #include #include +#include #include "iso7816_t0_apdu.h" //reads dataBuffer with dataLen size, translate it into a ISO7816_Command_APDU type //extra data will be pointed to commandDataBuffer -void iso7816_read_command_apdu( - struct ISO7816_Command_APDU* command, +uint8_t iso7816_read_command_apdu( + ISO7816_Command_APDU* command, const uint8_t* dataBuffer, uint32_t dataLen) { - UNUSED(dataLen); - command->CLA = dataBuffer[0]; command->INS = dataBuffer[1]; command->P1 = dataBuffer[2]; command->P2 = dataBuffer[3]; - command->Lc = dataBuffer[4]; + + if(dataLen == 4) { + command->Lc = 0; + command->Le = 0; + command->LePresent = false; + + return ISO7816_READ_COMMAND_APDU_OK; + } else if(dataLen == 5) { + //short le + + command->Lc = 0; + command->Le = dataBuffer[4]; + command->LePresent = true; + + return ISO7816_READ_COMMAND_APDU_OK; + } else if(dataLen > 5 && dataBuffer[4] != 0x00) { + //short lc + + command->Lc = dataBuffer[4]; + if(command->Lc > 0 && command->Lc < CCID_SHORT_APDU_SIZE) { //-V560 + memcpy(command->Data, &dataBuffer[5], command->Lc); + + //does it have a short le too? + if(dataLen == (uint32_t)(command->Lc + 5)) { + command->Le = 0; + command->LePresent = false; + return ISO7816_READ_COMMAND_APDU_OK; + } else if(dataLen == (uint32_t)(command->Lc + 6)) { + command->Le = dataBuffer[dataLen - 1]; + command->LePresent = true; + + return ISO7816_READ_COMMAND_APDU_OK; + } else { + return ISO7816_READ_COMMAND_APDU_ERROR_WRONG_LENGTH; + } + } else { + return ISO7816_READ_COMMAND_APDU_ERROR_WRONG_LENGTH; + } + } else { + return ISO7816_READ_COMMAND_APDU_ERROR_WRONG_LENGTH; + } } -//data buffer countains the whole APU response (response + trailer (SW1+SW2)) +//data buffer contains the whole APU response (response + trailer (SW1+SW2)) void iso7816_write_response_apdu( - const struct ISO7816_Response_APDU* response, + const ISO7816_Response_APDU* response, uint8_t* readerToPcDataBlock, - uint32_t* readerToPcDataBlockLen, - uint8_t* responseDataBuffer, - uint32_t responseDataLen) { + uint32_t* readerToPcDataBlockLen) { uint32_t responseDataBufferIndex = 0; //response body - if(responseDataLen > 0) { - while(responseDataBufferIndex < responseDataLen) { - readerToPcDataBlock[responseDataBufferIndex] = - responseDataBuffer[responseDataBufferIndex]; + if(response->DataLen > 0) { + while(responseDataBufferIndex < response->DataLen) { + readerToPcDataBlock[responseDataBufferIndex] = response->Data[responseDataBufferIndex]; responseDataBufferIndex++; } } diff --git a/applications/debug/ccid_test/iso7816_t0_apdu.h b/applications/debug/ccid_test/iso7816_t0_apdu.h index 48a189440..3b3450909 100644 --- a/applications/debug/ccid_test/iso7816_t0_apdu.h +++ b/applications/debug/ccid_test/iso7816_t0_apdu.h @@ -1,11 +1,14 @@ -#ifndef _ISO7816_T0_APDU_H_ -#define _ISO7816_T0_APDU_H_ +#pragma once #include #include "iso7816_atr.h" #include "core/common_defines.h" -struct ISO7816_Command_APDU { +#define ISO7816_READ_COMMAND_APDU_OK 0 +#define ISO7816_READ_COMMAND_APDU_ERROR_WRONG_LE 1 +#define ISO7816_READ_COMMAND_APDU_ERROR_WRONG_LENGTH 2 + +typedef struct { //header uint8_t CLA; uint8_t INS; @@ -13,24 +16,27 @@ struct ISO7816_Command_APDU { uint8_t P2; //body - uint8_t Lc; - uint8_t Le; -} FURI_PACKED; + uint16_t Lc; //data length + uint16_t Le; //maximum response data length expected by client -struct ISO7816_Response_APDU { + //Le can have value of 0x00, which actually meand 0x100 = 256 + bool LePresent; + uint8_t Data[0]; +} FURI_PACKED ISO7816_Command_APDU; + +typedef struct { uint8_t SW1; uint8_t SW2; -} FURI_PACKED; + uint16_t DataLen; + uint8_t Data[0]; +} FURI_PACKED ISO7816_Response_APDU; void iso7816_answer_to_reset(Iso7816Atr* atr); -void iso7816_read_command_apdu( - struct ISO7816_Command_APDU* command, - const uint8_t* dataBuffer, - uint32_t dataLen); +uint8_t iso7816_read_command_apdu( + ISO7816_Command_APDU* command, + const uint8_t* pcToReaderDataBlock, + uint32_t pcToReaderDataBlockLen); void iso7816_write_response_apdu( - const struct ISO7816_Response_APDU* response, + const ISO7816_Response_APDU* response, uint8_t* readerToPcDataBlock, - uint32_t* readerToPcDataBlockLen, - uint8_t* responseDataBuffer, - uint32_t responseDataLen); -#endif //_ISO7816_T0_APDU_H_ + uint32_t* readerToPcDataBlockLen); diff --git a/targets/f7/furi_hal/furi_hal_usb_ccid.c b/targets/f7/furi_hal/furi_hal_usb_ccid.c index 0fe5fe5d9..6a6527c3b 100644 --- a/targets/f7/furi_hal/furi_hal_usb_ccid.c +++ b/targets/f7/furi_hal/furi_hal_usb_ccid.c @@ -19,7 +19,8 @@ static const uint8_t USB_DEVICE_NO_PROTOCOL = 0x0; #define CCID_TOTAL_SLOTS 1 #define CCID_SLOT_INDEX 0 -#define CCID_DATABLOCK_SIZE 256 +#define CCID_DATABLOCK_SIZE \ + (4 + 1 + CCID_SHORT_APDU_SIZE + 1) //APDU Header + Lc + Short APDU size + Le #define ENDPOINT_DIR_IN 0x80 #define ENDPOINT_DIR_OUT 0x00 diff --git a/targets/furi_hal_include/furi_hal_usb_ccid.h b/targets/furi_hal_include/furi_hal_usb_ccid.h index cbd0bf092..223f16231 100644 --- a/targets/furi_hal_include/furi_hal_usb_ccid.h +++ b/targets/furi_hal_include/furi_hal_usb_ccid.h @@ -5,6 +5,8 @@ #include "hid_usage_consumer.h" #include "hid_usage_led.h" +#define CCID_SHORT_APDU_SIZE (0xFF) + #ifdef __cplusplus extern "C" { #endif From a0eab5a371da1c4a927f03ff0375296b4d322178 Mon Sep 17 00:00:00 2001 From: WillyJL <49810075+Willy-JL@users.noreply.github.com> Date: Sat, 6 Jul 2024 14:18:48 +0200 Subject: [PATCH 4/5] NFC: Cache plugin name not full path, saves some RAM (#3737) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * NFC: Cache plugin name not full path, saves some RAM * Remove file_path FuriString from context Co-authored-by: あく Co-authored-by: gornekich --- .../main/nfc/helpers/nfc_supported_cards.c | 42 +++++++++++-------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/applications/main/nfc/helpers/nfc_supported_cards.c b/applications/main/nfc/helpers/nfc_supported_cards.c index 1e0e7ba6b..dc23dd510 100644 --- a/applications/main/nfc/helpers/nfc_supported_cards.c +++ b/applications/main/nfc/helpers/nfc_supported_cards.c @@ -24,7 +24,7 @@ typedef enum { } NfcSupportedCardsPluginFeature; typedef struct { - FuriString* path; + FuriString* name; NfcProtocol protocol; NfcSupportedCardsPluginFeature feature; } NfcSupportedCardsPluginCache; @@ -41,7 +41,6 @@ typedef enum { typedef struct { Storage* storage; File* directory; - FuriString* file_path; char file_name[256]; FlipperApplication* app; } NfcSupportedCardsLoadContext; @@ -73,7 +72,7 @@ void nfc_supported_cards_free(NfcSupportedCards* instance) { !NfcSupportedCardsPluginCache_end_p(iter); NfcSupportedCardsPluginCache_next(iter)) { NfcSupportedCardsPluginCache* plugin_cache = NfcSupportedCardsPluginCache_ref(iter); - furi_string_free(plugin_cache->path); + furi_string_free(plugin_cache->name); } NfcSupportedCardsPluginCache_clear(instance->plugins_cache_arr); @@ -86,7 +85,6 @@ static NfcSupportedCardsLoadContext* nfc_supported_cards_load_context_alloc(void instance->storage = furi_record_open(RECORD_STORAGE); instance->directory = storage_file_alloc(instance->storage); - instance->file_path = furi_string_alloc(); if(!storage_dir_open(instance->directory, NFC_SUPPORTED_CARDS_PLUGINS_PATH)) { FURI_LOG_D(TAG, "Failed to open directory: %s", NFC_SUPPORTED_CARDS_PLUGINS_PATH); @@ -100,8 +98,6 @@ static void nfc_supported_cards_load_context_free(NfcSupportedCardsLoadContext* flipper_application_free(instance->app); } - furi_string_free(instance->file_path); - storage_dir_close(instance->directory); storage_file_free(instance->directory); @@ -111,16 +107,19 @@ static void nfc_supported_cards_load_context_free(NfcSupportedCardsLoadContext* static const NfcSupportedCardsPlugin* nfc_supported_cards_get_plugin( NfcSupportedCardsLoadContext* instance, - const FuriString* path, + const char* name, const ElfApiInterface* api_interface) { furi_assert(instance); - furi_assert(path); + furi_assert(name); const NfcSupportedCardsPlugin* plugin = NULL; + FuriString* plugin_path = furi_string_alloc_printf( + "%s/%s%s", NFC_SUPPORTED_CARDS_PLUGINS_PATH, name, NFC_SUPPORTED_CARDS_PLUGIN_SUFFIX); do { if(instance->app) flipper_application_free(instance->app); instance->app = flipper_application_alloc(instance->storage, api_interface); - if(flipper_application_preload(instance->app, furi_string_get_cstr(path)) != + + if(flipper_application_preload(instance->app, furi_string_get_cstr(plugin_path)) != FlipperApplicationPreloadStatusSuccess) break; if(!flipper_application_is_plugin(instance->app)) break; @@ -136,6 +135,7 @@ static const NfcSupportedCardsPlugin* nfc_supported_cards_get_plugin( plugin = descriptor->entry_point; } while(false); + furi_string_free(plugin_path); return plugin; } @@ -151,13 +151,21 @@ static const NfcSupportedCardsPlugin* nfc_supported_cards_get_next_plugin( instance->directory, NULL, instance->file_name, sizeof(instance->file_name))) break; - furi_string_set(instance->file_path, instance->file_name); - if(!furi_string_end_with_str(instance->file_path, NFC_SUPPORTED_CARDS_PLUGIN_SUFFIX)) - continue; + size_t suffix_len = strlen(NFC_SUPPORTED_CARDS_PLUGIN_SUFFIX); + size_t file_name_len = strlen(instance->file_name); + if(file_name_len <= suffix_len) break; - path_concat(NFC_SUPPORTED_CARDS_PLUGINS_PATH, instance->file_name, instance->file_path); + size_t suffix_start_pos = file_name_len - suffix_len; + if(memcmp( + &instance->file_name[suffix_start_pos], + NFC_SUPPORTED_CARDS_PLUGIN_SUFFIX, + suffix_len) != 0) + break; - plugin = nfc_supported_cards_get_plugin(instance, instance->file_path, api_interface); + // Trim suffix from file_name to save memory. The suffix will be concatenated on plugin load. + instance->file_name[suffix_start_pos] = '\0'; + + plugin = nfc_supported_cards_get_plugin(instance, instance->file_name, api_interface); } while(plugin == NULL); //-V654 return plugin; @@ -181,7 +189,7 @@ void nfc_supported_cards_load_cache(NfcSupportedCards* instance) { if(plugin == NULL) break; //-V547 NfcSupportedCardsPluginCache plugin_cache = {}; //-V779 - plugin_cache.path = furi_string_alloc_set(instance->load_context->file_path); + plugin_cache.name = furi_string_alloc_set(instance->load_context->file_name); plugin_cache.protocol = plugin->protocol; if(plugin->verify) { plugin_cache.feature |= NfcSupportedCardsPluginFeatureHasVerify; @@ -233,7 +241,7 @@ bool nfc_supported_cards_read(NfcSupportedCards* instance, NfcDevice* device, Nf const ElfApiInterface* api_interface = composite_api_resolver_get(instance->api_resolver); const NfcSupportedCardsPlugin* plugin = nfc_supported_cards_get_plugin( - instance->load_context, plugin_cache->path, api_interface); + instance->load_context, furi_string_get_cstr(plugin_cache->name), api_interface); if(plugin == NULL) continue; if(plugin->verify) { @@ -281,7 +289,7 @@ bool nfc_supported_cards_parse( const ElfApiInterface* api_interface = composite_api_resolver_get(instance->api_resolver); const NfcSupportedCardsPlugin* plugin = nfc_supported_cards_get_plugin( - instance->load_context, plugin_cache->path, api_interface); + instance->load_context, furi_string_get_cstr(plugin_cache->name), api_interface); if(plugin == NULL) continue; if(plugin->parse) { From d39c3b377652bfcec954303b8caf058716d4ed46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E3=81=82=E3=81=8F?= Date: Sat, 6 Jul 2024 15:05:29 +0100 Subject: [PATCH 5/5] Fix PVS Warnings (#3760) --- applications/main/nfc/helpers/nfc_supported_cards.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/applications/main/nfc/helpers/nfc_supported_cards.c b/applications/main/nfc/helpers/nfc_supported_cards.c index dc23dd510..d2950b584 100644 --- a/applications/main/nfc/helpers/nfc_supported_cards.c +++ b/applications/main/nfc/helpers/nfc_supported_cards.c @@ -151,15 +151,15 @@ static const NfcSupportedCardsPlugin* nfc_supported_cards_get_next_plugin( instance->directory, NULL, instance->file_name, sizeof(instance->file_name))) break; - size_t suffix_len = strlen(NFC_SUPPORTED_CARDS_PLUGIN_SUFFIX); - size_t file_name_len = strlen(instance->file_name); + const size_t suffix_len = strlen(NFC_SUPPORTED_CARDS_PLUGIN_SUFFIX); + const size_t file_name_len = strlen(instance->file_name); if(file_name_len <= suffix_len) break; size_t suffix_start_pos = file_name_len - suffix_len; if(memcmp( &instance->file_name[suffix_start_pos], NFC_SUPPORTED_CARDS_PLUGIN_SUFFIX, - suffix_len) != 0) + suffix_len) != 0) //-V1051 break; // Trim suffix from file_name to save memory. The suffix will be concatenated on plugin load.