Add some optional header includes, to make per-target changes to config easier #851

2022-06-05T17:50:07-05:00

kilograham commented

2022-06-05 17:50:07 -05:00

(Migrated from github.com)

The main motivation here is to allow inclusion of FreeRTOS on a per target basis, rather than as a project wide thing.
It also allows FreeRTOS to be added at any point in the CMakeLists.txts (rather than just before pico_sdk_init) which is much more natural if only a sub-part of your project uses it

The main motivation here is to allow inclusion of FreeRTOS on a per target basis, rather than as a project wide thing. It also allows FreeRTOS to be added at any point in the `CMakeLists.txt`s (rather than just before `pico_sdk_init`) which is much more natural if only a sub-part of your project uses it

lurch (Migrated from github.com) reviewed

2022-06-06 08:18:22 -05:00

src/common/pico_base/include/pico/config.h

				`@ -18,4 +18,9 @@`

Is this definitely specific to RTOS integration, or could it have other uses too?

lurch (Migrated from github.com) reviewed

2022-06-06 08:21:54 -05:00

src/common/pico_base/include/pico/config.h

					
				@ -20,1 +20,4 @@

				// PICO_CONFIG: PICO_CONFIG_RTOS_ADAPTER_HEADER, unquoted path to header include in the default pico/config.h for RTOS integration defines that must be included in all sources, default=none, group=pico_base

				#ifdef PICO_CONFIG_RTOS_ADAPTER_HEADER

				#include __PICO_XSTRING(PICO_CONFIG_RTOS_ADAPTER_HEADER)

Does the use of __PICO_XSTRING here mean that pico/config.h will only ever be included directly by pico.h ?

Does the use of `__PICO_XSTRING` here mean that `pico/config.h` will _only_ ever be included directly by `pico.h` ?

kilograham (Migrated from github.com) reviewed

2022-06-06 08:27:35 -05:00

src/common/pico_base/include/pico/config.h

				`@ -18,4 +18,9 @@`

It was intended to be specific (you could use it for something else, but then you have no idea if you're colliding with something else). There is no easy way to make a generic include interface (except for the CMake auto-gen which we already have but are deliberately not using), so I just picked this one out since it is the one I know that is needed now

kilograham (Migrated from github.com) reviewed

2022-06-06 08:28:56 -05:00

src/common/pico_base/include/pico/config.h

					
				@ -20,1 +20,4 @@

				// PICO_CONFIG: PICO_CONFIG_RTOS_ADAPTER_HEADER, unquoted path to header include in the default pico/config.h for RTOS integration defines that must be included in all sources, default=none, group=pico_base

				#ifdef PICO_CONFIG_RTOS_ADAPTER_HEADER

				#include __PICO_XSTRING(PICO_CONFIG_RTOS_ADAPTER_HEADER)

Good point, but I think it is fair. The user (and all our headers) are supposed to include pico.h as this includes all the other stuff in the right order.

Good point, but I think it is fair. The user (and all our headers) are supposed to include `pico.h` as this includes all the other stuff in the right order.

lurch (Migrated from github.com) reviewed

2022-06-06 09:17:34 -05:00

src/common/pico_base/include/pico.h

					
				@ -16,9 +16,18 @@

				 * This header may be included by assembly code

When I run the tools/extract_configs.py script against this PR, it complains that string isn't a valid config type - it currently only expects int, bool or enum.
I could add string as an additional config type, but https://github.com/raspberrypi/pico-project-generator/blob/master/pico_project.py#L547 (which is what makes use of the TSV files that extract_configs creates) doesn't support string yet either.

Let me know if you want me to add string config items to extract_configs (would we also want minimum or maximum string-length validation?), and I'll add a corresponding issue to pico-project-generator.

Hmmm, although looking at the CONFIG descriptions, I wonder if filename would be a more suitable type than just string? 🤔

When I run the `tools/extract_configs.py` script against this PR, it complains that `string` isn't a valid config type - it currently only expects `int`, `bool` or `enum`. I _could_ add `string` as an additional config type, but https://github.com/raspberrypi/pico-project-generator/blob/master/pico_project.py#L547 (which is what makes use of the TSV files that `extract_configs` creates) doesn't support `string` yet either. Let me know if you want me to add `string` config items to `extract_configs` (would we also want minimum or maximum string-length validation?), and I'll add a corresponding issue to `pico-project-generator`. Hmmm, although looking at the CONFIG descriptions, I wonder if `filename` would be a more suitable type than just `string`? :thinking:

Wren6991 (Migrated from github.com) reviewed

2022-06-07 05:28:04 -05:00

kilograham (Migrated from github.com) reviewed

2022-06-07 14:27:18 -05:00

src/common/pico_base/include/pico.h

					
				@ -16,9 +16,18 @@

				 * This header may be included by assembly code

i will just remove the type for now - feel free to add it back later

Wren6991 (Migrated from github.com) approved these changes

2022-06-07 20:08:21 -05:00

liamfraser (Migrated from github.com) approved these changes

2022-06-08 07:31:48 -05:00

Sign in to join this conversation.

No reviewers

Wren6991

liamfraser

No labels

compiler version issues

needs reporter detail

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-raspberrypi/pico-sdk!851

				`@ -16,9 +16,18 @@`
				`* This header may be included by assembly code`

Rows
Columns