Tags

, , ,

Update (1/2-/2-16) Nordic just posted a useful article on nRF51 SDK error codes here.

I just finished a code review with Ron.  I wanted to capture a few of the recommendations Ron made.

The Goal

The goal of this post is to capture what I consider the biggest changes I will make to the existing Ladybug Lite Blue firmware as well as future coding based on Ron’s feedback.

Thanks To Those That Went Before

I was thrilled to have 1:1 time with Ron for a code review as an outcome of the Contextual Electronics course.  What a terrific opportunity to evolve my firmware programming abilities.  Ron did not disappoint. 

Open Source

The Ladybug firmware I am writing about in this blog post can be found at this github location.  Doxygen documentation is included within the Eclipse project’s Documentation/html/Search folder (see index.html).

I’d be grateful for feedback on how to improve the code. 

#define versus static const

My “NEW! IMPROVED! …Rule of Thumb:”

  • Use #define for magic numbers that are global in scope.  Use static const for variables that span functions within a .c file because:
    • #define is global in scope – which means there is a greater chance for conflicting use.  I might use a #define to name a time out period to 10 (#define TIMEOUT 10) and then use the same name within another .c file with a time out period of 20 (#define TIMEOUT 20).  Using the “static” keyword keeps the variable within the scope of the .c file.
    • static const variables are strongly typed.  I’m telling the compiler the exact data type to use for the variable.
    • the code is more readable.
Example of a magic number that is global in scope from the nRF51 SDK (ble_gap.h):

/**@brief GAP device name maximum length. */

#define BLE_GAP_DEVNAME_MAX_LEN           31

 Example where I should have used static const.  In the Ladybug_Hydro.c file I had:

/**

 * \brief mapping the FET pins to the schematic

 */

#define EC_VIN_FET0

#define EC_VOUT_FET7

I changed these to:

/**

 * \brief mapping the FET pins to the schematic

 */

static uint32_t m_EC_VIN_FET=0;

static uint32_t m_EC_VOUT_FET=7;

  • avoid making any changes to any bit of the code of an external SDK (like Nordic’s).  For example, I started changing stuff like:

#define APP_ADV_TIMEOUT_IN_SECONDS      0

to:

static uint32_t const      m_app_adv_timeout_in_seconds = 0;

I would have preferred to use a static const instead of #define however the SDK requires a precompiled value since it is used within another #define.
Note: I name variables m_<variable name> to identify variables that are scoped to a .c file (versus local to a function).  

Error Handling

I like the way the Nordic SDK supports error handling.  My code as well as SDK code are peppered with:

APP_ERROR_HANDLER(err_code);

and

APP_ERROR_CHECK(err_code);

both are #define’d in the app_error.h file of the nRF51 SDK.  These resolve into calling error routines I defined within main.c:

/**@brief Callback function for asserts in the SoftDevice.

 *

 * @details This function will be called in case of an assert in the SoftDevice.

 *

 * @warning This handler is an example only and does not fit a final product. You need to analyze

 *          how your product is supposed to react in case of Assert.

 * @warning On assert from the SoftDevice, the system can only recover on reset.

 *

 * @param[in] line_num   Line number of the failing ASSERT call.

 * @param[in] file_name  File name of the failing ASSERT call.

 */

void assert_nrf_callback(uint16_t line_num, const uint8_t * p_file_name)

{

  app_error_handler(DEAD_BEEF, line_num, p_file_name);

}

/**@brief Function for error handling, which is called when an error has occurred.

 *

 * @warning This handler is an example only and does not fit a final product. You need to analyze

 *          how your product is supposed to react in case of error.

 * \note I decided to use the SEGGER_RTT printing functionality.  My understanding is the SEGGER_RTT calls can be left in code with no effect

 * when there is no terminal to output.  The con of this approach is I can’t hook up a UART enabled terminal session and see what is going on without the debugger present.

 *

 * @param[in] error_code  Error code supplied to the handler.

 * @param[in] line_num    Line number where the handler is called.

 * @param[in] p_file_name Pointer to the file name.

 */

void app_error_handler(uint32_t error_code, uint32_t line_num, const uint8_t * p_file_name)

{

  // This call can be used for debug purposes during application development.

  // The SEGGER_RTT APIs will not cause a problem in production so I’m leaving them in.

  SEGGER_RTT_WriteString(0,“—>>>BUMMER!! In app_error_handler\n”);

  SEGGER_RTT_printf(0,“error code: %d (or 0X%X if assert..base for BLE = 0x3000) Line number: %d file name: “,error_code,error_code,line_num);

  SEGGER_RTT_WriteString(0,p_file_name);

  SEGGER_RTT_WriteString(0,“\n”);

  //  ble_debug_assert_handler(error_code, line_num, p_file_name);

}

For now, error checking ends up printing out information about what/where the error occurred.  This has led to quickly figuring out where the error is and an idea of why it is happening.  In the future I could see mapping these error routines to LED indicators.  I use the SEGGER_RTT APIs because they are robust and work when the code is running in both run time and debug mode. Contrast this version of printf to the serial port to what I discussed in this post.

Checking for NULL Pointers

The basic point here is to check input values of called functions.  This is simple stuff that I should just do. For example, I was not checking for NULL pointer input to functions.  For example, I would pass in  a pointer to an int16_t array:

static void get_EC_reading(int16_t *p_EC) {

and assume p_EC pointed to a valid address.  I added in a simple check:

  if (p_EC == NULL){  //Shouldn’t be passing in a null pointer given the EC Vin and Vout values are planned to be stored at this memory location.

      APP_ERROR_HANDLER(LADYBUG_ERROR_NULL_POINTER);

  }

I also added error codes unique to the Ladybug to the Ladybug_Error.h file. 

Implement Time Outs

When using the pstorage APIs to access the nRF51822’s flash, the Nordic examples use code that has the potential to hang the system if feedback from the pstorage is not given to the pstorage handler in my code.  The pstorage APIs manage access to flash when the BLE stack is running.  This allows the BLE stack to figure out when it is safe to read from and (more difficult!) write to flash.  The function ladybug_Flash_Init() in the Ladybug_Flash.c file registers the ladybug_Flash_handler() function to be called back when a flash activity has completed.  The ladybug_flash_read() and ladybug_flash_write() functions rely on the static variable m_mypstorage_wait_flag to let them know when the flash action has been completed.  These functions wait until completion.  If the ladybug_Flash_Handler() is never called, the program will hang without having an app timer set up to fire after an amount of time.  I discuss using the nRF51 SDK’s app timers in this post.  Here is the code I use to surround a flash action.  In this case within ladybug_flash_write():

 // Start the timer up again to timeout if writing to flash doesn’t happen

  start_timer();

  m_mypstorage_wait_flag = 1;

  err_code = pstorage_store(p_handle, p_bytes_to_write, num_bytes_to_write, 0);

  while(m_mypstorage_wait_flag) {  }

  APP_ERROR_CHECK(err_code);

  stop_timer();

If the timer fires, the caller’s call back function is invoked.  This returns an error code to the caller.  It is then up to the caller to act on the error code.  Here is the definition of ladybug_flash_write() showing how the call back function did_flash_action() is included within the call to ladybug_flash_write():

void ladybug_flash_write(flash_rw_t what_data_to_write, uint8_t *p_bytes_to_write,pstorage_size_t num_bytes_to_write,void(*did_flash_action)(uint32_t err_code));

I call ladybug_flash_write() in main.c.  Thus main.c also include the call back function:

/**

 * \callgraph

 * \brief call back from Ladybug_Flash.c to let us know if the flash write was successful (or not)

 * @param err_code          0 if successful

 */

void did_flash_write(uint32_t err_code) {

  if (err_code == 0) {

      SEGGER_RTT_WriteString(0,“…Flash write SUCCESS!”);

  }

  else {

      APP_ERROR_HANDLER(err_code);

  }

}

That’s it for now.   Thank you for reading this far.  Please find many things to smile about.

Advertisements