8000 py/mpconfig.h: Define templates for "feature levels". by jimmo · Pull Request #7655 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

py/mpconfig.h: Define templates for "feature levels". #7655

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

Closed
wants to merge 2 commits into from

Conversation

jimmo
Copy link
Member
@jimmo jimmo commented Aug 13, 2021

This is a proposal/RFC for a larger piece of work (related to #7538) to simplify enabling/disabling features.

My goals are:

  • Remove redundancy from mpconfigport.h (never set a value to the default -- make it clear exactly what's being enabled).
  • Improve consistency between ports. All "similar" ports (i.e. approx same flash size) should get the same features.
  • Simplify mpconfigport.h -- just get default/sensible options for the size of the port.
  • Make it easy for defining constrained boards (e.g. STM32F0/L0) can just set a lower level.

This PR just makes a step towards this and defines the "core" level as the current default feature set, and a "minimal" level to turn off everything (and updates just the minimal port to use it). It highlights a few things that are enabled on the minimal port that perhaps shouldn't be (~2kiB).

I've added a couple of placeholder "levels" as candidate suggestions for where the other ports will roughly land.

This is a no-op change (the x86/arm minimal binary is the same size and contains the exact same symbols). All other ports unchanged.

@codecov-commenter
Copy link
codecov-commenter commented Aug 13, 2021

Codecov Report

Merging #7655 (f0b1cf0) into master (d9749f9) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head f0b1cf0 differs from pull request most recent head 169fc6f. Consider uploading reports for the commit 169fc6f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7655      +/-   ##
==========================================
- Coverage   98.24%   98.24%   -0.01%     
==========================================
  Files         154      154              
  Lines       20043    20042       -1     
==========================================
- Hits        19692    19691       -1     
  Misses        351      351              
Impacted Files Coverage Δ
py/bc.c 88.65% <0.00%> (-1.04%) ⬇️
py/map.c 99.47% <0.00%> (-0.53%) ⬇️
py/obj.c 97.61% <0.00%> (ø)
py/objtype.c 100.00% <0.00%> (ø)
py/parse.c 99.20% <0.00%> (+0.19%) ⬆️
py/smallint.c 96.00% <0.00%> (+4.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9749f9...169fc6f. Read the comment docs.

py/mpconfig.h Outdated
#define _MICROPY_CONFIG_ROM_LEVEL_COMMON (MICROPY_CONFIG_ROM_LEVEL >= 2)
#define _MICROPY_CONFIG_ROM_LEVEL_CONVENIENCE (MICROPY_CONFIG_ROM_LEVEL >= 3)
#define _MICROPY_CONFIG_ROM_LEVEL_ALL (MICROPY_CONFIG_ROM_LEVEL >= 4)
#define _MICROPY_CONFIG_ROM_LEVEL_EVERYTHING (MICROPY_CONFIG_ROM_LEVEL >= 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

'all' and 'everything' are quite similar; use 'coverage' for the latter?

Wrt 'rom level' naming: since it's a set of feature levels, perhaps just name it like that (MICROPY_FEATURE_LEVEL) as well?

I can imagine that instead of the single-line description of which level has what, there'd be more elaborate comments. In that case the explanation of which one does what is going to be further away from where it gets defined. And now you have to repeat the numbers. How about moving those comments here, together with their respective definition? I.e.

 // Only enable core features (constrained flash, e.g. STM32L072)
 #define _MICROPY_CONFIG_ROM_LEVEL_CORE (MICROPY_CONFIG_ROM_LEVEL >= 1)

Perhaps overkill, but people might want to adopt this principle and define their own levels which are somewhere in between existing ones, which can be done by leaving gaps here i.e. 10, 20, 30, ...

Copy link
Member Author
@jimmo jimmo Aug 14, 2021

Choose a reason for hiding this comment

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

'all' and 'everything' are quite similar; use 'coverage' for the latter?

Yep, I think you're right.

Wrt 'rom level' naming: since it's a set of feature levels, perhaps just name it like that (MICROPY_FEATURE_LEVEL) as well?

I did want to also add RAM levels too (e.g. rp2 is ram but not flash constrained), but I think given that ROM generally maps to features, this makes sense. My reasoning for calling it ROM was to emphasize that we're "saving ROM", not "cutting features" (although I agree that's a fairly abstract distinction).

Perhaps overkill, but people might want to adopt this principle and define their own levels which are somewhere in between existing ones, which can be done by leaving gaps here i.e. 10, 20, 30, ...

I guess so... perhaps this could be useful for board-specific overrides. I wanted to avoid having to decide "is this as 27 or 26 level feature discussions" in the future.

On that though, I think it's worth putting #defines for the levels (somewhere before mpconfigport.h gets imported) so that the numbers themselves are not meaningful. (i.e. minimal/mpconfigport.h should set it to "MINIMAL" not "0").

@jimmo
Copy link
Member Author
jimmo commented Sep 15, 2021

Updated the branch:

  • Renamed the levels to: minimal, core features, basic features, extra features, full features, everything.
  • Added defines for the levels (instead of using the numbers directly) and made them 0, 10, 20, etc.
  • The AT_LEAST macros are no longer private (mpconfigport.h might wish to use them, based on mpconfigboard.h setting the level).
  • Split the mpconfig change from the ports/minimal change.

I think it's still worth separating full from everything. Full would be a board with unlimited flash, but there might still be features that we might not enable that only m 8000 ake sense for coverage (extra debugging, etc). Can always remove it if we don't end up using it. Maybe when we add the RAM levels, it'll make sense that the "everything" level is combined.

@peterhinch
Copy link
Contributor

If this goes towards addressing #5564 then it's a big step forward.

@jimmo jimmo force-pushed the port-config-template branch 2 times, most recently from ef34ec5 to f0b1cf0 Compare September 15, 2021 12:24
Defines the "core" level as the current default feature set, and "minimal"
to turn off everything.

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
Update minimal port to use the new "minimal" rom level config (this is a
no-op change the binary is the same size and contains the exact same
symbols).

Signed-off-by: Jim Mussared <jim.mussared@gmail.com>
@jimmo jimmo force-pushed the port-config-template branch from f0b1cf0 to 169fc6f Compare September 15, 2021 12:29
@jimmo
Copy link
Member Author
jimmo commented Sep 15, 2021

Updated anything enabled via level=core that was dependent on a parent option also enabled via level=core to just be unconditionally enabled by default. i.e. MICROPY_PY_SYS_EXIT depends on MICROPY_PY_SYS.

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Sep 16, 2021
@dpgeorge
Copy link
Member

Very nice, thank you! I wanted to do this for a while now.

Merged in 01374d9 and 910e060

@dpgeorge dpgeorge closed this Sep 16, 2021
tannewt added a commit to tannewt/circuitpython that referenced this pull request Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0