feat: sctp connection using usrsctp #11
Conversation
| let udp = UdpTransport.new(laddr) | ||
| let stun = Stun.new(udp) | ||
| let dtls = Dtls.new(stun) | ||
| let sctp = Sctp.new(dtls) |
There was a problem hiding this comment.
If the final instance that matter is sctp, maybe we could just have something like Sctp.new(laddr)?
There was a problem hiding this comment.
There will be something similar in the datachannel PR: Webrtc.new(...) will be used.
If we use something like Sctp.new(laddr), it means we have to stop all transports if we choose to stop Sctp, which I don't like. I'd rather have a way to manage the whole stack by itself (with Webrtc) or to manage each part of the stack (like this example).
| let sctp = Sctp.new(dtls) | ||
|
|
||
| let conn = await sctp.connect(initTAddress("127.0.0.1:4242"), sctpPort = 13) | ||
| while true: |
There was a problem hiding this comment.
We could do this only a few times, maybe 10?
There was a problem hiding this comment.
It's an example, not a test, having an infinite loop is not really bad practice imo.
| import ../webrtc/sctp/[sctp_transport, sctp_connection] | ||
|
|
||
| proc main() {.async.} = | ||
| let laddr = initTAddress("127.0.0.1:4244") |
There was a problem hiding this comment.
why do we fix a local address?
There was a problem hiding this comment.
Because it's an example and ping.nim works with pong.nim. I could improve those with some arguments or with an interactive prompt, but, again, it's not meant to be run in the CI, just to be shown as an example of how you can create an SCTP ping.
tests/testsctp.nim
Outdated
|
|
||
| asyncTest "Two SCTP nodes connecting to each other, then sending/receiving data": | ||
| var | ||
| sctpServer = initSctpStack(initTAddress("127.0.0.1:4444")) |
There was a problem hiding this comment.
It's better to use 0 port to avoid errors with port reuse.
There was a problem hiding this comment.
I changed those. It makes me change some things in udp_transport.nim and Stun transport/connection. If you think it's not appropriate to make those changes in this PR, I can revert these changes and create another one.
| serverConn1 = await serverConn1Fut | ||
| serverConn2 = await serverConn2Fut | ||
|
|
||
| await serverConn1.write(@[1'u8, 2, 3, 4]) |
There was a problem hiding this comment.
Is there any particular meaning to these arrays? We can avoid the repetition by defining a const for the same values.
There was a problem hiding this comment.
No particular meaning, no. I just wanted different message sent every time I read/write.
| @@ -24,10 +24,18 @@ var cfg = | |||
| " --threads:on --opt:speed" | |||
|
|
|||
| when defined(windows): | |||
| cfg = cfg & " --clib:ws2_32" | |||
| cfg = cfg & " --clib:ws2_32 --clib:iphlpapi" | |||
There was a problem hiding this comment.
It might be good to add a comment explaining why this is necessary.
webrtc.nimble
Outdated
| runTest("runalltests") | ||
| exec "nimble build_example" |
There was a problem hiding this comment.
not sure this should be part of the test task
There was a problem hiding this comment.
I agree, I created a "Build examples" task.
| SctpConnected | ||
| SctpClosed | ||
|
|
||
| SctpMessageParameters* = object |
There was a problem hiding this comment.
Can we add some docs about those types and fields here?
There was a problem hiding this comment.
Done. Let me know if it's too verbose or not.
webrtc/sctp/sctp_utils.nim
Outdated
| type | ||
| # These three objects are used for debugging/trace only | ||
| SctpChunk* = object | ||
| chunkType*: uint8 | ||
| flag*: uint8 | ||
| length* {.bin_value: it.data.len() + 4.}: uint16 | ||
| data* {.bin_len: it.length - 4.}: seq[byte] | ||
|
|
||
| SctpPacketHeader* = object | ||
| srcPort*: uint16 | ||
| dstPort*: uint16 | ||
| verifTag*: uint32 | ||
| checksum*: uint32 | ||
|
|
||
| SctpPacketStructure* = object | ||
| header*: SctpPacketHeader | ||
| chunks*: seq[SctpChunk] |
There was a problem hiding this comment.
Could you add more info about these and why they are necessary?
webrtc/sctp/sctp_utils.nim
Outdated
| chunks*: seq[SctpChunk] | ||
|
|
||
| proc dataToString(data: seq[byte]): string = | ||
| # Only used for debugging/trace |
There was a problem hiding this comment.
By "debugging/trace" do you mean logging? If so, it's probably better to move them to another file or rename this one (if all the code here is for that) to something like logutils. Is it a pattern to use underscore for filenames in nim? I think it's the first time I see this in our codebase.
There was a problem hiding this comment.
Done.
I don't know if it's a pattern to use underscore for filenames, but there's at least three files with this pattern in libp2p so I guess it's not unusual.
Presentation
This PR is part of the stack to create the nim-libp2p webrtc-direct transport (defined here: https://github.com/libp2p/specs/blob/master/webrtc/webrtc-direct.md).
For this PR, we do not implement the full SCTP protocol, we are using the library usrsctp (nim wrapper: https://github.com/status-im/nim-usrsctp / C-library https://github.com/sctplab/usrsctp) to create and use SCTP channels.
SCTP
Stream Control Transmission Protocol (SCTP) is a transport-layer protocol used for transmitting multiple streams of data simultaneously between two endpoints, ensuring reliable, ordered delivery of messages. In the WebRTC stack, SCTP is employed to handle data channels, providing a reliable and unordered delivery of messages, which is critical for real-time communication applications. This allows WebRTC to efficiently transmit diverse types of data, such as text messages, file transfers, or application data, ensuring smooth and reliable communication between peers.