buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26397, SLIP polling support

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #26397: SLIP polling support

Submitted by:  Simon Kallweit <skallweit>
Submitted on:  Wed 29 Apr 2009 12:00:26 PM UTC  
 
Category:  None Severity:  3 - Normal
Item Group:  Feature Request Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

( Jump to the original submission)

Fri 28 Aug 2009 08:16:23 AM UTC, comment #14: 

Thanks for testing.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 27 Aug 2009 07:04:13 AM UTC, comment #13: 

I have just done some testing of the new SLIP and SIO code. It works perfectly fine for me, both in blocking and polling mode (NO_SYS=0 and NO_SYS=1). Thanks a lot for getting this in.

Simon Kallweit <skallweit>
Wed 26 Aug 2009 09:03:38 PM UTC, comment #12: 

I've just checked in the patches for slipif.c/.h (slightly modified). I do hope it works since I can only compile it, not really test it.

There is one (possible) bug fixed: I changed
pbuf_alloc(PBUF_LINK, PBUF_POOL_BUFSIZE, PBUF_POOL);
to
pbuf_alloc(PBUF_LINK, (PBUF_POOL_BUFSIZE - PBUF_LINK_HLEN), PBUF_POOL);

The old version must have had a (memory leak or data error) bug since PBUF_LINK + PBUF_POOL_BUFSIZE results in allocating 2 pbufs from the pool, one of which at least does not get filled or maybe even get lost while cat'ing the pbufs.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 25 Aug 2009 08:17:30 PM UTC, comment #11: 

Well, I hope my documentation is right. At least it seemed to make sense looking at the ppp and slip code. The slip patch should be fine with your proposed changes. Thanks

Simon Kallweit <skallweit>
Tue 25 Aug 2009 05:58:51 PM UTC, comment #10: 

>> which means the function sio_poll would block, too!
>
> No, it wouldn't. I would first like to fix the sio API and then > see how to best implement SLIP polling.


I meant as it is now, of course.

Anyway, I've checked in the patch to sio.h, thanks for that. I hope the function's documentations match the existing implementations. At least the win32 port's implementation is incorrect, as sio_read_abort() aborts sio_recv, not sio_read(), but that's a different issue.

I guess the slipip.h/.c patch must now be changed to use sio_read() or sio_tryread() depending on who calls it (the thread or the polling function). If that's all to be changed, I'll do it. If not, please tell me.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 25 Aug 2009 12:18:04 PM UTC, comment #9: 

Simon Goldschmidt wrote:

> Follow-up Comment #8, bug #26397 (project lwip):
>
>> Actually ppp.c uses sio_read() in blocking mode. It uses sio_read_abort() to cancel the blocking sio_read() calls.
>
> So the SLIP-polling patch wouldn't work, either, since both sio_read and
> sio_recv are blocking, which means the function sio_poll would block, too!


No, it wouldn't. I would first like to fix the sio API and then see how to best implement SLIP polling.

> In that case, there's no alternative to a new function, I guess. I'd be OK
> with sio_tryread() or something like that (try- to stay with the naming
> convention we already have - e.g. in sys_mbox_tryfetch).


Well, sio_tryread() would be the same as sio_read() but non-blocking? We could also add a function to peek at how many characters are waiting in line. But sio_tryread() sounds fine to me.

I add a little patch for sio.h which adds documentation and the new sio_tryread() prototype. Please review.

(file #18637)

Simon Kallweit <skallweit>
Tue 25 Aug 2009 10:52:14 AM UTC, comment #8: 

>> But to me it seems like sio_recv shouldn't block, and if so I think we should
>> keep the current behaviour to avoid upgrading problems. That way, we have
>> sio_read for blocking mode and sio_recv for non-blocking mode.


> Ehm, I think you mixed them up:


Yes, I did. Sorry.

> Actually ppp.c uses sio_read() in blocking mode. It uses
> sio_read_abort() to cancel the blocking sio_read() calls.


So the SLIP-polling patch wouldn't work, either, since both sio_read and sio_recv are blocking, which means the function sio_poll would block, too!

In that case, there's no alternative to a new function, I guess. I'd be OK with sio_tryread() or something like that (try- to stay with the naming convention we already have - e.g. in sys_mbox_tryfetch).

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 25 Aug 2009 09:38:23 AM UTC, comment #7: 

I thought sio_recv would not block since it is used that way in ppp.c: if it returns 0, nothing has been read and a sleep(1) is executed. If it would block, this would never happen.

However, the sio.h file isn't very well documented (or not documented at all, rather) - the only documentation seems to be in the win32 port's sio.c file, and sio_recv isn't documented there, too.

But to me it seems like sio_recv shouldn't block, and if so I think we should keep the current behaviour to avoid upgrading problems. That way, we have sio_read for blocking mode and sio_recv for non-blocking mode.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 24 Aug 2009 08:57:37 AM UTC, comment #6: 

Simon, I wonder if it is the right approach to just add a parameter to slipif_input. I think we would first have to clear up on what the sio functions are meant to do. To my guessing all the sio functions are currently meant to be blocking. The sio API does only support aborting a read operation yet, but no non-blocking reads. Would it make sense to introduce for example a function sio_read_nb() to do a non-blocking read?

The other question is how to configure polling/non-polling mode. In my lwip port, I use polling when running lwip with NO_SYS == 1 in a single thread. Is it possible to tie the configuration for blocking/non-blocking operation to some already existing configuration options? Or do you think that should be more explicit?

Simon Kallweit <skallweit>
Sat 22 Aug 2009 10:07:21 PM UTC, comment #5: 

I had a look at the new patch and it seems the comments referring to the sio_-functions used weren't adjusted.

As to the question about sio_recv/sio_read: I think sio_read() was used with a good reason: when used from the slipif_loop() thread, it must block until there's a character received. If I read the patch correctly, this behaviour gets broken with the patch?

If that is solved, I guess the patch is OK. Could be as easy a parameter to slipif_input() whether to block (call sio_read) or not (call sio_recv).

BTW: I just saw that the function prototypes in sio.h are not commented at all: how do we know the u32_t in sio_read is meant to be 'len'?

OT: Making PPP asynchronous (which is effectively needed when converting it from a thread to a netif) has been requested before but isn't trivial as much of the code obviously has been just copied from a BSD (which one was it, again?) and thus needs major rewriting to be asynchronous, I guess.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Fri 21 Aug 2009 07:58:13 AM UTC, comment #4: 

Thanks for the explanation of the changes, it makes a lot more sense now.  I'll try and apply this without the whitespace changes to the copyright message.

Anyone else want to comment before this goes in?

Kieran Mansley <kieranm>
Project Member
Fri 21 Aug 2009 07:56:40 AM UTC, comment #3: 

Follow-up from Simon Kallweit sent to mailing list copied here:

Kieran Mansley wrote:

> Follow-up Comment #2, bug #26397 (project lwip):
>
> I don't think there's any particular reason why this has failed to be
> applied, just that it hasn't been a high priority.


Ok, then let's look forward to get it included now. Similar work should
be done on the PPP netif, so it can also be used in a
polled/single-threaded environment.

> Looking at the patch, it seems to change a lot of code that isn't necessary
> to add the slipif_poll() function.


Well, actually there is not that much code. I think we should try to
share code for both the polled and the threaded versions, so that's what
most changes are for.

The struct slipif_priv is used for private data of the SLIP interface,
as we cannot have them as local variables anymore. The changes in
slipif_output() are due to the introduced slipif_priv structure. Nothing
else changed. slipif_input() had to be changed more significantly. The
old version would block until a new packet was received. The new version
will return NULL if all data has been read from the serial and no
complete packet was received, but it will keep track of partially
received packets, so the next call to slipif_input() can finish the process.

> If you could give more detail about the
> need for the other changes (a good example is the change from sio_read to
> sio_recv) that would help.  Some of the changes are obviously improvements and
> bug fixes, but a lot aren't obvious at first glance to someone not familiar
> with the sio code.


Well, the change from sio_recv() to sio_read() is quite simple,
sio_recv() directly returns the received character from the serial line
and is therefore not useable in a polled mode. sio_read() on the other
hand can return without actually receiving data.

The sio API in general is a bit lacking when it comes to
blocking/non-blocking operation. I was wondering if it should be
extended to support some sort of configuration. We should also add some
documentation I think.

Instead of using sio_read() we could also introduce some method
sio_peek() to check if there are characters waiting to be received. But
this would change the API.

Also, in regard to PPP, we should add an API to get/set modem control lines.

> There also seem to be a lot of whitespace changes that make the patch a bit
> harder to read: the whole copyright message for example shouldn't be in the
> patch.


I can remove this. But it's just a bit of cleaning up.

Simon

Kieran Mansley <kieranm>
Project Member
Thu 20 Aug 2009 02:03:21 PM UTC, comment #2: 

I don't think there's any particular reason why this has failed to be applied, just that it hasn't been a high priority.

Looking at the patch, it seems to change a lot of code that isn't necessary to add the slipif_poll() function.  If you could give more detail about the need for the other changes (a good example is the change from sio_read to sio_recv) that would help.  Some of the changes are obviously improvements and bug fixes, but a lot aren't obvious at first glance to someone not familiar with the sio code.

There also seem to be a lot of whitespace changes that make the patch a bit harder to read: the whole copyright message for example shouldn't be in the patch.

Kieran Mansley <kieranm>
Project Member
Thu 20 Aug 2009 01:32:03 PM UTC, comment #1: 

Is there any reason why this patch is not commited? Is it an issue that sio_recv() is used in place of sio_read(), as this could affect users? I'm preparing a new lwIP package for the eCos OS and would like to have support for polled SLIP devices.

Simon Kallweit <skallweit>
Wed 29 Apr 2009 12:00:26 PM UTC, original submission:  

This patch provides support running the SLIP interface in a polling mode. This can be useful to run lwIP/SLIP in a single thread with NO_SYS=1. The change should not affect current users of SLIP. Some of the memory will be moved from stack to heap, and instead of sio_recv() the patch uses sio_read(). Polling mode can be used by periodically calling slipif_poll().

Simon Kallweit <skallweit>

 

Attached Files
file #18637:  sio.patch added by skallweit (3KiB - text/x-patch)
file #18611:  slipif.c.patch added by skallweit (12KiB - text/x-patch)
file #18612:  slipif.h.patch added by skallweit (425B - text/x-patch)
file #18052:  slipif_poll.patch added by skallweit (8KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by skallweit (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-08-28 goldsimon StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2009-08-26 goldsimon StatusNone => Ready For Test
        Assigned toNone => goldsimon
    2009-08-25 skallweit Attached File- => Added sio.patch, #18637
    2009-08-21 kieranm Planned Release => 1.4.0
    2009-08-20 skallweit Attached File- => Added slipif.c.patch, #18611
        Attached File- => Added slipif.h.patch, #18612
    2009-04-29 skallweit Attached File- => Added slipif_poll.patch, #18052

    Back to the top


    Powered by Savane 3.5