ChibiOS Free Embedded RTOS

I keep missing the reason why you need to release buffers one by one instead of releasing them all with the descriptor when the pbuf chain is destroyed. Exactly why you cant do that in mac_lld_release_receive_descriptor()? Please a simple answer because I am unable to follow you.

Anyway, the driver looks much more complex with fragmented buffers support.

Giovanni

Several reasons:

1- mac_lld_release_receive_descriptor conceptually is meant to release the accessor, not the buffers themselves (which were the entities the accessor was used to get). The accessor contains state information to walk the frame data which is no longer necessary once the pbuf chain has been built in low_level_input (in lwipthread.c). It makes no sense to pass that information upwards to the application level, and then downwards again when the application calls pbuf_free.

2- If the accessor is meant to live outside of low_level_input function (in lwipthread.c), it must be allocated from a heap, pool or something (and not on the stack as it's done now) and then somehow attach it to the pbuf chain (to the WHOLE pbuf chain, not a single, concrete pbuf).

3- The lwIP pbuf deallocation callback mechanism (look for PBUF_FLAG_IS_CUSTOM in pbuf_free function in pbuf.c) works on a per pbuf basis. It will make a call for each pbuf in a chain, and each pbuf contains a just one buffer. So the natural way to use this mechanism forces the use of a per-buffer release function.

I will agree that (3) is party due to the multibuffer functionality (that I need) and in general is caused by my inclination to modify lwIP code as little as possible. If I was to implement all this "the dirty way", I would:

- Add a deallocation callback to the pbuf structure.
- Add an opaque void * argument to the pbuf structure, to be passed to the callback above.
- Modify pbuf_free to call this deallocation callback passing the opaque argument.
- Add a MACDriver pointer to the MACReceiveDescriptor structure.

Then in low_level_input (with multibuffer per frame functionality) I would build the pbuf chain for a frame, and attach to the last pbuf a callback and a pointer to the MACReceiveDescriptor. When the application calls pbuf_free, it will call my callback, which would typecast the void * argument to MACReceiveDescriptor *, grab the MACDriver pointer from it, call mac_lld_release_receive_descriptor(MACDriver, MACReceiveDescriptor), and deallocate the MACReceiveDescritor (which had to be allocated from a pool or heap in low_level_input). Still would not like the fact that I need a pool of MACReceiveDescriptors in low_level_input (and that pool should ideally have exatly the same number of elements than low level driver buffers... but that information -the number of low level driver buffers- does not belong there...)

Anyway, the driver looks much more complex with fragmented buffers support.

I beg to differ. Note that I added lots of lenghty comments to make sure everything was properly explained (and also followed your coding style to make things as easy for your as possible).

I'm also a big fan of code simplicity, but in this case I would definitely say the extra functionality is well worth it. Then again, this opinion is biased by the fact that I do need the functionality

Let me know if you'd rather stick to the one-frame-per-buffer model. I'm certainly going though more trouble than strictly necessary just to make a worthy contribution, so if it's not going to be, I might as well take a few dirty shortcuts and get over with it ASAP.

P.S: needless to say in that case I would totally understand and respect your decision.

Thanks, now it is more clear for me. About your points:

iggarpe wrote:1- mac_lld_release_receive_descriptor conceptually is meant to release the accessor, not the buffers themselves (which were the entities the accessor was used to get). The accessor contains state information to walk the frame data which is no longer necessary once the pbuf chain has been built in low_level_input (in lwipthread.c). It makes no sense to pass that information upwards to the application level, and then downwards again when the application calls pbuf_free.

This is purely a conceptual construct, you can see as well the descriptor as the object that encapsulates everything related to a frame, including buffers.

iggarpe wrote:2- If the accessor is meant to live outside of low_level_input function (in lwipthread.c), it must be allocated from a heap, pool or something (and not on the stack as it's done now) and then somehow attach it to the pbuf chain (to the WHOLE pbuf chain, not a single, concrete pbuf).

Agreed, this pool can be implemented in lwupthread, there is no need for the driver to handle the pool so it does not impact us in this moment.

iggarpe wrote:3- The lwIP pbuf deallocation callback mechanism (look for PBUF_FLAG_IS_CUSTOM in pbuf_free function in pbuf.c) works on a per pbuf basis. It will make a call for each pbuf in a chain, and each pbuf contains a just one buffer. So the natural way to use this mechanism forces the use of a per-buffer release function.

This is how lwIP see things, it knows buffers because it handles the lists by itself. I want to create a general driver model not one that caters to lwIP only. If we have to use two list-like structures is because lwip uses pbuf chains and the driver has its own lists mechanism. A more rational approach would be to let the driver handle lists in the manner more appropriate for the underlying HW architecture. I still think we should have a more rational TCP stack at some point. lwIP wants to cover too many scenarios IMHO.

iggarpe wrote:- Add a deallocation callback to the pbuf structure.
- Add an opaque void * argument to the pbuf structure, to be passed to the callback above.
- Modify pbuf_free to call this deallocation callback passing the opaque argument.
- Add a MACDriver pointer to the MACReceiveDescriptor structure.

I agree, we need a mechanism to associate a callback only to the last element of a pbuf chain and release the descriptor there. Handling buffers one by one is not really necessary, it is just how lwIP does things.

iggarpe wrote:I'm also a big fan of code simplicity, but in this case I would definitely say the extra functionality is well worth it. Then again, this opinion is biased by the fact that I do need the functionality

Let me know if you'd rather stick to the one-frame-per-buffer model. I'm certainly going though more trouble than strictly necessary just to make a worthy contribution, so if it's not going to be, I might as well take a few dirty shortcuts and get over with it ASAP.

Lets put things this way. The contributions you are working on:
1) STM32 MAC driver fixes (done, it is much better now after all the bugs you found).
2) MAC driver model enhancements to support a generic copy-less scenario (done, now the driver supports copy-less operations).
3) lwIP copy-less implementation (in progress and, IMO, this is huge).
4) Multi buffer implementation for STM32 (in progress or done).

I have problems only with points 2 and 4:
2) I want a simple, rational API that is not dictated by the lwIP way to see things.
4) I am in doubt not because I don't see the advantages but for another reason: It would be harder for me to maintain. No matter how well done it is once something is part of the trunk I have to answer questions and fix bugs so I prefer to keep things as simple as possible.
I think you should finish it and then offer it as an alternative implementation, this is why I think it should be a drop-in replacement for the current implementation. If somebody want the functionality it must be as simple as replacing the files, everything else should be transparent.
In perspective the two driver could be merged and the multi-buffer mode would become a configuration option, I have to build confidence first because the mentioned maintenance issues. Currently I am spending more time handling tickets and support than writing code...

Just to be clear, I really appreciate what you are trying to do, we are now in a much better position with the whole networking layer and it is going to be even better.

Giovanni

Giovanni wrote:2) I want a simple, rational API that is not dictated by the lwIP way to see things.
Giovanni

Given that your stated goal is easing your support burden, you would do well to adhere closely to the lwIP code and interfaces, whether or not you find them "rational". Let the lwIP devs do their thing and don't try to second guess them too much. A TCP/IP stack is a complex project. You will not do yourself any favors by trying to maintain your own.

As a case in point: they are currently working on improving the IPv6 support in lwIP. This is a pretty complex effort in itself but clearly one that must be undertaken. Do you really want to take on having to backport each new release of lwIP into the ChibiOS I/O paradigm? Wouldn't it really be less work to simply maintain a consistent interface to the unmodified lwIP code?

Hi,

jcbarlow wrote:Given that your stated goal is easing your support burden, you would do well to adhere closely to the lwIP code and interfaces

Because support burden I don't want to embed code and/or interfaces defined externally, I prefer to have both an API and a clear boundary.

Regardless my opinion on lwIP, the driver has to be neutral because lwIP is not the only stack on earth, the driver can support also uIP and commercial stacks.

jcbarlow wrote:As a case in point: they are currently working on improving the IPv6 support in lwIP. This is a pretty complex effort in itself but clearly one that must be undertaken. Do you really want to take on having to backport each new release of lwIP into the ChibiOS I/O paradigm? Wouldn't it really be less work to simply maintain a consistent interface to the unmodified lwIP code?

I have yet to find this consistent interface. You have to include the lwIP headers and fill its buffer lists, hardly an interface when you have no isolation. This is why we have this middle layer "lwipthread" that connects lwIP to the underlying driver, all the code that has to be adapted when one of the two actors changes is isolated in that module.

Giovanni

Giovanni,

Will get back to you ASAP on your message above, meanwhile, and before I forget to mention it, I noticed one minor optimization in the STM32 vanilla driver: restart the DMA engine after a frame is purged in mac_lld_get_receive_descriptor (this way DMA can make use of a purged descriptor immediately instead of having to wait until mac_lld_release_receive_descriptor is called).

Interesting, did you already test it? if so, do you mind posting a patch?

Giovanni

Interesting, did you already test it? if so, do you mind posting a patch?

Sure, just give me some time, I'm in the middle of the zero-copy multibuffer reception and the patch would not apply cleanly to your trunk.

I have tested the modified code and it works fine, but note that the code executes only when descriptors are purged due to a frame error, and as of now I don't have a way to purposely do that. Anyway, it makes total sense: as soon as we give back to DMA a descriptor which is ready to receive data, we should restart the DMA if it is stalled. And note that even if we erroneously restart the stalled DMA when there are no descriptors ready for it, it will just stall again immediately (since the RM says it tries to read the descriptor in advance before a frame is received).

QUESTION: I need a pointer to MACDriver in mac_lld_release_receive_descriptor. Would you rather add function argument or add a member to MACReceiveDescriptor and initialize it in mac_lld_get_receive_descriptor ?

Thinking in OOP that function would be a method of the descriptor not of the driver, if it cannot be avoided then I would add a member to the descriptor.

Edit: you could add a generic, application-defined, void pointer and let the upper layers use it if needed.

Giovanni