Device-level packet metadata identifiers

[datdenkikniet]

A generic way for the user to obtain an ID that allows them to ask a Device for information about the frame/packet with which a specific (UDP) packet was sent.

TODO:

• [x] Finalize API
• [x] Update documentation
• [ ] Write an example

HackMD description

[datdenkikniet]

Thank you for the comments @thvdveld ! Left some responses/feedback to what you've said. Will make the changes sometimes this week.

[korken89]

Hey @Dirbaio / @thvdveld - has there been any progress here or more questions/comments?

[Dirbaio]

Sorry for the :snail: response!

• I think the ID should be probably a u32, so that it doesn't change between 32bit and 64bit platforms.
• Ideally, the feature shouldn't add bloat for those who don't need it. Could you gate it behind a Cargo feature? Perhaps define PacketId to be () when not enabled, and u32 when enabled. I hope this trick works, it'd prevent most of the cfg spam while having the same results, because passing around a () should generate no code.
• Currently the IDs are all assigned by smoltcp incrementally. I wonder if instead letting the user assign them for TX, and the Device for RX, would be more flexible, or allow more use cases. I think it should still cover PTP fine, but it could allow other interesting things like using some bits flags, or encoding stuff like TX queue priority, or RSSI for wireless stuff. What do you think?

Also, do you have a working impl of PTP on top of this? (not a requirement for merge! I'm just curious, would like to have a look if you do have it)

[datdenkikniet]

• Changing it to a fixed-size int would make sense, yeah. A u32 is probably big enough? At any rate it'd be relatively easy to change in the future.
• Yes, feature-gating it sounds like a good plan. I've added it as per your suggestion, seems to work pretty well. When adding a relatively simple feature like this, how many tests should I add? I've only included it in the "as aggressive as possible" test for now.
• Hmm, that would be pretty nice. If we could somehow offer that as an alternative that would be quite nice, I think having an easy default (of being just a bog-standard counter) is quite important. Maybe just an additional send_manually_marked or something like that would work?

We have a "working" PTP implementation here, though I'm not 100% sure that still compiles. "Working" because you can only get out the timestamps so far, we're not actually doing any PTP syncing just yet.

[Dirbaio]

A u32 is probably big enough? At any rate it'd be relatively easy to change in the future.

u32 sounds good to me.

how many tests should I add? I've only included it in the "as aggressive as possible" test for now.

Don't worry too much about it, since there's no complex logic added, just passing a value around.

If we could somehow offer that as an alternative that would be quite nice, I think having an easy default (of being just a bog-standard counter) is quite important. Maybe just an additional send_manually_marked or something like that would work?

I think having both is not useful, I think we do need to choose now. If we do both, when the phy gets a packet with some ID, it can't know whether it's the counter value or a user-assigned special value, so it can't act on it on any special way. This defeats the point. If we want setting the packet mark to be useful, all other packets should default to a mark of 0, not to a counter.

Also, assigning counter values to regular packets (ie those not sent with send_marked) is not useful, because the user can't obtain these IDs, so they can't use the received IDs on the phy side to correlate with anything. Same for received packets.

Another interesting thing is, if you have lots of traffic in addition to the PTP packets, you're timestamping all packets, so you have to keep a big buffer of id->timestamp mappings or by the time you grab the timestamp it might be already gone. I wonder if this is a problem in practice? For TX, with custom marks you could do something like "0 means don't timestamp, 1 means timestamp" to tell the PHY which ones to timestamp, but this doesn't solve the problem for RX...

I'm not sure what's best :sweat_smile: .

going back on the design discussions: for rx'd packets, simply having the PHY return the timestamp and thread that through all layers up to the socket solves all these issues. It doesn't for tx'd packets though, since you don't know the timestamp after the tx is done, and we don't want TxToken::consume() to block until it's done. So we do need some kind of "packet ID" to retrieve it later... I wonder if we're overcomplicating this by sticking to just IDs, instead of some "packet metadata" struct that we can pass around that can contain IDs, timestamps, other stuff...?

[datdenkikniet]

I think having both is not useful, I think we do need to choose now. If we do both, when the phy gets a packet with some ID, it can't know whether it's the counter value or a user-assigned special value, so it can't act on it on any special way. This defeats the point. If we want setting the packet mark to be useful, all other packets should default to a mark of 0, not to a counter.

The initial idea was that we could just have some value to uniquely map packets to some physical frame. This sounds like we'd be stepping away from that and turning it into a value that can encode some value that only use code and the PHY driver can use properly. If that is what we want to end up with, we should rename it and somehow properly document that.

Also, assigning counter values to regular packets (ie those not sent with send_marked) is not useful, because the user can't obtain these IDs, so they can't use the received IDs on the phy side to correlate with anything. Same for received packets.

While true, it simplifies how handling the IDs (since there will always be one). Maybe too simple, but unless having an ID present somehow induces a bunch of overhead I don't think it makes a large difference.

Another interesting thing is, if you have lots of traffic in addition to the PTP packets, you're timestamping all packets, so you have to keep a big buffer of id->timestamp mappings or by the time you grab the timestamp it might be already gone. I wonder if this is a problem in practice? For TX, with custom marks you could do something like "0 means don't timestamp, 1 means timestamp" to tell the PHY which ones to timestamp, but this doesn't solve the problem for RX...

In STM32s, the timestamping is a zero-cost additional thing that the MCU can do "for free" (only costing a little bit of memory). The original idea assumes that all additional information/operations you can perform using this ID are also zero-cost in that sense.

In stm32-eth we only guarantee that the timestamp can be retrieved within one "interrupt cycle", so the amount of timestamps (= 64 bits) to buffer is limited to the total amount of ethernet frames that the DMA can handle.

going back on the design discussions: for rx'd packets, simply having the PHY return the timestamp and thread that through all layers up to the socket solves all these issues. It doesn't for tx'd packets though, since you don't know the timestamp after the tx is done, and we don't want TxToken::consume() to block until it's done. So we do need some kind of "packet ID" to retrieve it later... I wonder if we're overcomplicating this by sticking to just IDs, instead of some "packet metadata" struct that we can pass around that can contain IDs, timestamps, other stuff...?

You could do this using (an) associated type(s) on RxToken and TxToken and including that data (probably generated by Device, then) instead of the PacketId, assuming adding generics to sockets that would want to make use of such associated data is OK.

Otherwise smoltcp would have to decide the metadata that can or can't be included, which doesn't sound like a super good idea.

In the end I suppose we'd sooner end up with something closer to

pub trait TxToken {
    type Metadata;
    
    fn consume<R, F>(self, timestamp: Instant, len: usize, f: F) -> Result<(R, Self::Metadata)> 
        where F: FnOnce(&mut [u8]) -> Result<R>;
}

pub trait RxToken {
    type Metadata;

    fn consume<R, F>(self, timestamp: Instant, f: F) -> Result<(R, Self::Metadata)>
        where F: FnOnce(&mut [u8]) -> Result<R>;
        
}

and

pub struct UdpMetadata<T> {
    endpoint: IpEndpoint,
    packet_meta: T, // T = Device::RxToken::Metadata/Device::TxToken::Metadata
}

which honestly would be a much smoother solution. Then potentially letting the user generate some custom metadata value that sets some flags for one thing or the other would fall entirely on the implementor of Device. I'd be in favor of a solution like that.

Edit: ah, or some sort of associated type on Device, but with the same idea

[korken89]

Hi @Dirbaio, do you have any more comments/questions on this?

[korken89]

Hi @Dirbaio, I'd like to check in on how this is progressing on your side?

[korken89]

Just for completeness, we have a PTP PoC that here using this PR: https://github.com/stm32-rs/stm32-eth/tree/smoltcp-ptp

Checkout examples/server.rs and examples/client.rs

[Dirbaio]

I think the "assign IDs incrementally vs let user/device assign them concern" concern I initially raised is still unsolved. You mention:

In stm32-eth we only guarantee that the timestamp can be retrieved within one "interrupt cycle", so the amount of timestamps (= 64 bits) to buffer is limited to the total amount of ethernet frames that the DMA can handle.

This does work if both of these are true: 1- The amount of packets processed in a single poll is bounded, and 2- The user is doing manual polling, so they can ensure you do one PTP poll in between each iface/device poll.

1 might be true in stm32-eth, but it's not necessarily true in general. For example in the embassy-stm32 implementation, if the CPU is slow enough wrt DMA, it is possible for the same descriptor to be tx'd multiple times in the same poll if DMA has sent it and freed it by the time you try to tx again. This makes a single poll able to handle an unbounded amount of packets.

About 2, there's many ways to use smoltcp that don't involve manual polling, such as with async, for example like embassy-net does. I don't think manual polling should be required for correct usage. Smoltcp should support manual polling if the user wants to do it, sure, but we shouldn't have an API that can be used correctly only with manual polling.

[datdenkikniet]

That is a good point, thank you for the feedback. That use case lends itself to too much of a niche (manual polling).

I'll rework it a bit to see if I can find an elegant way of supporting optional packet IDs provided by the device

[datdenkikniet]

I'm redoing it now so that we leverage the Device much more when it comes to setting up PacketId's. Additionally, it means we don't actually have to break the Device trait.

I've reset the tree to master before performing these commits (since the approach is so different), but haven't dropped the old just yet.

[korken89]

Hi @Dirbaio

The requested fixed and updates were pushed a few months ago. Do you have any feedback or is it ready for merge?

[codecov[bot]]

Codecov Report

Merging #628 (2b0ad1a) into master (6139bc8) will decrease coverage by 0.03%. The diff coverage is 75.92%.

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
- Coverage   80.29%   80.27%   -0.03%     
==========================================
  Files          72       72              
  Lines       29075    29280     +205     
==========================================
+ Hits        23347    23505     +158     
- Misses       5728     5775      +47     

Impacted Files	Coverage Δ
src/iface/interface/ieee802154.rs	60.75% <0.00%> (-1.58%)	:arrow_down:
src/phy/fault_injector.rs	0.00% <0.00%> (ø)
src/phy/fuzz_injector.rs	0.00% <0.00%> (ø)
src/phy/pcap_writer.rs	0.00% <0.00%> (ø)
src/phy/tracer.rs	0.00% <0.00%> (ø)
src/iface/interface/mod.rs	50.03% <45.28%> (+0.32%)	:arrow_up:
src/iface/interface/sixlowpan.rs	70.85% <50.00%> (-0.29%)	:arrow_down:
src/iface/interface/ethernet.rs	91.48% <66.66%> (+0.18%)	:arrow_up:
src/socket/udp.rs	84.88% <86.95%> (+0.42%)	:arrow_up:
src/iface/interface/ipv4.rs	71.62% <100.00%> (+0.16%)	:arrow_up:
... and 3 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more