Skip to content

Conversation

@jhedberg
Copy link
Member

Qualification test case MESH/NODE/FRND/FN/BV-08-C requires that we do
not store more messages than the reported Friend Queue size. The
implementation was so far opportunistic and stored more if it could
(it would later discard if necessary to make sure all queues can store
the required amount). The spec also requires the queues to have new
messages overwrite old ones (in the style of a circular buffer), so we
have to keep track of which buffers are part of the same segmented
message (so we discard all buffers belonging to the same message).

To pass the test case, add APIs to check for space in the Friend
queue, and track the number of buffers for each incoming segmented
message.

Fixes #18090

@jhedberg jhedberg added bug The issue is a bug, or the PR is fixing a bug area: Bluetooth Backport Backport PR and backport failure issues area: Bluetooth Mesh area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests labels Aug 19, 2019
@jhedberg jhedberg force-pushed the friend_queue_limit branch 2 times, most recently from 7952dd2 to a8449b8 Compare August 19, 2019 08:27
@jhedberg
Copy link
Member Author

jhedberg commented Aug 19, 2019

Note: I'm not generally happy about the way the spec and test spec deals with Friend Queue overflow handling wrt segmented messages. If you read the spec it uses the term "message" when referring to how many entities the Friend Queue Size indicates will fit the Friend Queue. If we interpret this strictly it means an implementation should be able to cache the worst case scenario, i.e. if it's able to receive up to 32 segments per message, the queue must be able to hold 32 * friend_queue_size buffers. This would be incredibly wasteful for a small-footprint implementation like Zephyr. The curious thing is that there's a single test case for this in the test spec (the one referenced in #18090), however that one only uses unsegmented messages, meaning we get no hints from the test spec on how to interpret the spec.

Since we don't need to take the "wasteful" interpretation to pass qualification (at least currently) this PR takes the approach that the Friend Queue Size indicates the number of buffers (Network PDUs) the Friend Queue can store. When discarding buffers to make space for new PDUs we do go and make sure to discard the entire message, i.e. all segments of it.

@trond-snekvik how is this implemented in Nordic's Mesh SDK?

@jhedberg jhedberg force-pushed the friend_queue_limit branch from a8449b8 to 6cad931 Compare August 19, 2019 09:31
@trond-snekvik
Copy link
Contributor

In the Nordic Mesh SDK, we use the number of buffers as queue size, with no buffer sharing trickery. We attach a single byte TID to each buffer, and assign the same TID to all segments in the same SAR message. When making room for new buffers, we keep deleting the oldest buffer until we encounter a new TID.

@jhedberg
Copy link
Member Author

jhedberg commented Aug 19, 2019

In the Nordic Mesh SDK, we use the number of buffers as queue size, with no buffer sharing trickery.

@trond-snekvik I might go for this in the long run as well. It has the downside of consuming more memory (the net_buf_pool context per supported LPN), but it'll likely make the code simpler.

As for TID, we're using source address + SeqAuth, which consumes more memory but that's what the transport layer knows about. If we went for a shorter TID then friend.c would need to return this to transport.c.

Johan Hedberg added 2 commits August 19, 2019 13:56
When sent solely to the Friend Queue the send callbacks were not
getting called for unsegmented messages.

Signed-off-by: Johan Hedberg <[email protected]>
The bt_mesh_trans_resend() function had no users, and had in fact not
even a prototype in a header file. Just remove it.

Signed-off-by: Johan Hedberg <[email protected]>
@jhedberg jhedberg force-pushed the friend_queue_limit branch from 6cad931 to ea78278 Compare August 19, 2019 10:56
@trond-snekvik
Copy link
Contributor

As for TID, we're using source address + SeqAuth, which consumes more memory but that's what the transport layer knows about. If we went for a shorter TID then friend.c would need to return this to transport.c.

That works too. Our TIDs are just sequential numbers, since we only really need this info for deletion. We could have used a flag, really, but as far as I can remember, we wouldn't save any space by doing so. We can pick out the acks from the queue by looking at the buffer contents, as buffers are queued up unencrypted (looks like this is not the case in Zephyr. I think that is problematic, but I'll go through the code and create a separate issue).

@jhedberg jhedberg force-pushed the friend_queue_limit branch from ea78278 to 338c1b7 Compare August 19, 2019 11:37
Copy link
Contributor

@MariuszSkamra MariuszSkamra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the mesh friend related test cases pass with this PR

Qualification test case MESH/NODE/FRND/FN/BV-08-C requires that we do
not store more messages than the reported Friend Queue size. The
implementation was so far opportunistic and stored more if it could
(it would later discard if necessary to make sure all queues can store
the required amount). The spec also requires the queues to have new
messages overwrite old ones (in the style of a circular buffer), so we
have to keep track of which buffers are part of the same segmented
message (so we discard all buffers belonging to the same message).

To pass the test case, add APIs to check for space in the Friend
queue, and track the number of buffers for each incoming segmented
message.

Fixes zephyrproject-rtos#18090

Signed-off-by: Johan Hedberg <[email protected]>
@jhedberg jhedberg force-pushed the friend_queue_limit branch from 338c1b7 to 8044208 Compare August 19, 2019 12:32
@jhedberg jhedberg merged commit 8aba96b into zephyrproject-rtos:master Aug 19, 2019
@jhedberg jhedberg deleted the friend_queue_limit branch August 19, 2019 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Bluetooth Mesh area: Bluetooth Qualification Bluetooth Qualification -related issues and pull requests area: Bluetooth Backport Backport PR and backport failure issues bug The issue is a bug, or the PR is fixing a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[zephyr 1.14][MESH/NODE/FRND/FN/BV-08-C] Mesh Friend queues more messages than indicates it's Friend Cache

3 participants