You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2021/12/13 13:16:43 UTC
[GitHub] [mynewt-nimble] haukepetersen opened a new pull request #1129: host/gap: fix legacy advertising for BLE_EXT_ADV=1
haukepetersen opened a new pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129
When enabling extended advertising it was not possible to send legacy advertisements other than NONCONN. This PR fixes the GAP implementation to also allow advertising legacy connectable, scannable, and directed advertisements.
I am not sure if this is the most beautiful way to solve this, but it was the first thing that came to mind. But I am open for suggestions for improvement :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on pull request #1129: host/gap: fix legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
sjanc commented on pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#issuecomment-992515532
I'm not going to block this if you really prefer auto-enable for missing features, but I'd rather document this behavior for legacy configuration and keep returning error (and maybe provide convenience macros for legacy configurations?).
The thing is that one might not expect that just enabling legacy advertising might result in replying to scan responses (since scanable was not explicitly set)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on a change in pull request #1129: host/gap: improve legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#discussion_r769474462
##########
File path: nimble/host/src/ble_gap.c
##########
@@ -2694,6 +2694,19 @@ ble_gap_ext_adv_params_tx(uint8_t instance,
}
if (params->legacy_pdu) {
Review comment:
should we move this after include_tx_power to also cover anon an tx power? (those are also invalid for legacy PDUs)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #1129: host/gap: improve legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#issuecomment-1012007456
Moved the validity check after the anon and tx power assignments as suggested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen merged pull request #1129: host/gap: improve legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
haukepetersen merged pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on a change in pull request #1129: host/gap: improve legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on a change in pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#discussion_r783831946
##########
File path: nimble/host/src/ble_gap.c
##########
@@ -2694,6 +2694,19 @@ ble_gap_ext_adv_params_tx(uint8_t instance,
}
if (params->legacy_pdu) {
Review comment:
Yes, absolutely.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #1129: host/gap: improve legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#issuecomment-993402258
I pushed a new commit to this PR:
- it adds some documentation to `struct ble_gap_ext_adv_params` to tell the user which combinations are allowed
- it adds a validity check into `ble_gap_ext_adv_params_tx()`. This way the combination of options is already checked in the host and the user does not have to wonder about relatively non-specific HCI error return codes, but instead gets a way more easy to understand host error code.
After some trial and error I finally decided against implicit setting of flags for the following reasons:
- for coherency this would have been done for `legacy_pdu + connectable` (setting `scannable`), but also for `legacy_pdu + directed` and `legacy_pdu + directed + high_duty_directed` (setting `connectable`). But this would mean some ugly if/else/not blocks to make sure we do not accidentally set the scannable flag after the connectable was set implicitly for directed advertisements etc... All in all some coding in-efficiency that might as well skip by having the appropriate documentation in place
- by improving the error reporting (no generic HCI error codes) it becomes much more obvious to the user where things to wrong. This combined with appropriate doc should make this API user friendly enough
- as you stated first: its simply keeps the API cleaner and hopefully easier to maintian in the long term by not auto-enabling any flags
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #1129: host/gap: improve legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#issuecomment-1012106692
All green -> lets. go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] andrzej-kaczmarek commented on pull request #1129: host/gap: fix legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#issuecomment-992487987
I do not see why old code would not allow to enable e.g. connectable or scannable advertising using legacy PDU.
For example, if you want connectable using legacy, you need to set connectable+scannable+legacy (because connectable legacy are also scannable) - that will set 0x01 for connectable, 0x02 for scannable and 0x10 for legacy which is the same as 0x13 for new code. So probably just to avoid confusion I'd modify current code to assume scannable if connectable+legacy is requested or just return an error since connectable+legacy without scannable is an invalid combination.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #1129: host/gap: fix legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#issuecomment-992493140
True, you are right, I missed those combinations...
Big +1 for adapting the code for auto-filling and throwing errors already in the host code. I have been running into this issue multiple times now, and as it seems I am really bad in remembering how to apply it correctly :-)
I will rework this PR with a proposal on error checking/auto-filling of params, lets see if I can come up with something feasible...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] andrzej-kaczmarek commented on pull request #1129: host/gap: fix legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#issuecomment-992490880
btw, those combinations for legacy are defined in Core 5.3, Vol 4, Part E, 7.8.53. I think I'd prefer that host adds missing bits if possible (e.g. if connectable+legacy is requested, it's obvious we also need scannable) or return an error if impossible combination with legacy is requested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] sjanc commented on pull request #1129: host/gap: fix legacy advertising for BLE_EXT_ADV=1
Posted by GitBox <gi...@apache.org>.
sjanc commented on pull request #1129:
URL: https://github.com/apache/mynewt-nimble/pull/1129#issuecomment-992529789
alright, we talked this offline, let's go with auto-enabling (to keep it similar with legacy API) but at least document this behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org