Re: lwIP upgrade to CVS HEAD

Re: lwIP upgrade to CVS HEAD

[John Dallaway]

[ moving to the ecos-devel list ]

Hi Simon

Simon Kallweit wrote:

> John reminded my about our endeavor to upgrade the lwIP stack to an
> up-to-date version. I took a little time to upgrade my port to the CVS
> HEAD from yesterday. So everything should be cutting edge.

Excellent!

> There have
> been a few new configuration options since my last port, which I
> integrated into the CDL. During the source upgrade I could also recap on
> the changes that were necessary in the lwIP codebase. Here is a little
> summary:
> 
> * slipif.h/slipif.c: I changed the implementation to support polling.
> This does not break the current API. I will post a patch to lwIP and see
> if I can get this committed.

Good.

> * sys.h: Added an include. I don't know if this really is necessary.
> Will check with the lwIP mailing list.
> * init.c: Removed a check for PPP in NO_SYS mode, which is not valid
> with my current PPP changes.
> * ppp: I did some work on the PPP code, added polling support, added
> pppdump support. As said earlier, this is currently quite a mess, and I
> think we should work on the lwIP port first, not focusing on PPP.

Yes, let's focus on getting a clean port of the lwIP with solid ethernet
support committed. If I understand correctly, PPP support in the master
lwIP sources has some problems at present. Fixing the PPP support sounds
like a separate project to me.

> Well, that's it. Not many changes at all, and I think with some commits
> to the lwIP repository we can use the pure lwIP code with no changes at
> all. Getting PPP right on the other hand, will need some serious work.

Do you know if anyone in the lwIP community is intending to work on the
PPP support in the near future?

> So how does my port differ from the current port in eCos. First, I
> didn't try to be compatible with the previous port. But as the lwIP API
> itself is probably most important, usage is left mostly the same. I've
> written the CDL mostly from scratch, added options where I thought it's
> worth it. There are some options which are not yet configurable in the
> CDL. Here is a quick list:
> 
> * MEM_USE_POOLS, MEM_USE_POOLS_TRY_BIGGER_POOL:
>    This should not be necessary as we use ecos variable sized memory
> pools for these allocations.
> * MEMP_USE_CUSTOM_POOLS:
>    Not necessary as ecos provides such infrastructure.
> * LWIP_ALLOW_MEM_FREE_FROM_OTHER_CONTEXT:
>    Not necessary.
> * LWIP_NETIF_HOSTNAME, LWIP_NETIF_API, LWIP_NETIF_STATUS_CALLBACK,
> LWIP_NETIF_LINK_CALLBACK, LWIP_NETIF_HWADDRHINT, LWIP_NETIF_LOOPBACK:
>    These should probably be implemented, for support of the netif API.
> * LWIP_LOOPBACK_MAX_PBUFS:
>    May be added too.
> * LWIP_NETIF_LOOPBACK_MULTITHREADING:
>    Automatically set in opts.h, depending on NO_SYS.
> * TCPIP_MBOX_SIZE:
>    May be necessary for sequential support, but the message queue length
> could also be fixed (this will be the case when using eCos message queues).
> * DEFAULT_THREAD_NAME, DEFAULT_THREAD_STACKSIZE, DEFAULT_THREAD_PRIO,
> DEFAULT_RAW_RECVMBOX_SIZE, DEFAULT_UDP_RECVMBOX_SIZE,
> DEFAULT_TCP_RECVMBOX_SIZE, DEFAULT_ACCEPTMBOX_SIZE:
>    I don't think they are needed. Creating threads should always provide
> these settings.
> * LWIP_TCPIP_CORE_LOCKING:
>    Experimental, shouldn't be used right now.
> * RECV_BUFSIZE_DEFAULT:
>    Just a default value, set to INT_MAX.
> * SO_REUSE:
>    Should not be used.
> 
> I think this is more or less complete. Would be great if somebody
> reviewed my CDL as I'm sure there are some formal mistakes
> (copy'n'paste).

I will review the CDL.

> Also your mileage may vary about what should be
> configurable. People might think the current CDL is too excessive.
> 
> As I stated in earlier mails, my current port only supports the "Simple"
> mode, no netcomm or socket API.

Since this stack is likely to be chosen where memory is limited, the raw
API is most important. However, the Berkeley socket API is a "common
currency" understood by most network developers, is implemented in most
operating systems and is provided by the existing lwIP port in the eCos
repository so I think this really should be supported.

> I use the stack for GPRS on-demand
> connections, trying to use as less resources as necessary (very limited
> target). The goal was to run the complete lwIP stack with PPP as well as
> the higher level code (HTTP client) in a single thread. For testing on
> the synthetic target, I have also used the ethernet drivers (USB), again
> in a single threaded polled environment. There is currently no support
> for the "Sequential" mode.

The sequential API is required for the socket API and desirable in its
own right as it provides a good compromise between ease of use and
memory footprint. John Eigelaar has indicated that he would be willing
to help with this and already has a copyright assignment in place.

> I have adapted and cleaned up the eth io code (eth_drv.c). The interface
> towards the lwIP ecos clue code is:
> 
> extern void lwip_eth_drv_set_addr(struct netif *);
> Called by the ethernet driver to initialize the network address and
> bring up the network interface.
> 
> extern void lwip_eth_drv_dhcp_init(struct netif *);
> Called by the ethernet driver to start DHCP discovery on the network
> interface.
> 
> extern void lwip_eth_drv_dsr(void);
> Called by the ethernet driver when a DSR was processed.
> 
> extern void lwip_eth_drv_recv(struct netif *, struct pbuf *);
> Called by the ethernet driver when a new packet was received.
> 
> These functions are currently all implemented in simple.c for the Simple
> mode. They will have to be implemented again for the Sequential mode.
> 
> The current interface for using the Simple mode, is indeed very simple :)
> 
> externC void cyg_lwip_simple_init(void);
> Initializes the stack.
> 
> externC void cyg_lwip_simple_restart(void);
> Should be used to restart the stack when it has not been polled for a
> while (handles timers correctly).
> 
> externC void cyg_lwip_simple_poll(void);
> Should be called periodically to let the stack do it's work, poll
> devices etc.
> 
> The simple mode could be extended to have an implicit thread, so we
> would not have to care about calling cyg_lwip_simple_poll(). For me it
> was important to be able to do other things in the networking thread
> simultaneously, but this could also be done using a callback for
> example. What is the preferred way?

Your cyg_lwip_simple_poll() approach seems fine to me and provides
ultimate flexibility but one of the other eCos maintainers who is more
familiar with lwIP may wish to comment.

> In the old port, setting and running PPP and SLIP was done
> automatically. I currently do this explicitly. And I think for PPP this
> is the preferred way, as normally more control is needed. We could add
> implicit SLIP support. SLIP would then be as simple to use as ethernet.
> What do you think?

Again, comments from the other eCos maintainers are welcome.

> I have also done some cleaning up on the sys port (sys_arch.c) which is
> used in Sequential mode. Nothing is tested yet, but might be helpful for
> getting up the Sequential mode. I think we should also add a few more
> testcases, to cover both the Simple and Sequential modes more broadly.

Agreed.

> That's about it. Hope to get a bit of feedback on this. I'm willing to
> put some work into this to clean it up, so we can get it into the eCos
> repository. I started the port to work for my current project, so I
> think it's not yet generic enough for inclusion, but I think we can get
> there.

Thanks for your commitment to this.

> Currently I host the code on my own git server. This is handy as I can
> track the official eCos CVS and add my changes in a branch. To access
> the current code you will have to do:
> 
> # Go to a temporary directory
> cd /tmp
> # Clone the repository
> git clone git://git.inthemill.ch/ecos.git ecos
> # Change to the "triponsm" branch (my current working branch)
> cd ecos
> git checkout --track -b triponsm origin/triponsm
> 
> Alternatively you can just browse with git-web:
> http://git.inthemill.ch/?p=ecos.git;a=tree;h=refs/heads/triponsm;hb=refs/heads/triponsm
> 
> If we're going to collaborate on this and people want to use something
> else than git, I can setup an SVN repo or something, or we can move this
> to the eCos CVS as soon as possible and go from there.

We will need more functionality in place before we can replace the
existing lwIP port in the eCos repository trunk. We could use your own
tree or we could cut a branch in the eCos repository. I would prefer to
branch the eCos repository as this would enhance visibility within the
eCos community, gives a better sense of shared ownership, and allows the
eCos maintainers to monitor progress more easily. Be assured that I am
keen to support this lwIP port update and will ensure that patches are
committed to the branch quickly.

John Dallaway

Re: lwIP upgrade to CVS HEAD

[Simon Kallweit]

John Dallaway wrote:
>> * sys.h: Added an include. I don't know if this really is necessary.
>> Will check with the lwIP mailing list.
>> * init.c: Removed a check for PPP in NO_SYS mode, which is not valid
>> with my current PPP changes.
>> * ppp: I did some work on the PPP code, added polling support, added
>> pppdump support. As said earlier, this is currently quite a mess, and I
>> think we should work on the lwIP port first, not focusing on PPP.
> 
> Yes, let's focus on getting a clean port of the lwIP with solid ethernet
> support committed. If I understand correctly, PPP support in the master
> lwIP sources has some problems at present. Fixing the PPP support sounds
> like a separate project to me.

Totally agreed.

>> Well, that's it. Not many changes at all, and I think with some commits
>> to the lwIP repository we can use the pure lwIP code with no changes at
>> all. Getting PPP right on the other hand, will need some serious work.
> 
> Do you know if anyone in the lwIP community is intending to work on the
> PPP support in the near future?

No, currently there is nobody intending to work on it. With the latest 
commits to lwIP, PPP even got broken in a few places. Ultimately, 
someone should take the current pppd sources which the lwIP PPP port is 
originally based on and adapt to them. But this is quite a bit of work.

> I will review the CDL.

Good to hear :)

> The sequential API is required for the socket API and desirable in its
> own right as it provides a good compromise between ease of use and
> memory footprint. John Eigelaar has indicated that he would be willing
> to help with this and already has a copyright assignment in place.

Any help is appreciated. I will gladly help testing and do work on this 
too, when there is need. But I would like to have John Eigelaar commit 
his changes and go from there instead of going from scratch.

> We will need more functionality in place before we can replace the
> existing lwIP port in the eCos repository trunk. We could use your own
> tree or we could cut a branch in the eCos repository. I would prefer to
> branch the eCos repository as this would enhance visibility within the
> eCos community, gives a better sense of shared ownership, and allows the
> eCos maintainers to monitor progress more easily. Be assured that I am
> keen to support this lwIP port update and will ensure that patches are
> committed to the branch quickly.

I think that's a good idea.

Simon

Re: lwIP upgrade to CVS HEAD

[John Dallaway]

Hi Simon

John Dallaway wrote:

> I will review the CDL.

The CDL is looking good so far.

There's some scope for improving the dependency info. For example,
CYGIMP_LWIP_MEMP_MEM_MALLOC could specify:

   requires { CYGINT_ISO_MEMALLOC != 0 }
   default_value { CYGINT_ISO_MEMALLOC != 0 }

Many of the protocol components and options are also likely to "require"
either CYGPKG_LWIP_TCP or CYGPKG_LWIP_UDP.

There may also be some scope for active_if constraints to clarify the
relevance of certain options. For example, could CYGPKG_LWIP_NETCONF be
made "active_if !CYGPKG_LWIP_DHCP"? This may not be possible if the
static networking address definitions are still required for correct
compilation when DHCP is enabled.

Finally, I would make the display strings of the numerous protocol
support components and options just a little more verbose. For example:

  "DHCP" -> "DHCP support"
  "ICMP" -> "ICMP support"

and so on.

There are currently 27 items directly under the lwIP package node in the
tree. Perhaps there is some scope for further grouping of these nodes to
give the tree more depth and less breadth. For example, a "Protocol
support" component of "flavor none" containing the various protocol
components?

>> The sequential API is required for the socket API and desirable in its
>> own right as it provides a good compromise between ease of use and
>> memory footprint. John Eigelaar has indicated that he would be willing
>> to help with this and already has a copyright assignment in place.
> 
> Any help is appreciated. I will gladly help testing and do work on this
> too, when there is need. But I would like to have John Eigelaar commit
> his changes and go from there instead of going from scratch.
> 
>> We will need more functionality in place before we can replace the
>> existing lwIP port in the eCos repository trunk. We could use your own
>> tree or we could cut a branch in the eCos repository. I would prefer to
>> branch the eCos repository as this would enhance visibility within the
>> eCos community, gives a better sense of shared ownership, and allows the
>> eCos maintainers to monitor progress more easily. Be assured that I am
>> keen to support this lwIP port update and will ensure that patches are
>> committed to the branch quickly.
> 
> I think that's a good idea.

OK. Send me a tarball of the updated package when you're ready to start
collaborating and I'll check it in on a new branch.

John Dallaway

Re: lwIP upgrade to CVS HEAD

[Simon Kallweit]

John Dallaway wrote:
> There's some scope for improving the dependency info. For example,
> CYGIMP_LWIP_MEMP_MEM_MALLOC could specify:
> 
>    requires { CYGINT_ISO_MEMALLOC != 0 }
>    default_value { CYGINT_ISO_MEMALLOC != 0 }
> 
> Many of the protocol components and options are also likely to "require"
> either CYGPKG_LWIP_TCP or CYGPKG_LWIP_UDP.
> 
> There may also be some scope for active_if constraints to clarify the
> relevance of certain options. For example, could CYGPKG_LWIP_NETCONF be
> made "active_if !CYGPKG_LWIP_DHCP"? This may not be possible if the
> static networking address definitions are still required for correct
> compilation when DHCP is enabled.

I'll have a look at them tomorrow. There are lots of small improvements 
to be made with dependencies and constraints.

> Finally, I would make the display strings of the numerous protocol
> support components and options just a little more verbose. For example:
> 
>   "DHCP" -> "DHCP support"
>   "ICMP" -> "ICMP support"
> 
> and so on.

Done.

> There are currently 27 items directly under the lwIP package node in the
> tree. Perhaps there is some scope for further grouping of these nodes to
> give the tree more depth and less breadth. For example, a "Protocol
> support" component of "flavor none" containing the various protocol
> components?

I have reorganized the whole CDL quite a bit. Please check out the new 
hierarchy and give me some feedback.

> OK. Send me a tarball of the updated package when you're ready to start
> collaborating and I'll check it in on a new branch.

It's still in the git repo. Please get the current version from there, 
I'll still send you a tarball once I'm ready.

I will also look into sequential support, as there are no words from 
John Eigelaar. I think I can get do the basics myself.

Simon

Re: lwIP upgrade to CVS HEAD

[David Fernandez]

John Dallaway wrote:
> There's some scope for improving the dependency info. For example,
> CYGIMP_LWIP_MEMP_MEM_MALLOC could specify:
>
>    requires { CYGINT_ISO_MEMALLOC != 0 }
>    default_value { CYGINT_ISO_MEMALLOC != 0 }
>

Shouldn't it use "calculated_value" instead of "default_value"?

Cheers

Re: lwIP upgrade to CVS HEAD

[Simon Kallweit]

David Fernandez wrote:
> John Dallaway wrote:
>> There's some scope for improving the dependency info. For example,
>> CYGIMP_LWIP_MEMP_MEM_MALLOC could specify:
>>
>>    requires { CYGINT_ISO_MEMALLOC != 0 }
>>    default_value { CYGINT_ISO_MEMALLOC != 0 }
>>
> 
> Shouldn't it use "calculated_value" instead of "default_value"?

No, calculated_value would not let the user configure lwIP to use the 
internal allocator when memalloc is available. But I think there are 
scenarios where this is feasible, as it keeps all allocations for lwIP 
in a "safe place", not mixing up with the rest of the system. I was 
wondering to leave the default_value at 0, but I guess most users would 
expect lwIP to use the system memory allocator when not otherwise 
specified, so John's suggestion is fine I think.

Simon