fds issues

[Dak]

Hello,

I'm using a nrf51422_xxac_s110 chip, with the 12.3 SDK. My code is based on the ble_peripheral\ble_app_uart example in the 12.3 SDK.  I'm hoping you can provide some insights, and/or some relevant coding examples to review.

I added two characteristics to the existing uart service, one to change the baud rate of the BLE module, and one to change the device name, as used by the advertising. Each can be set within the same app running on a tablet. (I'm actually using a Nordic app called nRF Connect to test my code). They each have corresponding, independent event handlers, called from the

on_write(ble_nus_t * p_nus, ble_evt_t * p_ble_evt)

function, in ble_nus.c.   I'm having issues with getting the device name change to work as desired.  The sequence of events is as follows: 

When the event handler for the device name change is called, it'll run fds_write to save the value to flash, before exiting.  

After a power cycle, the value saved to flash will be read back, and the string value will be used by gap_params_init(), which will then use it to update the advertising functions. 

The first time I run the program after saving the hex files to flash, (i.e. when a device name change event hasn't happened yet, and fds_write hasn't run) a hardcoded default name is used.

I'm finding that I can change the name once, and have the system behave as expected.  When I try to change the name a second time, using the same sequence of operations, I found the device name as displayed on nRF Connect is not what I wrote, but a garbage value made up of non-standard ASCII characters.   

the main() executes as follows:

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

int main(void)
{
uint32_t err_code;
main_event_handler = uart_event_handle;
APP_SCHED_INIT(5, 5);
timers_init();
timer1_init();
uart_init();
ble_stack_init();

app_fds_init();
services_init();
read_device_name();
gap_params_init();
conn_params_init();
read_baudrate();
advertising_init();

err_code = fds_find_and_delete2();

APP_ERROR_CHECK(err_code);
while(fds_flag==1)
{
       ;
}


err_code = ble_advertising_start(BLE_ADV_MODE_FAST);

APP_ERROR_CHECK(err_code);

// Enter main loop.
for (;;)
{
power_manage();

}
}

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

The read_device_name() function will call the fds_read() function associated with the device name change event.  (Both of the two characteristics I added have their own fds_write and fds_read functions.  The fds_find_and_delete2() function finds and deletes the data associated with the file and record #'s for the device name change data.  This function, along with the fds_write and fds_read functions are mostly the example nordic code.  Within fds_find_and_delete2(), I set a global var called fds_flag, that I don't clear until it returns NRF_SUCCESS.  And then I wait till it's cleared before starting the advertising.  This while() I just added to see if the garbage collector needed to finish before adverstising started.  I didn't make a difference to what I observed.  

ps.  I found that commenting out the call to fds_find_and_delete2, and it's associated code, would result in the device name retaining  the value I first wrote to it, without changing at all, even to a garbage value.

Anyways any help would be greatly appreciated.  Hope I was clear

Thanks

Dak

[Einar Thorsrud]

Hi Dak,

It is a bit odd that writing a persistent name works the first time, and that the problem only occurs after updating it. However, it is difficult to understand much without seeing your code. Can you show the FDS related code, where you write, update and read the name?

[Dak]

Sure I can do that.  

Here is the on_write() function in ble_nus.c that calls the event handler for set device name.  It's the last else if statement, corresponding to handle: char_handles2:

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

static void on_write(ble_nus_t * p_nus, ble_evt_t * p_ble_evt)
{
ble_gatts_evt_write_t * p_evt_write = &p_ble_evt->evt.gatts_evt.params.write;

if ((p_evt_write->handle == p_nus->rx_handles.cccd_handle)&&(p_evt_write->len == 2))
{

if (ble_srv_is_notification_enabled(p_evt_write->data))
{
p_nus->is_notification_enabled = true;
}
else
{
p_nus->is_notification_enabled = false;
}
}
else if((p_evt_write->handle == p_nus->tx_handles.value_handle)&&(p_nus->data_handler != NULL))
{
p_nus->data_handler(p_nus, p_evt_write->data, p_evt_write->len);
}

else if(p_evt_write->handle == p_nus->char_handles1.value_handle)
{
uint32_t data_buffer0;

memcpy(&data_buffer0, p_evt_write->data, sizeof(uint32_t));

p_nus->set_baudrate_handler(p_nus, data_buffer0);

}

else if(p_evt_write->handle == p_nus->char_handles2.value_handle)
{
//uint32_t data_buffer1;
char data_buffer1[32];
char *data_buffer1_pointer = data_buffer1;

memcpy(data_buffer1, p_evt_write->data, sizeof(data_buffer1));
p_nus->set_device_name_handler(p_nus, data_buffer1_pointer);
}

}

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

and here is the device name handler function itself:

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

static void set_device_name_handler(ble_nus_t * p_nus, char* new_device_name)
{
uint32_t err_code;

memset(device_name_global,0,strlen(device_name_global));

strcpy(device_name_global, (const char*)new_device_name);

err_code = fds_write2();
APP_ERROR_CHECK(err_code);

}

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

And here is the fds_write2() function called from the device handler:

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

static ret_code_t fds_write2(void)
{

#define FILE_ID2 0x1111
#define REC_KEY2 0x2222

fds_record_t record;
fds_record_desc_t record_desc;
fds_record_chunk_t record_chunk;

record_chunk.p_data = device_name_pointer;

record_chunk.length_words = sizeof(device_name_global);

record.file_id = FILE_ID2;
record.key = REC_KEY2;
record.data.p_chunks = &record_chunk;
record.data.num_chunks = 1;
ret_code_t ret = fds_record_write(&record_desc, &record);


if (ret != FDS_SUCCESS)
{
return ret;
}
return NRF_SUCCESS;
}

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

Here is the read_device_name() function called in main():

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

static void read_device_name(void)
{
uint32_t err_code;

err_code = fds_read2();
APP_ERROR_CHECK(err_code);

}

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

Here is the fds_read2() function called from read_device_name():

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

static ret_code_t fds_read2(void)
{

#define FILE_ID2 0x1111
#define REC_KEY2 0x2222

fds_flash_record_t flash_record;
fds_record_desc_t record_desc;
fds_find_token_t ftok = {0};
uint32_t *data;

uint32_t err_code;

while (fds_record_find(FILE_ID2, REC_KEY2, &record_desc, &ftok) == FDS_SUCCESS)
{
err_code = fds_record_open(&record_desc, &flash_record);
if (err_code != FDS_SUCCESS)
{
return err_code;
}
data = (uint32_t *)flash_record.p_data;


device_name_pointer = (char*)data;
err_code = fds_record_close(&record_desc);
if (err_code != FDS_SUCCESS) {
return err_code;
}
}
return NRF_SUCCESS;
}

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

and here is the fds_find_and_delete2() function called from main().  As I stated earlier I've played with turning this off and on, with varying results:

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

static ret_code_t fds_find_and_delete2(void)
{
#define FILE_ID2 0x1111
#define REC_KEY2 0x2222
fds_record_desc_t record_desc;
fds_find_token_t ftok;

fds_flag = 1;

ftok.page=0;
ftok.p_addr=NULL;
// Loop and find records with same ID and rec key and mark them as deleted.
while (fds_record_find(FILE_ID2, REC_KEY2, &record_desc, &ftok) == FDS_SUCCESS)
{
fds_record_delete(&record_desc);
}
// call the garbage collector to empty them, don't need to do this all the time, this is just for demonstration
ret_code_t ret = fds_gc();
if (ret != FDS_SUCCESS)
{
return ret;
}
fds_flag = 0;
return NRF_SUCCESS;
}

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

Ok I hope thats enough.  I've made some modifications to ble_nus.h as well to support the characteristics I added, and for the UART Service event handler type, but I'll leave those out for now, unless you want to see them.

Thanks 

Dak

[Einar Thorsrud]

Hi Dak,

I am having problems following your data in the code snippet above. Where do you set device_name_pointer before calling fds_write2()? Remember tha tyou must point to valid data, and that the memory you point to must be valid at least until you get an event from FDS indication that the write has been performed.

PS: please use Insert -> Insert Code when inserting code to improve readability.

[Dak]

ok I will paste the code snippet again below, and answer your questions after that.  

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

static void on_write(ble_nus_t * p_nus, ble_evt_t * p_ble_evt)
{
ble_gatts_evt_write_t * p_evt_write = &p_ble_evt->evt.gatts_evt.params.write;

if ((p_evt_write->handle == p_nus->rx_handles.cccd_handle)&&(p_evt_write->len == 2))
{

if (ble_srv_is_notification_enabled(p_evt_write->data))
{
p_nus->is_notification_enabled = true;
}
else
{
p_nus->is_notification_enabled = false;
}
}
else if((p_evt_write->handle == p_nus->tx_handles.value_handle)&&(p_nus->data_handler != NULL))
{
p_nus->data_handler(p_nus, p_evt_write->data, p_evt_write->len);
}

else if(p_evt_write->handle == p_nus->char_handles1.value_handle)
{
uint32_t data_buffer0;

memcpy(&data_buffer0, p_evt_write->data, sizeof(uint32_t));

p_nus->set_baudrate_handler(p_nus, data_buffer0);

}

else if(p_evt_write->handle == p_nus->char_handles2.value_handle)
{
//uint32_t data_buffer1;
char data_buffer1[32];
char *data_buffer1_pointer = data_buffer1;

memcpy(data_buffer1, p_evt_write->data, sizeof(data_buffer1));
p_nus->set_device_name_handler(p_nus, data_buffer1_pointer);
}

}

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

and here is the device name handler function itself:

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

static void set_device_name_handler(ble_nus_t * p_nus, char* new_device_name)
{
uint32_t err_code;

memset(device_name_global,0,strlen(device_name_global));

strcpy(device_name_global, (const char*)new_device_name);

err_code = fds_write2();
APP_ERROR_CHECK(err_code);

}

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

And here is the fds_write2() function called from the device handler:

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

static ret_code_t fds_write2(void)
{

#define FILE_ID2 0x1111
#define REC_KEY2 0x2222

fds_record_t record;
fds_record_desc_t record_desc;
fds_record_chunk_t record_chunk;

record_chunk.p_data = device_name_pointer;

record_chunk.length_words = sizeof(device_name_global);

record.file_id = FILE_ID2;
record.key = REC_KEY2;
record.data.p_chunks = &record_chunk;
record.data.num_chunks = 1;
ret_code_t ret = fds_record_write(&record_desc, &record);


if (ret != FDS_SUCCESS)
{
return ret;
}
return NRF_SUCCESS;
}

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

Here is the read_device_name() function called in main():

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

static void read_device_name(void)
{
uint32_t err_code;

err_code = fds_read2();
APP_ERROR_CHECK(err_code);

}

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

Here is the fds_read2() function called from read_device_name():

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

static ret_code_t fds_read2(void)
{

#define FILE_ID2 0x1111
#define REC_KEY2 0x2222

fds_flash_record_t flash_record;
fds_record_desc_t record_desc;
fds_find_token_t ftok = {0};
uint32_t *data;

uint32_t err_code;

while (fds_record_find(FILE_ID2, REC_KEY2, &record_desc, &ftok) == FDS_SUCCESS)
{
err_code = fds_record_open(&record_desc, &flash_record);
if (err_code != FDS_SUCCESS)
{
return err_code;
}
data = (uint32_t *)flash_record.p_data;


device_name_pointer = (char*)data;
err_code = fds_record_close(&record_desc);
if (err_code != FDS_SUCCESS) {
return err_code;
}
}
return NRF_SUCCESS;
}

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

and here is the fds_find_and_delete2() function called from main().  As I stated earlier I've played with turning this off and on, with varying results:

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

static ret_code_t fds_find_and_delete2(void)
{
#define FILE_ID2 0x1111
#define REC_KEY2 0x2222
fds_record_desc_t record_desc;
fds_find_token_t ftok;

fds_flag = 1;

ftok.page=0;
ftok.p_addr=NULL;
// Loop and find records with same ID and rec key and mark them as deleted.
while (fds_record_find(FILE_ID2, REC_KEY2, &record_desc, &ftok) == FDS_SUCCESS)
{
fds_record_delete(&record_desc);
}
// call the garbage collector to empty them, don't need to do this all the time, this is just for demonstration
ret_code_t ret = fds_gc();
if (ret != FDS_SUCCESS)
{
return ret;
}
fds_flag = 0;
return NRF_SUCCESS;
}

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

[Dak]

The first function in the code snippet is the on_write event handler.  The last else if branch in this function handles the set_device_name characteristic - which is what I'm using to change the device name. The call to set_device_name_handler, has local vars for arguments that point to the data entered via the app.  The set_device_name_handler() function, which is in the code snippet , takes this data and copies it into the global array device_name_global that device_name_pointer points to.  

device_name_pointer and device_name_global are the global vars that hold the device name data when it is read from fds, or as its written to fds.  

char device_name_global[32] ;
char *device_name_pointer = device_name_global;

I've tried using the global's directly in on_write instead of using local vars, and it didn't make a difference.

Regarding your next point,  fds_read2, fds_write2, and fds_find_and_delete2 all access the same file and record values.  I posted my main() code snippet earlier in this ticket - after fds_init() runs, I read the device_name from fds - read_device_name(), and just before I start advertising I make a call to fds_find_and_delete2(), which should clear the file and record data in fds.  

As far as waiting till I get an FDS indication that the write has been performed, can you tell me the best way to do this?  

Thanks

Dak

[Dak]

By the way the gap_params() function takes the device_name_pointer, to update the device name code architecture, which is eventually used by the advertising functionality.  In the original sdk 12.3, this was a #define value.  All I did was change what gap_params() looks at

[Einar Thorsrud]

Hi,

There are a few issues with your code, but I do not see why you get corrupt data, but I am missing some context since I don't see all your relevant code. Some thoughts:

• Using device_name_pointer as you write should be OK, but there is clearly some issue and I cannot see everything (since it is not included in your code snippet).
  • One significant problem is that you should not set device_name_pointer to point to the FDS data in flash like you do in your fds_read2(), since it is not guaranteed to stay intact after you call fds_record_close().
• As mentionned, I don't see if you wait for FDS operations to complete before e.g. resetting. That should not lead to corruption, though.

Let me just mention a few other issues which are not directly related to this issue:

• You use always use fds_record_write(), which means that is you change name more than once you will add new records with new names, without removing the old. However, you do not have any method of choosing the right name, so it will probably not be what you would like (and you will fill up the flash if you change name may times). So you should use fds_record_update() to only keep the last name.
• You define FILE_ID2 and REC_KEY2 repeatedly. This is a bad idea and will generate a warning, but it works since you use the same value. If you change it at some point, it will not work.
• No indention in your code makes it difficult to read. That may only be an issue in the code you posted here, but please remember that in the future, as better readability makes it easier to understand what is going for us looking at it.