Skip to content

Update to embassy-net 0.5 #48

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

Closed
wants to merge 3 commits into from

Conversation

lu-zero
Copy link
Contributor

@lu-zero lu-zero commented Nov 28, 2024

Based on #35

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.

Looks quite OK!
If you could only address the question w.r.t. to_net_addr and to_emb_addr and if indeed not necessary - just get rid of these?

Also, I will (at least for the next couple of days) merge your PR against another branch.
Would you be willing to re-target the PR against the embassy-net-05 branch that I just created for that purpose? Thanks!

@@ -89,11 +89,11 @@ pub(crate) fn to_emb_bind_socket(socket: SocketAddr) -> IpListenEndpoint {
pub(crate) fn to_net_addr(addr: IpAddress) -> IpAddr {
Copy link
Owner

Choose a reason for hiding this comment

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

Is this function even necessary anymore given that now smoltcp (and embassy-net) did switch to core::net for IP addresses?

@@ -102,11 +102,11 @@ pub(crate) fn to_net_addr(addr: IpAddress) -> IpAddr {
pub(crate) fn to_emb_addr(addr: IpAddr) -> IpAddress {
Copy link
Owner

Choose a reason for hiding this comment

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

Ditto.

@lu-zero
Copy link
Contributor Author

lu-zero commented Nov 28, 2024

Done This can be closed and we look at the other PR.

@lu-zero lu-zero closed this Nov 28, 2024
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.

3 participants