Enabling MTU work-around for ESP32-S3

[lure23]

Doing two things:

• moving the comment about the MTU issue to where the choice currently happens
• changing ESP32-S3 to use the workaround

Three questions (before merge!!):

1. Should I introduce if_cfg::if_cfg! as a dependency, just for examples/esp32? Would make the work-around condition DRY (now repeated).
2. Is the switching from /// (doc comments) to normal // ok? Wanted to leave the details out from docs.
3. What to do with "ESP32, -S2"? Should the work-around be enabled for them as well, just in case? // I can vouch for C3 not needing the work-around

[sssilver]

Why not forego all the complexities and just settle on 255? What do, say, ESP32-C3 users lose, when we default MTU to 255 for them?

[jamessizeland]

With a quick check on the latest main, 255 working for both C3 and C6 on stable and S3 on esp nightly, though we need to re-add the following to .cargo/config.toml for the S3 to work.

[unstable]
build-std = ["alloc", "core"]

[lure23]

@sssilver

Why not forego all the complexities and just settle on 255? What do, say, ESP32-C3 users lose, when we default MTU to <255 for them?

We could. It's a matter of how closely the code needs to follow what we know. The core of it is, however, to "just" be a temporary work-around, until esp-rs/esp-hal#2984 makes it to the esp-hal codebase. At that time, any values should be fine.

I'm behind playing this "safe" (not writing any assumptions into code; rather waiting for bug reports like we did here), but "safe" for the codebase may be "annoying" for a newcomer, who's not coming in with the mindset of testing TrouBLE on the particular MCU type.

I'd like the main TrouBLE maintainers (@lulf @jamessizeland ) to chime in, which way you'd like to see the PR. I'll follow.

Then, I suggest we

• create an issue to follow the completion (eventually) of above mentioned esp-hal issue. So the work-around doesn't last beyond when it's no longer needed.

I am still wishing for input on:

1. introduction or avoidance of cfg_if?
2. dealing with /// vs //; minor

@jamessizeland

With a quick check on the latest main, 255 working for both C3 and C6 on stable and S3 on esp nightly, though we need to re-add the following to .cargo/config.toml for the S3 to work.

[unstable]
build-std = ["alloc", "core"]

Hmm... is it written down, when one would need nightly? I'm only running TrouBLE on stable, because I can (targeting -C3 and -C6 only). If S3 needs nightly (does it?) and the above (does it?), would you write a comment, placed above [unstable] so its purpose remains clear? I envision one day also Xtensa would no longer need it?

[jamessizeland]

Nightly is needed for Xtensa chips because it needs the special esp channel until llvm upstreams support for Rust Xtensa support. Hopefully that's not forever but it's not too much of a hardship as it is with espup.

It's on my to do list to add a readme in examples/esp/

[lure23]

Nightly is needed for Xtensa chips because it needs the special esp channel until llvm upstreams support for Rust Xtensa support. Hopefully that's not forever but it's not too much of a hardship as it is with espup.

I should have known that. In January, I seem to have suggested removing those lines (which passed).

Will bring them back. Tangential to this PR though (S3 being the common factor).

--
Brought. See here. @jamessizeland paraphrased your comment

[jamessizeland]

For my opinion, I like that you have detailed the acceptable MTU ranges (possibly worth putting a date/esp-wifi version on that comment as the underlying esp-wifi bits might change the acceptable ranges in future, we can't control that). Thanks for investigating!!

The comment is in the right place now with the MTU, in my opinion.

I'd keep it as /// to make it clear it's a comment associated with that specific variable.

I'm inclined to say let's unify to 255 if they all work with that. I feel like that wasn't the case before?

[lure23]

I squashed the commits, but would still edit the comment to be:

/// Size of L2CAP packets
///
/// 'esp-hal' likely has a bug, causing "Invalid HCI Command Parameters" BLE errors at launch,
/// if the L2CAP MTU is set low enough. In short, 'esp-hal' uses a fixed value of 255, regardless
/// of what TrouBLE would be asking. Thus, it's safest to ask for 255.
///
/// The error producing ranges go:
///   - ESP32-C3: x..<251             // https://github.com/esp-rs/esp-hal/issues/2984#issuecomment-2598810371
///   - ESP32-C6: x..<255             // examples with 128, 251 would fail
///   - ESP32-S3: 251 (presumably x..<255), see [2]:
///       [2]: https://matrix.to/#/#esp-rs:matrix.org/$ZzC-QWHocidnEXtn5vAxcsGnUDLTnk4NGf9Cr7kVrjo
///   - ESP32:    RANGE NOT KNOWN     // not having the hardware
///   - ESP32-C2: RANGE NOT KNOWN     // not having the hardware
///   - ESP32-H2: RANGE NOT KNOWN     // not having the hardware
///   - ESP32-22: RANGE NOT KNOWN     // not having the hardware
///
/// Note: ESP32, -C3, -S2, -S3 were considered not to be affected [1], but actual build (on -S3) showed otherwise,
///       so using a presumably safe value everywhere:
///       [1]: https://github.com/embassy-rs/trouble/pull/236#issuecomment-2586457641
///
pub const L2CAP_MTU: usize = 255;

[lulf]

/test

[lure23]

Tests pass.

@lulf Should I edit the comment? You've been aside from this so would like to hear your feeling about it. It is meant to be temporary - we just don't know how temporary that'll be..

Also worth noting: what's here is just an example for people. It won't help them crash their apps if they use erroneous values themselves. I don't think we should baby-sit that much either, especially when the issue is not fully understood / might be affected by library versions.

[lure23]

@jamessizeland wrote:

[...] possibly worth putting a date/esp-wifi version on that comment as the underlying esp-wifi bits might change the acceptable ranges in future, we can't control that.

I condered this - thanks for suggesting. The problem is that we don't have access to all the hardware, to test. Therefore, any claim in the comment would have different dates/versions for each of the MCU types. That feels overkill. I'd go with git blame on this, gaining a rough date when a test might have happened (and by whom).

[lulf]

I think this is fine to merge, thanks!