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

fix: fix interaction between central and peripheral in nrf #246

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Conversation

lulf
Copy link
Member

@lulf lulf commented Jan 17, 2025

  • Make l2cap_mtu a const generic pass in from application
  • Move esp32-specific logic into esp32 examples

lulf added 2 commits January 17, 2025 15:31
* Make l2cap_mtu a const generic pass in from application
* Move esp32-specific logic into esp32 examples
The new version supports the --artifact-dir flag
@lulf
Copy link
Member Author

lulf commented Jan 17, 2025

/test

2 similar comments
@lulf
Copy link
Member Author

lulf commented Jan 17, 2025

/test

@lulf
Copy link
Member Author

lulf commented Jan 17, 2025

/test

Copy link
Contributor

@lure23 lure23 left a comment

Choose a reason for hiding this comment

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

There's the change of all ESP32 1017 -> 255 that I'd advocate. Otherwise, just see my comments.

// Using a fixed "random" address can be useful for testing. In real scenarios, one would
// use e.g. the MAC 6 byte array as the address (how to get that varies by the platform).
let address: Address = Address::random([0xff, 0x8f, 0x1b, 0x05, 0xe4, 0xff]);
info!("Our address = {:?}", address);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is in an example for central. I thought it was on purpose that the central can have whatever address, whereas the peripheral's needs to be fixed. Does this not cause two nodes, both with the same BLE address? 😐

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it's different from the peripheral's fixed address:

Address::random([0x41, 0x5A, 0xE3, 0x1E, 0x83, 0xE7]);

a) What's the purpose of fixing the central's?
b) It worked before.. so why change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: since it doesn't matter what the bytes are - does it? - how about creating them from some wonky string?

b"centra"
b"periph"
  • more wonky/funny

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I will do it in a follow up pr if that's ok

Copy link
Collaborator

Choose a reason for hiding this comment

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

also while you're doing this how about printing the address in hex with info!("Our address = {:x?}", address);

examples/esp32/src/consts.rs Outdated Show resolved Hide resolved
@lulf
Copy link
Member Author

lulf commented Jan 17, 2025

/test

@lulf
Copy link
Member Author

lulf commented Jan 17, 2025

/test

@lulf lulf merged commit c2dc47b into main Jan 17, 2025
1 check passed
@lulf lulf deleted the fix-addresses branch January 17, 2025 16:17
@jamessizeland
Copy link
Collaborator

I'm getting this error on my c3

panicked at /home/jsizeland/Repos/Ephemera/embassy-trouble/examples/apps/src/ble_bas_peripheral.rs:96:13:
[ble_task] error: BleHost(Hci(Unsupported Feature or Parameter Value))

Backtrace:

0x4200945a
0x4200945a - embassy_executor::raw::util::SyncUnsafeCell<T>::get
    at /home/jsizeland/.cargo/registry/src/index.crates.io-6f17d22bba15001f/embassy-executor-0.7.0/src/raw/util.rs:55
0x4201095c
0x4201095c - main
    at /home/jsizeland/Repos/Ephemera/embassy-trouble/examples/esp32/src/bin/ble_bas_peripheral.rs:14
0x42000130
0x42000130 - hal_main
    at /home/jsizeland/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp-hal-0.23.0/src/lib.rs:511

@lure23
Copy link
Contributor

lure23 commented Jan 17, 2025

@jamessizeland What's the line you see just above that? I think the PR changed what was 251 to 128.

  • what's your C3 devkit?
  • espflash board-info say?

Will test on my C3 and C6, right now.

--

TL;DR: C6 is fine; C3 needs this line to be 251 (as it was; now we know the reason!).

C6:

Chip type:         esp32c6 (revision v0.0)
Crystal frequency: 40 MHz
Flash size:        4MB
Features:          WiFi 6, BT 5
$ cargo esp32c6 --bin ble_bas_peripheral
…
INFO - [host] configuring host buffers (8 packets of size 255)
INFO - [host] initialized
INFO - [adv] advertising

= ok

$ cargo esp32c6 --bin ble_l2cap_peripheral
…
INFO - [host] configuring host buffers (8 packets of size 255)
INFO - [host] initialized

= ok

C3:

Chip type:         esp32c3 (revision v0.4)
Crystal frequency: 40 MHz
Flash size:        4MB
Features:          WiFi, BLE
$ cargo esp32c3 --bin ble_bas_peripheral
…
INFO - [host] configuring host buffers (8 packets of size 128)


====================== PANIC ======================
panicked at /home/ubuntu/trouble.fork/examples/apps/src/ble_bas_peripheral.rs:96:13:
[ble_task] error: BleHost(Hci(Unsupported Feature or Parameter Value))

= panic. Likely would work with the earlier value of 251.

$ cargo esp32c3 --bin ble_l2cap_peripheral
…
INFO - [host] setting txq to 12, fragmenting at 251
INFO - [host] configuring host buffers (8 packets of size 128)

NOTE: It doesn’t state (as C6 does): ”INFO - [host] initialized”.

= …not ok? The same 251 -> 128 reason, perhaps?

So we don’t know what values are appropriate for C3. Let’s see (editing esp32/src/bin/consts.rs):

251		ok!
192		failed	
200		failed
250		failed

Seems the limit for C3 is >= 251, which I didn’t know, before.

For examples/esp32/…/ble_l2cap_peripheral, the same limit:

  • 251 shows ”INFO - [host] initialized”
  • 250 does not

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.

3 participants