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

docs: Update docs to replace ifconfig with ipconfig. #14140

Merged
merged 1 commit into from
Jul 5, 2024

Conversation

felixdoerre
Copy link
Contributor

Follow up for #13689, to update the documentation to refer to ipconfig instead of ifconfig where appropriate.

I've left wipy out, because I couldn't find, which network stack it is using. Also, I am not sure which network stack exactly network.WLAN and network.LAN are referring to. Should I just copy the adjustments form network.rst over to network.WLAN/network.LAN?

@felixdoerre felixdoerre force-pushed the ipv6_docs branch 2 times, most recently from 79f01be to a8e5c56 Compare March 20, 2024 20:37
@dpgeorge
Copy link
Member

Thanks for writing these docs, definitely needed!

But, ipconfig() is not yet implemented on esp32 or esp8266 ports. So the docs there shouldn't change. And it means mpremote still needs to use the old ifconfig().

Well, probably it would be better to implement ipconfig() on all the ports first, then we can make a consistent move to that new method, in the docs and everywhere.

@dpgeorge
Copy link
Member

These are the network interfaces that need ipconfig() added to them:

  • extmod/network_ninaw10.c
  • ports/cc3200/mods/modwlan.c
  • ports/esp32/network_lan.c
  • ports/esp32/network_ppp.c
  • ports/esp32/network_wlan.c
  • ports/esp8266/network_wlan.c
  • ports/mimxrt/network_lan.c
  • ports/stm32/network_lan.c

Some of those (eg stm32, mimxrt) are very easy.

@robert-hh
Copy link
Contributor

These are the network interfaces that need ipconfig() added to them:

I can start working in these. I have all the boards needed for testing.

@felixdoerre
Copy link
Contributor Author

Yes, @robert-hh, that would be very nice. I have none of these boards :). So I will not touch the networking code for the other ports, that you can do and test the necessary changes.

@robert-hh
Copy link
Contributor

@felixdoerre We can also co-operate in that you suggest changes and I will test them.

@dpgeorge
Copy link
Member

To start with I suggest just implementing the IPv4 config options for ipconfig(), on those additional NICs. That should be relatively straightforward, and would be enough to allow this docs PR to be merged.

@robert-hh
Copy link
Contributor

robert-hh commented Mar 27, 2024

So mimxrt and stm32 were indeed very easy. There others are more work, when they do not use the common LWIP stack. So the ipconfig() method has to be fully implemented, which will take a while. I'm not sure if it's worth the effort for the CC3200 port, since that one is due to be closed.

@felixdoerre
Copy link
Contributor Author

I've started a PR (#14199) to update the configuration function for the other network interfaces. Their current state is also not too consistent. The lwip-variants should all be included in there. I am not sure how consistent we will be able to make the new ipconfig interface.

But, ipconfig() is not yet implemented on esp32 or esp8266 ports. So the docs there shouldn't change. And it means mpremote still needs to use the old ifconfig().

Judging from what is documented (and being a kind-of devil's advocate), we'd only need to implement the esp-netif netconfig for esp32 and esp8266. All the other network interfaces are not documented explicitly anyways, right?

Regarding mpremote: note that this is just a configuration example. If this has ipconfig of ifconfig should not really matter, but I have no strong preference, which one is better.

@@ -56,16 +56,6 @@ Methods
Returns ``True`` if the physical Ethernet link is connected and up.
Returns ``False`` otherwise.

.. method:: WIZNET5K.ifconfig([(ip, subnet, gateway, dns)])
Copy link
Member

Choose a reason for hiding this comment

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

This is being removed because it's documented in the AbstractNIC class, right?

Maybe there needs to be some note in this file that this class does extend the AbstractNIC type and so does have the ifconfig/ipconfig methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, also there is also .active, and .status neither are directly documented here and only implicitly inherited. Interestingly the wiznet5k-class does not implement all functions from AbstractNIC (like .scan) but even before the change it implemented more than documented here.

To make it kind-of consistent we could remove the docs for WIZNET5K.isconnected and then add a paragraph linking to AbstractNIC for: isconnected, active, ifconfig (maybe leave that out to encourage ipconfig instead), ipconfig, config and status.

Copy link
Member

Choose a reason for hiding this comment

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

To make it kind-of consistent we could remove the docs for WIZNET5K.isconnected and then add a paragraph linking to AbstractNIC for: isconnected, active, ifconfig (maybe leave that out to encourage ipconfig instead), ipconfig, config and status.

Yes. But that sounds like something for a separate PR.

docs/library/network.rst Outdated Show resolved Hide resolved
AbstractNIC.ipconfig(param=value, ...)

Get or set interface-specific ip-configuration interface parameters.
Supported parameters are (availability subject to compile-time flags):
Copy link
Member

Choose a reason for hiding this comment

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

I suggest "(availability of a particular parameter depends on the port and the specific network interface)". Because users see the port/NIC and not so much which compile time flags are enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this wasn't changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh yes, I changed the other occurrence. Now I should have both.

docs/library/network.rst Outdated Show resolved Hide resolved
@felixdoerre
Copy link
Contributor Author

I think I've incorporated all suggestions and was a bit more thorough to remove ifconfig from the docs as now all nics should have ipconfig.

@felixdoerre felixdoerre force-pushed the ipv6_docs branch 2 times, most recently from 635df06 to 8f34cff Compare June 4, 2024 07:42
AbstractNIC.ipconfig(param=value, ...)

Get or set interface-specific ip-configuration interface parameters.
Supported parameters are (availability subject to compile-time flags):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this wasn't changed?

* ``0x40`` The address is duplicated and can not be used.

This method can be used to set a static IPv6
address.
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs more detail on how to set the IPv6 address. Is the thing you set it to a 4 tuple in the same format as returned?

Copy link
Contributor Author

@felixdoerre felixdoerre Jun 7, 2024

Choose a reason for hiding this comment

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

I have an example immediately after the property name, but I'll add it again here. Setting the address as 4-tuple doesn't really make sense. State is not determined by configuration but by duplicate-address-detection and validity is infinite on static addresses by definition.

docs/library/network.rst Outdated Show resolved Hide resolved
Follow up to 1c6012b

Signed-off-by: Felix Dörre <felix@dogcraft.de>
@dpgeorge dpgeorge merged commit 4d16a9c into micropython:master Jul 5, 2024
6 checks passed
@dpgeorge
Copy link
Member

dpgeorge commented Jul 5, 2024

Thanks for updating, now merged.

With this done we can now officially move forward with using ipconfig() over ifconfig(), and also say we support IPv6 (at least on ports that enable it).

Thanks for all your efforts on this @felixdoerre !

@felixdoerre
Copy link
Contributor Author

So, is it now time to enable ipv6 by default on some ports, e.g. pico-W?

@dpgeorge
Copy link
Member

dpgeorge commented Jul 7, 2024

So, is it now time to enable ipv6 by default on some ports, e.g. pico-W?

Yes! That would be a good idea. Feel free to make a PR for that, or otherwise I'll eventually get to it.

@felixdoerre felixdoerre deleted the ipv6_docs branch July 7, 2024 16:29
@razvanphp
Copy link

Seems like in here, ifconfig is not marked as deprecated and ipconfig is not even mentioned:
https://docs.micropython.org/en/latest/library/network.LAN.html#network.LAN.ifconfig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants