KW40Z_Connectivity_Software_1.0.1 review

lucianfiran · ‎02-14-2017

KW40Z_Connectivity_Software_1.0.1 (KSDK_1.3.0); IAR 7.50

Some observed potential code issues list:

1. \ConnSw\framework\Bootloader MKW40Z160_cfg.h contains:
* \file MKW40Z160.h
...
/** Kinetis ARM Cortex-M4 model */
#ifndef _MK21D256_CFG_H
#define _MK21D256_CFG_H
...
#endif//MK21D5_CFG_H
expected:
* \file MKW40Z160_cfg.h
....
/** Kinetis ARM Cortex-M0+ model */
#ifndef _MKW40Z160_CFG_H
#define _MKW40Z160_CFG_H
...
#endif//_MKW40Z160_CFG_H

mario_castaneda · ‎03-09-2017

Hi Lucian,

Thank you for feedback. We will take a look of this, we will consider your recommendations.

Best Regards.

Mario

lucianfiran · ‎03-09-2017

24. ble_general.h defines should be check against standard limits and

should be correlated to values used in gatt_db.h on Peripheral Preferred Connection Parameters Characteristic

/*! Boundary values for the Connection Parameters (Standard GAP) */
#define gcConnectionIntervalMin_c                   (0x0006)
#define gcConnectionIntervalMax_c                   (0x0C80)
#define gcConnectionSlaveLatencyMax_c               (0x01F3)
#define gcConnectionSupervisionTimeoutMin_c         (0x000A)
#define gcConnectionSupervisionTimeoutMax_c         (0x0C80)
/*! Default values for the Connection Parameters (Preferred) */
/*! connIntervalmin = Conn_Interval_Min * 1.25 ms */
/*! Value of 0xFFFF indicates no specific minimum */
#ifndef gcConnectionIntervalMinDefault_c
#define gcConnectionIntervalMinDefault_c            (40)
#endif
/*! connIntervalmax = Conn_Interval_Max * 1.25 ms */
/*! Value of 0xFFFF indicates no specific maximum */
#ifndef gcConnectionIntervalMaxDefault_c
#define gcConnectionIntervalMaxDefault_c            (160)
#endif
#ifndef gcConnectionSlaveLatencyDefault_c
#define gcConnectionSlaveLatencyDefault_c           (0)
#endif
.........

ble_sig_defines.h

/*! GAP Peripheral Preferred Connection Parameters Characteristic UUID */
/* https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.bluetooth.characteristic.g... */
#define gBleSig_GapPpcp_d                       0x2A04
....
/*! GAP Peripheral Preferred Connection Parameters Characteristic Value Fields */
    #define gBleSig_GapPpcp_Bytes_d         8
    /* Minimum Connection Interval uint16    6    3200   connInterval_min = Minimum Connection Interval * 1.25 ms */
    #define gBleSig_GapPpcp_mci_d   0x0A, 0x00
    /* Maximum Connection Interval uint16    6    3200 */
    #define gBleSig_GapPpcp_Mci_d   0x10, 0x00
    /* Slave Latency   uint16    0    1000 */
    #define gBleSig_GapPpcp_SL_d    0x64, 0x00
    /* Connection Supervision Timeout Multiplier uint16    10    3200 */
    #define gBleSig_GapPpcp_cstm_d    0xE2, 0x04

used in gatt_db.h

PRIMARY_SERVICE(service_gap, gBleSig_GenericAccessProfile_d)
....
CHARACTERISTIC(char_ppcp, gBleSig_GapPpcp_d, (gGattCharPropRead_c) )
VALUE(value_ppcp, gBleSig_GapPpcp_d, (gPermissionFlagReadable_c), gBleSig_GapPpcp_Bytes_d ,gBleSig_GapPpcp_mci_d, gBleSig_GapPpcp_Mci_d, gBleSig_GapPpcp_SL_d, gBleSig_GapPpcp_cstm_d)

0x1800 GenericAccessProfile

0x2A04 Peripheral Preferred Connection Parameters Characteristic

lucianfiran · ‎03-11-2017

25.OtapBootloader.h

\KW40Z_Connectivity_Software_1.0.1\ConnSw\framework\Bootloader\Bootloader_OTAP_Serial\src\OtapSerialBootloader\OtapBootloader.h

the comment, description here is nok

/*
* Name: gFlashProtection_c
* Description: The value for FPROT register. By default the Flash is not Protected
*/
#ifndef gSerialBootloaderEnable_c
#define gSerialBootloaderEnable_c FALSE
#endif

should be

/*
* Name: gSerialBootloaderEnable_c
* Description: Enable UART Serial bootloader. By default FALSE.
*/

lucianfiran · ‎03-16-2017

26. TimersManager.h

review TMR_StopTimer + TMR_GetRemainingTime behaviour

some defines to add
#define TmrSecondsToMiliseconds( n ) ( ((n) / 1000) )
#define TmrMilisecondsToSeconds( n ) ( ((n) * 1000) )

lucianfiran · ‎03-29-2017

27. fsl_tsi_driver.h

TSI_DRV_Recalibrate second parameter expected to be uint16_t

Kinetis SDK v.1.3 API Reference Manual

KSDK13APIRM Rev. 0 Sept 2015

page 2655

tsi_status_t TSI_DRV_Recalibrate (uint32_t instance, uint32_t ∗lowestSignal)

Automatically recalibrates all important TSI settings.

page 2664

70.7.7.12 tsi_status_t TSI_DRV_Recalibrate ( uint32_t instance, uint32_t ∗ lowestSignal )
This function forces the driver to start the recalibration procedure for the current operation mode to get the
best possible TSI hardware settings. The computed setting is stored into the operation mode data and can
be loaded and saved by the TSI_DRV_LoadConfiguration or the TSI_DRV_SaveConfiguration functions.
Warning
The function could take more time to return and is blocking.
// Recalibrate current mode

uint16_t signal;
// Recalibrate the mode to get the best performance for this one electrode
if(TSI_DRV_Recalibrate(0, &signal) != kStatus_TSI_Success)
{
// Error, the TSI driver can’t recalibrate this mode
}

platform/drivers/inc/fsl_tsi_driver.h

/*!
* @brief Automatically recalibrates all important TSI settings.
*
* This function forces the driver to start the recalibration procedure
*           for the current operation mode to get the best possible TSI hardware settings.
*           The computed setting is stored into the operation mode data and can be
*           loaded and saved by the @ref TSI_DRV_LoadConfiguration or the @ref TSI_DRV_SaveConfiguration
*           functions.
*
* @warning The function could take more time to return
*          and is blocking.
*
@code
    // Recalibrate current mode
    uint16_t signal;
    // Recalibrate the mode to get the best performance for this one electrode
    if(TSI_DRV_Recalibrate(0, &signal) != kStatus_TSI_Success)
    {
        // Error, the TSI driver can't recalibrate this mode
    }
@endcode
* @param instance   The TSI module instance.
* @param lowestSignal       The pointer to variable where will be store the lowest signal of all electrodes
* @return An error code or kStatus_TSI_Success.
*/
tsi_status_t TSI_DRV_Recalibrate(uint32_t instance, uint32_t * lowestSignal);

lucianfiran · ‎03-29-2017

28. framework uses uint32_t instance as first parameter to multiple functions.

As used values for the hw instances don't go after before 3 (like TPM), some are 0 like TSI,...

For speed issue might be 32bits, but to improve memory usage I'll recommend uint8_t.

lucianfiran · ‎03-04-2017

19. FwkInit.c

#ifndef _FWKINIT_H_
#define _FWKINIT_H_
...
#endif /* _FWKINIT_ */

It does not have separate h, but h looks.

#if defined(FWK_SMALL_RAM_CONFIG)
better be changed to
#if (FWK_SMALL_RAM_CONFIG)

so FWK_SMALL_RAM_CONFIG can be set to 0 or 1 (or TRUE/FALSE) easily in app_preinclude.h

* TIMER implementation file for the ARM CORTEX-M4 processor
should be ARM CORTEX-M0+

lucianfiran · ‎03-04-2017

20. TimersManager.c

why if 0

#if 0
   /*
   * NAME: PE_LDD_DeviceDataList
   * DESCRIPTION: Array of initialized device structures of LDD components.
   * VALUES:
   */
   LDD_TDeviceData *PE_LDD_DeviceDataList[] = {NULL};
#endif

why multiple #if defined(FWK_SMALL_RAM_CONFIG) and
#if !defined(FWK_SMALL_RAM_CONFIG)

maybe
#if (FWK_SMALL_RAM_CONFIG)

lucianfiran · ‎03-05-2017

21. app_preinclude.h

should contain all sw switches for

1.KW40Z_Connectivity_Software_1.0.1\ConnSw\framework

2. KW40Z_Connectivity_Software_1.0.1\ConnSw\bluetooth

selected by folder then by file:

Bootloader, Common, DCDC, Flash, FSCI, FunctionLib, GPIOIrq

Keyboard, LED, Lists, LowPower, MemManager, Messaging

ModuleInfo, MWS, NVM, OSAbstraction, OtaSupport, Panic

Reset, RNG, SecLib, SerialManager, Shell, TimersManager, XCVR

for bluetooth

controller, hci_transport, host, profiles, app

/*! *********************************************************************************
*    Framework Configuration
********************************************************************************** */

/*! *********************************************************************************
   *    Common
   ********************************************************************************** */
/* FwkInit.c */
/* Defines a smaller FWK configuration */
/* Range default not defined */
#define FWK_SMALL_RAM_CONFIG
..........
/*! *********************************************************************************
*     Bluetooth - BLE Stack Configuration
********************************************************************************** */

/*! *********************************************************************************
   *    controller
   ********************************************************************************** */
/*! *********************************************************************************
   *    hci_transport
   ********************************************************************************** */
    /* hci_transport.h*/
      #define APP_SERIAL_INTERFACE_TYPE      (gSerialMgrNone_c)
      #define APP_SERIAL_INTERFACE_INSTANCE (0)
........

lucianfiran · ‎03-06-2017

22. OtaSupport.c

void OTA_SetNewImageFlag(void)
{
........
        if( status != FTFx_OK )
        {
            return;
        }
        mNewImageReady = FALSE;
}

can be

    if( status == FTFx_OK )
   {
      mNewImageReady = FALSE;
   }

same

void OTA_ImageChunkReq(uint32_t offset, uint8_t len, uint16_t devId)
{
.....
    if( NULL == pPkt )
    {
        return;
    }
.....
}

and
void OTA_QueryImageReq(uint16_t devId, uint16_t manufacturer, uint16_t imgType, uint32_t fileVersion)

lucianfiran · ‎03-08-2017

23. implementation for uint24_t and int24_t

lucianfiran · ‎02-14-2017

2. BOARD_LTC_INSTANCE is defined in board.h and not in SecLib.h
add in SecLib.h
#ifndef BOARD_LTC_INSTANCE
#define BOARD_LTC_INSTANCE 0
#endif

lucianfiran · ‎02-14-2017

3. TimersManager.h missed osNumberOfTimers count

#define gTmrTotalTimers_c ( gTmrApplicationTimers_c + gTmrStackTimers_c )
should become (?):
gTmrTotalTimers_c ( gTmrApplicationTimers_c + gTmrStackTimers_c + osNumberOfTimers )

lucianfiran · ‎02-14-2017

4. app_preinclude.h total stack usage, overflow check (?), and how to dimension:

Stack Size values found in project:
#define gTmrTaskStackSize_c               500
#define gSerialTaskStackSize_c            1024
#define gMainThreadStackSize_c          1024
#define gControllerTaskStackSize_c       900
#define gHost_TaskStackSize_c             1300
#define gL2ca_TaskStackSize_c 600
#define gAppIdleTaskStackSize_c         (400)
#define gFwkCommonStackSize_c

#define gMacTaskStackSize_c 1280
#define gPhyTaskStackSize_c 600

#define USB_OTG_TASK_STACKSIZE             2000
#define USB_KHCI_TASK_STACKSIZE            3500
#define USB_KHCI_TASK_STACKSIZE            3500
#define USB_KHCI_TASK_STACKSIZE            3500
#define USB_KHCI_TASK_STACKSIZE            3500

lucianfiran · ‎02-14-2017

5. #ifdef gXcvrXtalTrimEnabled_d to #if (gXcvrXtalTrimEnabled_d) in KW4xXcvrDrv.c and KW3xXcvrDrv.c

#if (gXcvrXtalTrimEnabled_d)
#include "Flash_Adapter.h"
#endif
...
void XcvrInit ( radio_mode_t radioMode )
{
XcvrInit_ModeChg_Common(radioMode,FIRST_INIT);
#if (gXcvrXtalTrimEnabled_d)
if( 0xFFFFFFFF != gHardwareParameters.xtalTrim )
{
XcvrSetXtalTrim( (uint8_t)gHardwareParameters.xtalTrim );
}
#endif

lucianfiran · ‎02-14-2017

6. file Eeprom_Boot.h contains:
* \file Eeprom.h
...
#ifndef _EEPROM_H_
#define _EEPROM_H_
...
#endif /* _EEPROM_H_ */
expected
* \file Eeprom_Boot.h
...
#ifndef _EEPROM_BOOT_H_
#define _EEPROM_BOOT_H_
...
#endif /* _EEPROM_BOOT_H_ */

lucianfiran · ‎02-14-2017

7. Keyboard.h should include EmbeddedTypes.h if LED.h is not used
#ifndef _KEYBOARD_INTERFACE_H_
#define _KEYBOARD_INTERFACE_H_

#include "EmbeddedTypes.h"

lucianfiran · ‎02-14-2017

8. ble_sig_defines.h should include DESCRIPTOR support for gatt_db.h
https://www.bluetooth.com/specifications/gatt/descriptors
https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.bluetooth.descriptor.gatt....
example:
CHARACTERISTIC(char_temperature, gBleSig_Temperature_d, (gGattCharPropRead_c | gGattCharPropNotify_c))
   VALUE(value_temperature, gBleSig_Temperature_d, (gPermissionFlagReadable_c), 2, 0x00, 0xB4)
   DESCRIPTOR(desc_temperature, gBleSig_CharPresFormatDescriptor_d, (gPermissionFlagReadable_c), gBleSig_CharPresFormatDescriptorBytes_d, gBleSig_signed_16_bit_integer_d, gBleSig_Exponent_neg2_d, gBleSig_Celsius_temperature_d, gBleSig_No_Namespaces_d, gBleSig_unknown_d)
   CCCD(cccd_core_temperature)

based on:
/* DESCRIPTOR(.., gBleSig_CharPresFormatDescriptor_d, .., 7, Format, Exponent, Unit, Namespace, Description)
* https://www.bluetooth.com/specifications/gatt/viewer?attributeXmlFile=org.bluetooth.descriptor.gatt....
* The actual value = Characteristic Value * 10^Exponent. */
#define gBleSig_CharPresFormatDescriptorBytes_d         7
/* Format Mandatory 8bit    range 0x00    0x1B */
/* https://www.bluetooth.com/specifications/assigned-numbers/format-types */
    /* 4    unsigned 8-bit integer */
    #define gBleSig_unsigned_8_bit_integer   0x04
    /* 14    signed 16-bit integer */
    #define gBleSig_signed_16_bit_integer_d 0x0E

/* Exponent Mandatory sint8    -128 0 127 */
    /* 10^Exponent: 0 =1 */
    #define gBleSig_Exponent_0_d    0x00
    /* 10^Exponent: -2 */
    #define gBleSig_Exponent_neg2_d 0xFE
    /* 10^Exponent: -3 */
    #define gBleSig_Exponent_neg3_d 0xFD

/* Unit The Unit is a UUID. Mandatory uint16    N/A    N/A */
/* https://www.bluetooth.com/specifications/assigned-numbers/units */
    /* 0x2700   unitless   org.bluetooth.unit.unitless */
    #define gBleSig_unitless_d                       0x00, 0x27
    /* 0x272F   Celsius temperature (degree Celsius)   org.bluetooth.unit.thermodynamic_temperature.degree_celsius */
    #define gBleSig_Celsius_temperature_d            0x2F, 0x27
    /* 0x2728   electric potential difference (volt)   org.bluetooth.unit.electric_potential_difference.volt */
    #define gBleSig_electric_potential_difference_d 0x28, 0x27

/* Namespace (organization) Mandatory 8bit    0    1 */
    /* 0    No Namespace */
    #define gBleSig_No_Namespaces_d                   0x00
    /* 1    Bluetooth SIG Assigned Numbers */
    #define gBleSig_Bluetooth_SIG_Assigned_Numbers_d 0x01

/* Description Name Space field. Mandatory 16bit    N/A    N/A    */
/* https://www.bluetooth.com/specifications/assigned-numbers/gatt-namespace-descriptors */
    /* unknown   0x0000 */
    #define gBleSig_unknown_d 0x00, 0x00

lucianfiran · ‎02-14-2017

9. LED inverse output setting (LED1..LED4)
LED.h
/*
* Name: mLEDInvertedOut
* Description: Led outpus inverted. Default value TRUE.
*               TRUE
*                LedOn -> GPIO_DRV_ClearPinOutput(kGpioLEDx)
*                LedOff -> GPIO_DRV_SetPinOutput(kGpioLEDx)
*               FALSE
*                LedOn   -> GPIO_DRV_SetPinOutput(kGpioLEDx)
*                LedOff -> GPIO_DRV_ClearPinOutput(kGpioLEDx)
*/
#ifndef mLEDInvertedOut_c
#define mLEDInvertedOut_c                TRUE
#endif

LED.c
void LED_Operate
...
case gLedOn_c:
   #if (mLEDInvertedOut_c)
      GPIO_DRV_ClearPinOutput(kGpioLED1);
   #else
      GPIO_DRV_SetPinOutput(kGpioLED1);
   #endif /* mLEDInvertedOut_c */
   break;
case gLedOff_c:
   #if (mLEDInvertedOut_c)
      GPIO_DRV_SetPinOutput(kGpioLED1);
   #else
      GPIO_DRV_ClearPinOutput(kGpioLED1);
   #endif /* mLEDInvertedOut_c */
   break;
....

* \file LED.h
* LED export interface file for ARM CORTEX-M4 processor
should be updated to ARM Cortex-M0+

lucianfiran · ‎02-16-2017

10. missing _FWKINIT_H_ in FwkInit.c
should add:

...
#ifndef _FWKINIT_H_
#define _FWKINIT_H_
...
#endif /* _FWKINIT_ */

and
* TIMER implementation file for the ARM CORTEX-M4 processor
should be:
* TIMER implementation file for the ARM CORTEX-M0+ processor

KW40Z_Connectivity_Software_1.0.1 review

KW40Z_Connectivity_Software_1.0.1 review

General