Skip to content
This repository has been archived by the owner on Oct 27, 2022. It is now read-only.

[toble] fix softdevice config to mitigate MTU exchange error #42

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hyungsin
Copy link

@Hyungsin Hyungsin commented Nov 7, 2019

@turon, Thread allows MTU size up to 247 bytes, resulting in error during the MTU exchange procedure (https://b.corp.google.com/issues/142724131). Fixing two parameters resolve the problem.

Copy link
Contributor

@turon turon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Just for reference, MTU=247 lets the full MTU fit in a single packet when BLE 4.1 DLE (Data Length Extensions) are enabled.

@jaylogue
Copy link
Contributor

jaylogue commented Nov 7, 2019

Help me understand the motivation for this change. issues/142724131 seems to suggest that this works around a bug in Nordic's SoftDevice. However I'm unclear why ToBLE can't handle a connection MTU set larger than 247. I would expect ToBLE to work the same way WoBLE does and be completely agnostic to the MTU.

Also, this seems like a fragile change. If 247 is somehow a magic number, its unclear how application developers would know this.

@Hyungsin
Copy link
Author

Hyungsin commented Nov 7, 2019

@jaylogue, this simply comes from a value mismatch. 247 is the value used in ToBLE code.

ToBLE wants to set MTU size to 247 bytes, which is OT_BLE_ATT_MTU_MAX (https://github.com/openthread/ot-nest-nordic/blob/master-ble-cert/include/openthread/platform/ble.h#L192). But this lock app configures the softdevice to set MTU size to 251, which causes error at the first attempt of MTU exchange.

@turon
Copy link
Contributor

turon commented Nov 7, 2019

I agree with Jay that ToBLE should be agnostic to MTU size. Can we resolve the conflict by making sure ToBLE and WoBLE are configuring the MTU to the same value through a single constant that is passed down rather than two constants that are manually set to the same value?

We may also want to extend ToBLE setup to skip mtu negotiation if MTU >= to the minimum ToBLE requires for good performance.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants