From ecc4c765ccdcb7fe788a78cff3dff4a8b62a1dfe Mon Sep 17 00:00:00 2001 From: Inaky Perez-Gonzalez Date: Thu, 16 Jun 2016 14:52:44 -0700 Subject: [PATCH] doc: fix WARNING: Invalid definition" due to unamed structs/unions Fix with a workaround in unnamed unions / structs in bluetooth, i2c, sensor and uart. Current documentation parsers (sphinx under Doxygen) don't seem to understand well unnamed structs / unions. They will not generate any documentation for any documented members (see left side of http://imgur.com/mcpBXWc). A workaround is to make the parser think there is something like a struct/union/enum name that is actually something with no effect to the compiler. Naming it with __unnamed_workaround__ ensures it is clear it is a workaround while we wait for a final fix. It is #defined to be a NO-OP to the compiler and rearrange the member documentation as *@param* so we have some documentation that the non-worked around code fails to document. Anonymous structs/union that declare a variable are just given an internal name. Workarounds documented in the contribution guidelines. Change-Id: I4d32cf444f3c5e7d2fb11581e4b41f80e93c9786 Signed-off-by: Inaky Perez-Gonzalez --- doc/contribute/doxygen/structs.rst | 139 ++++++++++++++++++++++++++++- include/bluetooth/conn.h | 15 ++-- include/bluetooth/gatt.h | 23 +++-- include/i2c.h | 2 +- include/sensor.h | 4 +- include/toolchain.h | 15 ++++ include/uart.h | 17 ++-- 7 files changed, 185 insertions(+), 30 deletions(-) diff --git a/doc/contribute/doxygen/structs.rst b/doc/contribute/doxygen/structs.rst index 4eb0dcf723e..1fbdbcee87f 100644 --- a/doc/contribute/doxygen/structs.rst +++ b/doc/contribute/doxygen/structs.rst @@ -27,6 +27,143 @@ Structs have a simplified template: Doxygen does not require any commands to recognize the different comments. It does, however, require that line 8 be left blank. +Unnamed structures or unions +**************************** + +The *Sphinx* parser seems to get confused by unnamed structures, used +specially in nested unions / structs: + +.. code-block:: c + + struct sensor_value { + enum sensor_value_type type; + union { + struct { + int32_t val1; + int32_t val2; + }; + double dval; + }; + }; + +This will likely generate an error such as:: + + doc/api/io_interfaces.rst:14: WARNING: Invalid definition: Expected identifier in nested name. [error at 0] + + ^ + doc/api/io_interfaces.rst:14: WARNING: Invalid definition: Expected identifier in nested name. [error at 0] + + ^ + doc/api/io_interfaces.rst:14: WARNING: Invalid definition: Expected end of definition. [error at 12] + sensor_value.__unnamed__ + ------------^ + +A workaround is to introduce something that looks like a name to the +parser but it is a no-op to the compiler: + +.. code-block:: c + + /** + * Workaround for documentation parser limitations + * + * Current documentation parsers (sphinx under Doxygen) don't seem to + * understand well unnamed structs / unions. A workaround is to make + * the parser think there is something like a struct/union/enum name + * that is actually something with no effect to the compiler. + * + * Current choice is to give it a 1B alignment. This basically tells + * the compiler to do what is doing now: align it wherever it thinks + * it should, as a 1B alignment "restriction" fits any other alignment + * restriction we might have. + */ + #define __unnamed_workaround__ __attribute__ ((__aligned__ (1))) + +And then use it such as: + +.. code-block:: c + + struct sensor_value { + enum sensor_value_type type; + union __unnamed_workaround__ { + struct __unnamed_workaround__ { + int32_t val1; + int32_t val2; + }; + double dval; + }; + }; + +This is currently defined in :file:`include/toolchain.h`. The issue +reported to developers in +https://github.com/sphinx-doc/sphinx/issues/2683. + +When running into this issue, the member documentation has to be done +with *@param* indicators, otherwise they won't be extracted: + +.. code-block:: c + + /** + * @brief UART device configuration. + * + * @param port Base port number + * @param base Memory mapped base address + * @param regs Register address + * @param sys_clk_freq System clock frequency in Hz + */ + struct uart_device_config { + union __unnamed_workaround__ { + uint32_t port; + uint8_t *base; + uint32_t regs; + }; + + uint32_t sys_clk_freq; + ... + + +Unnamed structures or unions which declare a variable +----------------------------------------------------- + +A special case of this is non-anynoymous unnamed structs/unions, where +a variable is defined. In this case, the workaround is much more +simple. Consider: + +.. code-block:: c + + union dev_config { + uint32_t raw; + struct { + uint32_t use_10_bit_addr : 1; + uint32_t speed : 3; + uint32_t is_master_device : 1; + uint32_t is_slave_read : 1; + uint32_t reserved : 26; + } bits; + }; + +This will likely generate an error such as:: + + doc/api/io_interfaces.rst:28: WARNING: Invalid definition: Expected identifier in nested name. [error at 19] + struct dev_config::@61 dev_config::bits + -------------------^ + +The simple solution is to name that unnamed struct, with an internal +name (such as *__bits*): + +.. code-block:: c + + union dev_config { + uint32_t raw; + struct __bits { + uint32_t use_10_bit_addr : 1; + uint32_t speed : 3; + uint32_t is_master_device : 1; + uint32_t is_slave_read : 1; + uint32_t reserved : 26; + } bits; + }; + + Structure Documentation Example ******************************* @@ -56,4 +193,4 @@ Incorrect: The struct has no documentation. Developers that want to expand its functionality have no way of understanding why the struct is defined -this way, nor what its components are. \ No newline at end of file +this way, nor what its components are. diff --git a/include/bluetooth/conn.h b/include/bluetooth/conn.h index c54675f21f4..14d2c75ef07 100644 --- a/include/bluetooth/conn.h +++ b/include/bluetooth/conn.h @@ -135,19 +135,22 @@ enum { BT_CONN_ROLE_SLAVE, }; -/** Connection Info Structure */ +/** @brief Connection Info Structure + * + * + * @param type Connection Type + * @param role Connection Role + * @param le LE Connection specific Info + * @param br BR/EDR Connection specific Info + */ struct bt_conn_info { - /** Connection Type */ uint8_t type; - /** Connection Role */ uint8_t role; - union { - /** LE Connection specific Info */ + union __unnamed_workaround__ { struct bt_conn_le_info le; - /** BR/EDR Connection specific Info */ struct bt_conn_br_info br; }; }; diff --git a/include/bluetooth/gatt.h b/include/bluetooth/gatt.h index 8b471b6020b..a512b0c5ecf 100644 --- a/include/bluetooth/gatt.h +++ b/include/bluetooth/gatt.h @@ -821,24 +821,23 @@ typedef uint8_t (*bt_gatt_read_func_t)(struct bt_conn *conn, uint8_t err, struct bt_gatt_read_params *params, const void *data, uint16_t length); -/** @brief GATT Read parameters */ +/** @brief GATT Read parameters + * @param func Read attribute callback + * @param handle_count If equals to 1 single.handle and single.offset + * are used. If >1 Read Multiple Characteristic + * Values is performed and handles are used. + * @param handle Attribute handle + * @param offset Attribute data offset + * @param handles Handles to read in Read Multiple Characteristic Values + */ struct bt_gatt_read_params { - /** Read attribute callback */ bt_gatt_read_func_t func; - /** Handles count. - * If equals to 1 single.handle and single.offset are used. - * If >1 Read Multiple Characteristic Values is performed and handles - * are used. - */ size_t handle_count; - union { - struct { - /** Attribute handle */ + union __unnamed_workaround__ { + struct __single { uint16_t handle; - /** Attribute data offset */ uint16_t offset; } single; - /** Handles to read in Read Multiple Characteristic Values */ uint16_t *handles; }; }; diff --git a/include/i2c.h b/include/i2c.h index b7728717570..9637a6f3031 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -107,7 +107,7 @@ struct i2c_msg { union dev_config { uint32_t raw; - struct { + struct __bits { uint32_t use_10_bit_addr : 1; uint32_t speed : 3; uint32_t is_master_device : 1; diff --git a/include/sensor.h b/include/sensor.h index 64b09bd699b..0f45bad74d9 100644 --- a/include/sensor.h +++ b/include/sensor.h @@ -63,8 +63,8 @@ enum sensor_value_type { */ struct sensor_value { enum sensor_value_type type; - union { - struct { + union __unnamed_workaround__ { + struct __unnamed_workaround__ { int32_t val1; int32_t val2; }; diff --git a/include/toolchain.h b/include/toolchain.h index 37bf8f491df..490aab5ea25 100644 --- a/include/toolchain.h +++ b/include/toolchain.h @@ -31,4 +31,19 @@ #include #endif +/** + * Workaround for documentation parser limitations + * + * Current documentation parsers (sphinx under Doxygen) don't seem to + * understand well unnamed structs / unions. A workaround is to make + * the parser think there is something like a struct/union/enum name + * that is actually something with no effect to the compiler. + * + * Current choice is to give it a 1B alignment. This basically tells + * the compiler to do what is doing now: align it wherever it thinks + * it should, as a 1B alignment "restriction" fits any other alignment + * restriction we might have. + */ +#define __unnamed_workaround__ __attribute__((__aligned__(1))) + #endif /* _TOOLCHAIN_H */ diff --git a/include/uart.h b/include/uart.h index 80195c80b9e..d5503ec755a 100644 --- a/include/uart.h +++ b/include/uart.h @@ -88,20 +88,21 @@ typedef void (*uart_irq_callback_t)(struct device *port); */ typedef void (*uart_irq_config_func_t)(struct device *port); -/** @brief UART device configuration.*/ +/** + * @brief UART device configuration. + * + * @param port Base port number + * @param base Memory mapped base address + * @param regs Register address + * @param sys_clk_freq System clock frequency in Hz + */ struct uart_device_config { - /** - * Base port number - * or memory mapped base address - * or register address - */ - union { + union __unnamed_workaround__ { uint32_t port; uint8_t *base; uint32_t regs; }; -/** System clock frequency in Hz.*/ uint32_t sys_clk_freq; #ifdef CONFIG_PCI