Skip to content
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

Create sDDF LWIP Library #238

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Create sDDF LWIP Library #238

wants to merge 18 commits into from

Conversation

Courtney3141
Copy link
Contributor

The changes in this PR create a reusable interfacing library for sDDF networking and LWIP. The generic components of lwip.c have been moved into a separate library file lib_sddf_lwip.c and may now be used by any microkit component that wishes to use sDDF networking and LWIP. These functionalities are largely:

  • LWIP - sDDF initialisation
  • Passing incoming packets received from sDDF to LWIP upon being notified by the sDDF network RX virt
  • Passing outgoing packets from LWIP to the sDDF network TX virt
  • Deciding whether the network virtualisers need to be notified after packet processing
  • DHCP initialisation

Currently the library (of one c file) is built as a static archive using a makefile snippet. This implicitly relies on some standard library functions such as memcpy being available. Although the current makefile snippet works fine for the echo server example, there are two issues with it going forward:

  1. If the library is to be made via makefile snippets, the makefile it is being included in must provide the necessary standard library functions.
  2. The compilation of the library is dependent on the client's LWIP configuration. This is due to the library implicitly including the client's lwipopts.h config file, as well as the SDDF_LWIP_NUM_BUFFS constant which determines how much memory needs to be allocated for pbufs (currently passed in as a build time parameter).

This library has also been integrated in LionsOS for both NFS and micropython components, and works successfully. Although since the above two issues have not yet been resolved, the library object file is made in each of the makefiles of micropython and NFS, rather than in the main Kitty and Webserver makefiles themselves.

Signed-off-by: Courtney Darville <[email protected]>
Copy link
Contributor

@wom-bat wom-bat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General idea is good, some minor issues.

examples/echo_server/echo.mk Outdated Show resolved Hide resolved
examples/echo_server/echo.mk Outdated Show resolved Hide resolved
examples/echo_server/echo.mk Outdated Show resolved Hide resolved
examples/echo_server/echo.mk Show resolved Hide resolved
@@ -92,6 +90,9 @@ ${IMAGE_FILE} $(REPORT_FILE): $(IMAGES) $(SYSTEM_FILE)

include ${SDDF}/util/util.mk
include ${SDDF}/network/components/network_components.mk
# Specify how many pbufs sDDF LWIP library requires for all clients
SDDF_LWIP_NUM_BUFFS=512
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BUFFS with two Fs looks funny. Plus (a thought) you may want a config header for this value (in lwipopts.h perhaps) instead of a Make variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the spelling.

I agree that having this constant in the makefile not being the best option, but I am at a bit of a loss as to where to put it otherwise.

I really don't think it should be in lwipopts.h, since that is an LWIP specific configuration file used by all of LWIP. It seems strange to make users put an sDDF library constant in their LWIP config file, plus the sDDF LWIP library does not actually explicitly use lwipopts.h, and it only depends on it implicitly via a different LWIP include.

I am also hesitant to make another config file just for this constant, since we already have config files for the other sDDF subsystems, and this one constant is the only thing required by the library. It can also be deduced from other sDDF queue information, but since each user of the library needs to define it separately, I am not sure how to incorporate it into the build system neatly...

Since we haven't finished finalising our solution as to how this library will be rebuilt for each user, I was thinking of holding off on having a definitive solution until after that is solved.

include/sddf/network/util.h Show resolved Hide resolved
network/lib_sddf_lwip/lib_sddf_lwip.c Show resolved Hide resolved
network/lib_sddf_lwip/lib_sddf_lwip.mk Outdated Show resolved Hide resolved
network/lib_sddf_lwip/lib_sddf_lwip.mk Outdated Show resolved Hide resolved
network/lib_sddf_lwip/lib_sddf_lwip.mk Outdated Show resolved Hide resolved
@Ivan-Velickovic Ivan-Velickovic linked an issue Sep 17, 2024 that may be closed by this pull request
Signed-off-by: Courtney Darville <[email protected]>
Signed-off-by: Courtney Darville <[email protected]>
Signed-off-by: Courtney Darville <[email protected]>
Signed-off-by: Courtney Darville <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Making it easier to write networking clients
3 participants