mas-bandwidth / reliable

Packet acknowledgement system for UDP
BSD 3-Clause "New" or "Revised" License
594 stars 78 forks source link

why isn't there an acked_packet_function callback? #8

Closed mreinstein closed 6 years ago

mreinstein commented 6 years ago

I see 2 callback functions defined:

void (*transmit_packet_function)(void*,int,uint16_t,uint8_t*,int); is fired whenever a packet is ready to be sent. it's implementation should handle actually sending data over the socket.

int (*process_packet_function)(void*,int,uint16_t,uint8_t*,int); is fired whenever a packet is received (either a regular packet or a fragmented packet that's been fully re-assembled)

I'm surprised that there is no callback function that is invoked whenever a packet goes from non-acked -> acked. Isn't this the kind of callback one would need to build a reliable message system on top of reliable.io ? Otherwise you'd have to duplicate all of https://github.com/networkprotocol/reliable.io/blob/master/reliable.c#L1090-L1120 logic inside your process_packet_function. Or is there a better way to handle this?

gafferongames commented 6 years ago

The idea is that once per-frame you are able to query the set of acks from reliable.io, it gives you a list of all sequence numbers that were acked. Does this help?

gafferongames commented 6 years ago

For example, look at:

uint16_t * reliable_endpoint_get_acks( struct reliable_endpoint_t * endpoint, int * num_acks );

void reliable_endpoint_clear_acks( struct reliable_endpoint_t * endpoint );

The idea is that once per-frame, you call reliable_endpoint_get_acks, then when you are done with this, you call reliable_endpoint_clear_acks.

cheers

mreinstein commented 6 years ago

The idea is that once per-frame you are able to query the set of acks from reliable.io, it gives you a list of all sequence numbers that were acked

So basically per frame: 1 call reliable_endpoint_get_acks() to get the currently acked sequence numbers 2 compare last_acked to the current acks, and for any sequence numbers that are now acked, handle them 3 set last_acked to the current acks 4 go to 1

?

mreinstein commented 6 years ago

ah, nevermind I get it now. So basically in each endpoint there is num_acks and acks[], and these get buffered internally. It's the responsibility of the caller to read these out and clear them. If this buffer fills up completely, it will stop updating the acks array.

mreinstein commented 6 years ago

one thing that strikes me as odd: transmit_packet_function and process_packet_function are callbacks, but the ack getting/clearing is essentially poll based. Is this an accidental inconsistency in the API or an intentional design decision?

jakecoffman commented 6 years ago

@mreinstein those aren't asynchronous callbacks, they will be called immediately when send/receive_packet are called. In the case of transmit_packet_function, it is called once for each fragment. They are just for providing your own implementation of sending/receiving packets.

mreinstein commented 6 years ago

those aren't asynchronous callbacks

I'm aware that the callbacks are synchronous. What I'm getting at in my question is that there are already 2 callback functions for other event types. Why not just have process_ack_function as a callback too? There are 2 benefits that I can see from this:

I'm hoping @gafferongames can give some insight as to whether this was an intentional design decision, if there are things I'm overlooking, etc.