Skip to content

Revert "Remove MCU variable" - unbreaks esp8266#20409

Closed
benpicco wants to merge 2 commits intoRIOT-OS:masterfrom
benpicco:revert-20397
Closed

Revert "Remove MCU variable" - unbreaks esp8266#20409
benpicco wants to merge 2 commits intoRIOT-OS:masterfrom
benpicco:revert-20397

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Feb 21, 2024

Contribution description

This cleanup broke boot on esp8266, not sure if other things broke too.
I'm not convinced this 'cleanup' is even worthwhile since 'CPU' isn't an exact term either (could mean just the CPU core).

If you disagree, feel free to open an alternative PR that fixes the issue on a more fine grained level.

Testing procedure

Flash any application on e.g. esp8266-esp-12x - on master it will output nothing currently.

Issues/PRs references

reverts #20397

@benpicco benpicco requested a review from chrysn February 21, 2024 21:50
@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: build system Area: Build system Area: pkg Area: External package ports 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 Area: examples Area: Example Applications labels Feb 21, 2024
@benpicco benpicco requested a review from maribu February 21, 2024 21:50
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Feb 21, 2024
@riot-ci
Copy link
Copy Markdown

riot-ci commented Feb 21, 2024

Murdock results

✔️ PASSED

4c11433 Revert "pkg/micropython: Adjust to removed RIOT_MCU"

Success Failures Total Runtime
9997 0 9997 10m:40s

Artifacts

@maribu
Copy link
Copy Markdown
Member

maribu commented Feb 22, 2024

The following symbols disappeared:

 s_reent 	b 	240
__fsetlocking ⓘ 	T 	113
__libc_init_array ⓘ 	T 	96
syscalls_init ⓘ 	T 	79
environ 	B 	4
_init ⓘ 	T 	2
syscalls_init_arch ⓘ

I have a hard time to figure out why.

But honestly, let's not revert this. It exposed how fragile and weird things currently are. I'm working on a fix.

@chrysn
Copy link
Copy Markdown
Member

chrysn commented Feb 22, 2024

Thanks Ben for spotting this, Marian for picking it up, & sorry you're getting the fall-out.

I agree with Marian that we should better find what is really using MCU covertly here. That this went unnoticed shows how brittle relying on this kind of variables is, compared to using features that are (now about to be) rigorously checked and documented. (Then again, if we don't come up with something working, reverting is the next best option, to be followed by a deprecation and more thorough investigation).

I'm not convinced this 'cleanup' is even worthwhile since 'CPU' isn't an exact term either (could mean just the CPU core).

MCU is just as illdefined, especially given its only documentation was inaccurate. CPU really just means "whatever is grouped in one directory cpu/; the CPU features are what should be checked by code. (And there is CPU_FAM and CPU_MODEL, but IMO those too should rather use features).

@maribu maribu mentioned this pull request Feb 22, 2024
Closed
chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Feb 22, 2024
@chrysn
cpu/esp: Use CPU_ESP8266 define instead of the removed MCU_ESP8266
c3020ce 
Follow-up-for: RIOT-OS#20397
Closes: RIOT-OS#20409
Closes: RIOT-OS#20415
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Feb 22, 2024

Closing in favor of #20415

@benpicco benpicco closed this Feb 22, 2024
@benpicco benpicco deleted the revert-20397 branch February 22, 2024 13:29
@chrysn chrysn mentioned this pull request Feb 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@gschorcht gschorcht Awaiting requested review from gschorcht gschorcht is a code owner

@chrysn chrysn Awaiting requested review from chrysn

@maribu maribu Awaiting requested review from maribu

Assignees

No one assigned

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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benpicco @riot-ci @maribu @chrysn