Make boot/picobin.h self-contained#2944
Open
anoshyn wants to merge 1 commit into
Open
Conversation
Including boot/picobin.h before any other SDK header errored out: pico/platform.h:20: #error pico/platform.h should not be included directly; include pico.h instead picobin.h was reaching into pico/platform.h directly, which only works when something else (typically pico.h) has already set _PICO_H. The diagnostic blamed pico/platform.h while the actual offender was the non-obvious indirect include via boot/picobin.h. Drop the dependency on pico/platform.h entirely. The only platform.h facility picobin.h uses is _u(), which the header already defined as a fallback for NO_PICO_PLATFORM builds; promote that fallback to the unconditional definition. Also add <stdint.h> alongside the existing <stdbool.h> include in the C-only block so uint32_t is available without leaning on platform.h's transitive includes. Verified by: - Repro: a TU that only does `#include "boot/picobin.h"` now compiles. - Full SDK build remains clean; existing consumers (cyw43_driver, pico_crt0, pico_bootrom, cyw43_partition_firmware) include picobin.h alongside pico.h/bootrom.h and so already pull in platform.h via those, unaffected by this change. Fixes raspberrypi#2797. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
@kilograham @lurch when you have a moment, would you mind taking a look? This is a small standalone-include fix for |
Contributor
|
please don't tag people in issues |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#include "boot/picobin.h"before any other SDK header errors out with:boot/picobin.hwas pulling inpico/platform.hdirectly (gated only byNO_PICO_PLATFORM), which works only when something else (typicallypico.h) has already set_PICO_H. The diagnostic blamedpico/platform.h, while the actual offender was the non-obvious indirect include viaboot/picobin.h— a confusing experience flagged in #2797.The header only uses one platform.h facility: the
_u()literal-suffix macro. The same_u()fallback already existed in theNO_PICO_PLATFORMbranch of this header. Promote that fallback to the unconditional definition and drop thepico/platform.hinclude. Add<stdint.h>next to the existing<stdbool.h>in the C-only block souint32_tis available standalone.This keeps
boot/picobin.hat the lightweight "common" tier (no new heavy SDK dependency) and makes it includable in any order.Fixes #2797.
Test plan
#include "boot/picobin.h"fails to compile, citing thepico/platform.h should not be included directlyerror.__ASSEMBLER__-mode preprocessing still works (_u(x)falls through to the asm branch).cmake --build build) for the defaultPICO_PLATFORM=rp2040 PICO_BOARD=picoconfig. Existing internal consumers (pico_cyw43_driver,pico_crt0,pico_bootrom,cyw43_partition_firmware) include picobin.h alongsidepico.h/pico/bootrom.h, so they already pull in platform.h transitively and are unaffected.