-
Notifications
You must be signed in to change notification settings - Fork 3k
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
nrf52-ble: fix total links count #7966
Conversation
@ARMmbed/mbed-os-pan Please review |
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.
TOTAL_LINK_COUNT
is the number of concurrent connections which, at most, will be equal to the number of central and peripheral links count if peripheral and central roles operates in parallel.
If peripheral and central roles do not operate in parallel in the application then TOTAL_LINK_COUNT
should be equal to max(CENTRAL_LINK_COUNT, PERIPHERAL_LINK_COUNT)
.
In the end, TOTAL_LINK_COUNT
depends on the application. Unfortunately the fix proposed doesn't fit everyone needs.
@donatieng What should be the default value ?
The key to the problem is how to distinguish the role of the application. I think that it is not as good as user configuration by tools detection. CENTRAL_LINK_COUNT and PERIPHERAL_LINK_COUNT have been configured by the user, so we should trust that it is what the user needs. In other words, if the application is only in CENTRAL mode, then CENTRAL_LINK_COUNT=x PERIPHREAL_LINK_COUNT=0 should be configured. All we have to do is trust the user, not overdetect. |
@pingdan32 I believe an entry config file would solves everyone problems: "total_link_count": {
"help": "How many concurrent connections the system support.",
"value": "(NRF_SDH_BLE_CENTRAL_LINK_COUNT + NRF_SDH_BLE_PERIPHERAL_LINK_COUNT)",
"macro_name": "NRF_SDH_BLE_TOTAL_LINK_COUNT"
}, What do you think ? |
😁Good idea! I can't agree more. |
@pan- Mind re-reviewing? |
/morph build |
Build : FAILUREBuild number : 3438 |
Failures look unrelated, seems like CI did not cleanup properly, restarting /morph build |
Build : SUCCESSBuild number : 3441 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 3062 |
/morph mbed2-build |
Test : SUCCESSBuild number : 3229 |
Description
I think I found a bug about the NRF52 series. The
NRF_SDH_BLE_TOTAL_LINK_COUNT
is NOT defined in any configuration file exceptsdk_config.h
. This causes the actual number of total links to be inconsistent with the definition whencentral_link_count
orperipheral_link_count
is configured viafeatures/FEATURE_BLE/targets/TARGET_NORDIC/mbed_lib.json
.This PR fixes this bug and I think this is a good way. Please review.
Pull request type