nimble/host/include/host/ble_gap: expose ble_gap_adv_active_instance #1065
Conversation
fjmolinas
force-pushed
the
pr_ext_adv_is_active_instance
branch
from
41885f8
to
7d88fe8
Compare
nimble/host/src/ble_gap.c
Outdated
| @@ -1294,14 +1294,12 @@ ble_gap_master_in_progress(void) | |||
| #endif | |||
| } | |||
|
|
|||
| #if NIMBLE_BLE_ADVERTISE || NIMBLE_BLE_CONNECT | |||
| static int | |||
| int | |||
| ble_gap_adv_active_instance(uint8_t instance) | |||
| { | |||
| /* Assume read is atomic; mutex not necessary. */ | |||
There was a problem hiding this comment.
There is already ble_gap_adv_active() for legacy API.
So I'd add new API ble_gap_ext_adv_active() (under EXT_ADV ifdef) that would wrap this (and check if instance is valid)
There was a problem hiding this comment.
I had done that initially, but the function is used in legacy api code, just with instance=0 and so travis complained. I could wrap the static qualifier based on EXT_ADV ifdef. Would that be ok?
There was a problem hiding this comment.
I meant add new function ble_gap_ext_adv_active(uint8_t instance) that will internally use ble_gap_adv_active_instance()
There was a problem hiding this comment.
Pushed, is it what you had in mind?
There was a problem hiding this comment.
Maybe another return code is more appropriate when the instance is invalid?
There was a problem hiding this comment.
other functions typically use BLE_HS_EINVAL for such cases, lets keep it consistent (note no minus)
other than that looks OK to me
There was a problem hiding this comment.
squashed directly
fjmolinas
force-pushed
the
pr_ext_adv_is_active_instance
branch
2 times, most recently
from
e3102a4
to
6bdec5f
Compare
nimble/host/src/ble_gap.c
Outdated
| int ble_gap_ext_adv_active(uint8_t instance) | ||
| { | ||
| if (instance >= BLE_ADV_INSTANCES) { | ||
| return -BLE_HS_EINVAL; |
There was a problem hiding this comment.
this should be return BLE_HS_EINVAL (no minus)
There was a problem hiding this comment.
I must be dyslexic, you had said it before and I somehow read note the minus., sorry for this
There was a problem hiding this comment.
Addressed as you said, it might be confusing to have 1 be a valid output and 3 an error though.
There was a problem hiding this comment.
this is actually a good point, so lets just return 0 if instance is invalid... (sorry for jumping back and forth on this)
fjmolinas
force-pushed
the
pr_ext_adv_is_active_instance
branch
from
6bdec5f
to
5152b78
Compare
|
Is this one ok now @sjanc? |
fjmolinas
force-pushed
the
pr_ext_adv_is_active_instance
branch
from
5152b78
to
0c59457
Compare
fjmolinas
force-pushed
the
pr_ext_adv_is_active_instance
branch
from
0c59457
to
02c8b6c
Compare
|
Thank you for the review! @sjanc |
This PR exposes
ble_gap_adv_active_instance, it's sometimes useful to check if there is an active advertisement on an instance before stopping it or changing some parameter.