-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
base: master
Are you sure you want to change the base?
The process is polled when a udp packet is sent #1186
Conversation
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. |
e983721
to
ff52d4c
Compare
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. |
af7f021
to
35561c1
Compare
Regarding the Travis failure the relevant error messages are:
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.
I'm not sure what the best way to deal with this is. I could use some |
35561c1
to
d2a811d
Compare
7cad554
to
7a66824
Compare
Performed a Rebase.
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: |
This seems generally like a good idea to have something like "blocking sends" in ContikiOS. What I wonder however is if a regular 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. |
The main reason I chose
I'm not entirely sure about this either. In my own tests I always call
I don't believe that any platform specific code is necessary to use this. This should work as long as IPv6 is being used. |
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 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? |
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. |
7a66824
to
e89e04c
Compare
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:
|
e89e04c
to
40bb209
Compare
After using this patch myself for sometime I've realized that there were at least two serious deficiencies with the code I had:
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. |
40bb209
to
4bd404e
Compare
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? |
There are minor things that I could comment on right now like the name |
d152f15
to
64a102e
Compare
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:
|
I'll think about adding more docs but, just to answer your question,
Yes.
That's exactly right.
Yes, you'll need one
So long as
As in maximum time from return of
In my own code, I keep a small queue of
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. |
The Travis failure was on large-rpl so it probably just needs to be restarted. I'd appreciate if someone could do this. |
🕐 |
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:
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.
Agreed, I can certainly spend some time to fix this.
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? |
What I would do is split it in several PRs. I would do one only on refactoring |
I like this. Will do unless someone else has a reason to object. |
64a102e
to
892469a
Compare
- 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.
892469a
to
5c96bd3
Compare
I would like to have this merged before the year is over 😄 |
+1 |
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.