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

fix: Udp implementation and add more methods #57

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

elpiel
Copy link
Contributor

@elpiel elpiel commented Jul 22, 2023

Split out from #42

  • Fixes the UDP implementation and uses the socket's ring buffers accordingly for better performance.
  • Adds set_port and get_port for setting and getting respectively the socket port

Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
@elpiel elpiel mentioned this pull request Jul 22, 2023
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
@elpiel elpiel changed the title fix: udp implementation and add more methods fix: Udp implementation and add more methods Jul 22, 2023
Copy link
Collaborator

@ryan-summers ryan-summers left a comment

Choose a reason for hiding this comment

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

Looking really nice! Thanks for the changes. Some minor comments/cleanup requests, but it looks really good otherwise.

src/udp.rs Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
Signed-off-by: Lachezar Lechev <elpiel93@gmail.com>
@elpiel elpiel mentioned this pull request Jul 25, 2023
Copy link
Contributor Author

@elpiel elpiel left a comment

Choose a reason for hiding this comment

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

There's definitely a lot of room for improvements, however, this UDP implementation now works and for me that was more important.

You're welcome to improve on the current working implementation.

src/udp.rs Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Outdated Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
src/udp.rs Show resolved Hide resolved
@ryan-summers
Copy link
Collaborator

@elpiel It's not clear to me what this PR is actually fixing in the UDP API outside of adding the new functions and refactoring the code, but you claim this has resolved your issues. What issues does this PR fix? From my review, I would guess #59, but I don't see any changes that would address #46 or #44

@elpiel
Copy link
Contributor Author

elpiel commented Jul 26, 2023

Yes @ryan-summers there are a few things that this PR fixes and improves:

Copy link
Collaborator

@ryan-summers ryan-summers 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 the explanation on what this PR is solving, it helped me understand the purpose of this restructering. After taking a closer look, everything looks to be in order and makes sense to me. Thanks for taking the time to help me understand a bit more, and I appreciate the contribution here a lot!

@ryan-summers
Copy link
Collaborator

ryan-summers commented Jul 27, 2023

@elpiel Would you be able to resolve the merge conflicts? I am happy to merge afterwards :)

@elpiel
Copy link
Contributor Author

elpiel commented Jul 27, 2023

Will do.
Thanks for taking the time to look into it!

Hopefully we can improve the crate not to block while waiting for ARP response, make it async and allow usage of the interrupt pin.

@ryan-summers ryan-summers merged commit a45c33e into kellerkindt:master Jul 27, 2023
4 checks passed
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.

UDP receive incorrectly updates RX pointer When Device.connect is not called, Udp socket is not being opened
2 participants