Skip to content

top: Common network hostname/country configuration. #10635

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

Merged
merged 9 commits into from
Feb 28, 2023

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Feb 1, 2023

The overall goal is to allow configuration of the hostname on Pico W, but also unify the way networking works across ports. Fixes #8906, #10397

This PR adds network.hostname() and network.country() to globally set the device hostname (for all interfaces) and country (for radio compliance). For interfaces that previously supported nic.config(hostname) this will continue to work, as will pyb.country() (and rp2.country()).

Additionally configure the default hostname on a per-port and per-board basis. We may want to remove this change if it's considered a breaking change to change the default hostname?

It also updates the stm32 port to use the new cyw43-driver (in the lib/cyw43-driver submodule) and removes the old version from drivers. This relies on georgerobotics/cyw43-driver#58.

Next steps (separate PR):

@github-actions
Copy link

github-actions bot commented Feb 1, 2023

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:  +220 +0.056% PYBV10[incl +20(data)]
        rp2:    +0 +0.000% PICO

@jimmo jimmo force-pushed the cyw43-new-driver branch 2 times, most recently from 4276bc0 to 5db9012 Compare February 1, 2023 05:53
@jimmo
Copy link
Member Author

jimmo commented Feb 1, 2023

This removes pyb.country for boards that don't have networking... Is that going to be a problem?

@dpgeorge
Copy link
Member

dpgeorge commented Feb 1, 2023

This removes pyb.country for boards that don't have networking... Is that going to be a problem?

It might be... the default boot.py has a pyb.country() entry regardless of networking support or not. So a lot of boards are still going to have that line in their boot.py. The idea was that even on a PYBv1.1 you might want to configure the country.

@jimmo
Copy link
Member Author

jimmo commented Feb 2, 2023

It might be... the default boot.py has a pyb.country() entry regardless of networking support or not. So a lot of boards are still going to have that line in their boot.py. The idea was that even on a PYBv1.1 you might want to configure the country.

OK. I've added a "no-op" version of pyb.country for boards that don't have networking support so that existing boards with the default boot.py don't break. I've also updated the default boot.py to use network.country and network.hostname instead.

It seems unnecessary to add the whole network module for non-networking boards just to have a .country method that does nothing.

@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2023

Codecov Report

Merging #10635 (44aee8f) into master (fc4c47f) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master   #10635   +/-   ##
=======================================
  Coverage   98.50%   98.50%           
=======================================
  Files         155      155           
  Lines       20540    20540           
=======================================
  Hits        20232    20232           
  Misses        308      308           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jimmo
Copy link
Member Author

jimmo commented Feb 7, 2023

Updated the submodule, including removing the cyw43_resource.o handling in favour of the firmware header files. Note that this breaks Pico W until the next Pico SDK update (at which point I will update this PR).

@dpgeorge
Copy link
Member

Note that this breaks Pico W until the next Pico SDK update

See #10764 which does the update to pico-sdk.

@dpgeorge
Copy link
Member

Please rebase on latest master to get the new pico-sdk (and cyw43-driver).

@jimmo
Copy link
Member Author

jimmo commented Feb 21, 2023

Please rebase on latest master to get the new pico-sdk (and cyw43-driver).

Done.

@dpgeorge
Copy link
Member

This now passes the CI.

I tested BLE and WiFi on PYBD-SF2W and it looks good, but I want to do more tests.

@dpgeorge dpgeorge added the ports Relates to multiple ports, or a new/proposed port label Feb 27, 2023
@jimmo
Copy link
Member Author

jimmo commented Feb 28, 2023

Addressed all comments.

jimmo added 4 commits March 1, 2023 01:26
This provides a standard interface to setting the global networking config
for all interfaces and interface types.

For ports that already use either a static hostname (mimxrt, rp2) they will
now use the configured value. The default is configured by the port
(or optionally the board).

For interfaces that previously supported .config(hostname), this is still
supported but now implemented using the global network.hostname.

Similarly, pyb.country and rp2.country are now deprecated, but the methods
still exist (and forward to network.hostname).

Because ESP32/ESP8266 do not use extmod/modnetwork.c they are not affected
by this commit.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This allows for a port (e.g. esp8266/esp32) to use extmod/modnetwork.c
and provide the globals dict, rather than just a list of interfaces.

When this is used, the default implementation of `network.route` and the
NIC list is not enabled.

Also splits out the LWIP-specific helpers from modnetwork.c into
network_lwip.c.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Rather than duplicating the implementation of `network`, this allows ESP32
to use the shared one in extmod.  In particular this gains access to
network.hostname and network.country.

Set default hostnames for various ESP32 boards.

Other than adding these two methods and the change to the default hostname,
there is no other user-visible change.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Rather than duplicating the implementation of `network`, this allows
ESP8266 to use the shared one in extmod.  In particular this gains access
to network.hostname and network.country.

Other than adding these two methods, there is no other user-visible change.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
jimmo and others added 5 commits March 1, 2023 01:27
This removes the previous WiFi driver from drivers/cyw43 (but leaves behind
the BT driver), and makes the stm32 port (i.e. PYBD and Portenta) use the
new "lib/cyw43-driver" open-source driver already in use by the rp2 port.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Signed-off-by: Damien George <damien@micropython.org>
Also marks wlan.config(hostname) as deprecated.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
This is for boards without networking support so that the default boot.py
continues to work.

Also update boot.py to use network.country and network.hostname instead.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Signed-off-by: Damien George <damien@micropython.org>
@dpgeorge
Copy link
Member

Thank you, this looks good now. I rebased and updated some of the commit messages (minor changes) and force pushed to this PR branch.

@dpgeorge dpgeorge merged commit 36ae5d3 into micropython:master Feb 28, 2023
@robert-hh
Copy link
Contributor

Thanks @jimmo. That was quite a piece of work, and needed.

@DataMinded
Copy link

can we add a few DHCP options in the made discover and offer requests ?
If I capture an raspberry pi 3 with hostname set DHCP discover and request packages ,I can see it includes a few more parameters, and options in the 55- Parameter Request list .
The RPI is accessable by other devices within the same network by hostname.local . this is not true for the current micropython dhcp handshake , can we add these options to it ?? Unsure where to look to try it myself

Check images below , left is RPI , right is Pico W
image

image

image
I would assume these changes would make the pico W accessable by hostname in local networks

M3t0r added a commit to M3t0r/micropython that referenced this pull request Sep 11, 2023
PR micropython#10635 introduced this limit which seems rather arbirtrary and wildly
insufficient to me.

A 16 bytes length limits means a 15 character string limit, which even
`ubinascii.hexlify(machine.unique_id())` on most ports will break. And
then assuming you just supply a random 15 byte hex string, most people
won't be able to identify the hosts function in a list on their wifi
routers.

I wanted to set my hostname to `rpi-fan-controller-{hex_uid}` and just
got a ValueError without explanation. It took me 2 hours to figure what
went wrong. Because of this I added a note about the exception and
limitation to the documentation.

Since I didn't really understand the reason for 16 bytes I set the new
limit to the size used in all involved protocols: DHCP 64 bytes, DNS 63
"octets" (characters) plus length byte, and mDNS which points to the DNS
RFC. I can imagine that some ports for small boards might want to
decrease the size of this statically allocated buffer, and from what I
gathered from the C code it would be a simple as defining
`MICROPY_PY_NETWORK_HOSTNAME_MAX_LEN`, so I mentioned this possibility
for end users in the documentation as well.

Signed-off-by: Simon Lutz Brüggen <micropython@m3t0r.de>
M3t0r added a commit to M3t0r/micropython that referenced this pull request Sep 11, 2023
PR micropython#10635 introduced this limit which seems rather arbitrary and wildly
insufficient to me.

A 16 bytes length limits means a 15 character string limit, which even
`ubinascii.hexlify(machine.unique_id())` on most ports will break. And
then assuming you just supply a random 15 byte hex string, most people
won't be able to identify the hosts function in a list on their wifi
routers.

I wanted to set my hostname to `rpi-fan-controller-{hex_uid}` and just
got a ValueError without explanation. It took me 2 hours to figure what
went wrong. Because of this I added a note about the exception and
limitation to the documentation.

Since I didn't really understand the reason for 16 bytes I set the new
limit to the size used in all involved protocols: DHCP 64 bytes, DNS 63
"octets" (characters) plus length byte, and mDNS which points to the DNS
RFC. I can imagine that some ports for small boards might want to
decrease the size of this statically allocated buffer, and from what I
gathered from the C code it would be a simple as defining
`MICROPY_PY_NETWORK_HOSTNAME_MAX_LEN`, so I mentioned this possibility
for end users in the documentation as well.

Signed-off-by: Simon Lutz Brüggen <micropython@m3t0r.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ports Relates to multiple ports, or a new/proposed port
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't set hostname in AP mode
6 participants