Skip to content
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

build system: document riotbuild.h and deprecated RIOT_MCU #20566

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

maribu
Copy link
Member

@maribu maribu commented Apr 11, 2024

Contribution description

This adds a place where the macros provided via riotbuild.h can be documented and where old macros can be deprecated.

This place was previously missing, as riotbuild.h is generated by converting -DFOO=BAR paramters from $(CFLAGS) to #define FOO BAR definitions in riotbuild.h. Hence, the is in Makefiles all over the code base and Doxygen really sucks at extracting info from Makefiles.

It also adds the MACRO_DEPRECATED macro, which can be used in the definition of deprecated macros to trigger a warning if (and only if) the defined macro is actually used.

Finally, the macro RIOT_MCU is readded as an alias to RIOT_CPU and marked as deprecated.

Testing procedure

Apply

diff --git a/examples/hello-world/main.c b/examples/hello-world/main.c
index 213128ac64..b3e8165130 100644
--- a/examples/hello-world/main.c
+++ b/examples/hello-world/main.c
@@ -26,7 +26,7 @@ int main(void)
     puts("Hello World!");
 
     printf("You are running RIOT on a(n) %s board.\n", RIOT_BOARD);
-    printf("This board features a(n) %s CPU.\n", RIOT_CPU);
+    printf("This board features a(n) %s CPU.\n", RIOT_MCU);
 
     return 0;
 }

and compile the hello world example with WERROR=0. It should build fine, but issue a warning about a deprecated macro:

$ make BOARD=nucleo-f767zi -C examples/hello-world WERROR=0
make: Entering directory '/home/maribu/Repos/software/RIOT/master/examples/hello-world'
Building application "hello-world" for "nucleo-f767zi" with CPU "stm32".

"make" -C /home/maribu/Repos/software/RIOT/master/pkg/cmsis/ 
/home/maribu/Repos/software/RIOT/master/examples/hello-world/main.c: In function 'main':
/home/maribu/Repos/software/RIOT/master/examples/hello-world/main.c:29:13: warning: Code is using a deprecated macro
   29 |     printf("This board features a(n) %s CPU.\n", RIOT_MCU);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
"make" -C /home/maribu/Repos/software/RIOT/master/boards/common/init
"make" -C /home/maribu/Repos/software/RIOT/master/boards/nucleo-f767zi
"make" -C /home/maribu/Repos/software/RIOT/master/boards/common/nucleo
"make" -C /home/maribu/Repos/software/RIOT/master/core
"make" -C /home/maribu/Repos/software/RIOT/master/core/lib
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/stm32
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/cortexm_common
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/cortexm_common/periph
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/stm32/periph
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/stm32/stmclk
"make" -C /home/maribu/Repos/software/RIOT/master/cpu/stm32/vectors
"make" -C /home/maribu/Repos/software/RIOT/master/drivers
"make" -C /home/maribu/Repos/software/RIOT/master/drivers/periph_common
"make" -C /home/maribu/Repos/software/RIOT/master/sys
"make" -C /home/maribu/Repos/software/RIOT/master/sys/auto_init
"make" -C /home/maribu/Repos/software/RIOT/master/sys/div
"make" -C /home/maribu/Repos/software/RIOT/master/sys/libc
"make" -C /home/maribu/Repos/software/RIOT/master/sys/malloc_thread_safe
"make" -C /home/maribu/Repos/software/RIOT/master/sys/newlib_syscalls_default
"make" -C /home/maribu/Repos/software/RIOT/master/sys/pm_layered
"make" -C /home/maribu/Repos/software/RIOT/master/sys/preprocessor
"make" -C /home/maribu/Repos/software/RIOT/master/sys/stdio
"make" -C /home/maribu/Repos/software/RIOT/master/sys/stdio_uart
   text	  data	   bss	   dec	   hex	filename
   9440	   108	  2632	 12180	  2f94	/home/maribu/Repos/software/RIOT/master/examples/hello-world/bin/nucleo-f767zi/hello-world.elf
make: Leaving directory '/home/maribu/Repos/software/RIOT/master/examples/hello-world'

Issues/PRs references

#20397

@maribu maribu requested review from miri64 and Teufelchen1 April 11, 2024 12:40
@maribu maribu requested review from aabadie and jia200x as code owners April 11, 2024 12:40
@github-actions github-actions bot added Area: doc Area: Documentation Area: tools Area: Supplementary tools labels Apr 11, 2024
@chrysn chrysn added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Apr 11, 2024
@riot-ci
Copy link

riot-ci commented Apr 11, 2024

Murdock results

✔️ PASSED

29e963b build system: deprecate RIOT_MCU predefined macro

Success Failures Total Runtime
1 0 1 01m:33s

Artifacts

@chrysn
Copy link
Member

chrysn commented Apr 11, 2024

Marked as ready-to-build because I'd like to see the generated config output. If this doesn't go through Doxygen, then the Doxygen commands in the header are just going-through-the-motions.

Comment on lines 64 to 99

/**
* @brief Mark a preprocessor macro as deprecated by including this
* macro in the definition
*
* Usage:
* ``` C
* /// @deprecated, use @ref BAR instead of FOO
* #define FOO MACRO_DEPRECATED BAR
* ```
*/
#define MACRO_DEPRECATED /* implementation */

/**
* @brief Name of the MCU the app is compiled for as string literal
*
* @deprecated Use @ref RIOT_CPU instead. This will be removed soonest in
* release 2025.04
*
* This has been renamed to `RIOT_CPU` for consistency. Even though MCU would
* technically be the better name, CPU is used every else in the source code
* and folder structure.
*/
#define RIOT_MCU RIOT_CPU

#else /* DOXYGEN */

#if defined(__GNUC__) || defined(__clang__)
# define MACRO_DEPRECATED _Pragma("GCC warning \"Code is using a deprecated macro\"")
#else
# define MACRO_DEPRECATED
#endif

#define RIOT_MCU MACRO_DEPRECATED RIOT_CPU

#endif /* DOXYGEN */
Copy link
Member

Choose a reason for hiding this comment

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

While the top entries don't have corresponding actual implementations, I suggest grouping those that do with the implemenmtations. (Didn't check what the MACRO_DEPRECATED looks like in Doxygen, whether GNUC / clang is set there, but either way I think would be fine)

Suggested change
/**
* @brief Mark a preprocessor macro as deprecated by including this
* macro in the definition
*
* Usage:
* ``` C
* /// @deprecated, use @ref BAR instead of FOO
* #define FOO MACRO_DEPRECATED BAR
* ```
*/
#define MACRO_DEPRECATED /* implementation */
/**
* @brief Name of the MCU the app is compiled for as string literal
*
* @deprecated Use @ref RIOT_CPU instead. This will be removed soonest in
* release 2025.04
*
* This has been renamed to `RIOT_CPU` for consistency. Even though MCU would
* technically be the better name, CPU is used every else in the source code
* and folder structure.
*/
#define RIOT_MCU RIOT_CPU
#else /* DOXYGEN */
#if defined(__GNUC__) || defined(__clang__)
# define MACRO_DEPRECATED _Pragma("GCC warning \"Code is using a deprecated macro\"")
#else
# define MACRO_DEPRECATED
#endif
#define RIOT_MCU MACRO_DEPRECATED RIOT_CPU
#endif /* DOXYGEN */
#endif /* DOXYGEN */
/**
* @brief Mark a preprocessor macro as deprecated by including this
* macro in the definition
*
* Usage:
* ``` C
* /// @deprecated, use @ref BAR instead of FOO
* #define FOO MACRO_DEPRECATED BAR
* ```
*/
#if defined(__GNUC__) || defined(__clang__)
# define MACRO_DEPRECATED _Pragma("GCC warning \"Code is using a deprecated macro\"")
#else
# define MACRO_DEPRECATED
#endif
/**
* @brief Name of the MCU the app is compiled for as string literal
*
* @deprecated Use @ref RIOT_CPU instead. This will be removed soonest in
* release 2025.04
*
* This has been renamed to `RIOT_CPU` for consistency. Even though MCU would
* technically be the better name, CPU is used every else in the source code
* and folder structure.
*/
#define RIOT_MCU MACRO_DEPRECATED RIOT_CPU

Copy link
Member Author

Choose a reason for hiding this comment

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

At least for the MACRO_DEPRECATED I'd rather not have the implementation shown in the Doxygen output, but rather an opaque implementation detail.

Doxygen doesn't understand the semantics of the #if defined(__GNUC__) || defined(__clang__) and would render a misleading simplification by picking one of the two definitions. If it would render some /* implementation */ instead, it would at least be obvious that anyone interested in the implementation details would have to look at the code instead.

I did move the doc now closer to the implementation, though.

@maribu maribu force-pushed the buildsystem/document-riotbuild.h branch from 89790a3 to ae847ec Compare April 11, 2024 15:11
@Teufelchen1
Copy link
Contributor

LGTM

@mguetschow mguetschow added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 3, 2024
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

LGTM, nice addition!

I've looked at the generated docs and found that doxygen apparently automatically cuts after the first sentence. Changing e.g. to e.g., may or may not avoid this.

dist/tools/genconfigheader/riotbuild-prefix.h.in Outdated Show resolved Hide resolved
dist/tools/genconfigheader/riotbuild-prefix.h.in Outdated Show resolved Hide resolved
@Teufelchen1
Copy link
Contributor

@maribu little reminder ;)

This adds a `riotbuild-prefix.h` that is added to the `riotbuild.h`
and processed by Doxygen. It solves two problems:

1. The pre-defined macros where previously fully undocumented, but
   may be useful to real world applications
2. It provides a place where backward compatibility aliases can be
   added with a deprecation notice
Adding this macro in the definition of a macro causes a warning about
the deprecation to be emitted when used (and a build failure with
`WERROR=1`). This is useful as no other means to deprecate preprocessor
macros are provided.

The macro will be defined empty for compilers that are not GCC or
clang.
This re-adds `RIOT_MCU` as alias for `RIOT_CPU` and marks it as
deprecated. That should make life easier for downstream apps that may
still use `RIOT_CPU`.
@maribu maribu force-pushed the buildsystem/document-riotbuild.h branch from f1f1681 to 29e963b Compare September 28, 2024 16:33
@maribu
Copy link
Member Author

maribu commented Sep 29, 2024

@mguetschow Adding the , after e.g. did the trick and the full sentence is now shown.

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

Thanks, looks good indeed.

@mguetschow mguetschow enabled auto-merge October 1, 2024 12:57
@mguetschow mguetschow added this pull request to the merge queue Oct 1, 2024
Merged via the queue into RIOT-OS:master with commit 723d8cd Oct 1, 2024
26 checks passed
@maribu maribu deleted the buildsystem/document-riotbuild.h branch October 10, 2024 08:25
@benpicco benpicco added this to the Release 2024.10 milestone Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants