documentation and code style

[vit bernatik]

Hello I'm trying to learn your API using nrf5-ble-tutorial-advertising example. Over all the example is much help and API documentation is quite good. But part of API code is obfuscated and some links in documentation are missing. There is many such places but I will pick one example. In tutorial nrf5-ble-tutorial-advertising there is used:

static void device_manager_init(bool erase_bonds){
   ...
   dm_application_param_t register_param;
   ...
}

It is great you use struct I just click with my tool on "open declaration" and I see it documented.

typedef struct
{
    dm_event_cb_t        evt_handler;  /**< Event Handler to be registered. It will receive asynchronous notification from the module, see \ref dm_events for asynchronous events. */
    uint8_t              service_type; /**< Bit mask identifying services that the application intends to support for all peers. */
    ble_gap_sec_params_t sec_param;    /**< Security parameters to be used for the application. */
} dm_application_param_t;

evt_handler and sec_param are great I can jump to documentation what it is. But What about service_type?? it is uint8_t. What bit masking can I use?? How would I find documentation for that?? If I look into infocenter there is same piece of information.

When I look into example I see that you are filling it with:

register_param.service_type           = DM_PROTOCOL_CNTXT_GATT_SRVR_ID;

And the definition for DM_PROTOCOL_CNTXT_GATT_SRVR_ID is well described. I can use in eclipse "open declaration" and I see:

**
 * @defgroup dm_service_cntext_types Service/Protocol Types
 *
 * @brief Describes the possible types of Service/Protocol Contexts for a bonded/peer device.
 *
 * @details Possible Service/Protocol context per peer device. The Device Manager provides the
 *          functionality of persistently storing the Service/Protocol context and can automatically
 *          load them when needed.
 *          For example system attributes for a GATT Server. Based on the nature of the application, 
 *          not all service types may be needed. The application can specify
 *          only the service/protocol context it wants to use at the time of registration.
 * @{
 */
#define DM_PROTOCOL_CNTXT_NONE         0x00  /**< No Service Context, this implies the application does not want to associate any service/protocol context with the peer device */
#define DM_PROTOCOL_CNTXT_GATT_SRVR_ID 0x01  /**< GATT Server Service Context, this implies the application does associate GATT Server with the peer device and this information will be loaded when needed for a bonded device */
#define DM_PROTOCOL_CNTXT_GATT_CLI_ID  0x02  /**< GATT Client Service Context, this implies the application does associate GATT Client with the peer device and this information will be loaded when needed for a bonded device */
#define DM_PROTOCOL_CNTXT_ALL                                                                     \
        (DM_PROTOCOL_CNTXT_GATT_SRVR_ID | DM_PROTOCOL_CNTXT_GATT_CLI_ID) /**< All Service/Protocol Context, this implies that the application wants to associate all Service/Protocol Information with the bonded device. This is configurable based on system requirements. If the application has only one type of service, this define could be altered to reflect the same.  */
/** @} */

So the only sabotage is not linking this documentation from service_type description. If you just add See \ref dm_service_cntext_types then the documentation would be perfect. So the code would change from this:

uint8_t              service_type; /**< Bit mask identifying services that the application intends to support for all peers. */

into:

uint8_t              service_type; /**< Bit mask for services to store context to persistent memory. See \ref dm_service_cntext_types */

Furthermore if it is not #define but enum then it is perfect because my tool can automatically "open declaration" and find the allowed bits automatically. The perfect code IMHO would look something like this:

/**
 * @brief Describes the possible types of Service/Protocol Contexts for a bonded/peer device.
 *
 * @details Possible Service/Protocol context per peer device. The Device Manager provides the
 *          functionality of persistently storing the Service/Protocol context and can automatically
 *          load them when needed.
 *          For example system attributes for a GATT Server. Based on the nature of the application, 
 *          not all service types may be needed. The application can specify
 *          only the service/protocol context it wants to use at the time of registration.
  */
typedef enum
{
 my_DM_PROTOCOL_CNTXT_NONE      = 0x00,  /**< No Service Context, this implies the application does not want to associate any service/protocol context with the peer device */
 my_DM_PROTOCOL_CNTXT_GATT_SRVR_ID = 0x01,  /**< GATT Server Service Context, this implies the application does associate GATT Server with the peer device and this information will be loaded when needed for a bonded device */
 my_DM_PROTOCOL_CNTXT_GATT_CLI_ID  = 0x02,  /**< GATT Client Service Context, this implies the application does associate GATT Client with the peer device and this information will be loaded when needed for a bonded device */
 my_DM_PROTOCOL_CNTXT_ALL          = 0x03,  /**< All Service/Protocol Context, this implies that the application wants to associate all Service/Protocol Information with the bonded device. This is configurable based on system requirements. If the application has only one type of service, this define could be altered to reflect the same.  */
} dm_srvice_cntxt_bits_t;

typedef struct
{
    dm_event_cb_t          evt_handler;  /**< Event Handler to be registered. It will receive asynchronous notification from the module, see \ref dm_events for asynchronous events. */
    dm_srvice_cntxt_bits_t service_type; /**< Bit mask for services to store context to persistent memory. */
    ble_gap_sec_params_t   sec_param;    /**< Security parameters to be used for the application. */
} dm_application_param_t;

So my questions are:

1. Maybe there is obvious way how to find out what flags can user fill into:

   uint8_t service_type; /**< Bit mask identifying services that the application intends to support for all peers. */

If it so please tell me - how to use your documentation to know what mask I can fill into service_type?

2. If it is truly imperfection in documentation - are you planning on filling gaps in documentation in near future? (- probably in most places adding adding \ref would be enough)

3. Are you planning to use more enum instead of #define in your code? Or why would you prefer #define over enum? I'm really interested - perhaps you wanna have stronger control of type and enum can be sometimes encoded into uint32_t?

[Håkon Alseth]

Hi vbe,

1. Maybe there is obvious way how to find out what flags can user fill into:

uint8_t              service_type;
/**< Bit mask identifying services that the application intends to support for all peers. */

If it so please tell me - how to use your documentation to know what mask I can fill into service_type?

I agree with your conclusion. In the struct dm_application_param_t I cannot see any references to the bitmask that shall be populated in member .service_type. As you have already found; this shall not be of a type uint8_t, but of type service_type_t. Apart from this bug in the API (which I will report internally), the structure follows the SoftDevice API guidelines.

I agree that using enums instead of defines is easier to maneuver in, seen from the developers POV, but you do not have full control over your types, unless you type-cast at a later point, which may cause the flow to be a bit messy.

2. If it is truly imperfection in documentation - are you planning on filling gaps in documentation in near future? (- probably in most places adding adding \ref would be enough)

A \ref should have been added here, and type changed from uint8_t to service_type_t to align with generic API documentation.

3. Are you planning to use more enum instead of #define in your code? Or why would you prefer #define over enum? I'm really interested - perhaps you wanna have stronger control of type and enum can be sometimes encoded into uint32_t?

Stronger control of the types is the main reason. Enums can be 8/16/32 bits, depending on compiler, optimization, and settings used. Since our stack uses SVCalls in all API calls, it is important that the sizes match on "both ends of the SVCalls"

Thanks for reporting this back to us and helping us to improve our deliveries. It is always helpful to get feedback!

Cheers, Håkon