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 DHCP example #905

Merged
merged 3 commits into from
Feb 10, 2024
Merged

Conversation

jlogan03
Copy link
Contributor

@jlogan03 jlogan03 commented Feb 4, 2024

Fixes #783

  • Initialize interface with an unspecified (but populated) IP address so that there is something in place to mutate later
  • Update set_ipv4_addr to clear existing addresses and push a new one, and add a small docstring

Copy link

codecov bot commented Feb 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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
@@           Coverage Diff           @@
##             main     #905   +/-   ##
=======================================
  Coverage   79.92%   79.92%           
=======================================
  Files          80       80           
  Lines       28234    28234           
=======================================
  Hits        22566    22566           
  Misses       5668     5668           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jlogan03 jlogan03 marked this pull request as draft February 4, 2024 16:46
@jlogan03
Copy link
Contributor Author

jlogan03 commented Feb 4, 2024

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.

@jlogan03 jlogan03 marked this pull request as ready for review February 5, 2024 23:25
@jlogan03 jlogan03 marked this pull request as draft February 6, 2024 03:42
@jlogan03 jlogan03 marked this pull request as ready for review February 9, 2024 22:51
@Dirbaio
Copy link
Member

Dirbaio commented Feb 9, 2024

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
Copy link
Contributor Author

jlogan03 commented Feb 9, 2024

@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
Copy link
Member

Dirbaio commented Feb 9, 2024

Seems the cause is sunfishcode/is-terminal#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
Copy link
Contributor Author

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

@Dirbaio
Copy link
Member

Dirbaio commented Feb 10, 2024

have you seen this comment though? #905 (comment)

@jlogan03
Copy link
Contributor Author

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.

Copy link
Member

@Dirbaio Dirbaio left a comment

Choose a reason for hiding this comment

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

thanks!

@Dirbaio Dirbaio added this pull request to the merge queue Feb 10, 2024
Merged via the queue into smoltcp-rs:main with commit ca909a2 Feb 10, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

examples/dhcp_client.rs is misleading/wrong
2 participants