USB data transmission timeout handling && fingerprint/camera error prompts #229
Conversation
WalkthroughThe pull request introduces modifications across multiple files in the core embedded system, focusing on improving error handling, status tracking, and operational checks for various hardware components like USB, camera, and fingerprint modules. The changes enhance robustness by adding more precise status checks, modifying function return types, and implementing more comprehensive error detection mechanisms. Changes
🪧 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: 6
🧹 Nitpick comments (3)
core/embed/trezorhal/camera.c (3)
182-184: Log I2C errors.Silent failure in I2C operations.
229-231: Log camera offline errors.Silent failure when camera is offline.
233-233: Simplify return statement.Unnecessary temporary variable.
- ret = (read == GC2145_ADDR) ? true : false; - - return ret; + return read == GC2145_ADDR;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/embed/extmod/modtrezorio/modtrezorio-poll.h(1 hunks)core/embed/fp_sensor_wrapper/fingerprint.c(3 hunks)core/embed/fp_sensor_wrapper/fingerprint.h(1 hunks)core/embed/fp_sensor_wrapper/fpsensor_common.c(1 hunks)core/embed/trezorhal/camera.c(4 hunks)core/embed/trezorhal/camera.h(2 hunks)core/embed/trezorhal/camera_qrcode.c(1 hunks)core/embed/trezorhal/device.c(7 hunks)core/embed/trezorhal/usb_webusb-defs.h(1 hunks)core/embed/trezorhal/usb_webusb-impl.h(2 hunks)core/src/trezor/wire/codec_v1.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
🔇 Additional comments (11)
core/embed/trezorhal/usb_webusb-defs.h (1)
65-65: LGTM!The return type change from
secbooltointenables better error handling.core/embed/extmod/modtrezorio/modtrezorio-poll.h (1)
251-262: LGTM!The changes properly handle USB write timeouts and error conditions.
core/src/trezor/wire/codec_v1.py (1)
106-108: LGTM!The added timeout error handling improves robustness.
core/embed/trezorhal/usb_webusb-impl.h (1)
293-294: LGTM!The EP buffer flush improves reliability of subsequent transfers.
core/embed/fp_sensor_wrapper/fingerprint.h (1)
15-15: LGTM!The new function declaration follows standard C conventions.
core/embed/trezorhal/camera.h (1)
5-5: LGTM! Function signatures improved for type safety.The changes from numeric types to bool improve clarity and type safety.
Also applies to: 34-35
core/embed/trezorhal/device.c (4)
471-471: Test duration reduced from 3 to 2 hours.
553-555: LGTM! Added camera status check.
639-641: LGTM! Added fingerprint module status check.
862-869: LGTM! Added component validation before burn-in test.core/embed/fp_sensor_wrapper/fpsensor_common.c (1)
31-40: LGTM! Improved error handling in HWID retrieval.The changes properly validate SPI transaction status before using the data.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
core/embed/trezorhal/device.c (1)
1009-1039:⚠️ Potential issueAdd timeout to NFC polling loop.
The while loop could run indefinitely if no card is present.
if (loop_counter % 2) { if (flashled_value) { ble_set_flashled(0); } display_printf("Poll card...\n"); nfc_pwr_ctl(true); HAL_TIM_Base_Stop(&TimHandle); + uint32_t start_time = HAL_GetTick(); while (1) { + if (HAL_GetTick() - start_time > 5000) { + display_printf("Card test timeout\n"); + break; + } if (nfc_poll_card()) {core/embed/trezorhal/usb_webusb-impl.h (1)
113-113:⚠️ Potential issueStatic timer causes race conditions.
Move timer to interface struct.
🧹 Nitpick comments (2)
core/embed/trezorhal/camera.c (1)
310-316: Use a defined constant for error code.Replace magic number -1 with a defined constant.
+#define CAMERA_ERROR_OFFLINE -1 + int camera_init(void) { i2c_handle_camera = &i2c_handles[i2c_find_channel_by_device(I2C_CAMERA)]; i2c_init_by_device(I2C_CAMERA); if (!camera_is_online()) { - return -1; + return CAMERA_ERROR_OFFLINE; }core/embed/trezorhal/device.c (1)
1105-1114: Define constants for timing values.Replace magic numbers with named constants.
+#define FLASHLED_ON_DURATION_MS 500 +#define FLASHLED_OFF_DURATION_MS 20000 + - if (flashled_now - flashled_pre > 500) { + if (flashled_now - flashled_pre > FLASHLED_ON_DURATION_MS) { flashled_pre = flashled_now; flashled_value = 0; ble_set_flashled(flashled_value); } } else { - if (flashled_now - flashled_pre > 20000) { + if (flashled_now - flashled_pre > FLASHLED_OFF_DURATION_MS) { flashled_pre = flashled_now; flashled_value = 1; ble_set_flashled(flashled_value); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/embed/extmod/modtrezorio/modtrezorio-poll.h(1 hunks)core/embed/fp_sensor_wrapper/fingerprint.c(3 hunks)core/embed/fp_sensor_wrapper/fingerprint.h(1 hunks)core/embed/fp_sensor_wrapper/fpsensor_common.c(1 hunks)core/embed/trezorhal/camera.c(4 hunks)core/embed/trezorhal/camera.h(2 hunks)core/embed/trezorhal/camera_qrcode.c(1 hunks)core/embed/trezorhal/device.c(7 hunks)core/embed/trezorhal/usb_webusb-defs.h(1 hunks)core/embed/trezorhal/usb_webusb-impl.h(2 hunks)core/src/trezor/wire/codec_v1.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- core/embed/fp_sensor_wrapper/fingerprint.h
- core/src/trezor/wire/codec_v1.py
- core/embed/trezorhal/camera_qrcode.c
- core/embed/fp_sensor_wrapper/fpsensor_common.c
- core/embed/trezorhal/usb_webusb-defs.h
- core/embed/fp_sensor_wrapper/fingerprint.c
- core/embed/trezorhal/camera.h
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Gen check
- GitHub Check: Defs check
- GitHub Check: Style check
🔇 Additional comments (6)
core/embed/trezorhal/camera.c (3)
182-184: LGTM!
Line range hint
201-220: LGTM!
223-235: LGTM!core/embed/trezorhal/device.c (2)
471-471: LGTM!
639-641: LGTM!core/embed/trezorhal/usb_webusb-impl.h (1)
131-131: Magic number needs constant.Define 500 as USB_WRITE_TIMEOUT_MS.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores