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

Convert all circuitpython modules to use MP_REGISTER_MODULE #5183

Closed
jepler opened this issue Aug 19, 2021 · 9 comments
Closed

Convert all circuitpython modules to use MP_REGISTER_MODULE #5183

jepler opened this issue Aug 19, 2021 · 9 comments

Comments

@jepler
Copy link
Member

jepler commented Aug 19, 2021

This can reduce the amount of boilerplate required. Instead of introducing a macro such as QRIO_MODULE dependent on CIRCUITPY_QRIO and then adding an expansion of it in MICROPY_PORT_BUILTIN_MODULES_STRONG_LINKS, one simply writes in the bindings __init__.c file:

MP_REGISTER_MODULE(MP_QSTR_qrio, qrio_module, CIRCUITPY_QRIO);

where the first item is the module name as a qstr, the second item is the module's globals, and the third is the preprocessor macro that expands to (1) when enabled.

This can be done on a piecemail basis, and this bug would be closed once the last one is completed. I didn't prepare a checklist here, but basically everything in shared-bindings should be done unless a reason not to is identified.

Besides reducing boilerplate slightly, it also eases including modules with no hardware dependency in the "unix" build, which enables them to be testable. In time, such testability could be applied to bitmaptools and atexit, to name just two examples.

@capellini
Copy link

@jepler I'd like to help with this. I've been experimenting with this using displayio and my pybadge.

capellini added a commit to capellini/circuitpython that referenced this issue Aug 25, 2021
Convert from using MICROPY_PORT_BUILTIN_MODULES_STRONG_LINKS to using MP_REGISTER_MODULE for displayio, terminalio, and fontio modules.

Related to adafruit#5183.
capellini added a commit to capellini/circuitpython that referenced this issue Aug 26, 2021
Convert adafruit_bus_device, adafruit_pixelbuf, analogio, atexit, audiobusio, audiocore, audioio, audiomixer, and audiomp3 modules to use MP_REGISTER_MODULE.

Related to adafruit#5183.
@capellini
Copy link

I'm starting with the modules that are available on my pybadge, using the test procedure outlined here. I'm keeping track of the modules as I go. Once I'm done with the pybadge, I'll move to my adafruit funhouse, which has a few more (e.g. wifi, ipaddress).

@capellini
Copy link

@jepler Quick question about the bitmaptools module. I'm used to seeing the globals table for each module define a __name__ property like, for example, what is done on line 73 of __init__.c in the bitbangio module:

STATIC const mp_rom_map_elem_t bitbangio_module_globals_table[] = {
{ MP_ROM_QSTR(MP_QSTR___name__), MP_ROM_QSTR(MP_QSTR_bitbangio) },
{ MP_ROM_QSTR(MP_QSTR_I2C), MP_ROM_PTR(&bitbangio_i2c_type) },
#if CIRCUITPY_ONEWIREIO
{ MP_ROM_QSTR(MP_QSTR_OneWire), MP_ROM_PTR(&onewireio_onewire_type) },
#endif
{ MP_ROM_QSTR(MP_QSTR_SPI), MP_ROM_PTR(&bitbangio_spi_type) },
};

However, I don't see that in the bitmaptools globals table:

STATIC const mp_rom_map_elem_t bitmaptools_module_globals_table[] = {
{ MP_ROM_QSTR(MP_QSTR_readinto), MP_ROM_PTR(&bitmaptools_readinto_obj) },
{ MP_ROM_QSTR(MP_QSTR_rotozoom), MP_ROM_PTR(&bitmaptools_rotozoom_obj) },
{ MP_ROM_QSTR(MP_QSTR_arrayblit), MP_ROM_PTR(&bitmaptools_arrayblit_obj) },
{ MP_ROM_QSTR(MP_QSTR_fill_region), MP_ROM_PTR(&bitmaptools_fill_region_obj) },
{ MP_ROM_QSTR(MP_QSTR_boundary_fill), MP_ROM_PTR(&bitmaptools_boundary_fill_obj) },
{ MP_ROM_QSTR(MP_QSTR_draw_line), MP_ROM_PTR(&bitmaptools_draw_line_obj) },
};

Also, on the TTY terminal, an AttributeError is raised when attempting to access the module's __name__ attribute:

>>> import bitmaptools
>>> bitmaptools.__name__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'module' object has no attribute '__name__'

Should I include:

    { MP_ROM_QSTR(MP_QSTR___name__), MP_ROM_QSTR(MP_QSTR_bitmaptools) },

in the bitmaptools_module_globals_table?

@capellini
Copy link

I also noticed the __name__ attribute is missing from the board module, but to add that attribute, would I have to add it to every port where the global tables are defined, right?

@capellini
Copy link

I think I also found some code duplication:

#if CIRCUITPY_GAMEPADSHIFT
// Scan gamepad every 32ms
#define CIRCUITPY_GAMEPAD_TICKS 0x1f
#define GAMEPAD_ROOT_POINTERS mp_obj_t gamepad_singleton;
#else
#define GAMEPAD_ROOT_POINTERS
#endif

and

#if CIRCUITPY_GAMEPADSHIFT
// Scan gamepadshift every 32ms
#define CIRCUITPY_GAMEPAD_TICKS 0x1f
#define GAMEPAD_ROOT_POINTERS mp_obj_t gamepad_singleton;
#else
#define GAMEPAD_ROOT_POINTERS
#endif

It's safe to remove one of these, correct?

@jepler
Copy link
Member Author

jepler commented Aug 27, 2021

I believe so, yes.

capellini added a commit to capellini/circuitpython that referenced this issue Aug 27, 2021
…lio, framebufferio, frequencyio, gamepadshift, getpass, keypad, math, microcontroller, and msgpack modules to use MP_REGISTER_MODULE.

Related to adafruit#5183.
capellini added a commit to capellini/circuitpython that referenced this issue Aug 27, 2021
Convert bitbangio, bitmaptools, _bleio, board, busio, countio, digitalio, framebufferio, frequencyio, gamepadshift, getpass, keypad, math, microcontroller, and msgpack modules to use MP_REGISTER_MODULE.

Related to adafruit#5183.
capellini added a commit to capellini/circuitpython that referenced this issue Aug 31, 2021
Convert neopixel_write, onewireio, ps2io, pulseio, pwmio, rainbowio, random, rgbmatrix, rotaryio, rtc, sdcardio, sharpdisplay, _stage, storage, struct, supervisor, synthio, touchio, traceback, usb_cdc, usb_hid, usb_midi, and vectorio modules to use MP_REGISTER_MODULE.

Related to adafruit#5183.
capellini added a commit to capellini/circuitpython that referenced this issue Sep 1, 2021
Convert _eve, _pew, aesio, alarm, audiopwmio, bitops, camera, canio, dualbank, gnss, i2cperipheral, imagecapture, ipaddress, memorymonitor, sdioio, socketpool, ssl, uheap, ustack, watchdog, and wifi modules to use MP_REGISTER_MODULE.

Related to adafruit#5183.
capellini added a commit to capellini/circuitpython that referenced this issue Sep 1, 2021
Convert _eve, _pew, aesio, alarm, audiopwmio, bitops, camera, canio, dualbank, gnss, i2cperipheral, imagecapture, ipaddress, memorymonitor, sdioio, socketpool, ssl, uheap, ustack, watchdog, and wifi modules to use MP_REGISTER_MODULE.

Related to adafruit#5183.
@capellini
Copy link

I think that I've now converted all modules in shared bindings that have a *_MODULE defined in py/circuitpy_mpconfig.h. I didn't see a module listed in MICROPY_PORT_BUILTIN_MODULES_STRONG_LINKS for the following modules in shared bindings, so didn't use MP_REGISTER_MODULE:

  • multiterminal
  • paralleldisplay
  • nvm (submodule of microcontroller)
  • os (listed in MICROPY_PORT_BUILTIN_MODULE_WEAK_LINKS and alt name listed in MICROPY_PORT_BUILTIN_MODULE_ALT_NAMES and I don't quite know how to deal with those yet)
  • time (same reasons as 'os' above)

There are seven more modules defined in MICROPY_PORT_BUILTIN_MODULES_STRONG_LINKS:

  • BINASCII_MODULE
  • ERRNO_MODULE
  • ESPIDF_MODULE
  • JSON_MODULE
  • RE_MODULE
  • RP2PIO_MODULE
  • SAMD_MODULE

I started to register these with MP_REGISTER_MODULE but then thought it best if I consult with you before doing so, as I'm new to circuitpython and micropython. Even if I should proceed with these, I figured I should keep them in a separate PR, since dealing with them may be more nuanced than the shared-bindings boilerplate code. Would you like me to give a PR for these seven a shot?

@jepler
Copy link
Member Author

jepler commented Sep 1, 2021

@capellini I'm "out of the office" so I can't look into it in detail right now. If you want to try to PR just one of those changes, perhaps someone else can review it and give useful feedback.

@tannewt do you know why those modules are different, or if we need to handle the situation differently?

@tannewt
Copy link
Member

tannewt commented Sep 1, 2021

espidf, rp2pio and samd are port specific modules so I'd do those as a group.

The other four are micropython modules that we reorganized into shared-bindings. os and time are in this group too. I think they can be made strong links. MicroPython has uos and utime too which is why they have strong and weak links.

I'm not sure multiterminal is used anywhere. We used to use it on ESP8266 for webrepl but I'm not sure we'd go that route when we support ESP32 again.

paralleldisplay is brand new and should follow our others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants