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

DHCP improvements. #12

Merged
merged 18 commits into from
Dec 24, 2023
Merged

DHCP improvements. #12

merged 18 commits into from
Dec 24, 2023

Conversation

reitermarkus
Copy link
Contributor

Some small improvements to get this working in the esp-wifi/embassy_access_point example.

I guess some changes are not technically necessary to get it working, since I now figured out that the DHCPOFFER response needs to be sent as a broadcast, at least if the client IP is 0.0.0.0.

@ivmarkov
Copy link
Owner

@reitermarkus Ping me when you are done so that I can start reviewing. Or put the PR in Draft mode until it is ready.

@reitermarkus
Copy link
Contributor Author

@ivmarkov, I think that's it for now.

Copy link
Owner

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

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

Cab you revert the signature of the reply method to the original one? Or explain which of the new parameters are necessary, and why.

@@ -63,7 +63,7 @@ members = [
[workspace.dependencies]
embassy-futures = { version = "0.1", default-features = false }
embassy-sync = { version = "0.3", default-features = false }
embassy-time = { version = "0.1", default-features = false }
Copy link
Owner

Choose a reason for hiding this comment

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

This currently breaks the CI build because feature embedded-svc still needs embassy-time 0.1.

I plan to update embedded-svc and all esp-idf-* crates shortly post Dec 28 though.

edge-dhcp/src/lib.rs Outdated Show resolved Hide resolved
edge-dhcp/src/lib.rs Show resolved Hide resolved
edge-dhcp/src/server.rs Show resolved Hide resolved
@ivmarkov
Copy link
Owner

ivmarkov commented Dec 24, 2023

LGTM. Will merge later today once I'm back. Hope you don't mind if I don't merge the embassy-time upgrade immediately, but in a couple of weeks. Else I need to temporarily remove the embedded-svc feature, which is annoying.

@ivmarkov ivmarkov merged commit 58198e1 into ivmarkov:master Dec 24, 2023
1 check failed
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.

2 participants