Skip to content

Moved ORDEREDDICT define to central location #3554

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

Merged
merged 4 commits into from
Oct 15, 2020

Conversation

gamblor21
Copy link
Member

Resolving issue #3278 Unify ORDEREDDICT setting. Removed the setting from all the ports. Added it to circuitpy_mpconifg.h based on the CIRCUITPY_FULL_BUILD. As noted in the issue this will now include this feature in the mimxrt10xx port.

@gamblor21
Copy link
Member Author

I am getting a merge error I think in the test unix build. Digging through all the make files it seems the end of the test calls a custom make command including -DMP_CONFIGFILE="<mpconfigport_coverage.h>

This coverage file defines: #define MICROPY_PY_COLLECTIONS_NAMEDTUPLE__ASDICT (1) and the only place in all the source code it appears is here.

The one (and only) function this define includes references the ORDEREDDICT define that was moved to a central area. py/circuitpy_mpconfig.mk is never included in the unix build (it is in all the others). So the new define is never included.

I am sure if I include a define in mpconfigport_coverage.h the issue will go away but gets away from centralization.

I am not familiar enough with the whole build setup to want to make that choice on my own though. So any thoughts? If this isn't clear just ping me on discord.

@tannewt
Copy link
Member

tannewt commented Oct 14, 2020

Ya, that's a weird thing to define. Maybe just remove it and replace the uses with checking if ordereddict is enabled?

@gamblor21
Copy link
Member Author

New problem, on a small number of boards, for certain translations the build is failing. These are all M0 builds that were not set to CIRCUITPY_FULL_BUILD = 0 so they are building with more features then the other M0 boards. But for some build translations are running out of flash.

By moving ORDEREDDICT to a central location it was using the FULL_BUILD define. I could introduce a new define but then we are almost back to where we started from, except instead of per port build specifications it would not be per board.

Also for some reason this compiles on my PC in all these configurations but not in github actions.

@dhalbert
Copy link
Collaborator

So basically it's failing on SAMD21 express builds with a few translations. We can try to squeeze those particular translations by setting CFLAGS_INLINE_LIMIT lower. If you look at the way CFLAGS_INLINE_LIMIT is used in the various mpconfigboard.mk files, you can see some existing examples. Or we can simply disable it for all SAMD21 builds.

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.

25 for CFLAGS_INLINE_LIMIT is unfortunately too small.

@dhalbert
Copy link
Collaborator

So I'd say let's just not implement ordered dicts for SAMD21's. @tannewt How do you feel about that?

@tannewt
Copy link
Member

tannewt commented Oct 14, 2020

So I'd say let's just not implement ordered dicts for SAMD21's. @tannewt How do you feel about that?

Overall, I'm ok with that. We only need it for the BLE library as far as I know: adafruit/Adafruit_CircuitPython_BLE#97

@gamblor21
Copy link
Member Author

I'll undo those changes. Is there a pattern to use to exclude a feature only from SAMD21 I can copy?

@dhalbert
Copy link
Collaborator

I'll undo those changes. Is there a pattern to use to exclude a feature only from SAMD21 I can copy?

In circuitpy_mpconfig.h, you can just guard the definition so that if it's already defined in mpconfigport.h, then you don't redefine it:

#ifndef MICROPY_PY_COLLECTIONS_ORDEREDDIRECT
#define MICROPY_PY_COLLECTIONS_ORDEREDDICT    (CIRCUITPY_FULL_BUILD)
#endif

MICROPY_PY_URE is guarded in a similar way.

Thanks for your patience in working throgh this!

dhalbert
dhalbert previously approved these changes Oct 15, 2020
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.

Go ahead and turn it off in all SAMD21 boards; making the change inside #ifdef SAMD21 in the atmel-samd/mpconfigport.h

@dhalbert dhalbert dismissed their stale review October 15, 2020 01:11

wrong status

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.

Thanks! This kind of progression, where we run out of space after adding a feature, and have to figure out how to compensate, is completely typical.

@dhalbert dhalbert merged commit f1e8f2b into adafruit:main Oct 15, 2020
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