lpcware

Bugs in Board Support Package for MCB1500 KEIL uVision 5.12.0.0?

Discussion created by lpcware Employee on Jun 15, 2016
Latest reply on Jun 15, 2016 by lpcware
Content originally posted in LPCWare by capiman on Sat Nov 08 09:25:37 MST 2014
There is code like the following in LED.c (which is brought by BSP from KEIL):

const unsigned long led_port[] = {       1,       1,       0,       1 };
const unsigned long led_mask[] = { 1UL<< 2, 1UL<< 3, 1UL<< 0, 1UL<< 0 };

void LED_Initialize (void) {

  LPC_SYSCON->SYSAHBCLKCTRL0 |= ((1UL << 14) | (1UL << 15));  /* enable clock for GPIO0 and GPIO1 port */

  /* configure GPIO as output */
  LPC_GPIO_PORT->DIR[[color=#6f0]0[/color]]  |= led_mask[2];
  LPC_GPIO_PORT->SET[[color=#6f0]0[/color]]  [color=#f00]|=[/color] led_mask[2];
  LPC_GPIO_PORT->DIR[[color=#6f0]1[/color]]  |= (led_mask[0] | led_mask[1] | led_mask[3]);
  LPC_GPIO_PORT->SET[[color=#6f0]1[/color]]  [color=#f00]|=[/color] (led_mask[0] | led_mask[1] | led_mask[3]);
}

void LED_On (unsigned int num) {

  if (num < LED_NUM) {
    LPC_GPIO_PORT->CLR[led_port[num]] [color=#f00]|=[/color] led_mask[num];
  }
}

void LED_Off (unsigned int num) {

  if (num < LED_NUM) {
    LPC_GPIO_PORT->SET[led_port[num]]  [color=#f00]=[/color] led_mask[num];
  }
}


Is it still like in earlier version that SET bits is not using "|=", but just "="?
So correct code is something like

void LED_Initialize (void) {

  LPC_SYSCON->SYSAHBCLKCTRL0 |= ((1UL << 14) | (1UL << 15));  /* enable clock for GPIO0 and GPIO1 port */

  /* configure GPIO as output */
  LPC_GPIO_PORT->DIR[0]  |= led_mask[2];
  LPC_GPIO_PORT->SET[0]  [color=#f00]=[/color] led_mask[2];
  LPC_GPIO_PORT->DIR[1]  |= (led_mask[0] | led_mask[1] | led_mask[3]);
  LPC_GPIO_PORT->SET[1]  [color=#f00]=[/color] (led_mask[0] | led_mask[1] | led_mask[3]);
}

void LED_On (unsigned int num) {

  if (num < LED_NUM) {
    LPC_GPIO_PORT->CLR[led_port[num]] [color=#f00] = [/color] led_mask[num];
  }
}

void LED_Off (unsigned int num) {

  if (num < LED_NUM) {
    LPC_GPIO_PORT->SET[led_port[num]]  [color=#f00] = [/color] led_mask[num];
  }
}

I have seen that SETP is meanwhile "R/W", but CLRP is still "WO", so I think at least when switching LED on by CLR this is wrong.
I have no MCB1500, so I cannot check.

PS: I have seen they have changed it with SET in LED_Off, where it would perhaps not be needed, but not changed in LED_On, where it is needed...

PS2:
Second pitfall: They have defined "led_port", but only used it in LED_On and LED_off, but not in LED_Initialize nor LED_Uninitialize.
So when changing values in "led_port" and "led_mask", it could be that the code does not work anymore...

PS3:
Why LED_NUM and size of array led_port/led_mask is not checked? If LED_NUM is too small, some LED cannot be controlled.
If it is too big, possible access behind array is allowed.

Perhaps following would be better:

#define LED_NUM     4                        /* Number of user LEDs          */

const unsigned long led_port[[color=#fc0]LED_NUM[/color]] = {       1,       1,       0,       1 };
const unsigned long led_mask[[color=#fc0]LED_NUM[/color]] = { 1UL<< 2, 1UL<< 3, 1UL<< 0, 1UL<< 0 };

Can you confirm my finding?

If these are errors: Is someone from KEIL reading here? Is NXP support forward it to KEIL or shall I do it myself?



Outcomes