Skip to content

Smoltcp 0.9 upgrade #124

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

Merged
merged 10 commits into from
Feb 9, 2023
Merged

Smoltcp 0.9 upgrade #124

merged 10 commits into from
Feb 9, 2023

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Feb 8, 2023

Was a bigger job than I first anticipated 😅.

  • Adapt to new smoltcp::Device changes
  • Merges Network and Wifi to create WifiStack
  • Update examples
  • Use heapless instead of hand rolled queue
  • fmt the entire project

Closes #123

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 9, 2023

Thanks a lot - this looks very good! Also, good you replaced that Queue implementation since we depend on Heapless already

Code looks fine but I have no idea why the re-format changed so much - I thought I always do a format 🤷‍♂️

When testing I found a few things (which I cannot mark in the diff)

  • The ESP32 BLE example crashed for me because BT_INTERNAL_QUEUE was full when trying to enqueue data - increasing it to 10 in btdm.rs made it work

A couple of "unused Result" lints because of the new queue

warning: unused `Result` that must be used
   --> esp-wifi\src\esp_now\mod.rs:490:9
    |
490 | /         queue.enqueue(ReceivedData {
491 | |             len: slice.len() as u8,
492 | |             data: data,
493 | |             info,
494 | |         });
    | |__________^
    |
    = note: this `Result` may be an `Err` variant, which should be handled
    = note: `#[warn(unused_must_use)]` on by default

warning: unused `Result` that must be used
    --> esp-wifi\src\ble\npl.rs:1229:9
     |
1229 | /         queue.enqueue(ReceivedPacket {
1230 | |             len: (len + 3) as u8,
1231 | |             data,
1232 | |         });
     | |__________^
     |
     = note: this `Result` may be an `Err` variant, which should be handled

warning: unused `Result` that must be used
    --> esp-wifi\src\ble\npl.rs:1250:9
     |
1250 | /         queue.enqueue(ReceivedPacket {
1251 | |             len: (len + 1) as u8,
1252 | |             data,
1253 | |         });
     | |__________^
     |

But a real problem is, the static_ip example doesn't work anymore. Seems like https://github.com/esp-rs/esp-wifi/blob/5c66d0e15577c1f5d7b909701e2874ea71d02eb2/esp-wifi/src/wifi_interface.rs#L448-L464 got lost

@MabezDev
Copy link
Member Author

MabezDev commented Feb 9, 2023

The ESP32 BLE example crashed for me because BT_INTERNAL_QUEUE was full when trying to enqueue data - increasing it to 10 in btdm.rs made it work

I imagine this was silently failing, as the bool from enqueue was not checked, so I guess its good we now check it :D.

But a real problem is, the static_ip example doesn't work anymore. Seems like

Thanks! This got lost in the refactor. I forgot to mention in the top level PR, but work() on the sockets now just defer to the stack work function, reducing the amount of code doing the same thing, but I guess I borked it a little bit :D. Should be fixed now.

@MabezDev
Copy link
Member Author

MabezDev commented Feb 9, 2023

Code looks fine but I have no idea why the re-format changed so much - I thought I always do a format man_shrugging

This might be because I format with nightly, maybe there are changes in the way it formats in newer versions? 🤔

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 9, 2023

The ESP32 BLE example crashed for me because BT_INTERNAL_QUEUE was full when trying to enqueue data - increasing it to 10 in btdm.rs made it work

I imagine this was silently failing, as the bool from enqueue was not checked, so I guess its good we now check it :D.

Yes, I think it was like that - I think that old Queue was one of my earliest creations in Rust and got copied from project to project for way too long 🤣

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 9, 2023

Now the static_ip example crashes with

!! A panic occured in 'esp-wifi\src\wifi_interface.rs', at line 171, column 29

PanicInfo {
    payload: Any { .. },
    message: Some(
        index out of bounds: the len is 0 but the index is 0,
    ),
    location: Location {
        file: "esp-wifi\\src\\wifi_interface.rs",
        line: 171,
        col: 29,
    },
    can_unwind: true,
}

I think we lost this (storage for ip_addr and also multicast_storage) https://github.com/esp-rs/esp-wifi/blob/5c66d0e15577c1f5d7b909701e2874ea71d02eb2/esp-wifi/src/wifi/utils.rs#L21-L23

Probably multicasts also won't work because of this anymore but we don't have an example for it, currently

I haven't seen how this is done in 0.9.x now 🤷‍♂️

@MabezDev
Copy link
Member Author

MabezDev commented Feb 9, 2023

Okay, now it should work - I actually tested it properly now :D. In smoltcp 0.9 there are defaults: https://github.com/smoltcp-rs/smoltcp/blob/9027825c16c9c3fbadb7663e56d64b590fc95d5a/src/lib.rs#L141 so multicast should still work.

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.

Thanks for taking care of this

@MabezDev MabezDev merged commit e0bdc18 into main Feb 9, 2023
MabezDev added a commit to MabezDev/esp-wifi that referenced this pull request Feb 12, 2023
* switch to heapless for SimpleQueue

* Merge Network into WifiStack

- Merge the Network and Wifi structs into WifiStack
- Get it building & running with just TCP socket for now

* get dhcp example building with changes and no warnings

* fixup error checking

* Fix other examples

* fmt

* Remove redundant comment

* fix unused results

* bump BT_INTERNAL_QUEUE to 10

* add back static ip configuration in work()
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.

Upgrade to smoltcp 0.9
2 participants