FreeRTOS + TCP custom Ethernet protocols

I have custom implementation of 3 additional protocols ( PPPoE, LLDP, and PTPv2 on straight Ethernet frames although I will probably include multicast IP soon ). The implementations are quite crude but get the job done for me and for now. Eventually I might submit some of them for inclusion, but they still need a lot of work and I also need to consult with my employer.

Developing these and trying to keep up with the latest release of FreeRTOS+TCP has been a pain because I had to insert my patchwork into every new release. I’m proposing to make the stack more open to new protocol implementations. This would allow people to develop custom or non-supported protocols without going through my pains. Eventually if a protocol like that is stable and the community wants it, it can be submitted upstream to the developers for inclusion in the codebase. I finally was able to reorganize my code, so that the patches to the official release are minimal and I’d like to discuss those changes and do a pull request. Here’s what I boiled it down to and please feel free to comment on everything, including my naming choices.

  • I’ve added ipconfigPROCESS_CUSTOM_ETHERNET_FRAMES to FreeRTOSIPConfigDefaults with a default value of 0 of course.
  • If set to on, I’m adding an extern to the following hook function: eFrameProcessingResult_t eProcessCustomFrameHook( NetworkBufferDescriptor_t * const pxNetworkBuffer )
  • That custom hook is then called in the default case of prvProcessEthernetPacket() and it’s return controls if the buffer is released.

That is it. This is all it took. I make my custom .c file that holds the eProcessCustomFrameHook() function, I parse the frame and if I want to reuse the buffer I do and return eReturnEthernetFrame or if I don’t care for the frame, I return eReleaseBuffer.
Let me know what you think and if you believe this is worthy of a PR

Sounds pretty good Emil :+1:
I wonder if we should use an array of callbacks invoked as chain depending on their respective return codes to support multiple protocol plug-ins :thinking:
Similar to shared interrupt handling, you know.
That could allow mix and match your own and other plug-ins.

I’m not sure I understand what you mean Hartmut and I’m not familiar with shared interrupt handling. I read a little about it, but I’m not sure I understand the concept well.

Are you talking about just the ethernet level or are you expanding the concept to callbacks for anything that is not supported like: certain Eth protocols, IP protocols, etc?
Would you please elaborate a bit on your idea of chaining?

Please keep in mind one of my goals is that for example if I use the proposed scheme to develop let’s say LLDP and then the developers decide to include it, the path would be very sraight forward: cut/paste my switch code from my custom eProcessCustomFrameHook into the switch of prvProcessEthernetPacket().

We could also make a registration system where a user would call the stack and let’s say register a callback for all frames of type 0x8892 for example, and then the stack calls the user code. That approach sounds very nice but would require a lot more effort and is more of a run-time solution which I usually try to avoid on low resource systems… hence my simplistic approach.

I had something like your callback registration scheme in mind, but configured statically at compile time.
So instead of having 1 eProcessCustomFrameHook function a number of hook functions are set in an (static) array of function pointers. If configured a small loop invokes each callback one by one and the return code could tell that the protocol handler has consumed the ethernet packet or not. If not the next callback is invoked and so one.

I see…
In my opinion that is somewhat of a convoluted, over-complicated, and prone to user error approach. Maybe it’s just me, but function prototypes, iterating over a table where the iterator needs to be calculated by sizeof(table)/sizeof(pointer) and having that table reside in user code… I know it’s perfectly doable, I just try to avoid that approach because I’ve messed it up myself plenty of times.

I can see the benefit of this table creating a clear split between all the custom handlers instead of forcing the user write their own function that fans out to all the custom handlers. I should let you idea sink in a bit, maybe even implement it to see what it “looks” and “feels” like.

I really appreciate your input.

I clearly see your point. Maybe simpler is better applies also to this feature.
Let’s see the opinion of the maintainers.
Thanks for your effort !

Thank you for this discussion, I read it with interest.
I have seen the installable events handlers, and I agree that it would add a bit of complexity.

FreeRTOS uses both application hooks like vApplicationIdleHook(), and also function-like macros, like configASSERT().

I agree with Hartmut that simpler is better, so I propose to add a few macros.
Emil, if you like, can you show the code changes that you have made in FreeRTOS+TCP? Is it on Github somewhere?

Emil wrote:

Developing these and trying to keep up with
the latest release of FreeRTOS+TCP has been a pain

I understand that, it is frustrating. The changes are mostly related to make the code comply to the MISRA c 2012 rules.
Also, we are working on unit testing and CBMC proofs.

Thank you all for taking the time discuss this.
I’ve created a branch that demonstrates my proposal. I’ve also included a very crude implementation that will probably not compile, but it’s an illustration of how one might implement a new protocol. The code is HERE Anything in the FreeRTOSIP_Custom folder is not part of my proposal and is for illustration purposes only.

As you can see in the code, the eProcessCustomFrameHook() function closely mimics prvProcessEthernetPacket(). Imagine someone from the development team was working on developing IGMP. This could take them a while, but as far as parsing of frames goes, they would just move their code into prvProcessEthernetPacket() when that new protocol becomes official.

@htibosch , the reason I’m proposing a function hook instead of a macro, is again for lowering the chance of error. A macro is harder to step through and an error in my macro get’s “pasted” in your code, where as a function has a prototype and provide a little more compile-time error checking.

Now, don’t get me wrong, I love the idea a installable handlers. The OS already has the lists and the thread-safety to back such and approach up. I don’t mind doing some extra coding and revising my proposal to make ALL protocols go through a registration process. It will be cool and proper… I love it, but…
If you have handler registration, it is logical to have “un-registration” Whenever I’ve taken that approach, I end up having to solve the hardest part and that is the un-registration. Un-registering anything requires locks and careful planning on what code may have a reference to something you are about to un-register and free()… and here come the asserts in lists, semaphores, free, etc. It’s a mess. I have considered it, but then I ask myself: Why bother with the complication if the chances are that nobody will ever have to turn protocols on and off at runtime. Trust me I’ve talked myself out of implementing it this way more than once.

@hs2’s approach is much simpler to implement. Since it’s a static approach, one doesn’t have to implement registration/un-registration of handles.

I’ll see how this week goes and I might be able to push a branch along the lines of what @hs2 was offering with the static array of function pointers…

Let me know what you think

@epopov I fully agree with your concerns regarding dynamically un/register protocols at runtime. I can’t see broad usage of this feature worth the effort but also the subtle details. I think the static approach is fine.

I always like to visualise code:

#if ( ipconfigPROCESS_CUSTOM_ETHERNET_FRAMES != 0 )
    /* Custom frame handler. */
    eReturned = eProcessCustomFrameHook( pxNetworkBuffer );
#else
    /* No other packet types are handled.  Nothing to do. */
    eReturned = eReleaseBuffer;
#endif

That looks good to me!

You might have thought of the following: when your hook returns eFrameConsumed, you remain the owner of the packet. With the other 3 options, the IP-task remains the owner, and it will release the packet.

Do you want to create a PR for this? Or do you want us to do that?

@htibosch,
If you check out the link to my code, your code is almost exactly what I proposed.
Once we decide which approach is the best one for this scenario, I’d like to make the PR just so I get a little more practice with PRs and github

edit: in regards to ownership. Thanks for bringing that up. I will update the comments to remind the user of that.

Please also let me know what you think about the naming of the hook function. I tried making it similar to prvProcessIPPacket and eARPProcessPacket, but changed the prefix because the user hook returns an enum, thus becoming eProcessCustomFrameHook

I don’t know why prvProcessIPPacket has a “prv” prefix but I assumed it’s just leftover from times well past.

“prv” is a file scope static (private) function. These function names do not reveal their return type.

Global functions start with some letters that indicate the type of the return value.

You can read the full story on freertos.org.

Most hooks in FreeRTOS have the words “Application” and “Hook” in them:

  • vApplicationStackOverflowHook
  • vApplicationIdleHook
  • vApplicationMallocFailedHook

When a function returns an enum value, the return type would indeed be an ‘e’.

eApplicationProcessCustomFrameHook() is a bit long but it would be perfect.

We must be careful though: the first 31 characters of an identifier should make it unique. Otherwise a MISRA checker like Coverity will complain.

I haven’t seen that document. Thanks.
eApplicationProcessCustomFrameHook is certainly long while still unique. In that train of though, why not just eApplicationCustomFrameHook? We don’t need the “Process” portion, I was merely trying to mimic the IP and ARP handlers, but that shouldn’t matter.