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

Add LwIP Ethernet example using tap network driver #494

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lschuermann
Copy link
Member

@lschuermann lschuermann commented Feb 16, 2025

This adds an example application running an LwIP-based TCP/IP network
stack and HTTP server based on the EthernetTapDriver kernel capsule,
and the streaming process slice abstractions.

It includes commits from the following other PRs, which I will remove
once they have been merged:

@lschuermann lschuermann force-pushed the dev/lwip-ethernet-tap-example branch from e3e5d40 to 845a712 Compare February 16, 2025 20:43
@lschuermann
Copy link
Member Author

@bradjc @ppannuto I couldn't figure out how to exclude the lwip/lwip submodule from the formatting pass. I assume I'm missing something obvious, as it does work for all the other submodules...

Formatting ./lwip_ethernet_tap
../../lwip/Makefile:14: ../../lwip/lwip/src/Filelists.mk: No such file or directory
make: *** No rule to make target '../../lwip/lwip/src/Filelists.mk'.  Stop.
 ⤤ Failure formatting ./lwip_ethernet_tap
Rebuilding Verbose: ./lwip_ethernet_tap

*alarm_fired = true;
}

int main(void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool!

Can you add a README describing what this does and what is expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to include an example of "it worked on this board at this Tock release"

@bradjc
Copy link
Contributor

bradjc commented Feb 17, 2025

@lschuermann

libtock-c/u8g2/Makefile

Lines 25 to 26 in 14e590a

# Avoid failing in CI due to warnings in the library.
override CPPFLAGS_$(LIBNAME) += -Wno-error

@bradjc
Copy link
Contributor

bradjc commented Feb 17, 2025

Need a https://github.com/tock/libtock-c/blob/master/lvgl/Makefile.setup and can you add a readme to the libtock/util folder to explain what goes in it? I assume we want it to be only be utilities related to abstractions provided by the tock kernel.

@lschuermann lschuermann force-pushed the dev/lwip-ethernet-tap-example branch from 487fdd8 to c2bcca3 Compare February 20, 2025 23:58
This adds userspace helpers implementing the "streaming process slice"
contract as implemented in the Tock kernel in [1]. Instead of working
with two separate buffers, these helpers present an interface similar
to a regular read-write allow based on a single buffer. Internally,
this buffer is then subdivided into an "application-owned" and
"kernel-owned" part which are swapped atomically. The caller is
responsible for keeping a `streaming_process_slice_state_t`, and is
provided with ephemeral references to the kernel-written payloads on
each swap operation. The full, original buffer can be reclaimed by
deinitializing the `streaming_process_slice_state_t` struct.

Currently, these helpers are quite minimal and do not expose the
ability to set any optional flags (such as `halt`). They are tested to
work against against the interface implemented in [1] (with the fix of
[2] included) as part of the LwIP Ethernet tap-driver userspace app.

[1]: tock/tock#4208
[2]: tock/tock#4343
Instead of internally splitting a single buffer to create two halves
used in the streaming process slice abstraction, this changes it to
accept two separate buffers for the kernel- and application-owned
buffer respectively.

This was discussed on the pull request introducing this abstraction:
#492 (comment)

Suggested-by: Branden Ghena <[email protected]>
@lschuermann lschuermann force-pushed the dev/lwip-ethernet-tap-example branch from c2bcca3 to 911c62a Compare February 21, 2025 00:03
@lschuermann
Copy link
Member Author

lschuermann commented Feb 21, 2025

@lschuermann

libtock-c/u8g2/Makefile

Lines 25 to 26 in 14e590a

# Avoid failing in CI due to warnings in the library.
override CPPFLAGS_$(LIBNAME) += -Wno-error

@bradjc Still doesn't fix the formatting errors on the lwip submodule, unfortunately. Any hint on how to disable uncrustify for the submodule?

@lschuermann lschuermann force-pushed the dev/lwip-ethernet-tap-example branch from 911c62a to 55d5027 Compare February 21, 2025 00:05
Copy link
Contributor

@brghena brghena left a comment

Choose a reason for hiding this comment

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

Some small things and reminders. But this is just about ready to go once you have time to update it.

static void frame_rx_upcall(__attribute__((unused)) int p0,
__attribute__((unused)) int p1,
__attribute__((unused)) int p2,
__attribute__((unused)) void* data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy-paste error: data is actually used in this function

}

// LwIP packet (Ethernet frame) transmission callback:
static err_t tapif_output(__attribute__((unused)) struct netif* tapif,
Copy link
Contributor

Choose a reason for hiding this comment

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

tapif is used in this function

*alarm_fired = true;
}

int main(void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to include an example of "it worked on this board at this Tock release"

# submodule. This is not to be confused with `$(LIBNAME)_DIR` (expands to
# `lwip_DIR`), which is the top-level Tock directory name:
LWIPDIR := $($(LIBNAME)_DIR)/$(LIBNAME)/src
include $($(LIBNAME)_DIR)/lwip/src/Filelists.mk
Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed on the Network working group call moving this to include a vendored file, as the submodule might not be initialized yet by the time this line runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think we just copy the makefile and be done with it. Or just copy the lists. It's only ~100 source files.

lwip/Makefile Outdated
override CPPFLAGS += -I$(lwip_DIR)/include -I$(lwip_DIR)/include/lwip

# Avoid failing in CI due to warnings in the library.
override CPPFLAGS_$(LIBNAME) += -Wno-error
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
override CPPFLAGS_$(LIBNAME) += -Wno-error
override CPPFLAGS_$(LIBNAME) += -Wno-error

@@ -0,0 +1,3 @@
include $(TOCK_USERLAND_BASE_DIR)/lwip/Makefile
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be here. This makefile should only contain settings to help build an app using this library.

# submodule. This is not to be confused with `$(LIBNAME)_DIR` (expands to
# `lwip_DIR`), which is the top-level Tock directory name:
LWIPDIR := $($(LIBNAME)_DIR)/$(LIBNAME)/src
include $($(LIBNAME)_DIR)/lwip/src/Filelists.mk
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think we just copy the makefile and be done with it. Or just copy the lists. It's only ~100 source files.

@ppannuto
Copy link
Member

ppannuto commented Mar 6, 2025

Okay, I believe all the make/build nonsense should be fixed. One thing I'm not 100% sure is okay is 3552552, but I don't really know what else to do here.

I think it's okay because newlib doesn't undef before trying to define BYTE_ORDER, so hopefully if there were ever a conflict due to include ordering, it'd show up as a redefinition warning?

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.

5 participants