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

makefiles, treewide: Remove MCU variable #20397

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

chrysn
Copy link
Member

@chrysn chrysn commented Feb 16, 2024

Contribution description

The MCU variable, which appears to historically be a place where a CPU (which defines which cpu/... directory gets included) can be refined, is not in use as advertised (its documentation said that it is "set by the board's Makefile.include, or defaulted to the same value as CPU", but grepping for MCU in boards/*/Makefile.include just mentions the term in prose). CPU is always equal to MCU as we use it now. In its place, we nowadays use the CPU_FAM and the CPU_MODEL variables.

This PR removes MCU from the Makefiles, and changes all the places where MCU is used (mainly esp_common and some drivers, [edit] as well as some examples) into the equivalent CPU checks.

Testing procedure

  • Green CI is all the testing that makes sense. If we could assert that the compiled binaries (or even the C code after preprocessor) is the same, that'd be great, but we don't have those tools. [edit: Some binaries changed b/c they reported "the MCU" and now report "the CPU", and one example reported both CPU and MCU, thus shrank.]

@github-actions github-actions bot added Area: build system Area: Build system Area: drivers Area: Device drivers Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports Area: sys Area: System labels Feb 16, 2024
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: examples Area: Example Applications labels Feb 16, 2024
@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Feb 16, 2024
@chrysn
Copy link
Member Author

chrysn commented Feb 16, 2024

I'm flagging this as an "API change", not only because it's the right thing to do, but also because it will earn it a place around "changes possibly necessary in applications" section of the next release notes.

@riot-ci
Copy link

riot-ci commented Feb 16, 2024

Murdock results

✔️ PASSED

9716510 pkg/micropython: Adjust to removed RIOT_MCU

Success Failures Total Runtime
10011 0 10011 11m:46s

Artifacts

@chrysn chrysn added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 17, 2024
@github-actions github-actions bot added the Area: pkg Area: External package ports label Feb 17, 2024
@chrysn
Copy link
Member Author

chrysn commented Feb 17, 2024

One late addition is adding a RIOT_MCU variable to micropython because unlike for other packages, the RIOT specific code there is not in-tree but in a fork of upstream code; PR there incoming[edit: here -- in kaspar030/micropython#5]

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@maribu maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 17, 2024
@chrysn chrysn removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 17, 2024
@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 17, 2024
kaspar030 pushed a commit to kaspar030/micropython that referenced this pull request Feb 17, 2024
@chrysn
Copy link
Member Author

chrysn commented Feb 18, 2024

Updated once more because the micropython RIOT port now doesn't need a workaround any more (kaspar030/micropython#5 was merged); updating micropython in a separate commit instead.

There has been some flakiness in unrelated tests, let's see whether we're lucky this time.

Replacing RIOT_MCU with RIOT_CPU is the only change between the old and
the new version.
@chrysn
Copy link
Member Author

chrysn commented Feb 18, 2024

... and a few occurrences of (MCU) or {MCU} have been missing before (which were used in Makefiles, and have now been grepped for exhaustively).

@chrysn chrysn added this pull request to the merge queue Feb 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 18, 2024
@chrysn chrysn added this pull request to the merge queue Feb 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 18, 2024
@chrysn chrysn added this pull request to the merge queue Feb 18, 2024
Merged via the queue into RIOT-OS:master with commit fe2d590 Feb 19, 2024
25 checks passed
@chrysn chrysn deleted the remove-mcu branch February 19, 2024 11:32
@benpicco
Copy link
Contributor

This broke esp8266 😕

@miri64
Copy link
Member

miri64 commented Feb 27, 2024

Why wasn't this deprecated first? This also broke some external repos, such as https://github.com/netd-tud/riot-exercises/ (luckily we are currently still in the progress of deploying it still).

@chrysn
Copy link
Member Author

chrysn commented Feb 27, 2024

The discrepancies between documentation of the MCU makefile variable and the actual mechansims use by CPU implementations indicated that the mechanism was not used.

The removal of the RIOT_MCU define was a detail I didn't individually consider for whether or not it should have been deprecated (it could still have been set to indicate RIOT_CPU). I'd have no objections to re-introducing a RIOT_MCU define under a deprecation. However, given that compiler constants can not be annotated with deprecation warnings, I doubt doing a deprecation there would have any effect other than delaying when people notice that their builds are failing.

@maribu
Copy link
Member

maribu commented Feb 27, 2024

To be honest, I did not consider RIOT_MCU as part of any public API, but rather an internal implementation detail. There was no doc for it (and there is no doc for the new RIOT_CPU, either).

I think it would be possible to add a proper deprecation if our mechanism to generate riotbuild.h would be a bit more sophisticated. But someone(tm) would have to do that.

Note that in doc/doxygen/riot.doxyfile a number of predefined macros are given, e.g. RIOT_FILE_RELATIVE. But neither RIOT_CPU nor RIOT_MCU made it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Area: examples Area: Example Applications Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants