CC2530 ZNP Mini Kit - bug in znp_interface.c

Kristof Kovats

Other Parts Discussed in Thread: CC2530

Hi All !

I think i have found a bug in znp_interface.c that comes with CC2530 ZNP Mini Kit.

In the function "setSecurityKey" starting at line 555:

void setSecurityKey(unsigned char* key)
{
#ifdef ZNP_INTERFACE_VERBOSE     
    printf("Setting Security Key: ");
    printHexBytes(key, ZCD_NV_PRECFGKEY_LEN);
#endif
    znpBuf[0] = ZB_WRITE_CONFIGURATION_LEN + ZCD_NV_PRECFGKEY_LEN;
    znpBuf[1] = MSB(ZB_WRITE_CONFIGURATION);
    znpBuf[2] = LSB(ZB_WRITE_CONFIGURATION);    
    
    znpBuf[3] = ZCD_NV_PRECFGKEY;
    znpBuf[4] = ZCD_NV_PRECFGKEY_LEN;
    
    for (int i=5; i<(ZCD_NV_PRECFGKEY_LEN+5); i++)
        znpBuf[i] = key[i];            
    znpResult = sendMessage();
}

The security key is copied to the transfer buffer from the 5th index. The security code itself is

 only 16 bytes long, so with the above code the transfer buffer gets 5 bytes long garbage from the memory..

Otherwise this code is great help for developing applications !

Bye,

Kristof

over 13 years ago

Dirty Harry over 13 years ago

TI__Mastermind 19350 points

Nice catch, Kristof! What did you do to fix this, just use and increment the function parameter like this:

for (int i=5; i<(ZCD_NV_PRECFGKEY_LEN+5); i++)
znpBuf[i] = *key++;

I also tried this:

for (int i=5; i<(ZCD_NV_PRECFGKEY_LEN+5); i++)
znpBuf[i] = key[i-5];

but the output code is 4 bytes bigger. And this is 6 bytes bigger:

for (int i=5, j=0; i<(ZCD_NV_PRECFGKEY_LEN+5); i++, j++)
znpBuf[i] = key[j];

But this is 6 bytes SMALLER than the original, buggy code, and the most efficient fix above:

memcpy(znpBuf+5, key, ZCD_NV_PRECFGKEY_LEN);

Kristof Kovats over 13 years ago in reply to Dirty Harry

Intellectual 310 points

Thanks, Harry ! Without your proposed modifications the

preconfigured security key only works when you are lucky :)

I will use your last solution !

Regards,

Kristof

Dirty Harry over 13 years ago in reply to Kristof Kovats

TI__Mastermind 19350 points

Awesome. And as a little follow-up, I had played with the function a little more and achieved a total code size savings of 26 bytes with the following. Note that it comes at a cost of 5 bytes of constant memory though:

void setSecurityKey(unsigned char* key)
{
#ifdef ZNP_INTERFACE_VERBOSE
    printf("Setting Security Key: ");
    printHexBytes(key, ZCD_NV_PRECFGKEY_LEN);
#endif
static const char setSecKeyHdr[] = {
    ZB_WRITE_CONFIGURATION_LEN + ZCD_NV_PRECFGKEY_LEN,
    MSB(ZB_WRITE_CONFIGURATION),
    LSB(ZB_WRITE_CONFIGURATION),

ZCD_NV_PRECFGKEY,
ZCD_NV_PRECFGKEY_LEN
};

key = memcpy(znpBuf+sizeof(setSecKeyHdr), key, ZCD_NV_PRECFGKEY_LEN);
memcpy(key-sizeof(setSecKeyHdr), setSecKeyHdr, sizeof(setSecKeyHdr));

znpResult = sendMessage();
}

Derek over 13 years ago in reply to Dirty Harry

Mastermind 9812 points

Good catch in the code - my bad. I like the previous solution that doesn't use a struct (just uses memcpy) as it's easier to maintain. Since the amount of flash in the MSP430s is quite large compared to the cost of the chip, shaving bytes is unnecessary nowadays - saving 1B in flash on a typical 10k production run only saves a total of 13 cents. That's not 13 cents per device, that's 13 cents for all 10k devices. Compare that with the added loaded cost of a programmer at $100/hour over the lifetime of the project and you can see why many times we optimize for maintainability. But you get points for being creative, Harry. :)

--Derek (the guy who wrote said buggy code)

Zigbee & Thread

Zigbee & Thread forum

CC2530 ZNP Mini Kit - bug in znp_interface.c