From 1083a730ac550eb4c50a703a2d86079c5cd6fa2a Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 28 Jun 2019 13:14:10 +0200 Subject: [PATCH 1/4] Include fwk_assert.h instead of assert.h Change-Id: Ib2d3e2317136e5895667f7e1e38fd1b391ec3496 Signed-off-by: Ronald Cron --- product/juno/scp_romfw/config_sds.c | 1 - product/juno/scp_romfw_bypass/config_sds.c | 1 - product/n1sdp/module/n1sdp_flash/src/mod_n1sdp_flash.c | 2 +- product/n1sdp/module/n1sdp_system/src/mod_n1sdp_system.c | 2 +- product/n1sdp/scp_ramfw/config_ppu_v1.c | 2 +- product/n1sdp/src/n1sdp_core.c | 2 +- 6 files changed, 4 insertions(+), 6 deletions(-) diff --git a/product/juno/scp_romfw/config_sds.c b/product/juno/scp_romfw/config_sds.c index 9163d288d..166c63b50 100644 --- a/product/juno/scp_romfw/config_sds.c +++ b/product/juno/scp_romfw/config_sds.c @@ -5,7 +5,6 @@ * SPDX-License-Identifier: BSD-3-Clause */ -#include #include #include #include diff --git a/product/juno/scp_romfw_bypass/config_sds.c b/product/juno/scp_romfw_bypass/config_sds.c index 879397f56..64a7e141b 100644 --- a/product/juno/scp_romfw_bypass/config_sds.c +++ b/product/juno/scp_romfw_bypass/config_sds.c @@ -5,7 +5,6 @@ * SPDX-License-Identifier: BSD-3-Clause */ -#include #include #include #include diff --git a/product/n1sdp/module/n1sdp_flash/src/mod_n1sdp_flash.c b/product/n1sdp/module/n1sdp_flash/src/mod_n1sdp_flash.c index 05a8f1d03..e34e88a24 100644 --- a/product/n1sdp/module/n1sdp_flash/src/mod_n1sdp_flash.c +++ b/product/n1sdp/module/n1sdp_flash/src/mod_n1sdp_flash.c @@ -10,7 +10,7 @@ */ #include -#include +#include #include #include #include diff --git a/product/n1sdp/module/n1sdp_system/src/mod_n1sdp_system.c b/product/n1sdp/module/n1sdp_system/src/mod_n1sdp_system.c index 043d60aae..e3056f307 100644 --- a/product/n1sdp/module/n1sdp_system/src/mod_n1sdp_system.c +++ b/product/n1sdp/module/n1sdp_system/src/mod_n1sdp_system.c @@ -8,10 +8,10 @@ * N1SDP System Support. */ -#include #include #include #include +#include #include #include #include diff --git a/product/n1sdp/scp_ramfw/config_ppu_v1.c b/product/n1sdp/scp_ramfw/config_ppu_v1.c index 738218d11..a89ab4ace 100644 --- a/product/n1sdp/scp_ramfw/config_ppu_v1.c +++ b/product/n1sdp/scp_ramfw/config_ppu_v1.c @@ -5,10 +5,10 @@ * SPDX-License-Identifier: BSD-3-Clause */ -#include #include #include #include +#include #include #include #include diff --git a/product/n1sdp/src/n1sdp_core.c b/product/n1sdp/src/n1sdp_core.c index d48a6e307..f3181d7ec 100644 --- a/product/n1sdp/src/n1sdp_core.c +++ b/product/n1sdp/src/n1sdp_core.c @@ -5,7 +5,7 @@ * SPDX-License-Identifier: BSD-3-Clause */ -#include +#include #include unsigned int n1sdp_core_get_core_per_cluster_count(unsigned int cluster) -- GitLab From e6fedd0e767494bb5ddf0576e1269cac110f4a7c Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Tue, 25 Jun 2019 17:45:11 +0200 Subject: [PATCH 2/4] Clean-up stdlib.h inclusion Include stdlib.h only to get the prototype of bsearch() Change-Id: Ifba75477426cc621eade3d2bf2440f07e8a46d07 Signed-off-by: Ronald Cron --- arch/src/arm_nvic.c | 1 - arch/src/host_interrupt.c | 3 ++- framework/include/fwk_module.h | 1 - framework/src/fwk_assert.c | 1 - framework/src/fwk_interrupt.c | 1 - framework/src/fwk_mm.c | 2 -- framework/test/fwk_test.c | 2 +- framework/test/test_fwk_module.c | 1 + framework/test/test_fwk_multi_thread_common_thread.c | 1 + framework/test/test_fwk_multi_thread_create.c | 1 + framework/test/test_fwk_multi_thread_init.c | 1 + framework/test/test_fwk_multi_thread_put_event.c | 1 + framework/test/test_fwk_multi_thread_util.c | 1 + module/apcontext/include/mod_apcontext.h | 2 +- module/system_pll/src/mod_system_pll.c | 1 - product/juno/module/juno_soc_clock/src/mod_juno_soc_clock.c | 1 - product/synquacer/module/hsspi/include/internal/hsspi_api.h | 1 - product/synquacer/module/hsspi/include/internal/hsspi_driver.h | 1 - product/synquacer/module/hsspi/src/hsspi_api.c | 1 - product/synquacer/module/synquacer_memc/src/synquacer_ddr.c | 1 - product/synquacer/module/synquacer_rom/src/synquacer_init.c | 1 - product/synquacer/module/synquacer_system/src/sysoc.c | 1 - 22 files changed, 10 insertions(+), 17 deletions(-) diff --git a/arch/src/arm_nvic.c b/arch/src/arm_nvic.c index c087ea758..51063f157 100644 --- a/arch/src/arm_nvic.c +++ b/arch/src/arm_nvic.c @@ -10,7 +10,6 @@ #include #include -#include #include #include #include diff --git a/arch/src/host_interrupt.c b/arch/src/host_interrupt.c index c53ce0c32..6ce2121c7 100644 --- a/arch/src/host_interrupt.c +++ b/arch/src/host_interrupt.c @@ -8,7 +8,8 @@ * Interrupt management. */ -#include +#include +#include #include #include diff --git a/framework/include/fwk_module.h b/framework/include/fwk_module.h index fc66b258d..f1c533e78 100644 --- a/framework/include/fwk_module.h +++ b/framework/include/fwk_module.h @@ -13,7 +13,6 @@ #include #include -#include #include #include #include diff --git a/framework/src/fwk_assert.c b/framework/src/fwk_assert.c index e4834c518..0f9d92833 100644 --- a/framework/src/fwk_assert.c +++ b/framework/src/fwk_assert.c @@ -5,7 +5,6 @@ * SPDX-License-Identifier: BSD-3-Clause */ -#include #include #include diff --git a/framework/src/fwk_interrupt.c b/framework/src/fwk_interrupt.c index 7f3ecb029..d911ae6b6 100644 --- a/framework/src/fwk_interrupt.c +++ b/framework/src/fwk_interrupt.c @@ -10,7 +10,6 @@ #include #include -#include #include #include #include diff --git a/framework/src/fwk_mm.c b/framework/src/fwk_mm.c index cc4bb8bae..7bc0afcb0 100644 --- a/framework/src/fwk_mm.c +++ b/framework/src/fwk_mm.c @@ -12,8 +12,6 @@ #include #include #include -#include -#include #include #include #include diff --git a/framework/test/fwk_test.c b/framework/test/fwk_test.c index f21d93a04..956c5ce71 100644 --- a/framework/test/fwk_test.c +++ b/framework/test/fwk_test.c @@ -6,8 +6,8 @@ */ #include #include -#include #include +#include #include #include #include diff --git a/framework/test/test_fwk_module.c b/framework/test/test_fwk_module.c index 8322ba516..f7126bb38 100644 --- a/framework/test/test_fwk_module.c +++ b/framework/test/test_fwk_module.c @@ -4,6 +4,7 @@ * * SPDX-License-Identifier: BSD-3-Clause */ +#include #include #include #include diff --git a/framework/test/test_fwk_multi_thread_common_thread.c b/framework/test/test_fwk_multi_thread_common_thread.c index 486d1fb20..4cae23e60 100644 --- a/framework/test/test_fwk_multi_thread_common_thread.c +++ b/framework/test/test_fwk_multi_thread_common_thread.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include diff --git a/framework/test/test_fwk_multi_thread_create.c b/framework/test/test_fwk_multi_thread_create.c index 6e31aa06b..0d868467b 100644 --- a/framework/test/test_fwk_multi_thread_create.c +++ b/framework/test/test_fwk_multi_thread_create.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include diff --git a/framework/test/test_fwk_multi_thread_init.c b/framework/test/test_fwk_multi_thread_init.c index d0e24cd97..a7cca306c 100644 --- a/framework/test/test_fwk_multi_thread_init.c +++ b/framework/test/test_fwk_multi_thread_init.c @@ -7,6 +7,7 @@ #include #include +#include #include #include #include diff --git a/framework/test/test_fwk_multi_thread_put_event.c b/framework/test/test_fwk_multi_thread_put_event.c index 078d5c108..3dd4a94a4 100644 --- a/framework/test/test_fwk_multi_thread_put_event.c +++ b/framework/test/test_fwk_multi_thread_put_event.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include diff --git a/framework/test/test_fwk_multi_thread_util.c b/framework/test/test_fwk_multi_thread_util.c index c5d3fb5cd..2944ed5a8 100644 --- a/framework/test/test_fwk_multi_thread_util.c +++ b/framework/test/test_fwk_multi_thread_util.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include diff --git a/module/apcontext/include/mod_apcontext.h b/module/apcontext/include/mod_apcontext.h index 1a21a99ae..66e9eeb63 100644 --- a/module/apcontext/include/mod_apcontext.h +++ b/module/apcontext/include/mod_apcontext.h @@ -10,7 +10,7 @@ #define MOD_APCONTEXT_H #include -#include +#include /*! * \ingroup GroupModules Modules diff --git a/module/system_pll/src/mod_system_pll.c b/module/system_pll/src/mod_system_pll.c index 2735c7bc9..76aa9f7e4 100644 --- a/module/system_pll/src/mod_system_pll.c +++ b/module/system_pll/src/mod_system_pll.c @@ -6,7 +6,6 @@ */ #include -#include #include #include #include diff --git a/product/juno/module/juno_soc_clock/src/mod_juno_soc_clock.c b/product/juno/module/juno_soc_clock/src/mod_juno_soc_clock.c index 6cb1fa3bb..ade43c41f 100644 --- a/product/juno/module/juno_soc_clock/src/mod_juno_soc_clock.c +++ b/product/juno/module/juno_soc_clock/src/mod_juno_soc_clock.c @@ -9,7 +9,6 @@ */ #include -#include #include #include #include diff --git a/product/synquacer/module/hsspi/include/internal/hsspi_api.h b/product/synquacer/module/hsspi/include/internal/hsspi_api.h index e5727e831..de7048d67 100644 --- a/product/synquacer/module/hsspi/include/internal/hsspi_api.h +++ b/product/synquacer/module/hsspi/include/internal/hsspi_api.h @@ -10,7 +10,6 @@ #include #include -#include #include struct HSSPI_clk_config { diff --git a/product/synquacer/module/hsspi/include/internal/hsspi_driver.h b/product/synquacer/module/hsspi/include/internal/hsspi_driver.h index daafc32b2..540aa4eb7 100644 --- a/product/synquacer/module/hsspi/include/internal/hsspi_driver.h +++ b/product/synquacer/module/hsspi/include/internal/hsspi_driver.h @@ -9,7 +9,6 @@ #define HSSPI_DRIVER_H #include -#include #include #include diff --git a/product/synquacer/module/hsspi/src/hsspi_api.c b/product/synquacer/module/hsspi/src/hsspi_api.c index 7ef2a9701..e7754a624 100644 --- a/product/synquacer/module/hsspi/src/hsspi_api.c +++ b/product/synquacer/module/hsspi/src/hsspi_api.c @@ -7,7 +7,6 @@ #include #include -#include #include #include #include diff --git a/product/synquacer/module/synquacer_memc/src/synquacer_ddr.c b/product/synquacer/module/synquacer_memc/src/synquacer_ddr.c index 9cebba0f0..4a1137d1c 100644 --- a/product/synquacer/module/synquacer_memc/src/synquacer_ddr.c +++ b/product/synquacer/module/synquacer_memc/src/synquacer_ddr.c @@ -7,7 +7,6 @@ #include #include -#include #include #include #include diff --git a/product/synquacer/module/synquacer_rom/src/synquacer_init.c b/product/synquacer/module/synquacer_rom/src/synquacer_init.c index 3130b3e57..bc61bfe73 100644 --- a/product/synquacer/module/synquacer_rom/src/synquacer_init.c +++ b/product/synquacer/module/synquacer_rom/src/synquacer_init.c @@ -6,7 +6,6 @@ */ #include -#include #include #include #include diff --git a/product/synquacer/module/synquacer_system/src/sysoc.c b/product/synquacer/module/synquacer_system/src/sysoc.c index 2eb758fd5..5e056222d 100644 --- a/product/synquacer/module/synquacer_system/src/sysoc.c +++ b/product/synquacer/module/synquacer_system/src/sysoc.c @@ -7,7 +7,6 @@ #include #include -#include #include -- GitLab From d438423d550a87a7a9abb623a6eb9b92c49249d5 Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Mon, 24 Jun 2019 14:15:41 +0200 Subject: [PATCH 3/4] Split coding style documentation Move into the code_rules.md new file the coding rules that may have an impact on the compiled code. Keep in code_style.md only the coding rules and guidelines that don't have any impact on the compiled code. Change-Id: Ie43a68966457f1ff821ba4fd95ef01f1001cf9cf Signed-off-by: Ronald Cron --- contributing.md | 3 +- doc/code_rules.md | 172 +++++++++++++++++++++++++++++++++++++++++++++ doc/code_style.md | 174 +--------------------------------------------- doc/config.dxy | 1 + 4 files changed, 177 insertions(+), 173 deletions(-) create mode 100644 doc/code_rules.md diff --git a/contributing.md b/contributing.md index bb979c90a..9879bf34a 100644 --- a/contributing.md +++ b/contributing.md @@ -17,7 +17,8 @@ Making Changes - Make commits of logical units. See these general [Git guidelines](http://git-scm.com/book/ch5-2.html) for contributing to a project. -- Follow the [SCP coding style](./doc/code_style.md). +- Follow the project [coding style](./doc/code_style.md) and + [coding rules](./doc/code_rules.md). - Keep the commits on topic. If you need to fix another bug or make another enhancement, please address it on a separate topic branch. - Avoid long commit series. If you do have a long series, consider whether diff --git a/doc/code_rules.md b/doc/code_rules.md new file mode 100644 index 000000000..8dd9dfaa1 --- /dev/null +++ b/doc/code_rules.md @@ -0,0 +1,172 @@ +Coding Rules +============ + +To maintain consistency within the SCP/MCP software source code a series of +rules and guidelines have been created; these form the project's coding rules. + +Header Files +------------ +The contents of a header file should be wrapped in an 'include guard' to prevent +accidental multiple inclusion of a single header. The definition name should be +the upper-case file name followed by "_H". An example for fwk_mm.h follows: + +\code +#ifndef FWK_MM_H +#define FWK_MM_H + +(...) + +#endif /* FWK_MM_H */ +\endcode + +The closing endif statement should be followed directly by a single-line comment +which replicates the full guard name. In long files this helps to clarify what +is being closed. + +Space between definition inside the header file should be a single line only. + +If a unit (header or C file) requires a header, it must include that header +instead of relying on an indirect inclusion from one of the headers it already +includes. + +Types +----- +Import "stdint.h" (part of the C Standard Library) for exact-width integer types +(uint8_t, uint16_t, etc). These types can be used wherever the width of an +integer needs to be specified explicitly. + +Import "stdbool.h" (also part of the C Standard Library) whenever a "boolean" +type is needed. + +Avoid defining custom types with the "typedef" keyword where possible. +Structures (struct) and enumerators (enum) should be declared and used with +their respective keyword identifiers. If custom types are used then they must +have the suffix "_t" appended to their type name where it is defined. This makes +it easier to recognize types that have been defined using "typedef" when they +appear in the code. + +When using sizeof() pass the variable name as the parameter to be evaluated, and +not its type. This prevents issues arising if the type of the variable changes +but the sizeof() parameter is not updated. + +\code +size_t size; +unsigned int counter; + +/* Preferred over sizeof(int) */ +size = sizeof(counter); +\endcode + +Operator Precedence +------------------- +Do not rely on the implicit precedence and associativity of C operators. Use +parenthesis to make precedence and associativity explicit: + +\code +if ((a == 'a') || (x == 'x')) + do_something(); +\endcode + +Parenthesis around a unary operator and its operand may be omitted: + +\code +if (!a || &a) + do_something(); +\endcode + +Macros and Constants +-------------------- +Logical groupings of constants should be defined as enumerations, with a common +prefix, so that they can be used as parameter types. To find out the number of +items in an "enum", make the last entry to be \_COUNT. + +\code +enum command_id { + COMMAND_ID_VERSION, + COMMAND_ID_PING, + COMMAND_ID_EXIT, + /* Do not add entries after this line */ + COMMAND_ID_COUNT +}; + +void process_cmd(enum command_id id) +{ + (...) +} +\endcode + +Prefer inline functions instead of macros. + +Initialization +-------------- +When local variables require being initialized to 0, please use their respective +type related initializer value: +- 0 (zero) for integers +- 0.0 for float/double +- \0 for chars +- NULL for pointers +- false for booleans (stdbool.h) + +Array and structure initialization should use designated initializers. These +allow elements to be initialized using array indexes or structure field names +and without a fixed ordering. + +Array initialization example: +\code +uint32_t clock_frequencies[3] = { + [2] = 800, + [0] = 675 +}; +\endcode + +Structure initialization example: +\code +struct clock clock_cpu = { + .name = "CPU", + .frequency = 800, +}; +\endcode + +Device Register Definitions +--------------------------- +The format for structures representing memory-mapped device registers is +standardized. + +- The file containing the device structure must include stdint.h to gain access +to the uintxx_t and UINTxx_C definitions. +- The file containing the device structure must include fwk_macros.h to gain +access to the FWK_R, FWK_W and FWK_RW macros. +- All non-reserved structure fields must be prefixed with one of the above +macros, defining the read/write access level. +- Bit definitions should be declared via UINTxx_C macros. +- Bit definitions must be prefixed by the register name it relates to. If bit +definitions apply to multiple registers, then the name must be as common as +possible and a comment must explicit show which registers it applies to. +- The structure name for the programmer's view must follow the pattern "struct +_reg { ...registers... };" + +\code +#include +#include + +struct devicename_reg { + /* Readable and writable register */ + FWK_RW uint32_t CONFIG; + uint32_t RESERVED1; + + /* Write-only register */ + FWK_W uint32_t IRQ_CLEAR; + + /* Read-only register */ + FWK_R uint32_t IRQ_STATUS; + uint32_t RESERVED2[0x40]; +}; + +/* Register bit definitions */ +#define CONFIG_ENABLE UINT32_C(0x00000001) +#define CONFIG_SLEEP UINT32_C(0x00000002) + +#define IRQ_STATUS_ALERT UINT32_C(0x00000001) +\endcode + +__Note:__ A template file can be found in doc/template/device.h diff --git a/doc/code_style.md b/doc/code_style.md index eebc31453..946254fe8 100644 --- a/doc/code_style.md +++ b/doc/code_style.md @@ -1,8 +1,8 @@ Coding Style ============ -To maintain consistency within the SCP/MCP software source code a series of -rules and guidelines have been created; these form the project's coding style. +A coding style has been defined to give a consistent look to the source code and +thus help code readability and review. Encoding -------- @@ -27,31 +27,6 @@ macros and defines) to identify framework API. It is fine and encouraged to use a variable named "i" (index) for loops. -Header Files ------------- -The contents of a header file should be wrapped in an 'include guard' to prevent -accidental multiple inclusion of a single header. The definition name should be -the upper-case file name followed by "_H". An example for fwk_mm.h follows: - -\code -#ifndef FWK_MM_H -#define FWK_MM_H - -(...) - -#endif /* FWK_MM_H */ -\endcode - -The closing endif statement should be followed directly by a single-line comment -which replicates the full guard name. In long files this helps to clarify what -is being closed. - -Space between definition inside the header file should be a single line only. - -If a unit (header or C file) requires a header, it must include that header -instead of relying on an indirect inclusion from one of the headers it already -includes. - Inclusions ---------- Header file inclusions should follow a consistent sequence, defined as: @@ -212,51 +187,6 @@ int foo(void) __Note__ Such constructions like the example above should be avoided if possible. -Types ------ -Import "stdint.h" (part of the C Standard Library) for exact-width integer types -(uint8_t, uint16_t, etc). These types can be used wherever the width of an -integer needs to be specified explicitly. - -Import "stdbool.h" (also part of the C Standard Library) whenever a "boolean" -type is needed. - -Avoid defining custom types with the "typedef" keyword where possible. -Structures (struct) and enumerators (enum) should be declared and used with -their respective keyword identifiers. If custom types are used then they must -have the suffix "_t" appended to their type name where it is defined. This makes -it easier to recognize types that have been defined using "typedef" when they -appear in the code. - -When using sizeof() pass the variable name as the parameter to be evaluated, and -not its type. This prevents issues arising if the type of the variable changes -but the sizeof() parameter is not updated. - -\code -size_t size; -unsigned int counter; - -/* Preferred over sizeof(int) */ -size = sizeof(counter); -\endcode - -Operator Precedence -------------------- -Do not rely on the implicit precedence and associativity of C operators. Use -parenthesis to make precedence and associativity explicit: - -\code -if ((a == 'a') || (x == 'x')) - do_something(); -\endcode - -Parenthesis around a unary operator and its operand may be omitted: - -\code -if (!a || &a) - do_something(); -\endcode - Comments -------- To ensure a consistent look, the preferred style for single-line comments is to @@ -294,106 +224,6 @@ void function_b(int x, int y) \endcode -Macros and Constants --------------------- -All names of macros and constants must be written in upper-case to differentiate -them from functions and variables. - -Logical groupings of constants should be defined as enumerations, with a common -prefix, so that they can be used as parameter types. To find out the number of -items in an "enum", make the last entry to be \_COUNT. - -\code -enum command_id { - COMMAND_ID_VERSION, - COMMAND_ID_PING, - COMMAND_ID_EXIT, - /* Do not add entries after this line */ - COMMAND_ID_COUNT -}; - -void process_cmd(enum command_id id) -{ - (...) -} -\endcode - -Prefer inline functions instead of macros. - -Initialization --------------- -When local variables require being initialized to 0, please use their respective -type related initializer value: -- 0 (zero) for integers -- 0.0 for float/double -- \0 for chars -- NULL for pointers -- false for booleans (stdbool.h) - -Array and structure initialization should use designated initializers. These -allow elements to be initialized using array indexes or structure field names -and without a fixed ordering. - -Array initialization example: -\code -uint32_t clock_frequencies[3] = { - [2] = 800, - [0] = 675 -}; -\endcode - -Structure initialization example: -\code -struct clock clock_cpu = { - .name = "CPU", - .frequency = 800, -}; -\endcode - -Device Register Definitions ---------------------------- -The format for structures representing memory-mapped device registers is -standardized. - -- The file containing the device structure must include stdint.h to gain access -to the uintxx_t and UINTxx_C definitions. -- The file containing the device structure must include fwk_macros.h to gain -access to the FWK_R, FWK_W and FWK_RW macros. -- All non-reserved structure fields must be prefixed with one of the above -macros, defining the read/write access level. -- Bit definitions should be declared via UINTxx_C macros. -- Bit definitions must be prefixed by the register name it relates to. If bit -definitions apply to multiple registers, then the name must be as common as -possible and a comment must explicit show which registers it applies to. -- The structure name for the programmer's view must follow the pattern "struct -_reg { ...registers... };" - -\code -#include -#include - -struct devicename_reg { - /* Readable and writable register */ - FWK_RW uint32_t CONFIG; - uint32_t RESERVED1; - - /* Write-only register */ - FWK_W uint32_t IRQ_CLEAR; - - /* Read-only register */ - FWK_R uint32_t IRQ_STATUS; - uint32_t RESERVED2[0x40]; -}; - -/* Register bit definitions */ -#define CONFIG_ENABLE UINT32_C(0x00000001) -#define CONFIG_SLEEP UINT32_C(0x00000002) - -#define IRQ_STATUS_ALERT UINT32_C(0x00000001) -\endcode - -__Note:__ A template file can be found in doc/template/device.h - Doxygen Comments ---------------- The project APIs are documented using Doxygen comments. diff --git a/doc/config.dxy b/doc/config.dxy index ae6ebd104..92ac20072 100644 --- a/doc/config.dxy +++ b/doc/config.dxy @@ -117,6 +117,7 @@ INPUT = $(TOP_DIR)/readme.md \ $(TOP_DIR)/change_log.md \ $(BS_DIR)/doc.md \ $(DOC_DIR)/code_style.md \ + $(DOC_DIR)/code_rules.md \ $(DOC_DIR)/framework.md \ $(DOC_DIR)/cmsis.md \ $(FWK_DIR)/include \ -- GitLab From c1356acb42d4e99a3835d16cdbb0c9a1f82a89ac Mon Sep 17 00:00:00 2001 From: Ronald Cron Date: Fri, 28 Jun 2019 13:10:09 +0200 Subject: [PATCH 4/4] Improve coding style and rules Change-Id: I04745f89d1f25f36ab1d2c0df235c1bb839660c3 Signed-off-by: Ronald Cron --- doc/code_rules.md | 102 +++++++++++++++++++++++++++++++++++++++++----- doc/code_style.md | 29 ++++++++++++- 2 files changed, 119 insertions(+), 12 deletions(-) diff --git a/doc/code_rules.md b/doc/code_rules.md index 8dd9dfaa1..09318f8ae 100644 --- a/doc/code_rules.md +++ b/doc/code_rules.md @@ -1,8 +1,52 @@ Coding Rules ============ -To maintain consistency within the SCP/MCP software source code a series of -rules and guidelines have been created; these form the project's coding rules. +To maintain consistency within the SCP/MCP software source code and to reduce +the risk of bugs, a series of rules and guidelines have been created; these form +the project's coding rules. + +General Considerations +---------------------- +The software follows the ISO/IEC 9899:2011 standard (C11). This is enforced +through the use of compilation flags and all warnings being considered as +errors. Compilation flags are also used to avoid constructs usually considered +questionable, and that are easy to avoid. + +For robustness and reliability reasons, dynamic memory allocation is allocate- +only and must be done through the facilities provided by the firmware framework. +The intent is for memory allocations to be done during the pre-runtime phase or +early in the runtime phase based on configuration data or hardware detection. +Allocated memory cannot be freed or reallocated. + +The framework provides an optional multi-threading runtime, contingent on a +CMSIS-RTOS-based RTOS. Interaction with the RTOS happens exclusively through the +framework. + +C Standard Library +------------------ +When developing a module, only the following headers of the C standard library +are allowed to be included: +``, ``, ``, ``, ``, +``, `` and ``. + +Concerning the library functions defined in ``, only `snprintf()` may +be used. + +Concerning the library functions defined in ``, only `bsearch()`, +`qsort()`, `abs()` and `rand()` may be used. + +Concerning the library functions defined in ``, `strcat()` and +`strcpy()` cannot be used. Use `strncat()` and `strncpy()` instead. + +If not already defined by another standard library header, include `` +and not `` to define `size_t`. + +Additionally, the framework wraps the following standard library header files: +``, ``, `` and ``. These header +files must not be directly included in module code. This is because certain +compilers, while themselves C11-compliant, do not provide a full C11 standard +library implementation. In this situation, the framework provides a custom +implementation through these headers. Header Files ------------ @@ -31,11 +75,13 @@ includes. Types ----- -Import "stdint.h" (part of the C Standard Library) for exact-width integer types -(uint8_t, uint16_t, etc). These types can be used wherever the width of an -integer needs to be specified explicitly. +Import `` (part of the C Standard Library) for exact-width integer +types (`uint8_t`, `uint16_t`, etc). These types can be used wherever the width +of an integer needs to be specified explicitly. -Import "stdbool.h" (also part of the C Standard Library) whenever a "boolean" +Use `uintptr_t` to handle addresses as integers. + +Import `` (also part of the C Standard Library) whenever a "boolean" type is needed. Avoid defining custom types with the "typedef" keyword where possible. @@ -57,6 +103,27 @@ unsigned int counter; size = sizeof(counter); \endcode +Use the `const` type qualifier as appropriate to specify values that cannot be +modified. Quick reminder: + +- `const xyz_t object`, `object` is an object of type xyz_t which value cannot +be modified. +- `const xyz_t *ptr` or `xyz_t const *ptr`, `ptr` is a pointer to a constant +object of type xyz_t. The value of the object cannot be modified, the value of +the pointer can be modified. +- `xyz_t * const ptr`, `ptr` is a constant pointer to an object of type xyz_t. +The value of the object can be modified, the value of the pointer cannot be +modified. +- `const xyz_t * const ptr` or `xyz_t const * const ptr`, `ptr` is a constant +pointer to a constant object of type xyz_t. The value of the object and the +pointer cannot be modified. + +https://cdecl.org may help if in doubt. + +Static storage qualifier +------------------------ +Declare functions and variables private to a C file as static. + Operator Precedence ------------------- Do not rely on the implicit precedence and associativity of C operators. Use @@ -127,17 +194,32 @@ struct clock clock_cpu = { }; \endcode +Integers +-------- +To mitigate the risk of integer wrap-around, conversion or truncation errors: + +- represent integer values that should never be negative with the `unsigned` +type that you expect to hold all possible values. + +- represent integer values that may be negative with the `signed` type that you +expect to hold all possible values. + +- when taking untrusted integer input, ensure you check them against the lower +and upper bound of the integer type you store them in. + Device Register Definitions --------------------------- The format for structures representing memory-mapped device registers is standardized. -- The file containing the device structure must include stdint.h to gain access -to the uintxx_t and UINTxx_C definitions. -- The file containing the device structure must include fwk_macros.h to gain -access to the FWK_R, FWK_W and FWK_RW macros. +- The file containing the device structure must include `` to gain +access to the uintxx_t and UINTxx_C definitions. +- The file containing the device structure must include `` to +gain access to the FWK_R, FWK_W and FWK_RW macros. - All non-reserved structure fields must be prefixed with one of the above macros, defining the read/write access level. +- Avoid C structure bit-fields when representing hardware registers - how +bit-fields are represented in memory is implementation-defined. - Bit definitions should be declared via UINTxx_C macros. - Bit definitions must be prefixed by the register name it relates to. If bit definitions apply to multiple registers, then the name must be as common as diff --git a/doc/code_style.md b/doc/code_style.md index 946254fe8..a7e20afc6 100644 --- a/doc/code_style.md +++ b/doc/code_style.md @@ -25,7 +25,25 @@ Avoid using: Functions, macros, types and defines must have the "fwk_" prefix (upper case for macros and defines) to identify framework API. -It is fine and encouraged to use a variable named "i" (index) for loops. +Non-static names should be prefixed with the name of their translation unit to +avoid name collisions. + +It is acceptable to use the following common placeholder names for loop indices: + - `i` + - `j` + - `k` + +`xyz_idx` names are commonly used for indices that live longer than a single +loop. + +License +------- +All files must begin with a license header of the following form: + +Arm SCP/MCP Software +Copyright (c) 2015-2019, Arm Limited and Contributors. All rights reserved. + +SPDX-License-Identifier: BSD-3-Clause Inclusions ---------- @@ -37,6 +55,12 @@ Header file inclusions should follow a consistent sequence, defined as: For each group, order the individual headers alphabetically. +Header files (`.h` files) should include the headers needed for them to compile +and only these ones. + +Translation units (`.c` files) should not rely on indirect inclusion to provide +names they have otherwise not included themselves. + Indentation and Scope --------------------- Indentation is made of spaces, 4 characters long with each line being at most 80 @@ -230,7 +254,8 @@ The project APIs are documented using Doxygen comments. It is mandatory to document every API exposed to other elements of the project. By default, the provided Doxygen configuration omits undocumented elements from -the compiled documentation. +the compiled documentation. APIs are documented in the header files exposing +them. At a minimum: - All functions and structures must have at least a "\brief" tag. -- GitLab