8000 RFC: enabling external drivers · Issue #5618 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

RFC: enabling external drivers #5618

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.

Sign up for GitHub

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

Jump to bottom
Open
tve opened this issue Feb 7, 2020 · 10 comments
Open

RFC: enabling external drivers #5618

tve opened this issue Feb 7, 2020 · 10 comments

Comments

@tve
Copy link
Contributor
tve commented Feb 7, 2020 •
edited
Loading

I've been reviewing a bunch of older esp32 pull requests and am a little puzzled. A bunch fall into the category of essentially exposing ESP-IDF functionality. For example, I2S #4471, smartconfig #4404, machine.Counter #5496, adjtime #5453, sigma-delta #5452, probably CAN #5310. I assume other ports have the same situation, e.g. with the STM32 HAL.

I'm wondering whether there is a way to enable these types of drivers without having to pull them into the MP core given that the PRs are not exactly moving very fast and are often difficult to review and to reach agreement upon. With the new ability to put C code into loadable MPY modules the main thing that seems to be missing is the ability to link.

I'm wondering whether an initial approach where PRs that only add functions to the mp_fun_table are readily accepted such that the drivers can be implemented in external repos could work. (The new functions could go into a specially marked section such that they could be replaced by a more sophisticated mechanism later.) Thoughts?

Pros:

  • reduces PR bottleneck
  • allows multiple driver models to co-exist, e.g. evolution of more sophisticated APIs
  • allows machine-dependent drivers without "polluting" MP core repo

Cons:

  • makes it harder to locate code, e.g. libraries and drivers scattered all around github
  • may make higher-level libraries incompatible, e.g., lib A uses driver A while lib B uses driver B for the same peripheral
  • reduces ability to create portable drivers by scattering code
  • does it make debugging the native code harder?

See also #5463 (preliminary PR for docs on how to add functions to table)
@MrSurly
Copy link
Contributor
MrSurly commented Feb 7, 2020

I'm wondering whether there is a way to enable these types of drivers without having to pull them into the MP core given that the PRs are not exactly moving very fast and are often difficult to review and to reach agreement upon.

You can merge them yourself and use them if desired. I did this for a project that uses the I2S functionality.

@jcw
Copy link
jcw commented Feb 13, 2020

If µPy is serious about separately loadable/linkable extensions (which I assume - and hope - it is), then the µPy <=> extension "barrier" will need to be very carefully managed and grown: each call (in either direction) will be nearly impossible to remove or even alter once it's made available. I'm not saying anything particularly new here - just wanted to stress that there's a potentially brittle minefield ahead IMO. Perhaps µPy can avoid exposing many of its internals by offering just a script callback interface for extensions which need to query or adjust the Python context they're being called in. This might not be the most efficient approach, but it allows postponing many API decisions to when more extensions have been built, and experience gained about which parts need to be optimised.

(in case none of this makes sense: please ignore me - I've only recently started following µPy ...)

@tve
Copy link
Contributor Author
tve commented Feb 13, 2020

@jcw: agreed. But I believe that what you write applies mainly to exporting calls "into" MP, i.e., exposing more of the MP innards. I believe/hope that many drivers can be written that don't need anything new, other than perhaps some types that have been skipped (e.g. #5641). Ideally the only calls that get added are ones for the underlying HAL-level abstractions, such as FreeRTOS, STM32 HAL, ESP-IDF.

@jeremyherbert
Copy link
Contributor

With the current setup, I’m pretty sure if mp_fun_table changes then all existing compiled native modules will cease to work and need to be recompiled. So this makes accepting PRs to modify the table quite tricky I think.

@tve
Copy link
Contributor Author
tve commented Feb 14, 2020

Can you be more specific? Why would existing compiled modules break if the table gets extended?

@jeremyherbert
Copy link
Contributor

I guess it depends on what the solution is to my problem in #5641 ;)

Also see mp_ld.py, line 666, the “67 +” bit; that appears to me that symbol offsets are hard coded into the linker.

@jeremyherbert
Copy link
Contributor

There is also the problem of what if a newer compiled module depends on some symbols which are missing because the interpreter has the old table. It makes micropython versioning much more important than it is now as you basically need to lock down an ABI as version X, and then check if the module is compatible with version X.

@tve
Copy link
Contributor Author
tve commented Feb 14, 2020

I did a little test trying to convert my machine.Counter PR #5216 to a native module. Not much joy...

The "little test" took a whole bunch of hours. I had to add the following functions to the mp_fun_table:

  • mp_arg_parse_all
  • mp_obj_new_exception_msg_varg
  • machine_pin_get_id

the last one being an issue/hack 'cause it's defined in the port, not in py.

Having to define the locals_dict_table for the module dynamically isn't pretty or convenient, and having to init the type object dynamically isn't nice either. The "allowed_args" table also has to be inited dynamically.

I still didn't get everything right, mpy_ld dies with:

  File "/home/src/esp32/micropython/tools/mpy_ld.py", line 871, in build_mpy
    kind = 7 + kind
TypeError: unsupported operand type(s) for +: 'int' and 'str'

I didn't not attempt to troubleshoot that 'cause it's bedtime and the whole process so far has been rather painful...

@jeremyherbert
Copy link
Contributor

yep, it's a bit tricky at the moment. See also here for some points from Damien: #5643

@tve
Copy link
Contributor Author
tve commented Feb 14, 2020

Yup, I'm following that PR too... I'm gonna give it one more try, this time I'll only write the ESP-IDF calls in C. See how that works out.

This was referenced Feb 14, 2020
Closed
Closed
@jeremyherbert jeremyherbert mentioned this issue Feb 19, 2020
Closed
@pidou46 pidou46 mentioned this issue Mar 28, 2020
Closed
@mfp20 mfp20 mentioned this issue May 11, 2021
Open
tannewt pushed a commit to tannewt/circuitpython that referenced this issue Nov 30, 2021
@microdev1
Merge pull request micropython#5618 from weblate/weblate-circuitpytho…
ff92614 
…n-main

Translations update from Hosted Weblate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tve @jeremyherbert @jcw @MrSurly
0