-
Notifications
You must be signed in to change notification settings - Fork 8k
Bluetooth: Controller: Reduce assertion check code size #90599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
d9e0e27
to
4ff5213
Compare
4ff5213
to
61f7bbb
Compare
|
a11a45e
to
1c6db0b
Compare
efe0faa
to
901bf11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 78 out of 78 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (14)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
5f4ddf4
to
6fc003d
Compare
* :c:struct:`bt_hci_vs_fata_error_cpu_data_cortex_m` to | ||
:c:struct:`bt_hci_vs_fatal_error_cpu_data_cortex_m` and now contains the program counter | ||
value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "title" for this "section" is
* The following Kconfig option have been renamed:
Suggest to either start a new list, or modify the "title"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch... I only read renamed
and appended the new entry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is it with the bt_hci_vs
- Are they considered stable APIs and must go through the deprecation process? Or is this good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole Zephyr Vendor-Specific is so just between Zephyr Host and Zephyr Controller :-) .... not sure if this ever considered stable/unstable! Atleast, definitely the error event if generated makes the device/application unstable ;-)
I will not go with deprecation process for literal renames, these changes are caught straight at compile time, mandating a immediate migration task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably consider documenting how the hci
headers should in general be considered in terms of public/internal APIs :D
6fc003d
to
89f2101
Compare
Introduce development and fatal assertion classification in the Controller implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
89f2101
to
67896b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 78 out of 78 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Reduce Controller assertion check code size for ARM Cortex-M CPUs by using the undefined instruction exception. `arm-none-eabi-addr2line` commandline can be used to get the source file and line number. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Make overlay-all-bt_ll_sw_split configs consistent with the one used in hci_ipc for nRF5340 SoC. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Reuse hci_vs_err_assert implementation to generate the HCI vendor-specific hardware error event. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Enable HCI vendor-specific h/w error event generation in samples and tests. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Enable OUTPUT_DISASSEMBLY_WITH_SOURCE when using LTO to help find the assertion check in the source code corresponding to the faulting instruction address. arm-none-eabi-addr2line tool is not able to resolve the source file and line number when using LTO. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
67896b4
to
bc187a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 79 out of 79 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Introduce development and fatal assertion classification in the Controller implementation.
Reduce Controller assertion check code size for ARM Cortex-M CPUs by raising undefined instruction exception or use faulting instruction address in the HCI vendor-specific h/w error event.
hci_ipc for nRF5340 Audio DK with nRF21540 FEM:
Upstream memory use:
PR memory use when Controller assertion check use ARM Cortex-M faulting instruction address (and weirdly enabling HCI vendor-specific error event reduces ~8KB):
33.39 KB or ~ 13.78% reduction in code size.
Test/example Controller assertion:
where
ae 8b 01 00
is the program counter register value in little-endian octets.or
Commandline to use to get details:
Example:
When using LTO,
arm-none-eabi-addr2line
is not able to fetch the source file and line number. With LTO,.lst
file has to be used to look up the faulting instruction address. Hence, enableCONFIG_OUTPUT_DISASSEMBLY=y
to get the.lst
generated for the sample.Minimal
peripheral_hr
:This is an increase for the minimal, with this PR assertion checks are not removed in the minimal sample where as before it was removed when logging/console/serial was not included.