Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The process is polled when a udp packet is sent #1186

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bkozak-scanimetrics
Copy link
Contributor

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
Copy link
Contributor

msloth commented Aug 6, 2015

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
Copy link
Contributor Author

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 bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch from e983721 to ff52d4c Compare August 6, 2015 14:11
@bkozak-scanimetrics
Copy link
Contributor Author

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 bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch 3 times, most recently from af7f021 to 35561c1 Compare August 6, 2015 16:33
@bkozak-scanimetrics
Copy link
Contributor Author

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 bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch from 35561c1 to d2a811d Compare August 11, 2015 16:31
@bkozak-scanimetrics bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch 9 times, most recently from 7cad554 to 7a66824 Compare December 23, 2015 20:34
@bkozak-scanimetrics
Copy link
Contributor Author

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
Copy link
Contributor

vsaw commented Dec 29, 2015

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch from 7a66824 to e89e04c Compare February 8, 2016 15:55
@bkozak-scanimetrics
Copy link
Contributor Author

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 bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch from e89e04c to 40bb209 Compare February 25, 2016 23:58
@bkozak-scanimetrics
Copy link
Contributor Author

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 bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch from 40bb209 to 4bd404e Compare February 26, 2016 19:14
@bkozak-scanimetrics
Copy link
Contributor Author

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
Copy link
Contributor

vsaw commented Mar 23, 2016

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.

@bkozak-scanimetrics bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch 2 times, most recently from d152f15 to 64a102e Compare April 28, 2016 14:49
@arurke
Copy link
Contributor

arurke commented May 29, 2016

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@alignan
Copy link
Member

alignan commented May 30, 2016

🕐

@simonduq
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

What I would do is split it in several PRs.

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

@bkozak-scanimetrics bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch from 64a102e to 892469a Compare June 13, 2016 18:03
- brought out several sections of code into seperate helper functions
- reduced conditional nesting by adding new function exit points
- use gotos to eliminate some code duplication (the main goal here
  is to reduce code size so as to avoid compilation problems on some
  platforms).
- memset listenports to clear at tcpip_process init
- add setup_appstate function to initilize app state structs
It should now be possible to tell if the packet made it to the MAC
layer based on the return value from this function.
This new function adds two major improvements over uip_udp_packet_send:
- return an error code if the packet send fails before making it to
  the mac layer
- add a mechanism by which applications can be notified when packet
  sending completes along with transmit status from the MAC layer.
@bkozak-scanimetrics bkozak-scanimetrics force-pushed the call_application_on_udp_sent branch from 892469a to 5c96bd3 Compare June 13, 2016 18:34
@alignan
Copy link
Member

alignan commented Dec 1, 2016

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

@arurke
Copy link
Contributor

arurke commented Dec 4, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants