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

Support coex esp32c6 #300

Closed
wants to merge 5 commits into from
Closed

Support coex esp32c6 #300

wants to merge 5 commits into from

Conversation

TuEmb
Copy link
Contributor

@TuEmb TuEmb commented Oct 23, 2023

Hi @bjoernQ,

I have forked this repo from "esp-rs" and do some workaround to support coex for esp32c6.

Basicially, I tried to run ble and wifi seperately with commands, and It can work:
cargo run --example ble --release --features "ble"
cargo run --example dhcp --release --features "embedded-svc,wifi"

But when I tried to use command: cargo run --example coex --release --features "embedded-svc,wifi,ble"
I got an issue as below:
image

Could you help me review my changes and point me what should I do next ?

Thanks so much

@TuEmb
Copy link
Contributor Author

TuEmb commented Oct 23, 2023

Hi @bjoernQ,

I have added new commit to workaround with coex of ESP32C6. I debugged with enabling coex feature by command:
cargo run --example coex --release --features "embedded-svc,wifi,ble,coex" --> It seems that examples.md is wrong for coex example. I also tried with ESP32C3.

When we trigger interrupt for tasks --> it makes the main function hangs up. I don't know the main reason of it. So I change the priority of Interrupt::SYSTIMER_TARGET0 to Priority2

image

After that, wifi and ble can work with coexist mode. But I has an issue with wifi WARN - no Tx token available

image

Beside that, I just commented 2 functions ble_npl_callout_start and ble_npl_callout_stop in npl.rs file. If not, I also have an issue as blow:
image
The root cause is from ble_npl_callout_init was called over 12 times --> so that, CALLOUTS.iter() can't find the position of None
image

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 24, 2023

Awesome you were able to make progress on this! 🚀

I just commented 2 functions ble_npl_callout_start and ble_npl_callout_stop in npl.rs file

Probably a better solution would be to increase the amount of available CALLOUTS/CALLOUT_TIMERS/CALLOUT_EVENTS in npl.rs - I tried 20. With the function bodies commented out the original ble example didn't work anymore for me

When we trigger interrupt for tasks --> it makes the main function hangs up. I don't know the main reason of it. So I change the priority of Interrupt::SYSTIMER_TARGET0 to Priority2

This one is really suspicious. It seems like it gets stuck in timer/riscv.rs at while unsafe { crate::preempt::FIRST_SWITCH.load(core::sync::atomic::Ordering::Relaxed) } {} - the really scary part is that I can make it work again by just adding info!("here); before and after the loop. I really cannot explain this behavior

A last thing which might be a problem:
In esp-hal we define the RAM as RAM : ORIGIN = 0x40800000 + 32K, LENGTH = 512K - 32K which will set stack-top to 0x40880000 but apparently there are some global symbols used by ROM functions in that region, e.g.

coex_env_ptr = 0x4087ffc4;
coex_pti_tab_ptr = 0x4087ffc0;
coex_schm_env_ptr = 0x4087ffbc;
coexist_funcs = 0x4087ffb8;
g_coa_funcs_p = 0x4087ffb4;
g_coex_param_ptr = 0x4087ffb0;

Unfortunately, that is not the key to make things work

The thing about the while unsafe { crate::preempt::FIRST_SWITCH.load(core::sync::atomic::Ordering::Relaxed) } {} currently concerns me the most since if there is something weird happening there, there might be similar things happening elsewhere without us noticing

* Split wifi state for AP and STA

* Use atomic enum to store state

* Clean up unnecessary unsafe blocks
@TuEmb
Copy link
Contributor Author

TuEmb commented Oct 26, 2023

It seems BLE is not fully work for ESP32C6. I tried to use: cargo run --example ble --release --features "ble"
on main branch: https://github.com/esp-rs/esp-wifi/tree/main

BLE started advertising but can't connect from my phone (I used "nRF Connect" App to scan and connect)
image

I guess BLE now have issue. I tried using WIFI with coex and comment out ble_init() in npl.rs -> It works.
After that, I go deeper by commenting 2 functions ble_npl_callout_start and ble_npl_callout_stop in npl.rs file (It related to the QUEUE in worker_task2() ) -> wifi also works.

Could you confirm on your side about BLE on ESP32C6 ?

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 27, 2023

On the main branch BLE fully works on ESP32-C6. Tested with Windows Bluetooth LE Explorer and nRF Connect on an Android phone

@TuEmb
Copy link
Contributor Author

TuEmb commented Oct 27, 2023

Hi @bjoernQ,

Thanks for you confirmation. I have re-checked, BLE is OK (maybe yesterday I got some issues with my phone).

I have pushed new commit: e0ebed9

  • I removed all my workaround and commented codes.
  • Still keep your workaround for getting stuck with coex --> will investigate later.
    image
  • Increase CALLOUTS in npl.rs file from 12 to 20 and increase the Queue of Task2 from 20 -> 40. The ESP32C6 need more queues to work, I don't know exactly work inside of all lib *.a, so just increase to make it works. I think we should implement a notification when the queue is full.

Now, BLE and WIFI can work in Coex mode. But I still got an issue with WIFI as below picture.
image

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 30, 2023

Awesome! Great achievement! I can run the coex example and even see the BLE advertisements after commenting in

    // println!("{:?}", ble.init());
    // println!("{:?}", ble.cmd_set_le_advertising_parameters());

in the example again!

I think the WARN logs are because at that moment the Bluetooth adapter got priority while the example wants to send data via WiFi - so the TX queue fills up. Once the WiFI adapter gets priority again the code is able to send the data.

The thing that is really confusing me is that we need that workaround of logging "here" before and after that while loop.

@bugadani
Copy link
Contributor

bugadani commented Oct 30, 2023

Even that loop itself shouldn't be necessary IMO - but the main task could trigger a yield_task manually before that so we don't wait for the timer to fire first.

The Tx token issue can be a misconfig or you might be running into the mutex bug we're in the process of fixing in another PR. In general, you should configure a longer tx queue than rx queue.

This PR includes some changes that were already made. Please rebase instead of merge committing changes from main, it makes reviews a bit easier.

This PR will also race with at least one of my other ones. Sorry for that, but I'm digging up the codebas in a shotgun-surgery way. This PR should probably be landed before my refactors.


let current_timestamp = get_systimer_count();
critical_section::with(|_| unsafe {
memory_fence();
for i in 0..TIMERS.len() {
if let Some(ref mut timer) = TIMERS[i] {
if timer.active && current_timestamp >= timer.expire {
if timer.active && (current_timestamp >= timer.expire) {
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 unnecessary

@@ -33,14 +33,14 @@ pub extern "C" fn worker_task3() {

pub extern "C" fn worker_task2() {
loop {
let mut to_run = SimpleQueue::<_, 20>::new();
let mut to_run = SimpleQueue::<_, 40>::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be unnecessary. We should just stop checking timers if this queue fills up, and we should loop back.

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 30, 2023

Even that loop itself shouldn't be necessary IMO - but the main task could trigger a yield_task manually before that so we don't wait for the timer to fire first.

The Tx token issue can be a misconfig or you might be running into the mutex bug we're in the process of fixing in another PR. In general, you should configure a longer tx queue than rx queue.

This PR includes some changes that were already made. Please rebase instead of merge committing changes from main, it makes reviews a bit easier.

This PR will also race with at least one of my other ones. Sorry for that, but I'm digging up the codebas in a shotgun-surgery way. This PR should probably be landed before my refactors.

I also thought of using yield_task - would be nicer. Back when that loop was introduced, we didn't have yield.
However, it doesn't solve the issue apparently. It actually seems to be related to yield since I can make things work when I comment the yield in worker_task1 - which doesn't make much sense to me

@bugadani
Copy link
Contributor

If worker_task1 doesn't run anything yet, not yielding from it means the timer will trigger a yield on timeout. So, there must be some difference between issuing a yield from an IRQ context vs thread context, maybe?

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 30, 2023

Ok I guess I know what is going on - will create a PR tomorrow (hopefully)

@TuEmb
Copy link
Contributor Author

TuEmb commented Nov 1, 2023

Even that loop itself shouldn't be necessary IMO - but the main task could trigger a yield_task manually before that so we don't wait for the timer to fire first.

The Tx token issue can be a misconfig or you might be running into the mutex bug we're in the process of fixing in another PR. In general, you should configure a longer tx queue than rx queue.

This PR includes some changes that were already made. Please rebase instead of merge committing changes from main, it makes reviews a bit easier.

This PR will also race with at least one of my other ones. Sorry for that, but I'm digging up the codebas in a shotgun-surgery way. This PR should probably be landed before my refactors.

Thanks @bugadani,

I have created new PR to be clear from draft PR here.
Could you help me review at #327 ?

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 10, 2023

I guess we can close this in favor of #327

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