bluetooth: fix GATT service reregistering #82062
Conversation
zephyrbot
requested review from
hermabe,
jhedberg,
rugeGerritsen,
sjanc,
Thalley and
theob-pro
akredalen
force-pushed
the
fix_rereg_gatt_service
branch
from
218c7e1
to
599cbb4
Compare
tests/bluetooth/gatt/src/main.c
Outdated
|
|
||
| for (int i = 0; i < 10; i++) { | ||
|
|
||
| /* Attempt to register service A */ |
There was a problem hiding this comment.
(here and below) This comment is the same as the function name. Better to write the test procedure before the test function. Same for the other new tests.
| "Test service A registration failed"); | ||
|
|
||
| /* Attempt to unregister service A */ | ||
| zassert_false(bt_gatt_service_unregister(&local_test_svc), |
There was a problem hiding this comment.
The registration function may at some point be rewritten in such a way that the test still passes, but the handles are not reset. Therefore it is better to check that all handles are zero-ed after unregistering the service. This will guarantee that we get exactly what we want.
There was a problem hiding this comment.
I removed this check because I thought it was a test for the unregister function, which should probably not be done inside this test. Should I still add the check here or should this be added to the test of the unregister function instead?
There was a problem hiding this comment.
Yes, I think it makes sense to check handles here.
tests/bluetooth/gatt/src/main.c
Outdated
| "Test service A registration failed"); | ||
|
|
||
| /* Attempt to unregister service A */ | ||
| zassert_false(bt_gatt_service_unregister(&local_test_svc), |
There was a problem hiding this comment.
Same comment as above, but here (and below) we should check that the handles are the same after unregistering as they were before registering the service. Otherwise, if it is zero-ed, the service will still be registered, but this won't be what we expect.
tests/bluetooth/gatt/src/main.c
Outdated
| /* Pre-allocate handles for service A */ | ||
| for (int j = 0; j < local_test_svc.attr_count; j++) { |
There was a problem hiding this comment.
It is better to name both services such that it is clear which one has pre-allocated handles and which one will get handles by the host.
tests/bluetooth/gatt/src/main.c
Outdated
| zassert_false(bt_gatt_service_unregister(&local_test_svc), | ||
| "Test service A unregister failed"); |
There was a problem hiding this comment.
Same as two comments above: check handles after calling bt_gatt_service_unregister.
That should be in the PR description, not necessarily in the commit message (in fact, references like that in commit messages loose their meaning if the project ever moves to a different location or if someone looks at the commit history in some fork of upstream). |
Ok, I just find it useful to myself at least when going through the git history through |
Sure, which is why I wasn't trying to outright forbid it. However, GitHub doesn't look at commit messages when it links PRs and issues together, which is why it's important that the reference is at least in the PR description (a contributor might forget to update that if they just modify the commit message and push). |
I see. Usually, when PR has a single commit, it content is put into the description. But yes, you are right, this should be in the PR description for GitHub to match the PR with the issue. |
akredalen
force-pushed
the
fix_rereg_gatt_service
branch
2 times, most recently
from
150f1a9
to
6e66a3b
Compare
include/zephyr/bluetooth/gatt.h
Outdated
| uint16_t perm: 15; | ||
| uint16_t _auto_assigned_handle: 1; |
There was a problem hiding this comment.
@jhedberg correct me if I'm wrong, but I think we want to avoid bitfields in the public header? And prefer to use macros to get and set bits instead.
IIRC the reasoning is that these type of bitfields may not work well with non-arm architectures, but unsure if we actually have that in writing anywhere in Zephyr
There was a problem hiding this comment.
@jhedberg correct me if I'm wrong, but I think we want to avoid bitfields in the public header? And prefer to use macros to get and set bits instead.
That would be the case for any packed structs which are supposed to map to some canonical protocol binary representation. I think in this case that's not the case, i.e. nothing depends on a specific in-memory layout of these struct members.
There was a problem hiding this comment.
Okay. I'm pretty sure I've been asked to not use bitfields in the public header files, even for non-packed structs several times, and mostly for performance reasons since some architectures do not support it.
Anyways, since we do not (AFAIK) have any coding guidelines on this, it's OK as is :)
akredalen
force-pushed
the
fix_rereg_gatt_service
branch
2 times, most recently
from
5d27478
to
bf1660a
Compare
include/zephyr/bluetooth/gatt.h
Outdated
| * @note The `_auto_assigned_handle` flag in this bitfield indicates if the attribute | ||
| * handle was automatically assigned by the stack (1) or manually set by the | ||
| * application (0). | ||
| * This flag must not be modified by the application. |
There was a problem hiding this comment.
This should be moved to a separate Doxygen comment for _auto_assigned_handle, and should include @internal.
There was a problem hiding this comment.
@internal cannot be used to hide struct members from documentation. @cond INTERNAL_HIDDEN needs to be used for that purpose
There was a problem hiding this comment.
I played around with this. It seems Doxygen will hide members that being with an underscore.
@akredalen, Since this field starts with an underscore, it's already private and there is no need to add anything. Sorry for the confusion.
akredalen
force-pushed
the
fix_rereg_gatt_service
branch
2 times, most recently
from
2a37cc3
to
1157688
Compare
* Fixed issue with reregistering of GATT services. * Added unit tests covering the GATT reregistering scenario. Signed-off-by: Stine Åkredalen <[email protected]>
akredalen
force-pushed
the
fix_rereg_gatt_service
branch
from
1157688
to
94df12a
Compare
Fixes #67377