This post is older than 2 years and might not be relevant anymore
More Info: Consider searching for newer posts

What is the reason of making nRF APIs that complicated?

As an example, let's see this function from app_hrm (heart rate monitor):

static void conn_params_init(void)
{
    uint32_t               err_code;
    ble_conn_params_init_t cp_init;

    memset(&cp_init, 0, sizeof(cp_init));

    cp_init.p_conn_params                  = NULL;
    cp_init.first_conn_params_update_delay = FIRST_CONN_PARAMS_UPDATE_DELAY;
    cp_init.next_conn_params_update_delay  = NEXT_CONN_PARAMS_UPDATE_DELAY;
    cp_init.max_conn_params_update_count   = MAX_CONN_PARAMS_UPDATE_COUNT;
    cp_init.start_on_notify_cccd_handle    = m_hrs.hrm_handles.cccd_handle;
    cp_init.disconnect_on_fail             = true;
    cp_init.evt_handler                    = NULL;
    cp_init.error_handler                  = conn_params_error_handler;

    err_code = ble_conn_params_init(&cp_init);
    APP_ERROR_CHECK(err_code);
}

Quickly looking, it seems that there's a lot of code doing lots of things. However, after reading out the lines it turns out that the information is mostly redundant (like "cp_init.first_conn_params_update_delay = FIRST_CONN_PARAMS_UPDATE_DELAY"), and the function just bypasses parameters forward to another function. I used to see this type of coding at the beginning of 1990's, but it was quite soon replaced with much more comfortable way, using function parameters. With that style, the above code would shrink as follows:

ble_conn_params_init(
    NULL, FIRST_CONN_PARAMS_UPDATE_DELAY, NEXT_CONN_PARAMS_UPDATE_DELAY,
    MAX_CONN_PARAMS_UPDATE_COUNT, m_hrs.hrm_handles.cccd_handle, true, NULL,
    conn_params_error_handler);

And actually, the extra function definition conn_params_init() would no more be needed at all. This type of coding would make application much much more readable, yet it would save code lines and probably even flash space.

I have to say that your code looks much more complicated that it would need to be. I know this is a kind of "school of thought" issue, and maybe related to "linux-style" -coding. But still, your coding style doesn't make the APIs too easy for customers. Sorry to say, but it has been a horrendous experience to get familiar with nRF51822 SDK (and the DFU OTA is still not working, but that's another story).

Parents
  • And really, there is never real need for "dozens of arguments". In case a function seems to need more than four arguments, you can simply split it into two smaller functions making the code also more readable, and more self-documenting. Using a structure to pass "dozens of arguments" does not solve the issue.

    For example, instead of providing a single fuzzy "ble_conn_params_init()" you might provide ble_set_conn_delays() and ble_set_conn_handlers() and maybe a third one for something else (it is hard to say what is really needed, based on your approach). The result would be much more readable that way.

Reply
  • And really, there is never real need for "dozens of arguments". In case a function seems to need more than four arguments, you can simply split it into two smaller functions making the code also more readable, and more self-documenting. Using a structure to pass "dozens of arguments" does not solve the issue.

    For example, instead of providing a single fuzzy "ble_conn_params_init()" you might provide ble_set_conn_delays() and ble_set_conn_handlers() and maybe a third one for something else (it is hard to say what is really needed, based on your approach). The result would be much more readable that way.

Children
No Data
Related