Skip to content

correct weak module links; samd module only in m4 ports; update frozen libs#1566

Merged
kattni merged 6 commits into
adafruit:masterfrom
dhalbert:fix-weaklinks
Feb 19, 2019
Merged

correct weak module links; samd module only in m4 ports; update frozen libs#1566
kattni merged 6 commits into
adafruit:masterfrom
dhalbert:fix-weaklinks

Conversation

@dhalbert
Copy link
Copy Markdown
Collaborator

@dhalbert dhalbert requested a review from tannewt February 18, 2019 05:48
@dhalbert
Copy link
Copy Markdown
Collaborator Author

  • Also added name re for ure, json for ujson, and errno for uerrno, in the hope these are close to the CPython equivalents. I will double-check this later, but we're trying to get away from the u... names.

Comment thread py/objmodule.c
@notro
Copy link
Copy Markdown

notro commented Feb 18, 2019

Up until your changes io and re were weak links on M4 and not available on the others (only as uio and ure). I have relied on this to add a bit of functionality to these as well.

Ideally I would like all CPython std module names to be weak links and an underscore version being the C module. I don't think any of the C modules cover all the CPython functionality, so they all need to be extended to achieve CPython compatibility.

I saw that the new PyBoard will have 512kB of RAM, so with that amount of RAM porting the standard library is really starting to make sense. When there's 1MB of RAM, the size of the standard library modules doesn't matter much anymore. It will be other bigger libraries that will use most of the RAM.

@jerryneedell
Copy link
Copy Markdown
Collaborator

jerryneedell commented Feb 18, 2019

FYI - json appears twice in the modules list on both the feather_m4_express and feather_nrf52840_express -- any concern with this?

I built and ran this this on those boards and a pirkey - all seem to be running fine. No obvious issues


Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 4.0.0-beta.2-79-g339a22752 on 2019-02-18; Adafruit Feather nRF52840 Express with nRF52840
>>> help('modules')
__main__          busio             microcontroller   terminalio
_os               collections       micropython       time
_pixelbuf         digitalio         neopixel_write    touchio
_time             displayio         os                uerrno
analogio          errno             pulseio           uio
array             gamepad           random            ujson
binascii          gc                re                ure
bitbangio         io                storage           usb_hid
bleio             json              struct            usb_midi
board             json              supervisor
builtins          math              sys
Plus any modules on the filesystem
>>> 
Press any key to enter the REPL. Use CTRL-D to reload.
Adafruit CircuitPython 4.0.0-beta.2-79-g339a22752 on 2019-02-18; Adafruit Feather M4 Express with samd51j19
>>> 
>>> 
>>> help('modules')
__main__          collections       micropython       supervisor
_os               digitalio         neopixel_write    sys
_pixelbuf         displayio         network           terminalio
_time             errno             os                time
analogio          gamepad           pulseio           uerrno
array             gc                random            uio
audiobusio        i2cslave          re                ujson
audioio           io                rtc               ure
bitbangio         json              samd              usb_hid
board             json              socket            usb_midi
builtins          math              storage           wiznet
busio             microcontroller   struct
Plus any modules on the filesystem
>>> 

@notro
Copy link
Copy Markdown

notro commented Feb 18, 2019

FYI - json appears twice in the modules list on bot the feather_m4_express and feather_nrf52840_express

Looks like it's added both through the JSON_MODULE macro and directly in py/objmodule.c.

@dhalbert
Copy link
Copy Markdown
Collaborator Author

@notro How is MPy doing this? Would your sort of library completion work there? Do they have the equivalent of the _ version?

I'm a little concerned that listing a lot of _ versions will increase support issues. Is there a way not to do that, and have you import the original C module first, rename it, and then define yours? I'm not thinking very hard about this, so I may be missing somethign obvious.

@dhalbert
Copy link
Copy Markdown
Collaborator Author

I'm going to discuss the u... versions of the modules with the other team members later today, to see what we might do going forward about dropped the u... names. We seem to be going in the opposite direction from MicroPython: micropython#4370. Our philosophy is that a subset impl is OK, because it means that code can be written to run on regular CPython and CircuitPython, without doing any import ... as tricks, as long as you limit yourself to the functionality in the smaller library. Since we're writing more and more code like that, due to adafruit-blinka, this approach makes sense to me.

@notro
Copy link
Copy Markdown

notro commented Feb 18, 2019

If you look at mp_builtin___import__ you see that as a last resort it looks at the weak links (aliases) before giving up. So if time exists in the search path it is loaded, if not the one in mp_builtin_module_weak_links_map is used.

MicroPython uses u as a prefix for the modules that exist in the standard library. @tannewt wanted us to use _ instead to differentiate from MicroPython.

I suggest we use _ for all the CPython std modules. This means that if someone has the ported std library in /lib and they only need the functionality in the C-module, they can import the underscore version to save on RAM.

CPython lists underscore modules as well when doing help('modules').

Copy link
Copy Markdown

@kattni kattni left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@kattni kattni merged commit ef802c9 into adafruit:master Feb 19, 2019
@notro
Copy link
Copy Markdown

notro commented Feb 19, 2019

I'm going to discuss the u... versions of the modules with the other team members later today, to see what we might do going forward about dropped the u... names.

Did you come to any conclusion?

@notro
Copy link
Copy Markdown

notro commented Feb 19, 2019

Maybe something like this could be used to handle the standard library module aliases:

#if CIRCUITPY_TIME
extern const struct _mp_obj_module_t time_module;
#define TIME_MODULE(name)      { MP_OBJ_NEW_QSTR(name), (mp_obj_t)&time_module },
#else
#define TIME_MODULE(name)
#endif


#define CIRCUITPY_MODULES \
    ANALOGIO_MODULE \
    ...


#define CIRCUITPY_STD_LIB_MODULES(prefix) \
    IO_MODULE(MP_QSTR_##prefix##io) \
    OS_MODULE(MP_QSTR_##prefix##os) \
    RE_MODULE(MP_QSTR_##prefix##re) \
    TIME_MODULE(MP_QSTR_##prefix##time) \
    ...


#if MICROPY_MODULE_WEAK_LINKS
  #define MICROPY_PORT_BUILTIN_MODULES \
      CIRCUITPY_MODULES \
      CIRCUITPY_STD_LIB_MODULES(_)

  #define MICROPY_PORT_BUILTIN_MODULE_WEAK_LINKS \
      CIRCUITPY_STD_LIB_MODULES()

#else
  #define MICROPY_PORT_BUILTIN_MODULES \
      CIRCUITPY_MODULES \
      CIRCUITPY_STD_LIB_MODULES()
#endif

@tannewt
Copy link
Copy Markdown
Member

tannewt commented Feb 20, 2019

Could we drop the modules all together that the stdlibs implement?

What is your motivation for supporting them? It doesn't seem very useful because they will use a bunch of resources.

@notro
Copy link
Copy Markdown

notro commented Feb 20, 2019

What is your motivation for supporting them?

They add functionality that's missing from the C module.

For instance the time module adds these:

from _time import localtime, mktime, monotonic, sleep, struct_time, time

def asctime(t=None):
    ...

def ctime(secs=None):
    ...

def gmtime(secs=None):
    ...

def strftime(_format, t=None):
    ...

It doesn't seem very useful because they will use a bunch of resources.

Erh, CPython compatibility?

@dhalbert
Copy link
Copy Markdown
Collaborator Author

I'd like to move this discussion back to a place that's more visible instead of a closed PR, maybe #1155 or a new issue. I'll start there, and try to summarize the discussion up to this point.

@dhalbert dhalbert deleted the fix-weaklinks branch February 20, 2019 13:57
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.

5 participants