Update libtropic and speed up tropic communication#6956
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR updates Tropic firmware compilation configuration across all T3W1 build targets to use versioned silicon revision macros and L1 read retry parameters. It adjusts core Tropic operation time estimation helpers and retry classification logic, refactors firmware update and pairing key discovery into reusable components, and introduces a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| model | device_test | click_test | persistence_test |
|---|---|---|---|
| T2T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3B1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3T1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
| T3W1 |  test(all)  main(all)  |
 test(all)  main(all)  |
 test(all)  main(all)  |
Latest CI run: 26748061888
onvej-sl
force-pushed
the
onvej-sl/tropic-speedup
branch
from
ede851a to
26e56c6
Compare
[no changelog]
[no changelog]
onvej-sl
force-pushed
the
onvej-sl/tropic-speedup
branch
from
26e56c6 to
7e7e478
Compare
[no changelog]
[no changelog]
[no changelog]
[no changelog]
onvej-sl
force-pushed
the
onvej-sl/tropic-speedup
branch
from
7e7e478 to
e16922a
Compare
onvej-sl
requested review from
M-pasta
and removed request for
TychoVrahe and
obrusvit
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/embed/projects/prodtest/cmd/prodtest_tropic.c`:
- Around line 1634-1638: The cli_trace call is incorrectly passing CLI_ERROR as
the format string which corrupts variadic args; update the call site to use
cli_trace(cli, " `tropic_custom_session_start()` for key %d failed with error
'%s'", i, lt_ret_verbose(res)) (i.e., remove the CLI_ERROR argument), or if you
intend to log an error use cli_error(cli, ...) instead; locate and fix the call
to cli_trace in the loop where tropic_custom_session_start() is checked and
ensure the format string is the second argument and the subsequent i and
lt_ret_verbose(res) are the variadic parameters.
In `@core/embed/projects/prodtest/README.md`:
- Around line 1474-1486: The fenced code block showing the tropic-benchmark
output in README.md violates markdownlint MD040 because it lacks a language;
update the opening fence from ``` to ```text (or another appropriate language
such as `console`) so the block explicitly declares a language and resolves the
lint error; locate the block containing the "tropic-benchmark" output in
core/embed/projects/prodtest/README.md and change its fence accordingly.
In `@core/site_scons/models/T3W1/emulator.py`:
- Around line 124-126: The build macros LT_L1_READ_MAX_TRIES and
LT_L1_READ_RETRY_DELAY_MS added in emulator.py are not referenced anywhere in
the C/C++ code so they may have no effect; search for LT_L1_READ_MAX_TRIES and
LT_L1_READ_RETRY_DELAY_MS across libtropic/tropic and the firmware sources and
if no consumer exists either (A) wire them into the compiled code path that
implements the L1 read retry loop (add/consume them in the C source or header
that implements the L1 read retry) or (B) export them into a shared generated
header via the SCons rules so the C build sees the defines, or (C) remove the
misleading defines from core/site_scons/models/T3W1/emulator.py; update the
emulator.py change accordingly and document where the L1 retry behavior is
actually implemented (reference LT_L1_READ_MAX_TRIES, LT_L1_READ_RETRY_DELAY_MS,
and the L1 read retry implementation function/module you find).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60ed4ac7-4b3a-492c-89d3-6f90cad344a9
📒 Files selected for processing (8)
core/embed/projects/prodtest/README.mdcore/embed/projects/prodtest/cmd/prodtest_tropic.ccore/embed/sec/tropic/tropic.ccore/site_scons/models/T3W1/emulator.pycore/site_scons/models/T3W1/trezor_t3w1_revA.pycore/site_scons/models/T3W1/trezor_t3w1_revB.pycore/site_scons/models/T3W1/trezor_t3w1_revC.pyvendor/libtropic
| ``` | ||
| tropic-benchmark | ||
| # session_start() 204 ms | ||
| # mac_and_destroy() 28 ms | ||
| # r_mem_data_write() 37 ms | ||
| # r_mem_data_read() 33 ms | ||
| # r_mem_data_erase() 4 ms | ||
| # mcounter_init() 9 ms | ||
| # mcounter_get() 4 ms | ||
| # mcounter_update() 4 ms | ||
| # random_value_get() 10 ms | ||
| OK | ||
| ``` |
There was a problem hiding this comment.
Add a language to the new fenced example block.
This block currently violates markdownlint MD040 and can fail docs linting.
Suggested fix
-```
+```text
tropic-benchmark
# session_start() 204 ms
# mac_and_destroy() 28 ms
# r_mem_data_write() 37 ms
# r_mem_data_read() 33 ms
# r_mem_data_erase() 4 ms
# mcounter_init() 9 ms
# mcounter_get() 4 ms
# mcounter_update() 4 ms
# random_value_get() 10 ms
OK</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1474-1474: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/embed/projects/prodtest/README.md` around lines 1474 - 1486, The fenced
code block showing the tropic-benchmark output in README.md violates
markdownlint MD040 because it lacks a language; update the opening fence from
``` to ```text (or another appropriate language such as `console`) so the block
explicitly declares a language and resolves the lint error; locate the block
containing the "tropic-benchmark" output in
core/embed/projects/prodtest/README.md and change its fence accordingly.
| defines += [("LT_SILICON_REV_ABAB", "1")] | ||
| defines += [("LT_L1_READ_RETRY_DELAY_MS", "1")] | ||
| defines += [("LT_L1_READ_MAX_TRIES", "1250")] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the new LT_ defines are used in libtropic or tropic code
echo "=== Searching for LT_SILICON_REV_ABAB usage ==="
rg -n 'LT_SILICON_REV_ABAB' --type=c --type=h -C2
echo -e "\n=== Searching for LT_L1_READ_MAX_TRIES usage ==="
rg -n 'LT_L1_READ_MAX_TRIES' --type=c --type=h -C2
echo -e "\n=== Searching for LT_L1_READ_RETRY_DELAY_MS usage ==="
rg -n 'LT_L1_READ_RETRY_DELAY_MS' --type=c --type=h -C2
echo -e "\n=== Checking for retry/delay related comments or documentation ==="
rg -n -i 'retry.*delay|L1.*read.*retry' vendor/libtropic/ -C2Repository: trezor/trezor-firmware
Length of output: 1973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for p in LT_SILICON_REV_ABAB LT_L1_READ_MAX_TRIES LT_L1_READ_RETRY_DELAY_MS; do
echo "=== Matches for $p ==="
rg -n --hidden --no-ignore-vcs --max-count 200 "$p" . -S || true
done
echo "=== Likely L1 retry/delay logic ==="
rg -n --hidden --no-ignore-vcs --max-count 200 -i 'l1.*read.*retry|retry.*delay' . -S --glob='*.c' --glob='*.h' || trueRepository: trezor/trezor-firmware
Length of output: 1474
Ensure LT_L1_READ_MAX_TRIES / LT_L1_READ_RETRY_DELAY_MS actually drive L1 read retry behavior
LT_L1_READ_MAX_TRIES=1250 and LT_L1_READ_RETRY_DELAY_MS=1 implies up to ~1.25s of retry delay per L1 read; however, repo-wide search only finds LT_L1_READ_MAX_TRIES / LT_L1_READ_RETRY_DELAY_MS being defined in core/site_scons/models/T3W1/* (including emulator.py, revA/revB/revC) and shows no corresponding usage in any C/C++/header code. Confirm that libtropic/tropic’s L1 read retry loop consumes these macros (otherwise the emulator change may be ineffective or misleading).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/site_scons/models/T3W1/emulator.py` around lines 124 - 126, The build
macros LT_L1_READ_MAX_TRIES and LT_L1_READ_RETRY_DELAY_MS added in emulator.py
are not referenced anywhere in the C/C++ code so they may have no effect; search
for LT_L1_READ_MAX_TRIES and LT_L1_READ_RETRY_DELAY_MS across libtropic/tropic
and the firmware sources and if no consumer exists either (A) wire them into the
compiled code path that implements the L1 read retry loop (add/consume them in
the C source or header that implements the L1 read retry) or (B) export them
into a shared generated header via the SCons rules so the C build sees the
defines, or (C) remove the misleading defines from
core/site_scons/models/T3W1/emulator.py; update the emulator.py change
accordingly and document where the L1 retry behavior is actually implemented
(reference LT_L1_READ_MAX_TRIES, LT_L1_READ_RETRY_DELAY_MS, and the L1 read
retry implementation function/module you find).
| defines += [("LT_L1_READ_RETRY_DELAY_MS", "1")] | ||
| defines += [("LT_L1_READ_MAX_TRIES", "1250")] |
There was a problem hiding this comment.
Let me check I understand what is happening here. My interpretation is as follows: In version 4.0.0 libtropic added
Possibility to configure
LT_L1_READ_MAX_TRIESandLT_L1_READ_RETRY_DELAY_MS.
And this is our setting of these variables in our environment. Am I correct?
If so, I wanted to ask: In Tropic documentation I found a note saying
It is better to use the interrupt pin functionality of TROPIC01 if you need to shorten reaction time. See LT_USE_INT_PIN parameter.
My interpretation is that they claim it is better to use some interrupt pin functionality than these variables. the question is: Do you know what the functionality is and if it is better for us?
2 participants
No description provided.