-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
extmod/network: Implement IPv6 API to set and retrieve nic configuration. #13689
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13689 +/- ##
=======================================
Coverage 98.39% 98.39%
=======================================
Files 161 161
Lines 21156 21156
=======================================
Hits 20816 20816
Misses 340 340 ☔ View full report in Codecov by Sentry. |
Code size report:
|
08385a2
to
1b0f0d6
Compare
One small thing, lwIP doesn't have a concept of a routing table only a default interface (global variable So suggest exposing this, for the current API it would be And when configuring an interface:
|
This setting (and So I guess configuring |
(I made a small edit on the PR description to clarify this is a per-NIC instance method.) |
In #12600 (comment) you distinguished between NIC vs global settings, eg Does it make sense to keep this separation and put |
Thanks for the PR @felixdoerre ! It's going to be a bit confusing having both an Fortunately the existing
So the change would be:
We discussed the relative merits of the way the So, the existing cases would invoke the call operator on the config object, allowing: print(nic.ipconfig("foo")) # nic->attr->lwipipconfig->call("foo")
nic.ipconfig(foo=1) but now you'd also be able to write
this also lets us retain the Let's do that part in a future PR though :) Some other comments:
|
const char *addr_str = ipaddr_ntoa(netif_ip_addr6(netif, i)); | ||
mp_obj_t tuple[4] = { | ||
mp_obj_new_str(addr_str, strlen(addr_str)), | ||
MP_OBJ_NEW_SMALL_INT(netif_ip6_addr_state(netif, i)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the API quirks elsewhere in micropython networking is not standardising the constant values across ports/implementations. (e.g. wireless security modes, or error codes).
So if e.g. we need to implement this on a non-LWIP NIC, we should implement a static mapping to defined constant values.
But in this case, are these values actually bool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addr_state
is a flags variable, containing multiple bits. They are defined in ip6_addr.h:283
following. They contain information if an address is valid, tentative, duplicated, and if it is preferred.
The other values pref_life
/valid_life
are numeric timers that are decreasing over timed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks for the explanation. So we probably need to add constants for them then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add constants to the network module, but rather "bring-your-own-constants" to the docs, like it is done for bluetooth, as I would say, those constants are rarely needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the pref_life
/valid_life
values, I think it's fine to leave them as pure numbers, representing a count. So they don't need constants.
For addr_state
, that could have different values when/if ipconfig()
is implemented on non-lwIP NICs. So we either need to:
- provide constants in the network module itself
- standardise the bit fields/values of
addr_state
and enforce them to be the same for all implementations of all NICs, and then document these bits/values
I think the latter option is the best. And then for the "standard" bits/values that we choose, it can be the same as what lwIP exposes. That means that for now the code here can stay as-is, but if lwIP changes these values we will need to provide a translation mapping the new back to the old, so users can always rely on the same values working. And then we need to document these lwIP values as our standard.
b913020
to
a3cb0c9
Compare
First of all, thank you very much for your detailed responses and review comments. I've resolved most comments, except the one on the "wait"-logic for dhcp4 and autoconf6, as it does not seem clear if we want to keep it. And the concerns regarding compatibility. Regarding the questions posed:
I made it blocking, to mimic the current "ifconfig" behavior. Both "autoconf6" and "dhcp4", can easily be made non-blocking, and checking if they completed can be done, like in C-code from python, by checking if corresponding IP-addresses are there. If that's wanted, I'll remove the blocking logic.
Yes, I'll just skipped it out of lazyness. I've added the separation.
The gateway is set with a router advertisement. That is independent of slaac and of dhcp6 and cannot be disabled. (see nd6.c:569). You can only disable the automatic address derivation.
I think autoconf6 is the better name. It's called this was in lwip, and it is symmetrical to "autoconf4" (which we could want to implement/enable later). In linux the feature is also called
Yes, that's true.
I'm also not sure, but I found this inconsistency to be the best option. Accepting a tuple seems weird, and outputting in Slash-notation is probably worse for consumers who only want the address.
I'd leave that out for the future. I am not completely sure how one would extend the api, but one option would be to have
Yes, we can implement this in a future PR in the "global" function.
I'm not sure how non-LWIP builds handle network. Regarding compatibility: I'd want to have a clear path for deprecation and removal of So regarding the 2 points I'd suggest this: Compatibility I'd suggest to keep the both methods separate and drop |
63fe1ff
to
120fdb5
Compare
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
Could we please also add a |
Yes, sure. Keeping blocking without being able to deactivate is not good, I think no-one disputed that. I'm hesitant to add a Thinking about it again, I see two options:
Option 2: Make the
What do you all think about "Option 2"? If we all agree, I'd go forward with that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @felixdoerre for updating!
Happy to go with the new ipconfig
being separate to the now-deprecated ifconfig
, and also using autoconf6
as the name.
Now that the API is agreed on, are you able to write some initial documentation for this new method? (Or let us know and we can do this in a follow-up PR). Thanks
const char *addr_str = ipaddr_ntoa(netif_ip_addr6(netif, i)); | ||
mp_obj_t tuple[4] = { | ||
mp_obj_new_str(addr_str, strlen(addr_str)), | ||
MP_OBJ_NEW_SMALL_INT(netif_ip6_addr_state(netif, i)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks for the explanation. So we probably need to add constants for them then?
extmod/network_cyw43.c
Outdated
@@ -321,6 +321,13 @@ STATIC mp_obj_t network_cyw43_ifconfig(size_t n_args, const mp_obj_t *args) { | |||
} | |||
MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(network_cyw43_ifconfig_obj, 1, 2, network_cyw43_ifconfig); | |||
|
|||
mp_obj_t network_nic_config(struct netif *netif, size_t n_args, const mp_obj_t *args, mp_map_t *kwargs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be mod_network_nic_ipconfig
(mod
for consistency with the existing convention for non-static methods, ip
to match the Python method name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this forward declaration should be in modnetwork.h, as is already the case for mod_network_nic_ifconfig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you would call even the method that is used to implement nic.ipconfig
"static"? I see that ifconfig
also has the mod_
prefix, but that seems wrong to me.
The whole mod_
prefix thing does not seem to be used consistently in modnetwork
currently. From your comment, I looked around other modules, like modjson
, modhashlib
and others and from that I gather:
mod_
-prefix is used for static method- no
mod_
prefix is used for non-static methods
I didn't find information about the mod_
prefix in CODECONVENTIONS.md, did I overlook it?
For concrete examples for inconsistencies in the "modnetwork" module:
micropython/extmod/modnetwork.c
Line 111 in d712feb
static mp_obj_t network_country(size_t n_args, const mp_obj_t *args) { micropython/extmod/modnetwork.c
Line 128 in d712feb
mp_obj_t mod_network_hostname(size_t n_args, const mp_obj_t *args) {
These are both static methods, one is prefixed with mod
this other isn't. (gathering from the TODO, in L125), this probably occurred during refactoring.
Concluding, I will name the static method mod_network_ipconfig
and the non-static method network_nic_ipconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you would call even the method that is used to implement nic.ipconfig "static"?
No, this cannot be static
because it's in a different compilation unit. I agree the convention is a bit loose (but I think it's at least consistent across all of the network code) is that the mod_
prefix is used only on non-static methods.
So the convention as it stands is:
- Generally, in modfoo.c, the methods should be named
foo_bar
, and the wrapper objects namedfoo_bar_obj
. - Where possible, both the methods and the wrappers should be
static
. - Where these functions need to call into hooks provided (e.g. by the port or elsewhere), the name should be something like
mp_foo_
.
There is remnants of an older convention (modjson.c, modbinascii.c, modbtree.c) where mod_
was used as a prefix for methods. In general, almost all of these are static
, because typically most modules are self-contained.
However, the two conventions seem to have blended in the network code, and the current situation is that the mod_network_
prefix is used on non-static methods (i.e. because they are required to be implemented elsewhere), and just network_
on static methods (the default).
Anyway, in this case please just match the existing way that mod_network_nic_ifconfig
works (non-static, mod_
prefix).
Concluding, I will name the static method mod_network_ipconfig and the non-static method network_nic_ipconfig
Both of these methods are non-static, and they both work in the same way.
If I were to overhaul this convention, the main thing I would fix here is that both of these methods (and the existing mod_network_nic_ifconfig
should have lwip
somewhere in their name. In other works, network_lwip.c
is a collection of helper methods that can be used by lwip-based nics. (And the especially confusing thing here is that network_lwip.c
is not like, say, network_cyw43.c
, rather it provides helper methods for e.g. network_cyw43.c
)
These are both static methods, one is prefixed with mod this other isn't. (gathering from the TODO, in L125), this probably occurred during refactoring.
network_country
is static
, but mod_network_hostname
is non-static. Nothing else calls network_country
other than via Python, whereas mod_network_hostname
is used in a bunch of places around the codebase.
Perhaps the confusion here (and what that TODO on L125) is about, is that the wrapper object (the actual Python function object) is non-static. This allows a locals dict elsewhere to use the wrapper directly.
Also possibly we're talking at cross-purposes about the meaning of static
. I am exclusively using it in the sense of the C keyword (i.e. private to this compilation unit). The fact that network.ipconfig
is "static" in sort of the Python/C++ sense (versus an instance method like nic.ipconfig) has no bearing on the "mod_" prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also possibly we're talking at cross-purposes about the meaning of static. I am exclusively using it in the sense of the C keyword (i.e. private to this compilation unit).
Yes, we were :-), that's exactly what I was thinking. I've renamed network_nic_ipconfig
to mod_network_nic_ipconfig
.
9a565cc
to
6ed07ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thank you for the review.
Regarding adding documentation, I'd like to do that in a separate PR, as network.ifconfig
is not only explicitly documented, but referenced throughout the documentation (in 12+ files), including the documentation of specific NICs.
That brought my thought to the implementation of non-cyw43-nics. Wiznet5k and esphosted seem to have an LWIP-compatible interface so I added the corresponding (conversion) method. Wiznet5 in the non-lwip mode does not have such an interface, and for ninaw10
there is no comparable interface at all (also the dhcp
special case is not handled). So I'd be inclined to leave that ipconfig
method untouched for those variants.
extmod/network_cyw43.c
Outdated
@@ -321,6 +321,13 @@ STATIC mp_obj_t network_cyw43_ifconfig(size_t n_args, const mp_obj_t *args) { | |||
} | |||
MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(network_cyw43_ifconfig_obj, 1, 2, network_cyw43_ifconfig); | |||
|
|||
mp_obj_t network_nic_config(struct netif *netif, size_t n_args, const mp_obj_t *args, mp_map_t *kwargs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you would call even the method that is used to implement nic.ipconfig
"static"? I see that ifconfig
also has the mod_
prefix, but that seems wrong to me.
The whole mod_
prefix thing does not seem to be used consistently in modnetwork
currently. From your comment, I looked around other modules, like modjson
, modhashlib
and others and from that I gather:
mod_
-prefix is used for static method- no
mod_
prefix is used for non-static methods
I didn't find information about the mod_
prefix in CODECONVENTIONS.md, did I overlook it?
For concrete examples for inconsistencies in the "modnetwork" module:
micropython/extmod/modnetwork.c
Line 111 in d712feb
static mp_obj_t network_country(size_t n_args, const mp_obj_t *args) { micropython/extmod/modnetwork.c
Line 128 in d712feb
mp_obj_t mod_network_hostname(size_t n_args, const mp_obj_t *args) {
These are both static methods, one is prefixed with mod
this other isn't. (gathering from the TODO, in L125), this probably occurred during refactoring.
Concluding, I will name the static method mod_network_ipconfig
and the non-static method network_nic_ipconfig
const char *addr_str = ipaddr_ntoa(netif_ip_addr6(netif, i)); | ||
mp_obj_t tuple[4] = { | ||
mp_obj_new_str(addr_str, strlen(addr_str)), | ||
MP_OBJ_NEW_SMALL_INT(netif_ip6_addr_state(netif, i)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not add constants to the network module, but rather "bring-your-own-constants" to the docs, like it is done for bluetooth, as I would say, those constants are rarely needed.
This commit implements a new <AbstractNIC>.ipconfig() function for the NIC classes that use lwIP, to set and retrieve network configuration for a NIC. Currently this method supports: - ipconfig("addr4"): obtain a tuple (addr, netmask) of the currently configured ipv4 address - ipconfig("addr6"): obtain a list of tuples (addr, state, prefered_lifetime, valid_lifetime) of all currently active ipv6 addresses; this includes static, slaac and link-local addresses - ipconfig("has_dhcp4"): whether ipv4 dhcp has supplied an address - ipconfig("has_autoconf6"): if there is a valid, non-static ipv6 address - ipconfig(addr4="1.2.3.4/24"): to set the ipv4 address and netmask - ipconfig(addr6="2a01::2"): to set a static ipv6 address; note that this does not configure an interface route, as this does not seem supported by lwIP - ipconfig(autoconf6=True): to enable ipv6 network configuration with slaac - ipconfig(gw4="1.2.3.1"): to set the ipv4 gateway - ipconfig(dhcp4=True): enable ipv4 dhcp; this sets ipv4 address, netmask, gateway and a dns server - ipconfig(dhcp4=False): stops dhcp, releases the ip, and clears the configured ipv4 address. - ipconfig(dhcp6=True): enable stateless dhcpv6 to obtain a dns server There is also a new global configuration function network.ipconfig() that supports the following: - network.ipconfig(dns="2a01::2"): set the primary dns server (can be a ipv4 or ipv6 address) - network.ipconfig(prefer=6): to prefer ipv6 addresses to be returned as dns responses (falling back to ipv4 if the host does not have an ipv6 address); note that this does not flush the dns cache, so if a host is already in the dns cache with its v4 address, subsequent lookups will return that address even if prefer=6 is set This interface replaces NIC.ifconfig() completely, and ifconfig() should be marked as deprecated and removed in a future version. Signed-off-by: Felix Dörre <felix@dogcraft.de>
Merged! Thanks @felixdoerre for the effort on this one. |
fixes #12600
This change implements a new
<AbstractNIC>.ipconfig
function for the CYW43 NIC class, to set and retrieve network configuration for a NIC. Currently this change supports:ipconfig("addr4")
obtain a tuple(addr, netmask)
of the currently configured ipv4 addressipconfig("addr6")
obtain a list of tuples(addr, state, prefered_lifetime, valid_lifetime)
of all currently active ipv6 addresses. This includes static, slaac and link-local addressesipconfig(addr4="1.2.3.4/24")
to set the ipv4 address and netmaskipconfig(addr6="2a01::2")
to set a static ipv6 address. Note that this does not configure an interface route, as this does not seem supported by lwipipconfig(autoconf6=True)
to enable ipv6 network configuration with slaac (and wait up to 10s for an address to be received)ipconfig(gw4="1.2.3.1")
to set the ipv4 gatewasnetwork.ipconfig(dns="2a01::2")
set the primary dns server (can be a ipv4 or ipv6 address).ipconfig(dhcp4=True)
enable, and wait for (up to 10s), ipv4 dhcp. This sets ipv4 address, netmask, gateway and a dns server.ipconfig(dhcp4=False)
stops dhcp, releases the ip, and clears the configured ipv4 address.ipconfig(dhcp6=True)
enable stateless dhcpv6 to obtain a dns servernetwok.ipconfig(prefer=6)
to prefer ipv6 addresses to be returned as dns responses (falling back to ipv4 if the host does not have an ipv6 address). Note that this does not flush the dns cache, so if a host is already in the dns cache with its v4 address, subsequent lookups will return that address even ifprefer=6
is set.This interface replaces
network.ifconfig
completely, andifconfig
should be marked as deprecated and removed in a future version.