Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.39%. Comparing base (db1b5df) to head (e8dd519).

@@           Coverage Diff           @@
##           master   #14015   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         161      161           
  Lines       21200    21200           
=======================================
  Hits        20860    20860           
  Misses        340      340           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I just noticed that esp8266 and esp32 have these constants named AUTH_OPEN and AUTH_WPA_PSK. Furthermore, they live in the network module, not the network.WLAN class.

We'll need to make the cyw43 (and nina, esp-hosted) drivers match that.

We've had those for a few years now, and there's many scripts out there that use them, if we must use the prefix AUTH_ can we add aliases so we don't break scripts ?

    // Network is not secured.
    { MP_ROM_QSTR(MP_QSTR_OPEN),                MP_ROM_INT(NINA_SEC_OPEN) },
    // Security type WEP (40 or 104).
    { MP_ROM_QSTR(MP_QSTR_WEP),                 MP_ROM_INT(NINA_SEC_WEP) },
    // Network secured with WPA/WPA2 personal(PSK).
    { MP_ROM_QSTR(MP_QSTR_WPA_PSK),             MP_ROM_INT(NINA_SEC_WPA_PSK) },

You can keep the existing constants in the nina/esp-hosted WLAN objects, for backwards compatibility.

But adding new ones, they should go in network and be prefixed with AUTH_.

But adding new ones, they should go in network and be prefixed with AUTH_.

That would still break the networking scripts that have been using those constants, because the same scripts run on CYW43, Nina and ESP hosted.

That would still break the networking scripts that have been using those constants, because the same scripts run on CYW43, Nina and ESP hosted.

But cyw43 doesn't currently have any constants, so those scripts don't work on cyw43 at the moment.

Is it possible to update the networking scripts to use network.AUTH_xxx, and then require a firmware update to use these new scripts?

But cyw43 doesn't currently have any constants, so those scripts don't work on cyw43 at the moment.

That true, this PR is to make CYW43 compatible with the existing networking scripts by adding the missing constants. If we add different constants, existing scripts won't run and we'll need to change every other interface to match. Right now Nina, ESP hosted use those constants, and one more driver that's not in MicroPython (a WINC1500 very similar to Nina driver in structure).

Is it possible to update the networking scripts to use network.AUTH_xxx, and then require a firmware update to use these new scripts?

I will have to change 3 network interfaces and some examples but it will still break user code.

it will still break user code

How will it break user code if the old constants are left as they are?

You could also update the network scripts to try network.AUTH_xxx first, and fall back to network.WLAN.xxx.

Do the network scripts currently work on ARDUINO_NANO_ESP32?

How will it break user code if the old constants are left as they are?

The networking scripts expect those constants to be in network.WLAN.xxx so they will not run on CYW43. The goal is to have a common set of networking scripts, which we have right now.

Note if we add constants network.AUTH_xxx they will be enums, and then network interface drivers will need to be changed to map those constants to internal values, right ? It's a good idea, I like it better than duplicating the standard constants, but I'm just pointing out that it will require changes to all drivers.

Do the network scripts currently work on ARDUINO_NANO_ESP32?

No they don't but we don't support any ESP boards.

Just discussing this with @jimmo and we may actually want to go the other way: move everything out of network and into individual classes. Ie, this PR is correct (although I still think the constants need to be named AUTH_xxx).

Also then need to move things like network.PHY_xxx to network.LAN.PHY_xxx, etc.

The main justification for the above is the way constants work elsewhere, e.g. Pin.X rather than on machine directly. We should do the same for network.

There's one more issue with AUTH_xxx it doesn't match the keyword arg, security=AUTH_xxx should be SEC_ if we must have a prefix.

Also then need to move things like network.PHY_xxx to network.LAN.PHY_xxx, etc.

The main justification for the above is the way constants work elsewhere, e.g. Pin.X rather than on machine directly. We should do the same for network.

machine groups many modules, each one has its own constants, that aren't shared with any other module. network's constants on the other hand are common, standard constants that can be shared between all networking interfaces, and will result in less duplicate constants. I assume this change would also include networking.STA/AP_IF ? This is a very breaking change, every script assumes those are accessible from network. I don't think this is a good change, those constants should all be in network.

I assume this change would also include networking.STA/AP_IF ? This is a very breaking change, every script assumes those are accessible from network

It happens to me regularly that I type WLAN.STA_IF, because I expect it there. Just to remember that it's network.STA_IF, which seem not to be natural. So maybe there can be a period where both variants exist.

It happens to me regularly that I type WLAN.STA_IF, because I expect it there

If all constants are moved to one module, regardless of what that module is, instead of being split between network and WLAN, there should be less confusion about this. However I still think they should be in network, because this way they can be shared, instead of having to add the same constants to every network interface. Note that the ports in question, esp32 and esp8266, already have them in network via MICROPY_PY_NETWORK_MODULE_GLOBALS_INCLUDEFILE. I guess this config option, and include files, will not be needed either.

Note that stm32 and mimxrt already have network.LAN.PHY_xxx constants. It makes sense to have these PHY constants in the LAN class because:

1. Other LAN drivers may have a different set of PHYs that they support, and hence different constants.
2. Other LAN drivers may have different values for the PHY constants.

Similarly for network.WLAN, if you have multiple WLAN drivers compiled in then each one may have a different set of constants (different AUTH modes available, and they may have different values for their constants).

Similarly for network.WLAN, if you have multiple WLAN drivers compiled in then each one may have a different set of constants (different AUTH modes available, and they may have different values for their constants).

It's true some drivers will not support all modes, and constants have different values, but that's not a big problem we can use an enum and map to the actual value in drivers and fail if security mode is not supported.

I've pushed a change to move the constants to WLAN, they have to be duplicated in every driver. Note I used AUTH_ but this doesn't match the keyword arg, it should be SEC_.

I think we should put the constants on the class. It keeps the constants grouped together where they belong and with the class that they are used for.

There aren't going to be that many cases where there are common constants, eg WLAN and ETH have pretty much nothing in common.

Note I used AUTH_ but this doesn't match the keyword arg, it should be SEC_.

Yes, I agree, the constants should start with SEC_. I think we should do that here.

I assume this change would also include networking.STA/AP_IF ?

Yes... I think we should also move those constants to their class. And while we are moving them, we can rename them to IF_STA and IF_AP, to have a common prefix, to match all the other groups of constants.

Of course we would retain the old constants for backwards compatibility (eg until MicroPython 2.0).

Of course we would retain the old constants for backwards compatibility (eg until MicroPython 2.0).

Does this also mean that we should keep both AUTH_ and SEC_ ?

Updated. I've kept the existing constants to maintain backwards compatibility. As for esp ports, the constants in network.XXX are the same, included via the config file, so it should still be backwards compatible.

Documentation, folks! As far as I can tell these are not documented anywhere.

I'm happy to add documentation but it's not clear to me what is port-specific and what is general...

This is a real example: I was trying to create a simple, secured AP on ESP32 recently and couldn't figure out the appropriate configuration. Old forum posts with now-obsolete items make things worse - so I think we need to add a little more to the docs.

I'm happy to add documentation but it's not clear to me what is port-specific and what is general...

When ports register network interfaces they either uses network.LAN or network.WAN, with the exception of things like WIZNET5K which uses its own name. So I think the constants should be documented here https://docs.micropython.org/en/latest/library/network.WLAN.html with notes that some modes might not be supported.