The process is polled when a udp packet is sent

[bkozak-scanimetrics]

The application can now use the tcpip_udp_sent_event to know when a udp packet has finished sending. The data for the event also contains the status code from the MAC layer.

This will be useful for sending packets at an optimal rate without overwhelming the network stack or continuously polling uip.

The implementation is a bit hack-ish but I can't really see a better way of implementing what, I believe, is a very important feature without doing some serious clean-up of the uip / tcpip and ipv6 code.

[msloth]

I haven't been following the discussion regarding the necessity of this so I won't comment on that, however there are some files touched which shouldn't be, such as apple2 and 6502-files.

[bkozak-scanimetrics]

I haven't been following the discussion regarding the necessity of this so I won't comment on that, however there are some files touched which shouldn't be, such as apple2 and 6502-files.

This is strange, my local repo doesn't show those files in the diff and I certainly didn't make those changes. I'll look into this. Thanks.

[bkozak-scanimetrics]

I've fetched and rebased on master and now everything's fine. Not exactly sure how that could have happened or how the rebase might have fixed it.

[bkozak-scanimetrics]

Regarding the Travis failure the relevant error messages are:

GPG error: http://ppa.launchpad.net precise Release: The following signatures couldn't be verified because the public key is not available: NO_PUBKEY 83FBA1751378B444
WARNING: The following packages cannot be authenticated!
doxygen

This seems to be an issue with the PPA that Travis is using to download doxygen. My bet is that everyone's Travis builds are going to be failing because of this.

/tmp/cc65/cfg/apple2enh.cfg(21): Memory area overflow in LC', segmentLC' (65 bytes)

I'm not sure what the best way to deal with this is. I could use some #ifs to make the extra memory go away on certain builds but I think that this is a feature that should be enabled everywhere. Also, I think that there are certainly better places to be reducing memory. Any thoughts on this are welcome.

[bkozak-scanimetrics]

Performed a Rebase.
Changes include:

• fixed some minor merge conflicts
• fixed some new doxygen warnings.
• made some small adjustments so that 6502 wouldn't run out of memory in Travis

Any chance of this getting merged? Since I think this is such an important thing to have I am willing to completely restructure this patch if the maintainers think this isn't good in its current form; just please let me know.

To reiterate my case for adding this patch:
I believe that the application needs to be given more information so that it can modulate its sending rate. Bursty communications work very well in ContikiMAC but right now the application has no idea of how fast it can queue up packets during burst transmissions without overwhelming the network stack. Something like this patch is needed for this.

[vsaw]

This seems generally like a good idea to have something like "blocking sends" in ContikiOS. What I wonder however is if a regular process_post wouldn't be better than your process_post_synch? The reason is that I wonder if it's safe to send another UDP packet right from an process_post_synch context?

The other thing I wonder right now is if the platforms/netstacks need to use your callbacks to make that pull request useful right now. It would be bad if this "only" introduces the callback infrastructure that no-one can use out of the box.

[bkozak-scanimetrics]

What I wonder however is if a regular process_post wouldn't be better than your process_post_synch?

The main reason I chose process_post_synch is because process_post can fail and I see no good way to handle that failure. If there's going to be a callback for the application I think that it's important that it happens no matter what.

... I wonder if it's safe to send another UDP packet right from an process_post_synch context?

I'm not entirely sure about this either. In my own tests I always call tcpip_poll_udp in order to get a tcpip_event before sending another packet, just to be safe. I'll have to do some testing on this.

The other thing I wonder right now is if the platforms/netstacks need to use your callbacks to make that pull request useful right now. It would be bad if this "only" introduces the callback infrastructure that no-one can use out of the box.

I don't believe that any platform specific code is necessary to use this. This should work as long as IPv6 is being used.

[bkozak-scanimetrics]

My testing has shown that there is nothing inherent to the "sync context" which prevents packets from being sent. My application has no problems sending a packet as soon as it is called into with a tcpip_udp_sent_event.

I am still concerned that there could be some way for the packet send attempt to fail if a separate process attempts to send a packet around the same time (such as RPL for example). Is there any way that this might be an issue?

[bkozak-scanimetrics]

After studying the code some more, I don't believe that there is any reason why sending a packet from the sync context would be a problem.

I have realized, however, that my code will not work with fragmented packets. It's going to take me a while to fix that.

[bkozak-scanimetrics]

I've implemented a fix to get this working with fragmented packets. There are, however, a few important points I want to make about this code:

• Although it seems, based on what I'm seeing in wireshark, that my code works just fine with fragmented packets, I can't seem to receive fragmented packets in the first place (even with current master branch). Although I can see the fragmented packets on my sniffer they never seem to come out on the other end of the tunnel created by tunslip6 (known issue?) and so they don't make it to my test server.
• With non-fragmented packets, I've tested everything thoroughly; my code works just fine in this case.
• With my latest change, I added data structures to help me track the state of fragmented packets. I would have preferred to use any information which is already available in the network stack instead but couldn't figure out exactly how to do this (probably because I don't know enough about how IP packet fragmentation works). I would appreciate if anyone could explain how I could do this.

[bkozak-scanimetrics]

After using this patch myself for sometime I've realized that there were at least two serious deficiencies with the code I had:

1. The code's usefulness is seriously limited without a return code from uip_udp_packet_send2() since there may be times when packets are dropped without the application ever knowing.
2. There was no way to track multiple packets simultaneously while using a single udp connection object.

I've now addressed these issues with my latest change to this branch and have also done some major re-factoring of the code including better naming of the new data structures and functions I've added.

[bkozak-scanimetrics]

I don't mean to push anyone but I've been using this branch in my own projects for some-time now and it has been working very well. Is there anything else that needs to be done here?

@vsaw have I addressed your concerns?

[vsaw]

There are minor things that I could comment on right now like the name uip_udp_packet_send2(). However I'll hold off until a maintainer comments on the feature in general.

[arurke]

This functionality is very useful in my application so I have tested the PR briefly - and at first glance it works fine! So firstly, thanks a lot. I have not looked too deeply under the hood, so just some questions/comments from a user perspective:

• Some more doc. on the variables would be nice, e.g. in tcpip_track what is first_fail_status?
• Also, in tcpip_udp_track_status, is status the actual report from the MAC layer? Consequently, would the procedure to get full knowledge on the transmission be: 1) check the return value of uip_udp_packet_send2() and 2) check status in received tcpip_udp_sent_event?
• How do you track multiple frames in the same conn.? Separate tcpip_track structs pr frame? Which assumptions can be made around these - e.g. will there always be an tcpip_udp_sent_event? Any max time (this is probably platform/MAC dependent)? Just wondering how I could minimize the amount of the structs safely.
• As mentioned the naming could be improved, xxx_send2 -> xxx_send_tracked? Sometimes things are called tcpip_udp_track_xxx, other times tcpip_track_xxx. On purpose?

[bkozak-scanimetrics]

Some more doc. on the variables would be nice, e.g. in tcpip_track what is first_fail_status?

I'll think about adding more docs but, just to answer your question, first_fail_status is the failure code for the first failed MAC frame of a fragmented packet. This is the value that is set in the tcpip_udp_track_status which is returned to the application as data on a tcpip_udp_sent_event.

Also, in tcpip_udp_track_status, is status the actual report from the MAC layer?

Yes.

Consequently, would the procedure to get full knowledge on the transmission be: 1) check the return value of uip_udp_packet_send2() and 2) check status in received tcpip_udp_sent_event?

That's exactly right.

How do you track multiple frames in the same conn.? Separate tcpip_track structs pr frame?

Yes, you'll need one tcpip_track struct for each packet you want to have queued at once.

Which assumptions can be made around these - e.g. will there always be an tcpip_udp_sent_event?

So long as uip_udp_packet_send2 returns success, the tcpip_udp_sent_event should always occur barring some bug in the tcpip stack implementation. In all of my time using this code I have yet to see the event fail to occur.

Any max time (this is probably platform/MAC dependent)?

As in maximum time from return of uip_udp_packet_send2 until the tcpip_udp_sent_event occurs? Yes that would be extremely platform dependent and also depends on whether there are other packets already queued.

Just wondering how I could minimize the amount of the structs safely.

In my own code, I keep a small queue of tcpip_track structures and have a dedicated packet scheduler which creates new packets from my available data only as tcpip_track structs become free. Of course, this only works for me because I can afford to buffer available data for quite some time before giving it to the tcpip stack.

As mentioned the naming could be improved, xxx_send2 -> xxx_send_tracked? Sometimes things are called tcpip_udp_track_xxx, other times tcpip_track_xxx. On purpose?

Yes you're probably right about that. I was really struggling to come up with good names for some of the new functions and structs but maybe I'll think about it some more.

[bkozak-scanimetrics]

The Travis failure was on large-rpl so it probably just needs to be restarted. I'd appreciate if someone could do this.

[alignan]

🕐

[simonduq]

Are the changes on routing table, fallback interface etc intended?
Overall, this feels a bit hackish (to use OP's words), with still unclear implications on fragmentation, poor function naming etc. In such a sensitive part of the OS we need bullet-proof code. Setting a timeout for closing, unless @bkozak-scanimetrics shows interest in improving this significantly.

[bkozak-scanimetrics]

Are the changes on routing table, fallback interface etc intended?

These are intended. There is also a separate commit just for these modifications. The reason for the changes are:

1. This patch requires significant changes to tcpip_ipv6_output but I found the function borderline impossible to read. I wasn't about to touch it without some re-factoring for readability because I was afraid I'd break something otherwise.
2. I was fighting with the 6502 memory size for the whole time I was developing this. I really needed any code size reduction I could get and these changes eliminate some code duplication.

unclear implications on fragmentation

Fragmentation is working in all of my tests. The problem I mentioned earlier seems to have been caused by a separate part of the code.

poor function naming

Agreed, I can certainly spend some time to fix this.

unless @bkozak-scanimetrics shows interest in improving this significantly.

If there is anything else specific you'd like to mention I'd really appreciate it.

At the moment the changes I've made have been minimal in order to avoid breaking anything or increasing memory usage and I really don't think that I should change that. So other than naming and documentation, what else needs changing?

[simonduq]

What I would do is split it in several PRs. I would do one only on refactoring tcpip_ipv6_output, because the change is too sensitive to be linked to the particular new feature of the PR (BTW I totally agree the current tcpip_ipv6_output is a mess and yours looks so much cleaner). After the refactoring is done, the "packet sent event" feature should become a simple PR.

[bkozak-scanimetrics]

What I would do is split it in several PRs.

I like this. Will do unless someone else has a reason to object.

[alignan]

I would like to have this merged before the year is over 😄

[arurke]

+1
But it has been split and depends on #1725