FIDO supports BLE communication.#244
Merged
lihuanhuan merged 3 commits intoOneKeyHQ:mainfrom Feb 10, 2025
lihuanhuan:pro_main
Merged
FIDO supports BLE communication.#244lihuanhuan merged 3 commits intoOneKeyHQ:mainfrom lihuanhuan:pro_main
lihuanhuan merged 3 commits intoOneKeyHQ:mainfrom
lihuanhuan:pro_main
Conversation
WalkthroughThe pull request adds new source files and updates build scripts to include additional HAL and interrupt handlers. It introduces SRAM3 memory regions in linker scripts for DMA use. Several modules update SPI communication, including redefining interface constants and timeouts, adding DMA buffers, and revising callback and polling logic. Some obsolete interrupt handlers are removed and replaced with updated implementations. Changes also adjust UART DMA handling and update webauthn command handling to support both HID and SPI interfaces. Additionally, SPI configuration in utility code is revised. Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (3)
core/embed/trezorhal/stm32_it_handler.c (1)
12-13: Comment needs more detail."fp sensor irq" is too vague.
- // fp sensor irq + // Handle fingerprint sensor interrupt on GPIO_PIN_15core/embed/trezorhal/spi_legacy.h (1)
13-17: Inconsistent macro naming.SET_COMBUS vs ST_BLE_STATUS
-#define SET_COMBUS_HIGH() HAL_GPIO_WritePin(GPIOK, GPIO_PIN_6, GPIO_PIN_SET) -#define SET_COMBUS_LOW() HAL_GPIO_WritePin(GPIOK, GPIO_PIN_6, GPIO_PIN_RESET) +#define ST_BLE_COMBUS_HIGH() HAL_GPIO_WritePin(GPIOK, GPIO_PIN_6, GPIO_PIN_SET) +#define ST_BLE_COMBUS_LOW() HAL_GPIO_WritePin(GPIOK, GPIO_PIN_6, GPIO_PIN_RESET)core/embed/bootloader/memory.ld (1)
9-10: Fix comment style.Use single-line comments to match file style.
- /* SRAM3 is used for DMA */ + // SRAM3 is used for DMA
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/mocks/generated/trezorio/__init__.pyiis excluded by!**/generated/**
📒 Files selected for processing (18)
core/SConscript.bootloader(2 hunks)core/SConscript.firmware(1 hunks)core/embed/bootloader/memory.ld(2 hunks)core/embed/extmod/modtrezorio/modtrezorio-poll.h(4 hunks)core/embed/extmod/modtrezorio/modtrezorio-spi.h(1 hunks)core/embed/extmod/modtrezorio/modtrezorio.c(2 hunks)core/embed/firmware/memory_H.ld(2 hunks)core/embed/fp_sensor_wrapper/fpsensor_platform.c(0 hunks)core/embed/trezorhal/gt911.c(0 hunks)core/embed/trezorhal/spi_legacy.c(11 hunks)core/embed/trezorhal/spi_legacy.h(2 hunks)core/embed/trezorhal/stm32_it_handler.c(2 hunks)core/embed/trezorhal/usart.c(7 hunks)core/src/apps/webauthn/__init__.py(1 hunks)core/src/apps/webauthn/credential.py(1 hunks)core/src/apps/webauthn/fido2.py(25 hunks)core/src/trezor/utils.py(2 hunks)core/src/trezor/wire/__init__.py(2 hunks)
💤 Files with no reviewable changes (2)
- core/embed/trezorhal/gt911.c
- core/embed/fp_sensor_wrapper/fpsensor_platform.c
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Style check
- GitHub Check: Defs check
- GitHub Check: Gen check
🔇 Additional comments (12)
core/embed/firmware/memory_H.ld (2)
12-13: LGTM! Well-documented DMA memory region.
85-88: LGTM! Properly aligned section definition.core/embed/extmod/modtrezorio/modtrezorio.c (2)
83-84: LGTM! Clear interface separation.
138-139: LGTM! Consistent interface mapping.core/embed/extmod/modtrezorio/modtrezorio-poll.h (3)
39-40: LGTM! Clear interface numbering.
206-206: LGTM! Unified write handling for both interfaces.
184-184: Verify if 1024 bytes buffer is sufficient for FIDO/BLE operations.✅ Verification successful
🌐 Web query:
What is the maximum packet size for FIDO2 over BLE?💡 Result:
The maximum packet size for **FIDO2 over BLE** depends on protocol configurations and Bluetooth versions: ### Key Specifications 1. **Maximum ATT MTU** - **512 bytes** is the theoretical upper limit for ATT (Attribute Protocol) MTU defined in Bluetooth specifications[3][5][8]. - Android supports up to **517 bytes** for ATT MTU[4], but FIDO2 implementations cap this at **512 bytes**[3][8]. 2. **Data Length Extension (BLE 4.2+)** - Increases payload capacity per packet from 27 bytes (pre-4.2) to **251 bytes** for data channel PDUs[4][6]. - Combined with larger MTUs, this reduces overhead and improves throughput[4][6]. 3. **FIDO-Specific Fragmentation** - FIDO2/BLE splits large messages into fragments due to inherent BLE limitations[8][10]. - Example: A 400-byte payload is transmitted in one frame if `maxLen` (negotiated MTU) is 512 bytes[8]. ### Practical Limits | Layer/Feature | Maximum Payload | Notes | |---------------|-----------------|-------| | **ATT MTU** | 512 bytes | Enforced by FIDO specs[3][5][8] | | **Data Channel PDU (with DLE)** | 251 bytes | Requires Bluetooth 4.2+[4][6] | | **Application Data per Packet** | ~244–507 bytes | After subtracting headers (L2CAP/ATT)[4][6] | ### Implementation Notes - **MTU Negotiation**: FIDO2 clients should request the maximum supported MTU (up to 512 bytes)[3][5]. - **Fragmentation**: Packets exceeding `maxLen` are split into initialization/continuation fragments[8][10]. - **Throughput**: Using larger MTUs and DLE improves speed and reduces power consumption[4]. For example: - A 400-byte payload with a negotiated MTU of 512 is sent in one fragment[8]. - Smaller MTUs (e.g., default 23 bytes) require splitting data into multiple packets[6]. In summary, while BLE’s physical layer limits per-packet payloads to ~251 bytes with DLE, FIDO2’s use of large MTUs allows application-layer payloads up to **512 bytes**[3][5][8]. Citations: - 1: https://en.wikipedia.org/wiki/Bluetooth_Low_Energy - 2: https://developerhelp.microchip.com/xwiki/bin/view/applications/ble/introduction/bluetooth-architecture/bluetooth-controller-layer/bluetooth-link-layer/Packet-Types/ - 3: https://fidoalliance.org/specs/fido-u2f-bt-protocol-id-20150514.pdf - 4: https://punchthrough.com/ble-throughput-part-4/ - 5: https://fidoalliance.org/specs/u2f-specs-master/fido-u2f-bt-protocol.html - 6: https://community.renesas.com/wireles-connectivity/f/bluetooth-low-energy/25075/maximum-payload-for-gatt-services - 7: https://duo.com/blog/webauthn-passwordless-fido2-explained-componens-passwordless-architecture - 8: https://fidoalliance.org/specs/u2f-specs-1.0-bt-nfc-id-amendment/fido-u2f-bt-protocol.html - 9: https://e2e.ti.com/support/wireless-connectivity/bluetooth-group/bluetooth/f/bluetooth-forum/108807/max-packet-size-allowed-for-ble-communication - 10: https://fidoalliance.org/specs/fido-u2f-v1.2-ps-20170411/fido-u2f-bt-protocol-v1.2-ps-20170411.html
Buffer Check: Sufficient
- FIDO2 over BLE uses a maximum packet size of 512 bytes.
- The 1024-byte buffer can hold two full packets.
core/SConscript.bootloader (1)
90-90: LGTM!The additions of HAL and interrupt handler files are properly conditioned and support BLE communication.
Also applies to: 227-227
core/src/trezor/utils.py (1)
125-125: LGTM!The SPI interface handling is properly standardized using the io module.
Also applies to: 223-223
core/src/trezor/wire/__init__.py (1)
380-380: LGTM!The SPI interface check is properly updated to use io.SPI_FACE.
core/SConscript.firmware (1)
412-412: LGTM!The addition of the EXTI HAL driver is appropriate for BLE support.
core/src/apps/webauthn/__init__.py (1)
1-10: LGTM! Clean and focused changes.The modifications effectively add SPI interface support while maintaining code clarity and organization.
guowei0105
approved these changes
Feb 10, 2025
somebodyLi
approved these changes
Feb 10, 2025
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
core/embed/trezorhal/usart.c (3)
91-91:⚠️ Potential issueMissing error checks after DMA initialization.
- HAL_DMA_Init(&hdma_tx); + if (HAL_DMA_Init(&hdma_tx) != HAL_OK) { + ensure(secfalse, "DMA TX init failed"); + } - HAL_DMA_Init(&hdma_rx); + if (HAL_DMA_Init(&hdma_rx) != HAL_OK) { + ensure(secfalse, "DMA RX init failed"); + }Also applies to: 107-107
144-158:⚠️ Potential issueData loss on timeout.
if (HAL_GetTick() - start > 500) { + HAL_UART_Abort(huart); + HAL_UART_Receive_DMA(huart, dma_uart_rev_buf, sizeof(dma_uart_rev_buf)); return; }
206-240:⚠️ Potential issueBuffer overflow risk in packet parsing.
while (len > 0) { + if (index >= len) { + return; + } if (buf[index] != 0xA5 || buf[index + 1] != 0x5A) {core/embed/trezorhal/spi_legacy.c (2)
241-253:⚠️ Potential issueMissing EXTI error check.
- HAL_EXTI_SetConfigLine(&hexti, &pExtiConfig); + if (HAL_EXTI_SetConfigLine(&hexti, &pExtiConfig) != HAL_OK) { + return -1; + }
424-444:⚠️ Potential issueMissing error handling for buffer overflow.
- fifo_write_no_overflow(&spi_fifo_in, dma_recv_buf, recv_len); + if (!fifo_write_no_overflow(&spi_fifo_in, dma_recv_buf, recv_len)) { + // Handle overflow error + HAL_SPI_Abort(&spi); + }
🧹 Nitpick comments (3)
core/embed/trezorhal/spi_legacy.h (1)
16-17: Fix inconsistent abbreviation in macro names.ST_BLE_STATUS_IO uses mixed abbreviation styles. Either use STATUS or ST consistently.
core/src/apps/webauthn/fido2.py (1)
2174-2181: Improve error message."Invalid interface type" is too vague.
core/embed/trezorhal/spi_legacy.c (1)
308-349: Define packet format constants.+#define FIDO_MAGIC "fid" +#define STANDARD_MAGIC '?' +#define FIDO_HEADER_SIZE 3 +#define FIDO_LENGTH_SIZE 2 - if (memcmp(header, "fid", 3) == 0) + if (memcmp(header, FIDO_MAGIC, FIDO_HEADER_SIZE) == 0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/mocks/generated/trezorio/__init__.pyiis excluded by!**/generated/**
📒 Files selected for processing (18)
core/SConscript.bootloader(2 hunks)core/SConscript.firmware(1 hunks)core/embed/bootloader/memory.ld(2 hunks)core/embed/extmod/modtrezorio/modtrezorio-poll.h(4 hunks)core/embed/extmod/modtrezorio/modtrezorio-spi.h(1 hunks)core/embed/extmod/modtrezorio/modtrezorio.c(2 hunks)core/embed/firmware/memory_H.ld(2 hunks)core/embed/fp_sensor_wrapper/fpsensor_platform.c(0 hunks)core/embed/trezorhal/gt911.c(0 hunks)core/embed/trezorhal/spi_legacy.c(11 hunks)core/embed/trezorhal/spi_legacy.h(2 hunks)core/embed/trezorhal/stm32_it_handler.c(2 hunks)core/embed/trezorhal/usart.c(7 hunks)core/src/apps/webauthn/__init__.py(1 hunks)core/src/apps/webauthn/credential.py(1 hunks)core/src/apps/webauthn/fido2.py(25 hunks)core/src/trezor/utils.py(2 hunks)core/src/trezor/wire/__init__.py(2 hunks)
💤 Files with no reviewable changes (2)
- core/embed/trezorhal/gt911.c
- core/embed/fp_sensor_wrapper/fpsensor_platform.c
🚧 Files skipped from review as they are similar to previous changes (11)
- core/embed/extmod/modtrezorio/modtrezorio-spi.h
- core/src/apps/webauthn/init.py
- core/src/trezor/wire/init.py
- core/src/apps/webauthn/credential.py
- core/embed/firmware/memory_H.ld
- core/SConscript.bootloader
- core/src/trezor/utils.py
- core/embed/bootloader/memory.ld
- core/embed/trezorhal/stm32_it_handler.c
- core/embed/extmod/modtrezorio/modtrezorio-poll.h
- core/SConscript.firmware
👮 Files not reviewed due to content moderation or server errors (1)
- core/embed/extmod/modtrezorio/modtrezorio.c
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit