smoltcp-rs / smoltcp

a smol tcp/ip stack
BSD Zero Clause License
3.63k stars 402 forks source link

Update DHCP example #905

Closed jlogan03 closed 4 months ago

jlogan03 commented 4 months ago

Fixes #783

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (5eced2a) 79.92% compared to head (1ff5bc4) 79.92%. Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #905 +/- ## ======================================= Coverage 79.92% 79.92% ======================================= Files 80 80 Lines 28234 28234 ======================================= Hits 22566 22566 Misses 5668 5668 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jlogan03 commented 4 months ago

Converting to draft for the moment. During testing, found that while this allows the interface to obtain an IP address, if there are sockets already configured on the interface, they will not update properly unless there was already an IP address in place before they were initialized and before an IP address was obtained via DHCP.

Solution in my case is to initialize the interface with an UNSPECIFIED address and then replace that address with the new one. This is more similar to what the original example did, but with an added step in initialization.

Dirbaio commented 4 months ago

we should not have UNSPECIFIED addrs in the interface, it's not correct. The way to make the interface have no addresses is to make the address vec empty.

while this allows the interface to obtain an IP address, if there are sockets already configured on the interface, they will not update properly

what do you mean with "update"? if there's some issue in the socket logic we should fix that instead of workarounding it with UNSPECIFIED addrs.

jlogan03 commented 4 months ago

@Dirbaio @thvdveld , this is ready for review, but I'm not sure why tests are failing. Able to build & pass clippy locally on stable (1.76) and MSRV without specifying features. The test breakage is not related to these changes.

On main, the test breakage still occurs:

jlogan@jlogan-MS-7C56:~/git/smoltcp$ cargo +1.65.0 test --no-default-features --features default
   Compiling futures-macro v0.3.30
   Compiling regex-automata v0.4.5
   Compiling is-terminal v0.4.11
   Compiling rstest_macros v0.17.0
   Compiling rand_chacha v0.3.1
   Compiling smoltcp v0.11.0 (/home/jlogan/git/smoltcp)
   Compiling idna v0.5.0
   Compiling bitflags v1.3.2
   Compiling managed v0.8.0
   Compiling termcolor v1.4.1
   Compiling futures-timer v3.0.2
   Compiling unicode-width v0.1.11
   Compiling humantime v2.1.0
error[E0432]: unresolved imports `std::os::fd::AsFd`, `std::os::fd::AsRawFd`
  --> /home/jlogan/.cargo/registry/src/github.com-1ecc6299db9ec823/is-terminal-0.4.11/src/lib.rs:40:19
   |
40 | use std::os::fd::{AsFd, AsRawFd};
   |                   ^^^^  ^^^^^^^ no `AsRawFd` in `os::fd`
   |                   |
   |                   no `AsFd` in `os::fd`

error[E0603]: module `fd` is private
  --> /home/jlogan/.cargo/registry/src/github.com-1ecc6299db9ec823/is-terminal-0.4.11/src/lib.rs:40:14
   |
40 | use std::os::fd::{AsFd, AsRawFd};
   |              ^^ private module
   |
note: the module `fd` is defined here

Some errors have detailed explanations: E0432, E0603.
For more information about an error, try `rustc --explain E0432`.
error: could not compile `is-terminal` due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
Dirbaio commented 4 months ago

Seems the cause is https://github.com/sunfishcode/is-terminal/issues/36. Easiest fix is probably to bump our MSRV to 1.66. should be fixed now, that was fast wow.

To reproduce locally, delete Cargo.lock to get upated deps.

jlogan03 commented 4 months ago

Yep, that worked locally - if you want to rerun the failed workflows, it should be fine now

Dirbaio commented 4 months ago

have you seen this comment though? https://github.com/smoltcp-rs/smoltcp/pull/905#issuecomment-1936716913

jlogan03 commented 4 months ago

we should not have UNSPECIFIED addrs in the interface, it's not correct. The way to make the interface have no addresses is to make the address vec empty.

while this allows the interface to obtain an IP address, if there are sockets already configured on the interface, they will not update properly

what do you mean with "update"? if there's some issue in the socket logic we should fix that instead of workarounding it with UNSPECIFIED addrs.

I replaced both usages of setting an UNSPECIFIED address with just clearing the addrs vec, and I can confirm that works at least in my particular application (a bare-metal stm32h743 on ethernet).

I didn't dig too far into why that was happening, and I was casually inferring that the sockets had some state related to the IP address available when they were created. Removing that initial assignment of an UNSPECIFIED address has no effect today, and a UDP socket created between initialization and when a real IP address is acquired works fine. My best guess is that because I'm testing immediately after flashing a chip that is already connected to ethernet, I may have been seeing the dhcp socket get deconfigured by the router due to the link going down, causing an UNSPECIFIED address to be pushed.