bluetooth: improve the controller address resolution enablement #90705
Conversation
github-actions
bot
added
area: Bluetooth
area: Bluetooth Host
github-actions
bot
requested review from
alwa-nordic,
cvinayak,
hermabe,
jhedberg,
rugeGerritsen,
sjanc,
Thalley and
theob-pro
There was a problem hiding this comment.
Pull Request Overview
This PR refines when the Bluetooth controller’s address resolution is re-enabled by introducing a flag to skip re-enabling if disabling failed or the resolving list was cleared.
- Add
enable_controller_resflag initialized to true. - Set the flag to false on disable failure or after clearing the resolving list.
- Wrap the final
addr_res_enable(BT_HCI_ADDR_RES_ENABLE)call in a conditional based on the flag.
Comments suppressed due to low confidence (2)
subsys/bluetooth/host/id.c:1014
- [nitpick] Consider renaming
enable_controller_restoshould_enable_controller_resorreenable_controller_resfor clearer intent.
bool enable_controller_res = true;
subsys/bluetooth/host/id.c:1085
- Add a test case to cover the scenario where the resolving list is cleared so that address resolution is not re-enabled.
enable_controller_res = false;
MarkWangChinese
force-pushed
the
feature/improve_controller_addr_resolution_enable
branch
from
381c212 to
fa5b640
Compare
There was a problem hiding this comment.
Pull Request Overview
Improve controller address resolution enablement by skipping re-enablement when the resolving list is cleared or when a disable command fails.
- Introduce
enable_controller_resflag to track whether resolution should be re-enabled - Set flag to
falseon disable failure and after clearing the resolving list - Wrap final
addr_res_enable(BT_HCI_ADDR_RES_ENABLE)call in a conditional based on the flag
Comments suppressed due to low confidence (3)
subsys/bluetooth/host/id.c:1014
- [nitpick] The variable name
enable_controller_resis a bit ambiguous. Consider renaming it to something likeshould_enable_addr_resorshould_reenable_controller_resfor clarity.
bool enable_controller_res = true;
subsys/bluetooth/host/id.c:1126
- The return value of
addr_res_enableis not checked here. It would be safer to capture and log any error to aid debugging if re-enablement fails.
addr_res_enable(BT_HCI_ADDR_RES_ENABLE);
subsys/bluetooth/host/id.c:1085
- Add tests to cover the scenario where the resolving list is cleared and
enable_controller_resis false, ensuring address resolution isn’t re-enabled unexpectedly.
enable_controller_res = false;
subsys/bluetooth/host/id.c
Outdated
| /* since the controller resolving list is cleared, | ||
| * don't need to enable the Address Resolution. |
There was a problem hiding this comment.
[nitpick] Minor: consider adjusting comment capitalization and consistency, e.g., "address resolution" in lowercase, to match code style.
| /* since the controller resolving list is cleared, | |
| * don't need to enable the Address Resolution. | |
| /* Since the controller resolving list is cleared, | |
| * don't need to enable the address resolution. |
Copilot uses AI. Check for mistakes.
MarkWangChinese
force-pushed
the
feature/improve_controller_addr_resolution_enable
branch
from
fa5b640 to
892407c
Compare
If the controller resolving list is cleared by HCI_LE_Clear_Resolving_List, don't need to enable the controller address resolution. Signed-off-by: Mark Wang <yichang.wang@nxp.com>
MarkWangChinese
force-pushed
the
feature/improve_controller_addr_resolution_enable
branch
from
892407c to
56c5dbc
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refines when the controller’s address resolution is re-enabled by introducing a flag to skip re-enablement if the resolving list was just cleared or if disabling failed.
- Add
enable_controller_resflag to track whether to calladdr_res_enableat the end - Set flag to
falsewhen disable fails or when the resolving list is cleared - Wrap final
addr_res_enable(BT_HCI_ADDR_RES_ENABLE)in a conditional based on the new flag
Comments suppressed due to low confidence (2)
subsys/bluetooth/host/id.c:1014
- [nitpick] The flag name
enable_controller_rescan be ambiguous. Consider renaming it to something likeshould_enable_controller_resto clearly indicate it controls whether re-enablement should occur.
bool enable_controller_res = true;
subsys/bluetooth/host/id.c:1085
- This new path where
enable_controller_resis set to false after clearing the resolving list should be covered by a unit or integration test to ensure resolution is not re-enabled in this scenario.
enable_controller_res = false;
| @@ -1067,13 +1068,21 @@ void bt_id_add(struct bt_keys *keys) | |||
| err = addr_res_enable(BT_HCI_ADDR_RES_DISABLE); | |||
| if (err) { | |||
| LOG_WRN("Failed to disable address resolution"); | |||
| /* If it fails to disable, it should already be enabled, | |||
There was a problem hiding this comment.
[nitpick] The comment could be clearer and more concise. For example: /* On disable failure, resolution remains enabled—no need to re-enable. */
Copilot uses AI. Check for mistakes.
|
If the controller resolving list is cleared by HCI_LE_Clear_Resolving_List, don't need to enable the controller address resolution.
I think it is one improvement, please help to review.