gh-raspberrypi/pico-sdk

Watch

Fork 0

Propogate changed pico_cmake_set_default values to the compilation #2034

Merged

will-v-pi merged 3 commits from pico-board-cmake-overrides into develop

2024-11-21 17:47:51 -06:00

will-v-pi commented

2024-11-12 12:01:12 -06:00

(Migrated from github.com)

Currently, if you pass -DPICO_FLASH_SIZE_BYTES="(2 * 1024 * 1024)" to CMake, that modified flash size is only used in CMake (when generating the linker script) and doesn't propogate through to the compilation. To set it for the compilation you need to add a separate target_compile_definitions(my_exe INTERFACE PICO_FLASH_SIZE_BYTES=${PICO_FLASH_SIZE_BYTES}) to your CMakeLists.txt file.

This PR changes this behaviour, so pico_cmake_set_default values that have been overridden (eg with -DPICO_... or set(PICO_...) will propogate through to the compilation. If the default value isn't modified then the behaviour is not changed, and you can still use target_compile_definitions to set the values for compilation only.

Adds a PICO_BOARD_CMAKE_OVERRIDES common scope variable, to contain any pico_cmake_set_default CMake variables which have been overwritten. This is populated when reading the board header, and then read when compiling pico_base_headers.

Also add pico_cmake_set_default PICO_RP2350_A2_SUPPORTED = 1 to board headers to enable/disable the E10 abs-block fix, and modify check_board_header.py to support this. This allows specifying -DPICO_RP2350_A2_SUPPORTED=0 to CMake to prevent the creation of an abs_block, for example this could be used when you are not planning on dragging & dropping your UF2 into partition tables.

Currently, if you pass `-DPICO_FLASH_SIZE_BYTES="(2 * 1024 * 1024)"` to CMake, that modified flash size is only used in CMake (when generating the linker script) and doesn't propogate through to the compilation. To set it for the compilation you need to add a separate `target_compile_definitions(my_exe INTERFACE PICO_FLASH_SIZE_BYTES=${PICO_FLASH_SIZE_BYTES})` to your CMakeLists.txt file. This PR changes this behaviour, so `pico_cmake_set_default` values that have been overridden (eg with `-DPICO_...` or `set(PICO_...)` will propogate through to the compilation. If the default value isn't modified then the behaviour is not changed, and you can still use `target_compile_definitions` to set the values for compilation only. Adds a PICO_BOARD_CMAKE_OVERRIDES common scope variable, to contain any pico_cmake_set_default CMake variables which have been overwritten. This is populated when reading the board header, and then read when compiling `pico_base_headers`. Also add `pico_cmake_set_default PICO_RP2350_A2_SUPPORTED = 1` to board headers to enable/disable the E10 abs-block fix, and modify `check_board_header.py` to support this. This allows specifying `-DPICO_RP2350_A2_SUPPORTED=0` to CMake to prevent the creation of an abs_block, for example this could be used when you are not planning on dragging & dropping your UF2 into partition tables.

kilograham commented

2024-11-18 17:18:20 -06:00

(Migrated from github.com)

@will-v-pi can you be a bit more precise in the comment above "if you compile with... is only used in the CMake" doesn't make any sense (to me)... perhaps you mean "if you pass -Dxxx to CMake"

lurch commented

2024-11-18 19:10:36 -06:00

(Migrated from github.com)

So my limited CMake knowledge means that I don't fully understand all of this, but it seems to me like there are now a couple of settings (PICO_FLASH_SIZE_BYTES and PICO_RP2350_A2_SUPPORTED) which have to be set to the same value in both the "cmake world" (via pico_cmake_set_default) and also the "compiler world" (via #define). Is there any way we could come up with some generic mechanism, rather than needing e.g.

// pico_cmake_set_default PICO_FLASH_SIZE_BYTES = (4 * 1024 * 1024)
#ifndef PICO_FLASH_SIZE_BYTES
#define PICO_FLASH_SIZE_BYTES (4 * 1024 * 1024)
#endif

// pico_cmake_set_default PICO_RP2350_A2_SUPPORTED = 1
#ifndef PICO_RP2350_A2_SUPPORTED
#define PICO_RP2350_A2_SUPPORTED 1
#endif

in the header-file for every board that uses the RP2350? Or is this already the "best" way of handling this? (I guess my concern is how badly would "things go wrong" if the "cmake world" sees a different value to the "compiler world"?)

So my limited CMake knowledge means that I don't fully understand all of this, but it seems to me like there are now a couple of settings (`PICO_FLASH_SIZE_BYTES` and `PICO_RP2350_A2_SUPPORTED`) which have to be set to the **same** value in both the "cmake world" (via `pico_cmake_set_default`) and also the "compiler world" (via `#define`). Is there any way we could come up with some generic mechanism, rather than needing e.g. ```c // pico_cmake_set_default PICO_FLASH_SIZE_BYTES = (4 * 1024 * 1024) #ifndef PICO_FLASH_SIZE_BYTES #define PICO_FLASH_SIZE_BYTES (4 * 1024 * 1024) #endif // pico_cmake_set_default PICO_RP2350_A2_SUPPORTED = 1 #ifndef PICO_RP2350_A2_SUPPORTED #define PICO_RP2350_A2_SUPPORTED 1 #endif ``` in the header-file for every board that uses the RP2350? Or is this already the "best" way of handling this? (I guess my concern is how badly would "things go wrong" if the "cmake world" sees a different value to the "compiler world"?)

lurch commented

2024-11-18 20:42:55 -06:00

(Migrated from github.com)

@will-v-pi I just cloned this branch and tried running tools/check_all_board_headers.sh locally, and it seems that you somehow missed out adding the

// pico_cmake_set_default PICO_RP2350_A2_SUPPORTED = 1

line to the hellbender_0001.h and pico2_w.h header files!

Also, here's a small tweak to the tools/check_board_header.py (feel free to include it or ignore it as you see fit) :

diff --git a/tools/check_board_header.py b/tools/check_board_header.py
index 3849e14..c259c81 100755
--- a/tools/check_board_header.py
+++ b/tools/check_board_header.py
@@ -29,6 +29,11 @@ chip_interfaces = {
     'RP2350B': "src/rp2350/rp2350b_interface_pins.json",
 }
 
+compulsory_cmake_settings = set(['PICO_PLATFORM'])
+compulsory_cmake_default_settings = set(['PICO_FLASH_SIZE_BYTES'])
+matching_cmake_default_settings = set(['PICO_FLASH_SIZE_BYTES', 'PICO_RP2350_A2_SUPPORTED'])
+compulsory_defines = set(['PICO_FLASH_SIZE_BYTES'])
+
 DefineType = namedtuple("DefineType", ["name", "value", "resolved_value", "lineno"])
 
 def list_to_string_with(lst, joiner):
@@ -230,7 +235,13 @@ with open(board_header) as header_fh:
             if name in cmake_settings:
                 raise Exception("{}:{}  Multiple values for pico_cmake_set {} ({} and {})".format(board_header, lineno, name, cmake_settings[name].value, value))
             else:
-               cmake_settings[name] = DefineType(name, value, None, lineno)
+                if value:
+                    try:
+                        # most cmake settings are integer values
+                        value = int(value, 0)
+                    except ValueError:
+                        pass
+                cmake_settings[name] = DefineType(name, value, None, lineno)
             continue
 
         # look for "// pico_cmake_set_default BLAH_BLAH=42"
@@ -246,7 +257,13 @@ with open(board_header) as header_fh:
             if name in cmake_default_settings:
                 raise Exception("{}:{}  Multiple values for pico_cmake_set_default {} ({} and {})".format(board_header, lineno, name, cmake_default_settings[name].value, value))
             else:
-               cmake_default_settings[name] = DefineType(name, value, None, lineno)
+                if value:
+                    try:
+                        # most cmake settings are integer values
+                        value = int(value, 0)
+                    except ValueError:
+                        pass
+                cmake_default_settings[name] = DefineType(name, value, None, lineno)
             continue
 
         # look for "#else"
@@ -362,8 +379,9 @@ if board_header_basename == "none.h":
     chip = 'RP2040'
     other_chip = 'RP2350'
 else:
-    if 'PICO_PLATFORM' not in cmake_settings:
-        raise Exception("{} is missing a pico_cmake_set {} comment".format(board_header, 'PICO_PLATFORM'))
+    for setting in compulsory_cmake_settings:
+        if setting not in cmake_settings:
+            raise Exception("{} is missing a pico_cmake_set {} comment".format(board_header, setting))
     if cmake_settings['PICO_PLATFORM'].value == "rp2040":
         chip = 'RP2040'
         other_chip = 'RP2350'
@@ -375,19 +393,25 @@ else:
             chip = 'RP2350B'
         if not board_header.endswith("amethyst_fpga.h"):
             if 'PICO_RP2350_A2_SUPPORTED' not in cmake_default_settings:
-                raise Exception("{} uses chip {} but is missing a pico_cmake_set_default {}".format(board_header, chip, 'PICO_RP2350_A2_SUPPORTED'))
+                raise Exception("{} uses chip {} but is missing a pico_cmake_set_default {} comment".format(board_header, chip, 'PICO_RP2350_A2_SUPPORTED'))
             if 'PICO_RP2350_A2_SUPPORTED' not in defines:
                 raise Exception("{} uses chip {} but is missing a #define {}".format(board_header, chip, 'PICO_RP2350_A2_SUPPORTED'))
-            if int(cmake_default_settings['PICO_RP2350_A2_SUPPORTED'].value) != defines['PICO_RP2350_A2_SUPPORTED'].resolved_value:
-                raise Exception("{} has mismatched pico_cmake_set_default and #define values for {}".format(board_header, 'PICO_RP2350_A2_SUPPORTED'))
             if defines['PICO_RP2350_A2_SUPPORTED'].resolved_value != 1:
                 raise Exception("{} sets #define {} {} (should be 1)".format(board_header, chip, 'PICO_RP2350_A2_SUPPORTED', defines['PICO_RP2350_A2_SUPPORTED'].resolved_value))
-    if 'PICO_FLASH_SIZE_BYTES' not in cmake_default_settings:
-        raise Exception("{} is missing a pico_cmake_set_default {} comment".format(board_header, 'PICO_FLASH_SIZE_BYTES'))
-    if 'PICO_FLASH_SIZE_BYTES' not in defines:
-        raise Exception("{} is missing a #define {}".format(board_header, 'PICO_FLASH_SIZE_BYTES'))
-    if cmake_default_settings['PICO_FLASH_SIZE_BYTES'].value != defines['PICO_FLASH_SIZE_BYTES'].resolved_value:
-        raise Exception("{} has mismatched pico_cmake_set_default and #define values for {}".format(board_header, 'PICO_FLASH_SIZE_BYTES'))
+    for setting in compulsory_cmake_default_settings:
+        if setting not in cmake_default_settings:
+            raise Exception("{} is missing a pico_cmake_set_default {} comment".format(board_header, setting))
+    for setting in matching_cmake_default_settings:
+        if setting in cmake_default_settings and setting not in defines:
+            raise Exception("{} has pico_cmake_set_default {} but is missing a matching #define".format(board_header, setting))
+        elif setting in defines and setting not in cmake_default_settings:
+            raise Exception("{} has #define {} but is missing a matching pico_cmake_set_default comment".format(board_header, setting))
+        elif setting in defines and setting in cmake_default_settings:
+            if cmake_default_settings[setting].value != defines[setting].resolved_value:
+                raise Exception("{} has mismatched pico_cmake_set_default and #define values for {}".format(board_header, setting))
+    for setting in compulsory_defines:
+        if setting not in defines:
+            raise Exception("{} is missing a #define {}".format(board_header, setting))
 
 if chip is None:
     raise Exception("Couldn't determine chip for {}".format(board_header))

@will-v-pi I just cloned this branch and tried running `tools/check_all_board_headers.sh` locally, and it seems that you somehow missed out adding the ```c // pico_cmake_set_default PICO_RP2350_A2_SUPPORTED = 1 ``` line to the `hellbender_0001.h` and `pico2_w.h` header files! Also, here's a small tweak to the `tools/check_board_header.py` (feel free to include it or ignore it as you see fit) : ```patch diff --git a/tools/check_board_header.py b/tools/check_board_header.py index 3849e14..c259c81 100755 --- a/tools/check_board_header.py +++ b/tools/check_board_header.py @@ -29,6 +29,11 @@ chip_interfaces = { 'RP2350B': "src/rp2350/rp2350b_interface_pins.json", } +compulsory_cmake_settings = set(['PICO_PLATFORM']) +compulsory_cmake_default_settings = set(['PICO_FLASH_SIZE_BYTES']) +matching_cmake_default_settings = set(['PICO_FLASH_SIZE_BYTES', 'PICO_RP2350_A2_SUPPORTED']) +compulsory_defines = set(['PICO_FLASH_SIZE_BYTES']) + DefineType = namedtuple("DefineType", ["name", "value", "resolved_value", "lineno"]) def list_to_string_with(lst, joiner): @@ -230,7 +235,13 @@ with open(board_header) as header_fh: if name in cmake_settings: raise Exception("{}:{} Multiple values for pico_cmake_set {} ({} and {})".format(board_header, lineno, name, cmake_settings[name].value, value)) else: - cmake_settings[name] = DefineType(name, value, None, lineno) + if value: + try: + # most cmake settings are integer values + value = int(value, 0) + except ValueError: + pass + cmake_settings[name] = DefineType(name, value, None, lineno) continue # look for "// pico_cmake_set_default BLAH_BLAH=42" @@ -246,7 +257,13 @@ with open(board_header) as header_fh: if name in cmake_default_settings: raise Exception("{}:{} Multiple values for pico_cmake_set_default {} ({} and {})".format(board_header, lineno, name, cmake_default_settings[name].value, value)) else: - cmake_default_settings[name] = DefineType(name, value, None, lineno) + if value: + try: + # most cmake settings are integer values + value = int(value, 0) + except ValueError: + pass + cmake_default_settings[name] = DefineType(name, value, None, lineno) continue # look for "#else" @@ -362,8 +379,9 @@ if board_header_basename == "none.h": chip = 'RP2040' other_chip = 'RP2350' else: - if 'PICO_PLATFORM' not in cmake_settings: - raise Exception("{} is missing a pico_cmake_set {} comment".format(board_header, 'PICO_PLATFORM')) + for setting in compulsory_cmake_settings: + if setting not in cmake_settings: + raise Exception("{} is missing a pico_cmake_set {} comment".format(board_header, setting)) if cmake_settings['PICO_PLATFORM'].value == "rp2040": chip = 'RP2040' other_chip = 'RP2350' @@ -375,19 +393,25 @@ else: chip = 'RP2350B' if not board_header.endswith("amethyst_fpga.h"): if 'PICO_RP2350_A2_SUPPORTED' not in cmake_default_settings: - raise Exception("{} uses chip {} but is missing a pico_cmake_set_default {}".format(board_header, chip, 'PICO_RP2350_A2_SUPPORTED')) + raise Exception("{} uses chip {} but is missing a pico_cmake_set_default {} comment".format(board_header, chip, 'PICO_RP2350_A2_SUPPORTED')) if 'PICO_RP2350_A2_SUPPORTED' not in defines: raise Exception("{} uses chip {} but is missing a #define {}".format(board_header, chip, 'PICO_RP2350_A2_SUPPORTED')) - if int(cmake_default_settings['PICO_RP2350_A2_SUPPORTED'].value) != defines['PICO_RP2350_A2_SUPPORTED'].resolved_value: - raise Exception("{} has mismatched pico_cmake_set_default and #define values for {}".format(board_header, 'PICO_RP2350_A2_SUPPORTED')) if defines['PICO_RP2350_A2_SUPPORTED'].resolved_value != 1: raise Exception("{} sets #define {} {} (should be 1)".format(board_header, chip, 'PICO_RP2350_A2_SUPPORTED', defines['PICO_RP2350_A2_SUPPORTED'].resolved_value)) - if 'PICO_FLASH_SIZE_BYTES' not in cmake_default_settings: - raise Exception("{} is missing a pico_cmake_set_default {} comment".format(board_header, 'PICO_FLASH_SIZE_BYTES')) - if 'PICO_FLASH_SIZE_BYTES' not in defines: - raise Exception("{} is missing a #define {}".format(board_header, 'PICO_FLASH_SIZE_BYTES')) - if cmake_default_settings['PICO_FLASH_SIZE_BYTES'].value != defines['PICO_FLASH_SIZE_BYTES'].resolved_value: - raise Exception("{} has mismatched pico_cmake_set_default and #define values for {}".format(board_header, 'PICO_FLASH_SIZE_BYTES')) + for setting in compulsory_cmake_default_settings: + if setting not in cmake_default_settings: + raise Exception("{} is missing a pico_cmake_set_default {} comment".format(board_header, setting)) + for setting in matching_cmake_default_settings: + if setting in cmake_default_settings and setting not in defines: + raise Exception("{} has pico_cmake_set_default {} but is missing a matching #define".format(board_header, setting)) + elif setting in defines and setting not in cmake_default_settings: + raise Exception("{} has #define {} but is missing a matching pico_cmake_set_default comment".format(board_header, setting)) + elif setting in defines and setting in cmake_default_settings: + if cmake_default_settings[setting].value != defines[setting].resolved_value: + raise Exception("{} has mismatched pico_cmake_set_default and #define values for {}".format(board_header, setting)) + for setting in compulsory_defines: + if setting not in defines: + raise Exception("{} is missing a #define {}".format(board_header, setting)) if chip is None: raise Exception("Couldn't determine chip for {}".format(board_header)) ```

will-v-pi commented

2024-11-19 03:47:05 -06:00

(Migrated from github.com)

@will-v-pi can you be a bit more precise in the comment above "if you compile with... is only used in the CMake" doesn't make any sense (to me)... perhaps you mean "if you pass -Dxxx to CMake"

Yes, that's what I meant - I've updated the comment

> @will-v-pi can you be a bit more precise in the comment above "if you compile with... is only used in the CMake" doesn't make any sense (to me)... perhaps you mean "if you pass -Dxxx to CMake" Yes, that's what I meant - I've updated the comment

will-v-pi commented

2024-11-20 04:46:17 -06:00

(Migrated from github.com)

So my limited CMake knowledge means that I don't fully understand all of this, but it seems to me like there are now a couple of settings (PICO_FLASH_SIZE_BYTES and PICO_RP2350_A2_SUPPORTED) which have to be set to the same value in both the "cmake world" (via pico_cmake_set_default) and also the "compiler world" (via #define). Is there any way we could come up with some generic mechanism, rather than needing e.g.
// pico_cmake_set_default PICO_FLASH_SIZE_BYTES = (4 * 1024 * 1024)
#ifndef PICO_FLASH_SIZE_BYTES
#define PICO_FLASH_SIZE_BYTES (4 * 1024 * 1024)
#endif

// pico_cmake_set_default PICO_RP2350_A2_SUPPORTED = 1
#ifndef PICO_RP2350_A2_SUPPORTED
#define PICO_RP2350_A2_SUPPORTED 1
#endif
in the header-file for every board that uses the RP2350? Or is this already the "best" way of handling this? (I guess my concern is how badly would "things go wrong" if the "cmake world" sees a different value to the "compiler world"?)

Personally, I think this is the "best" way of handling things - everything that directly affects the compilation is in normal pre-processor #defines, and then the things that affect the CMake configuration are in comments. The board checking scripts are available to check they match up, but even if the don't match it won't cause any major problems.

It would be fine for the "CMake World" to see a different value to the "compiler world" - for example for PICO_FLASH_SIZE_BYTES it just means that the linker script will have one flash size, and functions using the flash size (eg flash_range_erase) will assert values are in range based on a different size. This could mean that when linking my code it only uses 2MB of flash, but when erasing I can erase all 4MB of the flash, which could be a valid use case.

> So my limited CMake knowledge means that I don't fully understand all of this, but it seems to me like there are now a couple of settings (`PICO_FLASH_SIZE_BYTES` and `PICO_RP2350_A2_SUPPORTED`) which have to be set to the **same** value in both the "cmake world" (via `pico_cmake_set_default`) and also the "compiler world" (via `#define`). Is there any way we could come up with some generic mechanism, rather than needing e.g. > > ```c > // pico_cmake_set_default PICO_FLASH_SIZE_BYTES = (4 * 1024 * 1024) > #ifndef PICO_FLASH_SIZE_BYTES > #define PICO_FLASH_SIZE_BYTES (4 * 1024 * 1024) > #endif > > // pico_cmake_set_default PICO_RP2350_A2_SUPPORTED = 1 > #ifndef PICO_RP2350_A2_SUPPORTED > #define PICO_RP2350_A2_SUPPORTED 1 > #endif > ``` > > in the header-file for every board that uses the RP2350? Or is this already the "best" way of handling this? (I guess my concern is how badly would "things go wrong" if the "cmake world" sees a different value to the "compiler world"?) Personally, I think this is the "best" way of handling things - everything that directly affects the compilation is in normal pre-processor `#defines`, and then the things that affect the CMake configuration are in comments. The board checking scripts are available to check they match up, but even if the don't match it won't cause any major problems. It would be fine for the "CMake World" to see a different value to the "compiler world" - for example for `PICO_FLASH_SIZE_BYTES` it just means that the linker script will have one flash size, and functions using the flash size (eg `flash_range_erase`) will assert values are in range based on a different size. This could mean that when linking my code it only uses 2MB of flash, but when erasing I can erase all 4MB of the flash, which could be a valid use case.

👍 1

lurch commented

2024-11-20 06:25:16 -06:00

(Migrated from github.com)

It would be fine for the "CMake World" to see a different value to the "compiler world"

Brilliant, thanks for reassuring my paranoid little brain 🧠 😂

> It would be fine for the "CMake World" to see a different value to the "compiler world" Brilliant, thanks for reassuring my paranoid little brain :brain: :joy:

kilograham (Migrated from github.com) approved these changes

2024-11-21 17:47:42 -06:00

kilograham (Migrated from github.com) left a comment

I'm still not happy with what I originally did here, and was worried about proliferation of requirements for more // pico_cmake_set but with the validation script changes, we're at least a little better off.

Also proliferating the settings override one way (Cmake->compile_defs) is a good improvement

I'm still not happy with what I originally did here, and was worried about proliferation of requirements for more `// pico_cmake_set` but with the validation script changes, we're at least a little better off. Also proliferating the settings override one way (Cmake->compile_defs) is a good improvement