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

Update embedded svc #84

Merged
merged 6 commits into from
Nov 30, 2022
Merged

Update embedded svc #84

merged 6 commits into from
Nov 30, 2022

Conversation

ImUrX
Copy link
Contributor

@ImUrX ImUrX commented Nov 22, 2022

This is based on the #82 branch, so until that's merged this is blocked.
I only implemented the Wifi trait on the Wifi struct, but where should I implement the Interface trait. This contains the actual IP info, should I directly implement it into Wifi or should we change places into Network, what should we do with the poll_dhcp function then in Wifi

Closes #83

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 22, 2022

Thanks for your contribution! I thought about doing this, once things upstream settle a bit but great you took this over.

Regarding their Interface trait - I guess it logically fits Network better than Wifi so implementing it on Network makes sense

I guess we can just keep poll_dhcp in the impl of Wifi or is there something I'm overlooking?

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 22, 2022

Yeah I thought of dhcp because the Wifi trait in svc doesn't mean it has osi 2 support. That's what the other trait is for. But keeping it where it is is also ok.

Btw forgot to update examples, reminding myself.

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 22, 2022

@bjoernQ Done.
I tried running clippy but wow, the codebase is filled with warnings. That should be fixed in another PR, and maybe add a CI check.

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 22, 2022

@bjoernQ Done. I tried running clippy but wow, the codebase is filled with warnings. That should be fixed in another PR, and maybe add a CI check.

Not unexpected ... but yeah definitely a good thing to do

There is one thing to keep in mind and why I am very cautious when even changing small things: we still see miscompilations for Xtensa which are sometimes triggerd by something like moving a static to a different file .... things got much, much better but still. Just mentioning - not to apologize but to explain

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 22, 2022

clippy warnings are mostly things like needing to do a let if instead of checking if a result is_ok() or doing casting when it's not needed. There were some errors in unsafe code not being wrapped in an unsafe {} which seem important if we care about rust safety

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 22, 2022

@bjoernQ I'm getting a broken pipe in Linux when doing cargo espflash flash --monitor --features wifi,esp32c3,embedded-svc --example dhcp and it seems to end up in some kind of boot loop because the JTAG appears and disappears

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 22, 2022

I also tested this in main branch and it still happens :/, can you see if the same happens to you? I suspect its somewhere in init_heap() or initialize()

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 23, 2022

I tested this on all targets via USB-to-UART bridge (on the dev-board) and it worked well (besides the coex example which needs to get adapted)

BUT I also tried on the ESP32-C3-DevKit-RUST-1 (which you probably also use) and see the same problem (even more problems since I'm on Windows and have trouble to get it to even flash)

I have no idea what could cause this but at least I'm pretty sure it gets until initialize - it doesn't make sense to me to be honest. I'm also almost sure it worked at some point earlier

@jessebraham do you remember testing it on ESP32-C3-DevKit-RUST-1 earlier?

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 23, 2022

Seems I found a commit that works for me on the DevKit-RUST-1: 7bd2487

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 23, 2022

Created issue #85 for this

IMHO we could still merge this once the coex example is fixed

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 23, 2022

The ESP32-C3-DevKit-RUST-1 issue should be fixed with #86 - maybe you can test that on your side

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 23, 2022

rebased because im using my branch currently lol

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 23, 2022

@bjoernQ I'm unable to get an IP address from the DHCP now, I also tested in the main branch. I tested with a fortigate router and an iOS hotspot.
Tried using wifi_logs but got an exception while on a wifi scan

INFO - V (0) wifi:
INFO - get_scan_id=0
INFO - 

Exception 'Store/AMO access fault' mepc=4202617e, mtval=0
TrapFrame { ra: 42026172, t0: 3c0cd7c0, t1: 400015c0, t2: 3, t3: fff80000, t4: 400000, t5: 3c0cd7c0, t6: 0, a0: 0, a1: 4, a2: 90c, a3: 4, a4: 3c0c18f0, a5: 4200dde2, a6: 3fc98f48, a7: fffffff, s0: 3fc8a950, s1: 0, s2: 8, s3: 8a4, s4: 0, s5: 3fcdf930, s6: 3fce0000, s7: fff00000, s8: 60034000, s9: 60033000, s10: 3fce0000, s11: 0, gp: 0, tp: 0, sp: 3fc8a8d0, pc: 4202617e, mstatus: 1881, mcause: 7, mtval: 0 }
0x4200c5c8
0x4200c5c8 - malloc
    at /home/uri/proyects/esp-wifi/src/wifi/os_adapter.rs:844
0x4200ddf6
0x4200ddf6 - esp_wifi::wifi::os_adapter::wifi_malloc
    at /home/uri/proyects/esp-wifi/src/wifi/os_adapter.rs:1636
0x40040ccc
0x40040ccc - r_lld_ext_adv_dynamic_aux_pti_process
    at ??:??

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 24, 2022

but main still works? strange - I double checked and it still works for me on your branch (with and without wifi_logs)

from the stack trace it seems like it dies when allocating memory - you can try to increase HEAP_SIZE in lib.rs but it also might be just a red herring - maybe worth to try commenting out the wifi scan and try again with wifi_logs

my Rust compiler version is rustc 1.66.0-nightly (81f391930 2022-10-09)

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 24, 2022

Yeah code works, but DHCP never ends so I get stuck in there waiting for an IP.

I will try what you just said. Still, do you know why DHCP never gets resolved, or does it work in your side?

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 24, 2022

It completely works on my side (only tested ESP32-C3 today) - I remember there were some issues with DHCP in the past which should be resolved

There is also the dump_packets feature which should show if DHCP packets (or all packets in general) are coming in / out

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 27, 2022

So, I have tried many things, for some reason when I have wifi_logs it gets stuck after noticing the auth is not none.
With only dump_packets it starts making broadcasts but it never gets answered, I didnt see anything in the AP's side.
I also tried commenting out the wifi scan but wifi_logs still gets stuck on the same part.
I tried using an esp-wroom-32 and it works, it's beautiful haha. I truly dont know whats up with this.
My current esp32c3 is a Wemos Lolin C3 Mini v1.0.0, dont know if that gives you any info.
EDIT: Scan works btw

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 29, 2022

as mentioned in #88 we had similar problems before which were related to wrong timestamp calculations which shouldn't be the case now (unfortunately I was never able to reproduce it with any of my APs)

Important thing to know: which was the last commit which worked for you? No need to git-bisect - just a commit that works would be a good hint

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 29, 2022

Can we merge this meanwhile?

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 29, 2022

Can we merge this meanwhile?

Generally yes - but the CoEx example needs to get fixed

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 29, 2022

oops

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 30, 2022

I don't want to be annoying but unfortunately the coex example doesn't compile yet

❯ cargo "+nightly" run --example coex --release --target riscv32imc-unknown-none-elf --features "esp32c3,embedded-svc,wifi
,ble"
   Compiling esp32c3-hal v0.3.0
   Compiling esp-wifi v0.1.0 (C:\projects\review\ImUrX\esp-wifi)
error[E0382]: use of moved value: `wifi_interface`
   --> examples\coex.rs:163:32
    |
73  |     let mut wifi_interface = esp_wifi::wifi_interface::Wifi::new(ethernet);
    |         ------------------ move occurs because `wifi_interface` has type `esp_wifi::wifi_interface::Wifi<'_>`, which does not implement the `Copy` trait
...
128 |     let network = Network::new(wifi_interface, current_millis);
    |                                -------------- value moved here
...
163 |     let network = Network::new(wifi_interface, current_millis);
    |                                ^^^^^^^^^^^^^^ value used here after move

For more information about this error, try `rustc --explain E0382`.
error: could not compile `esp-wifi` due to previous error
err process exited unsuccessfully: exit code: 101

Deleting line 163 should fix that

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 bjoernQ merged commit fe6fcfd into esp-rs:main Nov 30, 2022
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.

update embedded-svc
2 participants