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

No seamless workaround for missing descriptor support #6

Closed
cefn opened this issue Feb 25, 2018 · 8 comments
Closed

No seamless workaround for missing descriptor support #6

cefn opened this issue Feb 25, 2018 · 8 comments

Comments

@cefn
Copy link
Contributor

cefn commented Feb 25, 2018

There's a fundamental contrast in the build flags between CircuitPython and Micropython seen here...

At https://github.com/micropython/micropython/blob/6e675c1baaf0de0d56a2345376d6b5600bfab3aa/py/mpconfig.h the default is set, which applies to official pyboard and esp8266 builds.

// Whether to support descriptors (__get__ and __set__)
// This costs some code size and makes all load attrs and store attrs slow
#ifndef MICROPY_PY_DESCRIPTORS
#define MICROPY_PY_DESCRIPTORS (0)
#endif

Versus the build flags for esp8266 and atmel-samd in CircuitPython

cefn@cefn-artful-thinkpad:~/Documents/shrimping/clients/adafruit/git/circuitpython-latest$ grep -rin MICROPY_PY_DESCRIPTORS .
./ports/atmel-samd/mpconfigport.h:56:#define MICROPY_PY_DESCRIPTORS      (1)
./ports/esp8266/mpconfigport.h:43:#define MICROPY_PY_DESCRIPTORS      (1)

While perhaps an average user wouldn't be reliant on descriptors, some libraries use them or encourage their use, for example adafruit_register fundamentally depends on descriptors, meaning that e.g. the BNO055 library and RFM69 libraries intended to be used in #2 cannot be made to work seamlessly on Micropython simply by making compatible I2C or SPI objects available.

The list of libraries affected in this way includes at least...

  • adafruit_register
  • adafruit_bno055
  • adafruit_tlc59711
  • adafruit_rfm69.py
  • adafruit_thermal_printer

This list is based on searching for libraries using __get__() or __set__() within the Adafruit CircuitPython Bundle.

Since it doesn't trigger a load-time error, it may be feasible to monkey-patch classes such as BNO055 with property accessors to hide the descriptor logic, a step performed before using any of the constructors. However, this step would have to be explicit in any Micropython-hosted use of said CircuitPython libraries, or a separate bundle could be distributed in which descriptor logic has been substituted.

@cefn cefn changed the title No obvious workaround for missing descriptor support No seamless workaround for missing descriptor support Feb 25, 2018
@tannewt
Copy link
Member

tannewt commented Feb 26, 2018

I don't think we should change our libraries for this. Data descriptors simplify the drivers a bunch.

Would you mind opening an issue in MicroPython's repo for them to enable it? They should be interested in having our libraries compatible. I can follow up on it as well.

@cefn
Copy link
Contributor Author

cefn commented Feb 26, 2018

I'm taking Damien's inline comment...

// This costs some code size and makes all load attrs and store attrs slow

...and the statements at https://github.com/micropython/micropython/wiki/Differences to mean they have already decided to exclude it by default, so I think the debate has been had once and settled in favour of performance and resource-efficiency, given there are always substitute language structures.

Do you think we should open it as an issue in any case to record the decision?

BTW I am not proposing that libraries are changed. Rather that people running on Micropython would have to run a post-load monkey-patching operation to switch descriptors for properties on classes where they are found. This would be after import and before use, and not require any code-changes to the module source. Not sure when and how this strategy might fail, though. E.g. are frozen-modules any different in terms of dynamic manipulation. It might be a crazy thought until I've worked through it.

@tannewt
Copy link
Member

tannewt commented Feb 27, 2018

I think opening an issue is still worth while. That decision was made before we had 50+ libraries. :-) Determining the code size increase would be a good piece of info to have too.

What would they switch the descriptors to? Properties are implemented using data descriptors.

Frozen modules won't be an issue because we're assuming its a MicroPython build. mpy files are identical to py files after load.

@cefn
Copy link
Contributor Author

cefn commented Feb 27, 2018

What would they switch the descriptors to? Properties are implemented using data descriptors.

I'm just picking up on the https://github.com/micropython/micropython/wiki/Differences comment;
"So far, we were able to implement all native Python features (like properties) without explicit descriptors."

If it's not been covered again recently I'll open an issue.

@cefn
Copy link
Contributor Author

cefn commented Feb 27, 2018

Ancient discussion of strategy for implementing properties at micropython/micropython#474 and
https://github.com/micropython/micropython/blob/6e675c1baaf0de0d56a2345376d6b5600bfab3aa/py/mpconfig.h shows properties configured by default across all current builds, so the property patching strategy should be feasible if no official descriptor support (or while waiting for it).

@cefn
Copy link
Contributor Author

cefn commented Feb 27, 2018

You can find the issue on enabling descriptor support on official images as recently raised at https://github.com/micropython/micropython/issues?utf8=%E2%9C%93&q=is%3Aissue+descriptor

@cefn cefn mentioned this issue Feb 27, 2018
17 tasks
@tannewt
Copy link
Member

tannewt commented Feb 27, 2018

Upstream issue is micropython/micropython#3644 and I've subscribed to it. Thanks!

makermelissa pushed a commit that referenced this issue Mar 8, 2021
@makermelissa
Copy link
Contributor

Closing as it appears descriptors were added.

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

No branches or pull requests

3 participants