Codecov Report

Merging #8918 (a89edc6) into master (d84220b) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8918      +/-   ##
==========================================
+ Coverage   98.33%   98.35%   +0.01%     
==========================================
  Files         157      156       -1     
  Lines       20346    20229     -117     
==========================================
- Hits        20008    19896     -112     
+ Misses        338      333       -5     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d84220b...a89edc6. Read the comment docs.

How did you test this change? The call to netif_set_hostname() is done when wlan.active(True) is called, so if you reconfigure the hostname after that, it probably won't work.

It might be worth adding a call to netif_set_hostname() in network_cyw43's config method.

I didn't want to use an arbitrary hostname length, so I used DNS_MAX_NAME_LENGTH which is 256 bytes, but it can be redefined.

I think 256 is overkill. I'd suggest to make a new define and default to something like 16.

How did you test this change? The call to netif_set_hostname() is done when wlan.active(True) is called, so if you reconfigure the hostname after that, it probably won't work.

The idea was to support config('hostname') followed by active(True), the hostname can't (shouldn't ?) be changed after active. I tested this change and it works both with the router web interface and with avahi-resolve-address.

The idea was to support config('hostname') followed by active(True)

On esp32 you need to run active(True) before config(hostname=...), or it raises a OSError: TCP/IP IF Not Ready exception. We need to try and make these things consistent across ports.

the hostname can't (shouldn't ?) be changed after active

The hostname is not used (at least by DHCP) until you run connect(ssid, ...), so you have a chance to set the hostname between activating and connecting (that's not true though for bringing up an AP interface, which needs to be configured before activating).

We need to settle on a standard for the order of configuring a NIC. Probably it makes sense to configure before activating (eg for ETH it would, because activating means starting up the ETH interface, there is no separate connect call).

But, actually, I think this PR will still work running config after active, because netif_set_hostname() just sets a pointer to the hostname string, and you can change that string after setting it and the changes will still be seen by lwIP.

We need to settle on a standard for the order of configuring a NIC. Probably it makes sense to configure before activating

I agree, that's just the logical order, and I think it's the other port that should match this.

Where should the default hostname (i.e PYBD) and default hostname length (16) be defined ? rp2 port doesn't have an mpconfigboard_common.h file, which was my first thought.

Where should the default hostname (i.e PYBD) and default hostname length (16) be defined ?

Where you put it in cyw43.h is fine, thanks.

Rebased and merged in b6c2196 and 9dfabcd

@dpgeorge Why does MICROPY_PY_NETWORK_CYW43_USE_LIB_DRIVER disable some of the config options like security and hostname ?

@iabdalkader:
Trying to build the binaries with MICROPY_PY_NETWORK_CYW43_USE_LIB_DRIVER=0 fails.
How did you succeed and test setting hostname?

@robert-hh I'm not really sure, @dpgeorge added in some last minute changes when merging this PR. I see some errors with MICROPY_PY_NETWORK_CYW43_USE_LIB_DRIVER=0 which might not be related to this PR at all.

If I compare that to your initial commit, the #if !MICROPY_PY_NETWORK_CYW43_USE_LIB_DRIVER were added around the hostname handling statements. But then hostname support does not exist in the standard build, which sets MICROPY_PY_NETWORK_CYW43_USE_LIB_DRIVER=1.
And if I drop that conditional compile, the compile using the lib driver fails because of the missing hostname element in the cyw43_t.
Which configuration did you use for testing? The drivers/cyw43 or lib/cyw43?

I tested with stm32 port, I guess that doesn't use the open-sourced lib/cyw43.

That works, and indeed it does not use lib/cyw43. People expected support for Pico_W as well.

Damien changed the code such that it compiles at least on the rp2, but only when using the lib version. To make it conformant, lib/cyw43 had to be changed in having the hostname field in the cyw43_t as well, and the related #define symbols. Or hostname has to move into a different data structure.

Note there are other config options that get disabled as well with that define (like security), and I agree they should be compatible.

Hey, are there any updates on this?
Can the hostname be set?

It should be available in the nightly builds.

It should be available in the nightly builds.

@CubicDE @robert-hh In the nightly build for Pico W, in progress (planned for upcoming v1.20) for PYBD/Portenta.

@CubicDE #10635