Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a bug when setting the size of the send's TX_MTU #256

Merged
merged 5 commits into from
Jan 24, 2025

Conversation

hicklin
Copy link
Contributor

@hicklin hicklin commented Jan 20, 2025

The ch1.send's TX_MTU generic const was set to the PAYLOAD_LEN which is always going to be wrong as the encode method adds 4 to 6 bytes at the start of the payload. This has been updated to L2CAP_MTU; the maximum allowed size.

@lulf
Copy link
Member

lulf commented Jan 20, 2025

The payload len can be bigger than l2cap mtu, and it will be fragmented and reassembled internally. 27 may be a bit confusing though, it should probably be set to a larger number to not be confused with the l2cap mtu.

@lulf
Copy link
Member

lulf commented Jan 20, 2025

There are basically 3 different MTUs here:

  1. The l2cap mtu, which is configured in the HostResources. This is the MTU used when interacting with the controller.
  2. The l2cap connection oriented channel MTU: this is configured in the L2capChannelConfig mtu field. This is the max 'payload size' that can be sent. May be smaller or larger than the l2cap mtu. Also called SDU, and the tx buffer passed to send() should be <= this size.
  3. The l2cap connection oriented channel MPS: this is automatically set to the l2cap mtu internally, and the channel will fragment/reassembly packets according to this.

The first two are documented a bit here https://software-dl.ti.com/lprf/simplelink_cc2640r2_sdk/1.35.00.33/exports/docs/ble5stack/ble_user_guide/html/ble-stack/l2cap.html

Another source https://community.nxp.com/t5/Wireless-MCU/Bluetooth-LE-Credit-Based-Flow-Control-for-L2CAP-Connection/m-p/437369

Edit: @hicklin fixed some inaccuracies where I mixed up MPS and MTU

@lulf
Copy link
Member

lulf commented Jan 21, 2025

@hicklin Maybe an alternative here would be to increase the PAYLOAD_LEN to some larger arbitrary value like 42?

@lulf
Copy link
Member

lulf commented Jan 23, 2025

@hicklin Just fixed some inaccuracies where I mixed up MPS and MTU in my previous comment

@lulf
Copy link
Member

lulf commented Jan 23, 2025

@hicklin Let's merge this for now then, and sorry for the misunderstanding!

@lulf
Copy link
Member

lulf commented Jan 23, 2025

/test

Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

Needs a rebase

@lulf
Copy link
Member

lulf commented Jan 24, 2025

/test

@lulf lulf merged commit a1daee2 into embassy-rs:main Jan 24, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants