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

pkg/lwip: implement netif_get_name() #16741

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Aug 15, 2021

Contribution description

This function is needed so we can use netutils_get_ipv6() with LWIP.
The code is from _netif_list().

Testing procedure

Issues/PRs references

#16739

@benpicco benpicco requested a review from miri64 as a code owner August 15, 2021 11:50
@benpicco benpicco requested review from fjmolinas and jia200x August 15, 2021 11:50
@github-actions github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports labels Aug 15, 2021
@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Area: network Area: Networking Area: pkg Area: External package ports labels Aug 15, 2021
pkg/lwip/contrib/_netif.c Outdated Show resolved Hide resolved
@benpicco benpicco removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 24, 2021
@github-actions github-actions bot added the Area: pkg Area: External package ports label Aug 24, 2021
@benpicco benpicco force-pushed the net_lwip_netif_get_name branch from f51593f to 8b26b1e Compare August 24, 2021 11:04
@miri64
Copy link
Member

miri64 commented Aug 24, 2021

You forgot to add testing procedures. How can I test this?

@benpicco
Copy link
Contributor Author

I'm afraid there is no test case for netif_get_name()

@miri64
Copy link
Member

miri64 commented Aug 24, 2021

You could use the function in tests/lwip ;-).

@chrysn
Copy link
Member

chrysn commented Aug 25, 2021

Well as for testing, I can recommend the procedure of #16378:

  • Inside the gcoap example, remove all gnrc USEMODULE lines, and add the ones listed there in green.
  • Run on native. Before this PR, this produces build errors; after, it builds.

I don't know an actual code path to trigger the new function, though: I would have expected the breakpoint on the function to be hit when entering coap get fe80::157b:e0b6:d952:d2ca%ET0 5683 /.well-known/core, but it wasn't, because netif_get_by_name_buffer iterates over netif_list, and that's NULL in my debugger ... which is where I don't know the lwip/netif parts well enough to follow further.

@miri64
Copy link
Member

miri64 commented Aug 25, 2021

I don't know an actual code path to trigger the new function, though:[…]

I guess ifconfig should suffice, once the function is added to the shell command :-)

@chrysn
Copy link
Member

chrysn commented Aug 25, 2021 via email

@miri64
Copy link
Member

miri64 commented Aug 25, 2021

Yepp, the new function's functionality was taken from there, but it is at the moment only duplicating the code there instead of calling the function straight-away (which I was asking both to decrease duplication and allow for testing this function :-))

@github-actions github-actions bot added the Area: sys Area: System label Aug 25, 2021
@benpicco benpicco force-pushed the net_lwip_netif_get_name branch from 11089b3 to 9cf149c Compare August 25, 2021 20:05
@benpicco
Copy link
Contributor Author

Well there you go

main(): This is RIOT! (Version: 2021.10-devel-358-g11089-net_lwip_netif_get_name)
Running mqtt paho example. Type help for commands info
> ifconfig
ifconfig
Iface ET0 HWaddr: 7a:37:fc:7d:1a:af Link: up State: up
        Link type: wired
        inet6 addr: fe80:0:0:0:7837:fcff:fe7d:1aaf scope: link

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 25, 2021
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK! Ran tests/lwip on both master and this PR. The test still passes and the ifconfig output stays the same.

@miri64
Copy link
Member

miri64 commented Aug 25, 2021

(only ran on native, but the code looks very portable, so it should pass on other boards too).

@benpicco benpicco force-pushed the net_lwip_netif_get_name branch from 9cf149c to 50c5848 Compare August 25, 2021 21:31
pkg/lwip/Makefile.dep Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the net_lwip_netif_get_name branch from 50c5848 to 4a00361 Compare August 25, 2021 22:36
pkg/lwip/Makefile.dep Show resolved Hide resolved
pkg/lwip/contrib/_netif.c Show resolved Hide resolved
@benpicco benpicco force-pushed the net_lwip_netif_get_name branch 2 times, most recently from 33922c8 to 2cf2120 Compare August 26, 2021 10:36
pkg/lwip/Makefile.dep Outdated Show resolved Hide resolved
@benpicco benpicco force-pushed the net_lwip_netif_get_name branch from 2cf2120 to 4464999 Compare August 26, 2021 11:20
@benpicco benpicco merged commit 149de73 into RIOT-OS:master Aug 26, 2021
@benpicco benpicco deleted the net_lwip_netif_get_name branch August 26, 2021 12:38
@miri64
Copy link
Member

miri64 commented Aug 26, 2021

🚀 🎉

@benpicco benpicco added this to the Release 2021.10 milestone Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants