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

Add support coex for ESP32C6 #327

Merged
merged 3 commits into from
Nov 13, 2023
Merged

Add support coex for ESP32C6 #327

merged 3 commits into from
Nov 13, 2023

Conversation

TuEmb
Copy link
Contributor

@TuEmb TuEmb commented Nov 1, 2023

Hi @bugadani, @bjoernQ,

I created a PR to support Coex for ESP32C6. Those changes were discussed from #300

  • Enabled COEX feature and created new COEX example for ESP32C6.
  • Added big-heap when run COEX mode.
  • Calling G_COEX_FUNCS in COEX mode
  • Increase tx_queue_size to be greater than rx_queue_size

@TuEmb TuEmb mentioned this pull request Nov 1, 2023
@@ -109,7 +109,7 @@ const DEFAULT_TICK_RATE_HZ: u32 = 100;
struct Config {
#[default(5)]
rx_queue_size: usize,
#[default(3)]
#[default(10)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should change the default just to get rid of some warnings

We probably will revise the defaults at some point (e.g. check #233 (review) ) and when we do it, we also need to update the documentation about it

Especially since this is user-configurable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can keep default value as #233. We can change when using because it user-configurable

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 1, 2023

Works great! Thanks a lot for this

I'm not sure about changing the defaults and this needs a cargo fmt - otherwise I'm fine with merging this.

I tested on all supported targets and nothing broke 👍

@MabezDev
Copy link
Member

MabezDev commented Nov 1, 2023

Be sure to remove esp32c6 from this list when you rebase :https://github.com/esp-rs/esp-wifi/blob/7632e74285a70e155a10e8dee4b6dde50d78394f/esp-wifi/build.rs#L22

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 10, 2023

I'd really love to get this merged.

Unfortunately reviewing this is a bit difficult since there is a merge commit in bc3017a (i.e. +3,083 −9,239) - would you mind removing that commit and rebase on main instead?

It also seems that in esp-wifi/lib.rs the default of tx_queue_size: usize, is still changed to 10

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 10, 2023

it also seems it stopped working for me 😢

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 10, 2023

it also seems it stopped working for me 😢

Ah - it works when using the big-heap feature. Maybe something to address in src\lib.rs when defining the DEFAULT_HEAP_SIZE

Got it to work by reverting bc3017a, doing what Scott said in #327 (comment) , reverting the change to the default of tx_queue_size and using big-heap

@TuEmb do you want to do these changes or should I take over if you don't have the time to do this (certainly keeping your original commit)

@TuEmb
Copy link
Contributor Author

TuEmb commented Nov 10, 2023

Thanks for reviewing @bjoernQ ,

Let me do these changes soon 🤝

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks for your contribution!

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 13, 2023

You just need to fix the formatting issue then we can get this in

@TuEmb
Copy link
Contributor Author

TuEmb commented Nov 13, 2023

I updated formatting issue

@bjoernQ bjoernQ merged commit 7ad023a into esp-rs:main Nov 13, 2023
7 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.

3 participants