PR #65 broke the MagTag constructor in some circumstances

[denschub]

As far as I can tell, PR #65 broke the MagTag() constructor.

From the "Simple test" example in the docs, you are instructed to use the MagTag constructor to get an instance to the MagTag class, which you need to access MagTag-specific APIs:

from adafruit_magtag.magtag import MagTag

magtag = MagTag()
magtag.add_text(
    text_position=(
        50,
        (magtag.graphics.display.height // 2) - 1,
    ),
    text_scale=3,
)

that constructor ends up calling the Network class, which then ends up calling the constructor from PortalBase' Network:

https://github.com/adafruit/Adafruit_CircuitPython_MagTag/blob/32ec2d95131c5dea8ba33093cb3c5ad92133c9d5/adafruit_magtag/network.py#L70-L74

This is called without any secrets, and that's fine because PortalBase defaults to None. However, if you don't pass None, this piece of code will still be executed:

Adafruit_CircuitPython_PortalBase/adafruit_portalbase/network.py

Lines 114 to 119 in fc73ddc

	self._secrets_network = [
	{
	"ssid": self._secrets["ssid"],
	"password": self._secrets["password"],
	}
	]

secrets here is the global secrets import. This code assumes that ssid and password will always be defined at the root level of the secrets. However, this assumption isn't really documented anywhere, and I've been previously fine with having a secrets structure of, for example,

secrets = {
  "api_root": "https://example.com",
  "wifi": {
    "ssid": "example",
    "password": "example"
  }
}

If you have a structure like mine, the aforementioned access of self._secrets["ssid"] and self._secrets["password"] will fail with a KeyError.

In my very humble opinion, this whole code should make sure that the secrets are actually there, and if not, it probably should just not attempt to connect to a network. Users can connect manually via wifi.radio.connect anyway, and provide the credentials there. Moreover, even if "you need to provide the SSID and password in the secrets root if you want to use networking" would be a acceptable compromise, this would still break the library for users who want to use MagTag-specific things, but aren't actually interested in networking. So either way, a bit of extra protection probably doesn't hurt. :)

I would submit a PR, but unfortunately I'm not set up to build CircuitPython myself, and it looks like it's quite a task to get that done... :(

/cc @tekktrik

[tekktrik]

The secrets dictionary always needed the ssid and password defined at the base level even before #65, if you're using the secrets dictionary from the user-made secrets import.

I'm a little unclear as to what you are saying, so please correct me if I'm wrong - you're saying that the issue is that users are required to provide networking information even if they don't want to use that functionality due to the parsing of the secrets information during initialization, correct? If so, I can generate a PR to fix that.

[denschub]

The secrets dictionary always needed the ssid and password defined at the base level even before #65, if you're using the secrets dictionary from the user-made secrets import.

Not really, no. If you're using magtag.network.connect() then yes - you're right. But there's also:

import wifi
wifi.radio.connect("ssid", "password")

That API is documented, and it's even used on learn.adafruit.com in the "CircuitPython Internet Test", so I assume that's a valid way to connect to the network as well? I have been using wifi.radio.connect for a while now without issue, and I've never had ssid/password at the root level.

The main reason I hope to continue using that API instead of calling out to magtag.network.connect() is the recent addition of wifi.radio.set_ipv4_address, allowing you to skip DHCP, which would - in theory - be handy!

you're saying that the issue is that users are required to provide networking information even if they don't want to use that functionality due to the parsing of the secrets information during initialization, correct? If so, I can generate a PR to fix that.

Yeah. Sorry if that was not clear. :) With how it is right now, you can't even get the "SImple test" I linked above running - it will error out with a KeyError, even though that example is not using networking at all. I think that's rather confusing.

[tekktrik]

Okay, got it, I think I understand the issue. I'll patch that.