Hi Marcel,
On 05/27/2015 09:23 PM, Marcel Holtmann wrote:
Hi Florian,
patch set introducing the infrastructure for multi-advertising
capability to the HCI core and mgmt API.
v1 -> v2:
- add missing braces in read_adv_features()
v2 -> v3 (changes due to Johan's review):
- split change-set into several patches
- replace err == 0 by !err
- fix coding style problems
v3 -> v4 (changes due to Arman's review):
- bug fix: when calling remove_advertising with an instance value of
zero (i.e. remove all instances), the command response also returns
an instance value of zero now as it doesn't make sense to return a
single instance id when removing several instances
- splitting the change set into a much larger number of patches to
facilitate review
- use HCI_MAX_ADV_INSTANCES in the same patch that introduces it
- use adv_info->hdev in the same patch that introduces it
- make the logic of hci_find_adv_instance() more readable
- make sure that hci_find_adv_instance() is called while hci_dev is
locked
- replace hci_num_adv_instances() by an instance counter in hci_dev
- add inline comment in get_adv_instance_flags() explaining its return
value in the error case
- generate zero-length adv data if the current instance identifier is
invalid
- revert erroneous changes to the logic in clear_adv_instance()
- use hci_adv_instances_clear() in clear_adv_instance() when removing
all advertising instances also removing a redundant error check
- inserting TODO messages to make sure that advertising will not be
switched off prematurely once we allow more than one advertising
instance
- inserting TODO messages to make sure that multiple advertising
instances will be advertised in a round-robin fashion once we allow
for more than one advertising instance
- identify peding advertising instances (just added but not yet
confirmed in add_advertising_complete) by a boolean flag in the
adv_info struct so that we can identify and remove them even when the
pending add_advertising command cannot be retrieved for some reason -
makes sure that we do not leak advertising instances in this case
- only send HCI commands to update advertising data when a new instance
has actually been added
v4 -> v5 (changes due to Marcel's review):
- split into separate change sets for kernel and userspace
- do not trust the adv instance list length when compiling the adv
features struct
- introduce adv_info->pending in the same patch it's used for the first
time
v5 -> v6 (changes due to Marcel's review):
- fix the add adv override bug
- fix "Using plain integer as NULL pointer" warning in
set_advertising_complete()
- reword the "remove adv instance" commit comment to make it clear that
an existing bug is being fixed
so the max adv instances variable is still 1 after applying this patch set. So fundamentally nothing has changed. However when I set this to any other value, the feature does not really work. Doing a simple thing and adding multiple instances with a default duration value only keeps the last one active forever. It just never rotates. Also when I have two or more instances and remove instance 1, then advertising gets disabled and trying to remove the other instances return an error.
Are multiple instances suppose to work with this patch set?
No. There are a few remaining TODOs (documented in the code) for actual
multi-advertising. I mention these in the commit messages where applicable.
The idea was to do the more complex changes in a functionally neutral
refactoring first as a safe starting point for functional changes (see
my conv. with Arman on the list).
Of course I'm prepared to resolve those remaining TODOs in a second
patch set. I just think it is a good idea if I go full cycle once to
avoid those beginner's errors during the next iteration.
If you think that there are no more fundamental flaws in that first
patch set then I can start with the second one now. What do you think?
Florian
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
