Skip to content

Mismatch between API design guidelines and Coding Guidelines with user_data #91418

@Thalley

Description

@Thalley

Summary

Zephyr's API Design guidelines has the following:

The final parameter should be a void *user_data pointer carrying context that allows a shared callback function to locate additional material necessary to process the callback.

Zephyr's Coding Guidelines has rule 70 that prohibits the removal of const when casting (even to void *).

This means that in any case where there are some const data that you'd would like to pass onto a callback for any such APIs that use void *user_data, there will be a coding guideline compliance issue.

For example, a common example of this is the scan result callback in Bluetooth that may look like this:

static void device_found(const bt_addr_le_t *addr, int8_t rssi, uint8_t type,
             struct net_buf_simple *ad)
{
    char dev[BT_ADDR_LE_STR_LEN];

    bt_addr_le_to_str(addr, dev, sizeof(dev));
    printk("[DEVICE]: %s, AD evt type %u, AD data len %u, RSSI %i\n",
           dev, type, ad->len, rssi);

    /* We're only interested in legacy connectable events or
     * possible extended advertising that are connectable.
     */
    if (type == BT_GAP_ADV_TYPE_ADV_IND ||
        type == BT_GAP_ADV_TYPE_ADV_DIRECT_IND ||
        type == BT_GAP_ADV_TYPE_EXT_ADV) {
        bt_data_parse(ad, eir_found, (void *)addr);
    }
}

where bt_data_parse is

void bt_data_parse(struct net_buf_simple *ad,
           bool (*func)(struct bt_data *data, void *user_data),
           void *user_data);

Our coding guidelines will prohibit this.

Other cases could be drivers that does

static int uarte_async_init(const struct device *dev)
{
	struct uarte_nrfx_data *data = dev->data;
	NRF_UARTE_Type *uarte = get_uarte_instance(dev);
	static const uint32_t rx_int_mask =
		NRF_UARTE_INT_ENDRX_MASK |
		NRF_UARTE_INT_RXSTARTED_MASK |
		NRF_UARTE_INT_ERROR_MASK |
		NRF_UARTE_INT_RXTO_MASK |
		((IS_ENABLED(CONFIG_UART_NRFX_UARTE_ENHANCED_RX) &&
		  !IS_ENABLED(UARTE_HAS_FRAME_TIMEOUT)) ? NRF_UARTE_INT_RXDRDY_MASK : 0);

#if !defined(CONFIG_UART_NRFX_UARTE_ENHANCED_RX)
	int ret = uarte_nrfx_rx_counting_init(dev);

	if (ret != 0) {
		return ret;
	}
#endif

	nrf_uarte_int_enable(uarte, rx_int_mask);

	k_timer_init(&data->async->rx.timer, rx_timeout, NULL);
	k_timer_user_data_set(&data->async->rx.timer, (void *)dev);
	k_timer_init(&data->async->tx.timer, tx_timeout, NULL);
	k_timer_user_data_set(&data->async->tx.timer, (void *)dev);

	return 0;
}

where const is also removed from dev when passing to __syscall void k_timer_user_data_set(struct k_timer *timer, void *user_data);

At the time of writing this, there are 1.9k violations of this rule in Zephyr.

Describe the solution you'd like

I don't see any easy solution for this. The following could be options:

  1. Add an exception for the rule for void *
  2. Remove the rule from the coding guidelines
  3. Remove the API Design guideline
  4. Instead of supplying the pointer directly, make a copy and supply that (where applicable)
  5. Instead of supplying the pointer directly, store the pointer in a global variable and access that in the callback (where applicable)
  6. Add duplication of APIs that use user_data that support supplying const void *user_data

Alternatives

No response

Additional Context

See also my original comment on Discord that received no responses (at the time of writing this): https://discord.com/channels/720317445772017664/1375902831927890041

Metadata

Metadata

Assignees

Labels

EnhancementChanges/Updates/Additions to existing featuresarea: Coding GuidelinesCoding guidelines and style

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions