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

update wifi module doc types to str #6104

Merged
merged 2 commits into from
Mar 10, 2022

Conversation

FoamyGuy
Copy link
Collaborator

@FoamyGuy FoamyGuy commented Mar 2, 2022

If I understand correctly this is the resolution that was discussed here: adafruit/Adafruit_CircuitPython_Typing#5

Changed all of the ReadableBuffer typed arguments in wifi to str

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

I've looked at the low-level stuff in ports/espressif/common-hal/wifi/Radio.c, and it's not so easy. The ESP-IDF routines take uint8_t for the ssid and password, and take const char for the hostname. I don't know if they assume ASCII or not, or just "some bytes".

For the string-like values, we may want to convert them to bytes() before passing them down, and describe them as Union[str | ReadableBuffer]. The fact that you can pass either right now is an accident of MicroPython treating strs as obeying the buffer protocol.

shared-bindings/wifi/Radio.c Outdated Show resolved Hide resolved
shared-bindings/wifi/Radio.c Outdated Show resolved Hide resolved
shared-bindings/wifi/Radio.c Outdated Show resolved Hide resolved
shared-bindings/wifi/Radio.c Outdated Show resolved Hide resolved
shared-bindings/wifi/Radio.c Outdated Show resolved Hide resolved
@FoamyGuy
Copy link
Collaborator Author

FoamyGuy commented Mar 4, 2022

Latest commit changes mac_address and mac_address_ap back to ReadableBuffer. Also change the str types to be Union[str | ReadableBuffer] and remove the bytes b from the default values that had it.

I think these are the changes that you suggested, but let me know if I've misinterpreted something.

I wasn't sure what you meant by "convert them to bytes() before passing them down" Does it mean like on this line: https://github.com/FoamyGuy/circuitpython/blob/6a792ab373bff7e47adcaffaca02019506037f20/shared-bindings/wifi/Radio.c#L291

it would wrap that value in bytes() call like this?

common_hal_wifi_radio_start_ap(self, bytes(ssid.buf), ssid.len, bytes(password.buf), password.len, args[ARG_channel].u_int, authmode, args[ARG_max_connections].u_int);

@dhalbert
Copy link
Collaborator

wasn't sure what you meant by "convert them to bytes() before passing them down" Does it mean like on this line: https://github.com/FoamyGuy/circuitpython/blob/6a792ab373bff7e47adcaffaca02019506037f20/shared-bindings/wifi/Radio.c#L291

it would wrap that value in bytes() call like this?

common_hal_wifi_radio_start_ap(self, bytes(ssid.buf), ssid.len, bytes(password.buf), password.len, args[ARG_channel].u_int, authmode, args[ARG_max_connections].u_int);

Ah, OK, it is already passing raw bytes down, so nothing needs to be done. We can't do bytes(ssid.buf), etc. -- that is strictly a Python construct.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

OK, I think this is good as it stands. Thanks!

@dhalbert dhalbert merged commit b5504a8 into adafruit:main Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants