-
Notifications
You must be signed in to change notification settings - Fork 447
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
Move device-specific parts of EthernetInterface
to new EthernetDevice
type
#49
Conversation
cce219e
to
0415463
Compare
You'll need to motivate these changes significantly better, because so far I don't understand the rationale for most of them.
It's passed around a lot, and it seems like a completely pointless structure to copy.
So... why do you want that? |
See embed-rs/net#1 (comment) for some reasons why we don't like the device trait in its current form. This change would allow us to reuse most parts of smoltcp (e.g. the TCP state machine, packet construction and processing) with our own device API. I also think that the separation makes sense: The |
I have a few issues with this approach. I've mentioned before that I really don't like the Device trait as it exists before, right? Well, I've just figured out how I want to do it. Beyond that, I think that the way you've done the split doesn't result in an interface that makes sense in the context of design choices of smoltcp.
In the future, please discuss such changes with me first, this will save both of us time and effort. In a nutshell, the new design for the Device trait that I have in mind is presented below: mod phy {
pub trait Device<'a> {
type RxToken: RxToken;
type TxToken: TxToken;
fn receive(&'a mut self) -> Option<(Self::RxToken, Self::TxToken)>;
fn transmit(&'a mut self) -> Option<Self::TxToken>;
}
pub trait RxToken {
fn consume<R, F: FnOnce(&[u8]) -> R>(self, f: F) -> R;
}
pub trait TxToken {
fn consume<R, F: FnOnce(&mut [u8]) -> R>(self, len: usize, f: F) -> R;
}
}
struct StmPhy {
rx_buffer: [u8; 1536],
tx_buffer: [u8; 1536],
}
impl<'a> StmPhy {
fn new() -> StmPhy {
StmPhy {
rx_buffer: [0; 1536],
tx_buffer: [0; 1536],
}
}
}
impl<'a> phy::Device<'a> for StmPhy {
type RxToken = StmPhyRxToken<'a>;
type TxToken = StmPhyTxToken<'a>;
fn receive(&'a mut self) -> Option<(Self::RxToken, Self::TxToken)> {
Some((StmPhyRxToken(&mut self.rx_buffer[..]),
StmPhyTxToken(&mut self.tx_buffer[..])))
}
fn transmit(&'a mut self) -> Option<Self::TxToken> {
Some(StmPhyTxToken(&mut self.tx_buffer[..]))
}
}
struct StmPhyRxToken<'a>(&'a [u8]);
impl<'a> phy::RxToken for StmPhyRxToken<'a> {
fn consume<R, F: FnOnce(&[u8]) -> R>(self, f: F) -> R {
let ret = f(self.0);
println!("rx called");
ret
}
}
struct StmPhyTxToken<'a>(&'a mut [u8]);
impl<'a> phy::TxToken for StmPhyTxToken<'a> {
fn consume<R, F: FnOnce(&mut [u8]) -> R>(self, len: usize, f: F) -> R {
let ret = f(&mut self.0[..len]);
println!("tx called {}", len);
ret
}
}
fn main() {
use phy::{Device, TxToken, RxToken};
let mut phy = StmPhy::new();
if let Some(tx_token) = phy.transmit() {
tx_token.consume(40, |buf| {
println!("got tx buf len {}", buf.len());
});
}
if let Some((rx_token, tx_token)) = phy.receive() {
rx_token.consume(|buf| {
println!("got rx buf");
});
tx_token.consume(80, |buf| {
println!("got tx buf len {}", buf.len());
});
}
} This has some desirable properties:
|
Well, sometimes it's difficult to anticipate how certain changes would work out without implementing them. I already tried to change smoltcp to a closure based API in this branch. It worked out very well, but it relies on the assumption that it's always possible to cleanly separate the TX and RX halves of a device. For our use case, this assumption was fine, but I assumed that it wouldn't be for smoltcp. Therefore I tried to come of with a smaller set of changes that would allow us to implement such an API externally. Your new design exploits the same idea (separating the TX and RX halves), so I'm really happy about it. I think it would solve most of our problems. |
Yes, but I've already considered, if briefly, most of the challenges smoltcp faces. It's not really documented anywhere, though, hence, discussion. |
Oh, and please create a meta-issue like the redox people did for whatever it is you want to use smoltcp for. It's handy for me to have a bird's eye view. |
@phil-opp Do you think you can take the abovementioned outline and implement this yourself? I'm fairly busy right now. |
Sure, I will give it a try. |
Should we coordinate this effort with #55? It would be great if the new |
@whitequark Sure, I just think we should keep the issues in sync. |
Superseded by #57 I think. |
Example taken from smoltcp-rs#49 (comment)
Example taken from smoltcp-rs#49 (comment)
Example taken from smoltcp-rs#49 (comment)
Also:
write_into_buffer
and aPacket::required_buffer_size
methodEthernetDevice::dispatch
methodwrite_into_buffer
methodlookup_hardware_addr
no longer sends ARP request packets. Instead it returns a newUnknownEthernetAddress
error, which is caught by theEthernetDevice::dispatch
method, which in turn sends an ARP request.Packet
andprocess_frame
(namedprocess_ethernet
before) publicChecksumCapabilities
Copy (I see no reason why it shouldn't be Copy)The goal is to make
EthernetInterface
independent from the underlying device and allow one to use it without implementing thephy::Device
trait. Feedback appreciated!TODO:
EthernetInterface
cc @oli-obk