~~Rename and~~ split of esp feature based on ESP32 chip types

[lure23]

I was somewhat bothered by the "magical" 1017 MTU limit for "Some ESP[32] chips". Which chips? Where are the details?

So I tested on the two chips I have access to: ESP32-C3 and ESP32-C6. Results:

C6	
	500 ok
	251 PANIC
	300 ok
	260 ok
	255 ok
	254 PANIC

C3
	300 ok
	251 ok
	128 ok

Seems that a) ESP32-C3 doesn't need any special handling in the TrouBLE examples.

b) The limit for ESP32-C6 is >= 255, not 1017.

I don't claim this to be the whole truth. I haven't found any mention of such MTU limits. What I do suggest is renaming the feature and splitting it so that for each chip we run as closed to the (by trial and error) found limit as possible.

Is this okay?

What to do with the other ESP32 chips? In the initial version of the PR I left their behaviour unchanged (safe bet). Actually I'd like to not use MTU limits with each, until someone reports seeing a panic in the examples.

If this approach looks okay-ish, I'd also like to experiment with a better way to define the repeating blocks. Have two ideas about it, but having build.rs create a constant would mean generating a code snippet. Don't know if you are okay with such added complexity (it would clean the example code, though).

[jamessizeland]

Good investigating, I like how much you're asking why on these things!

I wonder if someone at espressif might have a good answer on how (and why) we should be setting lower limits on these based on chip selected, and what the interaction here is with esp-wifi. @bjoernQ perhaps?

[lure23]

@jamessizeland Thanks.

Espressif surely will know. My current guess is it's something within their wifi blob. I don't really understand why there would be a lower limit, at all. Could even be a bug in their blob.

References:

• ESP-IDF Programming Guide > section on MTU

  Says nothing about such limit.

  Note: In these guides, one changes the MCU type from web page top left.

[bjoernQ]

ESP32, ESP32-S2, ESP32-S3 and ESP32-C3 use different Bluetooth drivers than later chips (which are based on Nimble).

For the later chips there is a config ble_acl_buf_size which is currently hardcoded to 255. We currently don't allow the user to change this config. @lure23 would you mind creating an issue asking for a way to configure it in https://github.com/esp-rs/esp-hal ?

[lure23]

@lure23 would you mind creating an issue asking for a way to configure it in

I might. However, thanks to your information I'm able to make a change to TrouBLE that works for now. This fixes my personal itch.

@ALL: I've crafted another approach, where the forcing of 255 happens directly from examples/esp32/Cargo.toml to the trouble-host project, bypassing the examples/apps. This simplifies the apps code and means other platform users don't need to be bothered by the detail. I hope you like this.

Once esp-hal would have a fix, trouble-host can simply make such feature a no-op.

+ I wish to mention a theory I have about the origins of the 1017 magic number. Could it be from Ethernet / IP MTU's? It does fall in that value range (800..1500), whereas L2CAP MTU should be 251 at most. If I understood things right...

[lulf]

@lure23 Would it make more sense maybe to pass the MTU as a const generic to the APP functions? Afaik, this avoid having to use features at all, with only a const generic needing to be added to the app run() fn. The reason it was not passed as a param before was the the host resources had to be in a static, but it does not need to be anymore.

[lure23]

@lure23 Would it make more sense maybe to pass the MTU as a const generic to the APP functions? Afaik, this avoid having to use features at all, with only a const generic needing to be added to the app run() fn. The reason it was not passed as a param before was the the host resources had to be in a static, but it does not need to be anymore.

I don't know exactly what you mean. It's being a const generic today as in here. btw, I tried making a max of the passed value and 255, but it seems "generic parameters may not be used in const operations".

But if I pass the other branch (that works) into this PR, we can discuss that as a proposal. :) If there are structural ways that you guys know of to make the change better, great. Also keeping in mind that @bjoernQ 's comment implies the problem could be solved by esp-hal, at some point. So let's plan for a deprecable fix.

edit Now I get it. Sure, we can let the MCU-specific projects pass the MTU's that way. I don't really like the app/MCU specific arrangement, to begin with; would rather have it the other way round (MCU specific stuff as dependency of the apps; but the build time differences have likely caused the current structure..).

---

We can also do a combined approach: pass the MTU as generic to examples/apps as you suggest, and have it steered by the feature within examples/esp32. This way, the mess (if it's that) would be contained within the esp32 folder - and also vanish therein.

[lulf]

We can also do a combined approach: pass the MTU as generic to examples/apps as you suggest, and have it steered by the feature within examples/esp32. This way, the mess (if it's that) would be contained within the esp32 folder - and also vanish therein.

Yes, this is what I'm proposing.

[lure23]

Yes, this is what I'm proposing.

Would you be okay to check a PR that moves all of this to the MCU-specific side:

    let mut resources = Resources::new(PacketQos::None);

    // Hardcoded peripheral address
    let address: Address = Address::random([0xff, 0x8f, 0x1a, 0x05, 0xe4, 0xff]);
    info!("Our address = {:?}", address);

    let (stack, mut peripheral, _, mut runner) = trouble_host::new(controller, &mut resources)
        .set_random_address(address)
        .build();

Moving the address generation there has the benefit that we could showcase how to make non-fixed (MAC-based) addresses. Something that was mentioned a while back. I'll toy with this and see how it feels.

Anyhow, the outcome will be towards your proposal.

p.s. This does mean trouble-host becomes a dependency of the MCU-specific examples, but I guess that's okay.

[lulf]

Yes, this is what I'm proposing.

Would you be okay to check a PR that moves all of this to the MCU-specific side:

    let mut resources = Resources::new(PacketQos::None);

    // Hardcoded peripheral address
    let address: Address = Address::random([0xff, 0x8f, 0x1a, 0x05, 0xe4, 0xff]);
    info!("Our address = {:?}", address);

    let (stack, mut peripheral, _, mut runner) = trouble_host::new(controller, &mut resources)
        .set_random_address(address)
        .build();

I think it's better if the trouble setup is within the example to make it more self contained. This separates the controller setup which is MCU-specific and the host stack which is MCU independent.

Something like the following type signature is what I'm thinking of:

fn run<C: Controller, const L2CAP_MTU: usize)(controller: C, address: Address) { }

[lulf]

/test

[lulf]

/test

[lure23]

Something like the following type signature is what I'm thinking of:

fn run<C: Controller, const L2CAP_MTU: usize)(controller: C, address: Address) { }

I've tried two approaches now (links below). Neither seems perfect and I'll try to analyse the pros/cons here. Trying to give optional overrides for const generics in Rust is... kind of painful. :) Actually, cannot be done. Also, structs can have default const generics but functions cannot (in stable). All in all, it feels like a rally path. Rocky.

Most importantly, I feel the const values should be kept in one place (examples/apps) and not spread between it and examples/{mcu} - but that cannot be avoided for L2CAP_MTU, because of above.

My suggestion is becoming a hybrid of these two. I would

a) move Address to the MCU-level, but not feed L2CAP_MTU as a const param (leaving it in the examples/apps). There's (a completely orthogonal) benefit of having the Address on the MCU side; we can now show how to pick a non-fixed value for each MCU.

b) follow the earlier work I did, to steer the ESP32-specific overrides in trouble-host itself, yes, with a feature. This is the most transparent (to non-ESP32 examples) approach I can see, and one easy to dismantle once / if esp-hal has a fix for the whole thing.

Let's see the code. // It might be good if you open these to another window. I'll narrate them below.

lure23/trouble.fork@main...lure23:trouble.fork:examples-feature-l2cap-mtu2

This has changes for all the four examples.

• esp feature is removed from examples/apps. Instead, a feature l2cap-mtu-255 is introduced to trouble-host. It steers the value 255 to be used, instead of whatever the L2CAP_MTU const generic. It's a force override. The examples/esp32 adds a dependency to trouble-host (but other MCU examples don't need to); so that it can enable the feature for the specific chip types that actually need it.

Pros:

• all consts are in the examples/apps, which also gets simplified
• removing the hackish feature is easy; only ESP32 affected at that point
• no extra complexity introduced to function APIs
• short diff

Cons:

• still uses a feature
• there's a non-DRY part in trouble-host where the documentation of a function gets duplicated, because of #cfg. Sorry.

lure23/trouble.fork@main...lure23:trouble.fork:examples-feature-l2cap-mtu3

I did this only for the ble_bas_peripheral example, since it's more elaborate.

Pros:

• I don't see any; no features? // the one feature addition relates to Address

Cons:

• More hoopla with generics in the examples/apps; affects all platforms; hinders readability of code
• More work to deprecate the override: L2CAP_MTU would come back to examples/apps side
• Changes to all examples (all platforms) would be needed!

---

I'd like to hear your response. Personally, I'm fine pushing the force-featuring branch into this PR and then crafting a separate one of the Address changes.

[lulf]

My vote goes to the examples-feature-l2cap-mtu3 - I really don't think it is good to have esp32-specific features mixed into the trouble-host crate, it should stay independent of the controller.

For the CI, I have another suggestion which is to use https://doc.rust-lang.org/std/macro.option_env.html instead of features to allow overriding the chip-specific address with a fixed one. Invoked like this:

FIXED_ADDRESS='aa:bb:cc:dd:ee:ff' cargo build --release

[lure23]

My vote goes to the examples-feature-l2cap-mtu3 - I really don't think it is good to have esp32-specific features mixed into the trouble-host crate, it should stay independent of the controller.

I hear you. There's also one more approach, which I've updated this PR itself to show.

• minimize the footprint of the PR (changes only examples/esp32/Cargo.toml)
• brings applying the workaround from all ESP32 chips (as is now) to those where we know/suspect it is needed. This relies on my tests (C3, C6) and @bjoernQ 's comment (other chips)
• adds comments about what we currently know

Once this is merged, I would raise an issue at esp-hal itself and we'll see if the problem gets analysed and ironed out at the root (could be deeper than esp-hal; we don't know).

I chose to leave the 1017 magic numbers to a) limit PR size, b) change behaviour from the current state as little as possible.

The reason I find this better than above discussed options 2 and 3 is that it doesn't add complexity. Both of those did, either to trouble-host or examples/apps. Hope you are fine with the suggested. :)

p.s. Do we know people who'd have ESP32-C2 and ESP32-H2 hardware, willing to test?

[lure23]

/test

[lulf]

/test

[lulf]

My vote goes to the examples-feature-l2cap-mtu3 - I really don't think it is good to have esp32-specific features mixed into the trouble-host crate, it should stay independent of the controller.

I hear you. There's also one more approach, which I've updated this PR itself to show.

* minimize the footprint of the PR (changes only `examples/esp32/Cargo.toml`)

* brings applying the workaround from _all_ ESP32 chips (as is now) to those where we know/suspect it is needed. This relies on my tests (C3, C6) and @bjoernQ 's comment (other chips)

* adds comments about what we currently know

Once this is merged, I would raise an issue at esp-hal itself and we'll see if the problem gets analysed and ironed out at the root (could be deeper than esp-hal; we don't know).

I chose to leave the 1017 magic numbers to a) limit PR size, b) change behaviour from the current state as little as possible.

The reason I find this better than above discussed options 2 and 3 is that it doesn't add complexity. Both of those did, either to trouble-host or examples/apps. Hope you are fine with the suggested. :)

Looks good to me, thanks for taking the time exploring the alternatives here. There are clearly trade-offs, and I agree the current approach looks most attractive right now.

p.s. Do we know people who'd have ESP32-C2 and ESP32-H2 hardware, willing to test?

I do not have any of those (only c3 which I'm planning to get added to the HIL hardware soon).