Skip to content

cpu/esp8266: Add missing MCU_ESP8266 define for vendor code #20415

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

Conversation

maribu
Copy link
Member

@maribu maribu commented Feb 22, 2024

Contribution description

Dropping the MCU variable in #20397 resulted in breaking the compilation of the vendor code in a non-obvious way. Adding a CFLAGS += -DMCU_ESP8266 specifically to the modules containing the vendor code that need this define fixes the issue.

Testing procedure

ESP8266 should boot again

Issues/PRs references

Follow up to #20397

Alternative to #20409

@maribu maribu requested a review from gschorcht as a code owner February 22, 2024 12:22
@github-actions github-actions bot added Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Feb 22, 2024
@maribu maribu requested review from chrysn and benpicco February 22, 2024 12:22
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 22, 2024
@riot-ci
Copy link

riot-ci commented Feb 22, 2024

Murdock results

✔️ PASSED

f726b63 cpu/esp8266: Add missing MCU_ESP8266 define for vendor code

Success Failures Total Runtime
9997 0 9997 11m:13s

Artifacts

chrysn added a commit to chrysn-pull-requests/RIOT that referenced this pull request Feb 22, 2024
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

ESP8266 is working again, thank you for figuring this out!

@benpicco benpicco enabled auto-merge February 22, 2024 13:28
@benpicco benpicco added this pull request to the merge queue Feb 22, 2024
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Feb 22, 2024
@chrysn chrysn removed this pull request from the merge queue due to a manual request Feb 22, 2024
@chrysn
Copy link
Member

chrysn commented Feb 22, 2024

These two files may originally be vendor files, but have been heavily customized, including AIU the addition of the MCU_. I'm not objecting to this fix, but think that #20416 is the approach that leaves less legacy sitting around.

Either of you please have a look and either ACK the other one, or just hit the "merge when ready" button again here.

@maribu maribu closed this Feb 22, 2024
@maribu
Copy link
Member Author

maribu commented Feb 22, 2024

Let's go with #20416 instead.

I incorrectly assumed the vendor files were unmodified and just adding the required CFLAGS would ease updating.

@maribu maribu deleted the cpu/esp8266/add_missing_defines branch February 22, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer 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
Development

Successfully merging this pull request may close these issues.

4 participants