Skip to content

Rework use of pico_cmake_set in board headers to make it slightly less magic/confusing #2397

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

Merged
merged 5 commits into from
Apr 14, 2025

Conversation

kilograham
Copy link
Contributor

Particularly, the fact that the lines are commented is confusing

  • prefer "pico_cmake_set(var, value)" over "// pico_cmake_set var = value"
  • prefer "pico_cmake_set_default(var, value)" over "// pico_cmake_set_default var = value"
  • move these inside the header include guards as CLion complains

Note that the macros are defined in "pico.h" however that is not explicitly included by the board headers; this will probably confuse some VS code syntax highligting, so lets see how it looks - i'd prefer to avoid having to include a header just for this

…s magic/confusing

- prefer "pico_cmake_set(var, value)" over "// pico_cmake_set var = value"
- prefer "pico_cmake_set_default(var, value)" over "// pico_cmake_set_default var = value"
- move these inside the header include guards as CLion complains

Note that the macros are defined in "pico.h" however that is not explicitly included by the board headers; this
will probably confuse some VS code syntax highligting, so lets see how it looks - i'd prefer to avoid having
to include a header just for this
@kilograham kilograham requested a review from will-v-pi April 4, 2025 18:17
@@ -73,7 +73,7 @@
#endif

// board has 16M onboard flash
// pico_cmake_set_default PICO_FLASH_SIZE_BYTES = (16 * 1024 * 1024)
pico_cmake_set_default(PICO_FLASH_SIZE_BYTES, 16 * 1024 * 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this "looks" like a proper macro, is there a risk that people might expect it to behave like a proper macro? E.g. if somebody did:

// board has 16M onboard flash
#define DEFAULT_MEMORY_SIZE (16 * 1024 * 1024)
pico_cmake_set_default(PICO_FLASH_SIZE_BYTES, DEFAULT_MEMORY_SIZE)
#ifndef PICO_FLASH_SIZE_BYTES
#define PICO_FLASH_SIZE_BYTES DEFAULT_MEMORY_SIZE
#endif

would it work as expected?

@lurch
Copy link
Contributor

lurch commented Apr 4, 2025

IMHO it would be great if this PR could also update https://github.com/raspberrypi/pico-sdk/blob/develop/tools/check_board_header.py to cope with this new syntax. That's normally something that I'd do myself, but I'm about to go on holiday 😎

Copy link
Contributor

@will-v-pi will-v-pi left a comment

Choose a reason for hiding this comment

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

This does seem to work fine with VS Code when using compile_commands.json - although it might make people think they can use pico_cmake_set outside of the board headers, so it might be good to add some sort of guard against that? Or change the definition to pico_board_cmake_set to make it clearer that it only works in board headers?

@kilograham
Copy link
Contributor Author

pico_board_cmake_set good idea - this also helps with something that had been bothering me that a commented out new style one looks a lot like an old style one!

@will-v-pi
Copy link
Contributor

I've updated the python script, added back the brackets for PICO_FLASH_SIZE_BYTES (as the python script checks for exact matches), and renamed to pico_board_cmake_set and pico_board_cmake_set_default

If you do want the brackets removed, then I can modify the python script to ignore the brackets instead

@kilograham
Copy link
Contributor Author

no brackets are good ;-) i was gonna put them back, but lost interest ;-)

@kilograham kilograham added this to the 2.1.2 milestone Apr 14, 2025
@kilograham kilograham merged commit fed7188 into develop Apr 14, 2025
8 checks passed
@kilograham kilograham deleted the pico_cmake_set branch April 14, 2025 15:50
lurch added a commit that referenced this pull request Apr 26, 2025
- make the error messages less misleading
- suggest replacing the old comment-style syntax with the new macro-style syntax
kilograham pushed a commit that referenced this pull request Apr 26, 2025
- make the error messages less misleading
- suggest replacing the old comment-style syntax with the new macro-style syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants