Add example application of TCP-over-ringbuffers#1214
Conversation
ba365ce to
faef6c3
Compare
149cc19 to
291bec5
Compare
faef6c3 to
db929ce
Compare
3e47f7b to
0f244e8
Compare
db929ce to
bea8fa4
Compare
0f244e8 to
1786069
Compare
65b988c to
93e7bb7
Compare
d0eb688 to
1c7a78c
Compare
929107c to
4e85b86
Compare
1c7a78c to
0f17d8c
Compare
07d563c to
89888e9
Compare
d73fb01 to
6dba7e2
Compare
0bbd504 to
78f03dc
Compare
09e1b80 to
1760fca
Compare
3db706c to
7fd9729
Compare
hydrolarus
left a comment
There was a problem hiding this comment.
Took a first look at the Rust code changes
| @@ -0,0 +1,394 @@ | |||
| #![no_std] | |||
| #![cfg_attr(not(test), no_main)] | |||
| #![feature(sync_unsafe_cell)] | |||
There was a problem hiding this comment.
Does this crate actually need this feature? It doesn't seem to be used here anywhere?
There was a problem hiding this comment.
It's for the *LOGGER.get()
| return None; | ||
| } | ||
| self.rx_buffer.buffer.set_enable(false); | ||
| core::sync::atomic::fence(core::sync::atomic::Ordering::AcqRel); |
There was a problem hiding this comment.
Our core doesn't support any atomic operations (it lacks the A extension), are fences supported and do what they should?
There was a problem hiding this comment.
So this was a point of discussion @rslawson and I had and I even made an issue about it (which turned out to not be necessary as there was no real problem) smoltcp-rs/smoltcp#1134
I was not sure how I'd have to verify it, but as far as I'm aware this fence prevents the compiler from moving stores / loads across the fence boundary, preserving the order of accesses. I'll add a comment :)
martijnbastiaan
left a comment
There was a problem hiding this comment.
Round 1! I've got more review comments, but I've already accumulated 30. Paired with the ones from @hydrolarus that should already be quite a lot to handle.
martijnbastiaan
left a comment
There was a problem hiding this comment.
Thanks for applying the suggestions! I thought I had more, but I don't remember now..
Increase MU processing element instruction and data memory from 16KB to 160KB (10 RAMB18E2 blocks each) to accommodate the larger smoltcp-demo firmware binary. Refactor the RingBuffer simulation test infrastructure to load firmware binaries by name instead of building PeConfig from Haskell. Add simSmolTcp for simulating the smoltcp ring buffer test. Support multiple TX/RX ring buffer pairs in the DUT circuit. Remove IO from simResultRingBuffer return type.
Update all internal doc references to use the new hyphenated ring-buffer naming convention.
Replace the pinned git smoltcp 0.11 dependency in bittide-hal with crates.io smoltcp 0.12. Add the medium-ip feature needed for point-to-point links. Remove workspace lints inherited section. Fix ring_buffer_test to use the renamed device instance fields (transmit_ring_buffer_0, receive_ring_buffer_0) after the ring buffer HAL was updated to support multiple ring buffers per device.
Move MAC address utilities from smoltcp.rs into smoltcp/mac.rs and convert the smoltcp module from a single file to a directory module. Update smoltcp_client to use the new import path. Also update smoltcp_client to remove workspace lints, add medium-ip feature, and fix format string and mutability warnings.
Log state changes in the link startup state machine to aid debugging link bring-up issues on hardware.
Add modules for TCP-over-ring-buffer communication between nodes: - net_state: UGN graph types (UgnEdge, UgnReport) for collecting network state across nodes - smoltcp/ring_buffer: smoltcp Device implementation over aligned ring buffers with CRC integrity and sequence numbers - smoltcp/link_protocol: wire format types for inter-node commands and UGN edge data - smoltcp/link_interface: high-level LinkInterface combining smoltcp TCP with ring buffer transport, providing typed send/recv over point-to-point links Add crc and zerocopy dependencies. Enable generic_const_exprs feature gate. Remove workspace lints inherited section. Add medium-ip feature to smoltcp.
Add three new firmware crates: - smoltcp-demo: HITL demo for soft UGN collection over TCP links, with ring buffer alignment, DNA exchange, and UGN report aggregation via a manager/subordinate protocol - ring_buffer_smoltcp_test: simulation test for TCP over ring buffers - tcp_simultaneous_open_test: simulation test for TCP simultaneous open (both sides connect at the same time)
One demo needs significantly larger memories, this way we dont have to do that for each demo
We almost never use the whole target so it's nice to have separate names for these
hydrolarus
left a comment
There was a problem hiding this comment.
Seems fine! Some form suggestions
| fn socket(&self) -> &tcp::Socket<'a> { | ||
| self.sockets.get::<tcp::Socket>(self.socket_handle) | ||
| } | ||
|
|
||
| fn socket_mut(&mut self) -> &mut tcp::Socket<'a> { | ||
| self.sockets.get_mut::<tcp::Socket>(self.socket_handle) | ||
| } |
There was a problem hiding this comment.
Would it make sense to use Deref<Target = tcp::Socket<'a>> and DerefMut to get rid of a lot of the accessor functions below? (like is_open() or is_active())
| self.socket_mut() | ||
| .recv_slice(buffer.as_bytes_mut()) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
this last .unwrap_or(0) doesn't seem to have any effect, it's just ignored right after. If you want to ignore the result then _ = self.socket_mut()... is the more intentional way to express this.
| // Get direct pointer to hardware buffer (base pointer points to array of [u8; 8]) | ||
| let hw_buffer_ptr = self.rx_buffer.buffer.base_ptr() as *const u8; | ||
|
|
||
| unsafe { |
There was a problem hiding this comment.
This is a really large unsafe block, I'm sure not all of this needs to be in there, but I can't see from a glance what exactly needs this. So I think this should be split up into smaller blocks where they are actually needed.
What (what did you do)
This pull request adds the necessary library components plus a demo application that showcases asynchronous commmunication between nodes over a bittide network.
With these library components we can add a networking layer between nodes that allow them to arbitrarily exchange data in a reliable way by leveraging the following techniques:
Aligned ringbuffers
The hardware architecture allows CPUs to communicate through a ringbuffer extraction that is shared with its neighbour. Through the
transmitRingbufferandreceiveRingbufferimplemented in #1208 we now have a way to share unidirectional memory regions with our link partners.Packetless transmission header
Since we simply share a memory region with our neighbor to which we can write and they can read, there is no notion of packet transmission and arrival, the data will just end up in memory accessible to our neighbor.
To still allow us to transfer packets, we made a minimal header that allows our neighbor to reliably determine if there is valid data in its memory.
The packet header consists of a checksum, sequence counter, length and payload. We use the checksum to see if there are new packets, the length to determine how long the packet is and for which bytes to calculate the checksum and the sequence counter ensures that when the same packet is send twice, the checksum changes so we can distinguish it from a stale packet.
TCP based communication statemachine
To still deal with data corruption, we leverage the TCP statemachine as offered by the smoltcp package to take care of reliable data transmission.
LinkInterface
Each node will have a LinkInterface abstraction that contains everything necessary to facilitate this asynchronous communication channel. It allows the CPU to send and receive arbitrary data to/from its direct neighbor.
Why (context, issues, etc.)
To be able to perform asynchronous communication in-band over the bittide network. Having an arbitrary asynchronous communication layer allows us to distribute programs, schedules and relevant parameters over the network.
Dear reviewer (anything you'd like the reviewer to pay close attention to?)
I use MaybeUnInit to deal with the self-referential nature of the smoltcp elements in LinkInterface, is this the way?
Is the LinkInterface API (over)complete?
I'm not sure if this
WireEncode/WireDecodeapproach is the way to got. I saw the other way required you to use certain typesu32<LE>for example.AI disclaimer (heads-up for more than inline autocomplete)
AI has been used throughout the engineering of different approaches to implement this layer and used to combine all aspects into the LinkInterface.
TODO
Cross-outany that do not applydocs/