search

ARMmbed / mbed-os

Arm Mbed OS is a platform operating system designed for the internet of things
https://mbed.com
Other
4.67k stars 2.98k forks source link

mbed OS 5.3.5 - Blinky does not work on xdot_l151cc target #3823

Closed janjongboom closed 7 years ago

janjongboom commented 7 years ago

Note: This is just a template, so feel free to use/remove the unnecessary things

Description

Bug

Target xdot_l151cc

Toolchain: GCC_ARM

Toolchain version: 4.9.3

mbed-cli version: (mbed --version)

meed-os sha: bcf7085d85b2811b5d68bdda192c754eadfb8f88

Behavior:

Run mbed-os-example-blinky. Expected: LED1 blinks.

Actual: LED1 does not blink. LED stays on.

If you switch to mbed-os-5.3.1 it works...

cc @andcor02 @mfiore02

mfiore02 commented 7 years ago

I hooked up a debugger: the device is not locking up, but the write(0) isn't succeeding in changing the state of the pin.

Looks like the migration to a common GPIO source for all ST parts broke the behavior of gpio_write for the xDot and other L15XXC devices. I'm a little surprised this change wasn't tested more thoroughly.

mbed-os-5.3.1 implementation:

static inline void gpio_write(gpio_t *obj, int value)
{
    MBED_ASSERT(obj->pin != (PinName)NC);
    if (value) {
        *obj->reg_set = obj->mask;
    } else {
#if defined(TARGET_STM32L152RC) || defined(TARGET_STM32L151RC) || defined (TARGET_STM32L151CC)
        *obj->reg_set = obj->mask << 16;
#else
        *obj->reg_clr = obj->mask;
#endif
    }
}

mbed-os-5.3.5 implementation:

static inline void gpio_write(gpio_t *obj, int value)
{
    MBED_ASSERT(obj->pin != (PinName)NC);
    if (value) {
        *obj->reg_set = obj->mask;
    } else {
#ifdef GPIO_IP_WITHOUT_BRR
        *obj->reg_clr = obj->mask << 16;
#else
        *obj->reg_clr = obj->mask;
#endif
    }
}

This looks like a perfect spot for a new unit test - verify that GPIO outputs & inputs are functional.

0xc0170 commented 7 years ago

This looks like a perfect spot for a new unit test - verify that GPIO outputs & inputs are functional.

+1

Would this be satisfactory or can be improved: https://github.com/ARMmbed/ci-test-shield/blob/master/TESTS/API/DigitalIO/DigitalIO.cpp

cc @bcostm @adustm @LMESTM @jeromecoutant

mfiore02 commented 7 years ago

@0xc0170 LGTM. Love the use of templates to keep it clean!

bcostm commented 7 years ago

@0xc0170 please change label to devices:multitech

0xc0170 commented 7 years ago

@mfiore02 Can you send a fix ?

@0xc0170 please change label to devices:multitech

I changed however as the above message indicates, the issue is in the common target code?

bcostm commented 7 years ago

Since PR #3665, the #define GPIO_IP_WITHOUT_BRR must be set for devices which don't have the BRR register. The STM32L151 has the BRR register (following datasheet) and the #define GPIO_IP_WITHOUT_BRR is NOT defined, so everything is correct to me.

I double checked the blinky LED example on the NUCLEO_L152RE (same family) and it is OK.

I tested again the ci-test digitalIO test and also OK:

+-----------------------+---------------+---------------------+----------------------------+--------+--------+--------+--------------------+
| target                | platform_name | test suite          | test case                  | passed | failed | result | elapsed_time (sec) |
+-----------------------+---------------+---------------------+----------------------------+--------+--------+--------+--------------------+
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-api-digitalio | Digital I/O on DIO_2/DIO_3 | 1      | 0      | OK     | 0.06               |
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-api-digitalio | Digital I/O on DIO_3/DIO_2 | 1      | 0      | OK     | 0.04               |
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-api-digitalio | Digital I/O on DIO_4/DIO_5 | 1      | 0      | OK     | 0.05               |
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-api-digitalio | Digital I/O on DIO_5/DIO_4 | 1      | 0      | OK     | 0.05               |
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-api-digitalio | Digital I/O on DIO_6/DIO_7 | 1      | 0      | OK     | 0.05               |
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-api-digitalio | Digital I/O on DIO_7/DIO_6 | 1      | 0      | OK     | 0.07               |
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-api-digitalio | Digital I/O on DIO_8/DIO_9 | 1      | 0      | OK     | 0.05               |
| NUCLEO_L152RE-GCC_ARM | NUCLEO_L152RE | tests-api-digitalio | Digital I/O on DIO_9/DIO_8 | 1      | 0      | OK     | 0.05               |
+-----------------------+---------------+---------------------+----------------------------+--------+--------+--------+--------------------+
mbedgt: test case results: 8 OK

I don't see where the error could be on your device ???

mfiore02 commented 7 years ago

@bcostm I agree that everything looks great on the L152RE. I added some debug logging to the app and the gpio driver:

#include "mbed.h"

DigitalOut led1(LED1);

// main() runs in its own thread in the OS
// (note the calls to wait below for delays)
int main() {
    int val;
    while (true) {
        val = led1.read();
        printf("led = %d\r\n\r\n", val);

        led1 = !led1;

        val = led1.read();
        printf("led = %d\r\n\r\n", val);

        wait(5);
    }
}
static inline void gpio_write(gpio_t *obj, int value)
{
    MBED_ASSERT(obj->pin != (PinName)NC);
    printf("writing IO\r\n");
    printf("value           %d\r\n", value);
    printf("obj->reg_set    %08X\r\n", obj->reg_set);
    printf("obj->reg_clr    %08X\r\n", obj->reg_clr);
    printf("obj->mask       %08X\r\n\r\n", obj->mask);
    if (value) {
        *obj->reg_set = obj->mask;
    } else {
#ifdef GPIO_IP_WITHOUT_BRR
        *obj->reg_clr = obj->mask << 16;
#else
        *obj->reg_clr = obj->mask;
#endif
    }
}

static inline int gpio_read(gpio_t *obj)
{
    MBED_ASSERT(obj->pin != (PinName)NC);
    printf("reading IO\r\n");
    printf("obj->reg_in     %08X\r\n", obj->reg_in);
    printf("*obj->reg_in    %08X\r\n", *obj->reg_in);
    printf("obj->mask       %08X\r\n\r\n", obj->mask);
    return ((*obj->reg_in & obj->mask) ? 1 : 0);
}

I ran this code on both the xDot and the Nucleo-L152RE. The Nucleo worked perfectly, but the xDot did not. It looks like the writes to GPIOx->BRR are not having any effect.

This snippet is from the xDot's debug output.

writing IO
value           0
obj->reg_set    40020018
obj->reg_clr    40020028
obj->mask       00000010

reading IO
obj->reg_in     40020010
*obj->reg_in    0000E608
obj->mask       00000010

led = 0

reading IO
obj->reg_in     40020010
*obj->reg_in    0000E608
obj->mask       00000010

writing IO
value           1
obj->reg_set    40020018
obj->reg_clr    40020028
obj->mask       00000010

reading IO
obj->reg_in     40020010
*obj->reg_in    0000E618
obj->mask       00000010

led = 1

reading IO
obj->reg_in     40020010
*obj->reg_in    0000E618
obj->mask       00000010

led = 1

reading IO
obj->reg_in     40020010
*obj->reg_in    0000E618
obj->mask       00000010

writing IO
value           0
obj->reg_set    40020018
obj->reg_clr    40020028
obj->mask       00000010

reading IO
obj->reg_in     40020010
*obj->reg_in    0000E618
obj->mask       00000010

led = 1

The value of the IO doesn't change with writes to GPIOx->BRR after it's been set using GPIOx->BSRR.

However, the Nucleo works perfectly.

writing IO
value           0
obj->reg_set    40020018
obj->reg_clr    40020028
obj->mask       00000020

reading IO
obj->reg_in     40020010
*obj->reg_in    0000B808
obj->mask       00000020

led = 0

reading IO
obj->reg_in     40020010
*obj->reg_in    0000B808
obj->mask       00000020

writing IO
value           1
obj->reg_set    40020018
obj->reg_clr    40020028
obj->mask       00000020

reading IO
obj->reg_in     40020010
*obj->reg_in    0000B828
obj->mask       00000020

led = 1

reading IO
obj->reg_in     40020010
*obj->reg_in    0000B828
obj->mask       00000020

led = 1

reading IO
obj->reg_in     40020010
*obj->reg_in    0000B828
obj->mask       00000020

writing IO
value           0
obj->reg_set    40020018
obj->reg_clr    40020028
obj->mask       00000020

reading IO
obj->reg_in     40020010
*obj->reg_in    0000B808
obj->mask       00000020

led = 0

reading IO
obj->reg_in     40020010
*obj->reg_in    0000B808
obj->mask       00000020

led = 0

I didn't see anything about the BRR in the STM32L151CC Errata document.

If I define GPIO_IP_WITHOUT_BRR for the L1 devices, the blinky example works properly again for the xDot.

Is there any advantage in using GPIOx->BRR instead of GPIOx->BSRR to write 0s to GPIO pins if the register is available? Nothing I've seen has given me that impression. My understanding is that both registers provide atomic GPIO operations. Some discussion here.

Why don't we toss the GPIO_IP_WITHOUT_BRR macro completely and use GPIOx->BSRR for all ST devices? It would simplify the codebase and resolve the issue for the xDot. I'd be happy to make a PR with the change. Is there a downside to that solution?

bcostm commented 7 years ago

It looks like the BRR register is not present in the L151CC device despite the fact that it is written in its datasheet. Very strange, I will check internally in ST.

Concerning the usage of BSRR, the only difference I can see is the addition of a 16-bits shift. This will slower the performances.

I think the best is to add the GPIO_IP_WITHOUT_BRR only for platforms using L151xx devices. I suggest to add it in the objects.h file. What do you think ?

mfiore02 commented 7 years ago

I have in fact already tested that solution for the xDot and it works like a charm. The Nucleo works fine as is. I'm not sure if TARGET_MOTE_L152RC needs the fix. I'd assume TARGET_NZ32_SC151 does as it appears to be a L151CC as well.

I'm happy to include either of these boards in my PR if they need the fix, but I'm not sure if they do since I don't have them to test. Can somebody let me know?

mfiore02 commented 7 years ago

@0xc0170 Are the ci-test-shield unit tests you referenced above part of the validation of a mbed-os release for all platforms?