BT: host: Service Change indication sent regardless of whether it is needed or not. #23485
Assignees
Labels
area: Bluetooth
bug
The issue is a bug, or the PR is fixing a bug
priority: medium
Medium impact/importance bug
Comments
daee-demant
added a commit
to daee-demant/zephyr
that referenced
this issue
Mar 16, 2020
Fixes zephyrproject-rtos#23485 When we create a GATT table dynamically, we also create a hash identifying this table. This hash can be stored in persistent memory and we can thus determine after recreating the GATT table whether the services have changed or not from before the reboot. When these hashes are identical, it implies that the table has not changed, wherefore a service changed indication should not be sent to any bonded clients. The method for achieving this was to remove the gatt_sc.work entry from the work queue. This work queue entry was to send an indication to the clients when the table had been allocated. If the final entry then caused the hashes to match, the indication would be cancelled. On unit testing this behaviour in simulation and in practice, we found that the indication was sent nonetheless, and the issue was located to be tied to the SERVICE_RANGE_CHANGED flag which is set when the services are changed and is cleared when the indications are being sent out. It was the job of the work queue entry to clear this flag, and as the entry was never serviced, the flag was never cleared, and when sc_commit() is called at the end of the process, it believes that there is a new service change pending and therefore starts the job over, thus creating a redundant indication to the clients. This commit fixes the issue by clearing the flag when the work entry is removed due to a hash match. This has been unittested in a live environment, in a simulation environment, and sanitycheck has been run on it. Signed-off-by: Dan Erichsen <daee@demant.com>
aescolar
changed the title
daee-demant
added a commit
to daee-demant/zephyr
that referenced
this issue
Mar 16, 2020
Fixes zephyrproject-rtos#23485 When we create a GATT table dynamically, we also create a hash identifying this table. This hash can be stored in persistent memory and we can thus determine after recreating the GATT table whether the services have changed or not from before the reboot. When these hashes are identical, it implies that the table has not changed, wherefore a service changed indication should not be sent to any bonded clients. The method for achieving this was to remove the gatt_sc.work entry from the work queue. This work queue entry was to send an indication to the clients when the table had been allocated. If the final entry then caused the hashes to match, the indication would be cancelled. On unit testing this behaviour in simulation and in practice, we found that the indication was sent nonetheless, and the issue was located to be tied to the SERVICE_RANGE_CHANGED flag which is set when the services are changed and is cleared when the indications are being sent out. It was the job of the work queue entry to clear this flag, and as the entry was never serviced, the flag was never cleared, and when sc_commit() is called at the end of the process, it believes that there is a new service change pending and therefore starts the job over, thus creating a redundant indication to the clients. This commit fixes the issue by clearing the flag when the work entry is removed due to a hash match. This has been unittested in a live environment, in a simulation environment, and sanitycheck has been run on it. Signed-off-by: Dan Erichsen <daee@demant.com>
jhedberg
pushed a commit
that referenced
this issue
Mar 16, 2020
Fixes #23485 When we create a GATT table dynamically, we also create a hash identifying this table. This hash can be stored in persistent memory and we can thus determine after recreating the GATT table whether the services have changed or not from before the reboot. When these hashes are identical, it implies that the table has not changed, wherefore a service changed indication should not be sent to any bonded clients. The method for achieving this was to remove the gatt_sc.work entry from the work queue. This work queue entry was to send an indication to the clients when the table had been allocated. If the final entry then caused the hashes to match, the indication would be cancelled. On unit testing this behaviour in simulation and in practice, we found that the indication was sent nonetheless, and the issue was located to be tied to the SERVICE_RANGE_CHANGED flag which is set when the services are changed and is cleared when the indications are being sent out. It was the job of the work queue entry to clear this flag, and as the entry was never serviced, the flag was never cleared, and when sc_commit() is called at the end of the process, it believes that there is a new service change pending and therefore starts the job over, thus creating a redundant indication to the clients. This commit fixes the issue by clearing the flag when the work entry is removed due to a hash match. This has been unittested in a live environment, in a simulation environment, and sanitycheck has been run on it. Signed-off-by: Dan Erichsen <daee@demant.com>
carlescufi
pushed a commit
that referenced
this issue
Mar 17, 2020
Fixes #23485 When we create a GATT table dynamically, we also create a hash identifying this table. This hash can be stored in persistent memory and we can thus determine after recreating the GATT table whether the services have changed or not from before the reboot. When these hashes are identical, it implies that the table has not changed, wherefore a service changed indication should not be sent to any bonded clients. The method for achieving this was to remove the gatt_sc.work entry from the work queue. This work queue entry was to send an indication to the clients when the table had been allocated. If the final entry then caused the hashes to match, the indication would be cancelled. On unit testing this behaviour in simulation and in practice, we found that the indication was sent nonetheless, and the issue was located to be tied to the SERVICE_RANGE_CHANGED flag which is set when the services are changed and is cleared when the indications are being sent out. It was the job of the work queue entry to clear this flag, and as the entry was never serviced, the flag was never cleared, and when sc_commit() is called at the end of the process, it believes that there is a new service change pending and therefore starts the job over, thus creating a redundant indication to the clients. This commit fixes the issue by clearing the flag when the work entry is removed due to a hash match. This has been unittested in a live environment, in a simulation environment, and sanitycheck has been run on it. Signed-off-by: Dan Erichsen <daee@demant.com>
hakehuang
pushed a commit
to hakehuang/zephyr
that referenced
this issue
Mar 18, 2020
Fixes zephyrproject-rtos#23485 When we create a GATT table dynamically, we also create a hash identifying this table. This hash can be stored in persistent memory and we can thus determine after recreating the GATT table whether the services have changed or not from before the reboot. When these hashes are identical, it implies that the table has not changed, wherefore a service changed indication should not be sent to any bonded clients. The method for achieving this was to remove the gatt_sc.work entry from the work queue. This work queue entry was to send an indication to the clients when the table had been allocated. If the final entry then caused the hashes to match, the indication would be cancelled. On unit testing this behaviour in simulation and in practice, we found that the indication was sent nonetheless, and the issue was located to be tied to the SERVICE_RANGE_CHANGED flag which is set when the services are changed and is cleared when the indications are being sent out. It was the job of the work queue entry to clear this flag, and as the entry was never serviced, the flag was never cleared, and when sc_commit() is called at the end of the process, it believes that there is a new service change pending and therefore starts the job over, thus creating a redundant indication to the clients. This commit fixes the issue by clearing the flag when the work entry is removed due to a hash match. This has been unittested in a live environment, in a simulation environment, and sanitycheck has been run on it. Signed-off-by: Dan Erichsen <daee@demant.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
A service change indication is sent to bonded clients upon a reboot even if there is no change to the GATT table on the server.
To Reproduce
Bond with a client. Reboot the server. A service change indication is transmitted
Expected behavior
Upon a reboot when there is no change, no indication should be sent.
Impact
A redundant message is transmitted and acknowledged during startup.
Environment (please complete the following information):
Zephyr 2.2
Additional context
The issue seems to center around the SC_RANGE_CHANGED flag not being cleared.
The text was updated successfully, but these errors were encountered: