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

[WIP] Adapt changes to upstream embassy-net #35

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

showier-drastic
Copy link
Contributor

Commit embassy-rs/embassy@be0d977 made a rather large change to lifetime/generics semantics to embassy-net. Update edge-nal-embassy to adapt to it.

Tested to compile, but not tested to run yet.

@ivmarkov
Copy link
Owner

ivmarkov commented Oct 8, 2024

Yes - thank you!

I think we should wait for this PR to get merged as well, as it will bring even more changes to embassy-net (it will switch to the core::net types).

Given that the upstream PR that switches smoltcp to core::net types already got merged, I don't think the embassy-net one would be delayed by a lot, if at all.

@ivmarkov
Copy link
Owner

ivmarkov commented Oct 8, 2024

Also - you need to run cargo fmt

@AnthonyGrondin
Copy link
Contributor

Thank you for taking care of this. I believe we should also wait for embassy-rs/embassy#3368 as it allows implementing proper readable() / writable() for embassy

@showier-drastic
Copy link
Contributor Author

Understood, thanks for letting me know!

@AnthonyGrondin
Copy link
Contributor

Both PRs have been merged, so feel free to continue this PR with the needed changes.

Smoltcp and embassy-net haven't had a release yet, at the time of writing this, but we can get by with using patches, I believe.

@showier-drastic
Copy link
Contributor Author

Both PRs have been merged, so feel free to continue this PR with the needed changes.

Smoltcp and embassy-net haven't had a release yet, at the time of writing this, but we can get by with using patches, I believe.

Nice to hear that, I will update this PR hopefully in a day or two.

@showier-drastic
Copy link
Contributor Author

Hi @AnthonyGrondin ,

It is not clear to me how to implement readable and writable.

For example, for edge_nal_embassy::tcp::TcpSocketRead you only have embassy_net::tcp::TcpReader inside it; however, in order to call wait_read_ready, you need embassy_net::tcp::TcpSocket.

Maybe we should add wait_read_ready to embassy_net::tcp::TcpReader (and the counterpart for Writer & Udp)?

I'd appreciate it if you could give some suggestions.

@AnthonyGrondin
Copy link
Contributor

I'm not sure if it's a hard requirement to implement readable for edge_nal_embassy::tcp::TcpSocketRead at the moment.

As long as it's implemented for TcpSocket and UdpSocket. For edge_nal_embassy::tcp::TcpSocketRead, it might require more changes to be upstreamed into embassy-net.

@ivmarkov Can comment more on that.

@ivmarkov
Copy link
Owner

ivmarkov commented Oct 22, 2024

Yes (updated), this is an omission. TcpReader should also have wait_read_ready, and the other way around, for TcpWriter.

@ivmarkov
Copy link
Owner

And btw same for the UDP socket splits.

@showier-drastic
Copy link
Contributor Author

It seems that we don't have a Writable trait, so there's no writable for writers.

@ivmarkov
Copy link
Owner

It seems that we don't have a Writable trait, so there's no writable for writers.

Yes, but regardless of that having wait_write_ready on TcpWriter still makes a lot of sense, outside of edge-nal-embassy and for the cases when you use embassy-net directly.

@ivmarkov
Copy link
Owner

ivmarkov commented Nov 1, 2024

@showier-drastic this PR just got merged, so if you would like to resume your work here, I think all obstacles should hopefully be cleared by now. :)

Thanks to @AnthonyGrondin

@showier-drastic
Copy link
Contributor Author

Nice to hear, I will work on it tomorrow.

BTW, I want to know when should this PR supposedly be merged? when next embassy-net is released?

@ivmarkov
Copy link
Owner

ivmarkov commented Nov 1, 2024

We can branch the repo and merge it earlier.

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