Should FDS return code be checked after event not after call

[dgerman]

The examples include checking the return code from various FDS calls immediately after the call.

Only some errors can be detected at the time of the call (for example NULL arg, Not initialized, unaligned address)
It seems that others must wait until the operation is complete ( especially if more than one operation has been queued) as per the callback event which makes this example misleading.

Can you clarify this?

For example WRITING A RECORD:

rc = fds_record_write(&record_desc, &record);

if (rc != NRF_SUCCESS)

{

/* Handle error. */

}

"The command is queued up, and the success or failure is indicated through event callbacks."

https://infocenter.nordicsemi.com/topic/sdk_nrf5_v17.1.0/lib_fds_usage.html

[Einar Thorsrud]

Hi,

You are right that there are different errors you can get returned from FDS calls and as events (callbacks), so you need to check both. This is the case with most asynchronous API calls in the nRF5 SDK. I did not get what was misleading in which example though, nor what you need clarification on. Can you elaborate?

[dgerman]

Thank you for the clarification about both kinds of errors and the timing.

The example shows only testing the return code immediately after the call and does not show any indication that other errors may occur which will NOT be detected by this code segment example.

Perhaps including a delay, testing for the operation to complete  before checking the return code and/or including the callBack handler testing the return code would warn users not to depend on only testing the return code after the operation call.

[Einar Thorsrud]

Hi,

Ah, I see. This example print a log on any error, which is probably what you want in such an interactive application. Specifically, look at the beginning of fds_evt_handler() from <SDK 17.1.0>\examples\peripheral\flash_fds\main.c:

static void fds_evt_handler(fds_evt_t const * p_evt)
{
    if (p_evt->result == NRF_SUCCESS)
    {
        NRF_LOG_GREEN("Event: %s received (NRF_SUCCESS)",
                      fds_evt_str[p_evt->id]);
    }
    else
    {
        NRF_LOG_GREEN("Event: %s received (%s)",
                      fds_evt_str[p_evt->id],
                      fds_err_str(p_evt->result));
    }

You could argue that the color for errors could be something other than green, though...

For a real application you probably want to do something more than just logging that an error occurred, but how to handle an error is application specific (sometimes you may want to ignore it, other you may want to try again, or do something differently altogether).

[dgerman]

Thank you for additional clarification and example.

I think it is very important that the example be modified to consider post-call error checking.

[dgerman]

https://infocenter.nordicsemi.com/topic/sdk_nrf5_v17.1.0/lib_fds_usage.html

[Einar Thorsrud]

 We are very interested in feedback that can improve examples, but I am unfortunately not able to see what you are missing. Where would you want to add this additional error handling, and how would you do it? There is no point between the API call is made and where the callback is received where you should or can check errors.

[dgerman]

Thanks again. I, personally, am not missing anything. My previous extensive experience with asynchronous I/O  contributed to my understanding of when the function error codes need to be checked. The example I referenced originally will cause users without our level of understanding to write code that will only fail sometimes and be very difficult to diagnose.

I understand that adding a callback to the example(especially the write) will make the example more complicated but this is a complicated process. 

I would like to suggest something like the following (*UNTESTED*):

static void fds_evt_handler(fds_evt_t const * p_evt)
{ static lastRC = p_evt->result;}

/* ******** */

...
fds_register (fds_evt_handler);

rc = fds_record_write(&record_desc, &record);

if (  (rc != NRF_SUCCESS )

   && (rc != FDS_ERR_BUSY)  ) /* is this necessary??*/

           {  handle problems Qing the write }

while { FDS_ERR_BUSY == lastRC ){ nrf_delay_ms(1);}

if (lastRC != NRF_SUCCESS)

           {  handle problem performing the write }

[Einar Thorsrud]

Hi,

Thanks for the input. I fully agree that an event handler is needed. It is used in the FDS example, where it is called fds_evt_handler() and is implemented in <SDK 17.1.0>\examples\peripheral\flash_fds\main.c.

Based on the code snippet what you want is to write the result to a variable that you read after making the call. That has the down side that it involves a busy wait for the operation to finish, then handle the error. I believe the way it is done in the example is better in general, doing it fully asynchronously. Then expanding on that if you wan to busy wait and handle the error immediately afterwards like you demonstrate is easily done. I am personally not too fond of examples promoting busy waiting when not needed though, as it is often done too much, though I absolutely see that it can make sense in many applications.

[dgerman]

I agree that promoting busy-wait is not optimal, after all the is the whole point of having asynchronous operations in the first place is to allow for processing to continue. Although code examples include event handler, I though it was important to include it in the example in the infocenter  documentation rather than simply checking the return code when queueing the operation.