nrf_gpio_pin_read and optimized code might return wrong value

tamisoft gravatar image

asked 2017-09-13 16:12:19 +0100

updated 2017-09-13 20:30:11 +0100

Hi, I have found an interesting behavior from this code:

nrf_gpio_cfg_input(7,NRF_GPIO_PIN_NOPULL);
if (nrf_gpio_pin_read(7)==0)
{
 NRF_LOG_INFO("ID1(P0.%d) is LOW.",7);
}else{
 NRF_LOG_INFO("ID1(P0.%d) is HIGH.",7);
}

the nrf_gpio_pin_read will always return 0 as if the pin was low, but the pin is in fact connected to 1.8V through a 10k resistor. Further investigating first added a delay_ms(1) before the if, and that fixed the issue, but still didn't make sense. Then rebuilt the code with

#pragma GCC push_options
#pragma GCC optimize ("O0")
...
#pragma GCC pop_options

around the gpio_init function and it was working without the delay. Started suspecting that maybe it is an analog issue, so we tried without the 10k resistor, but the behavior was the same.

After this we started dissecting the generated assembly code and found that with the default optimized code output has the str.w instruction (used to set the pin direction) directly followed by the read of the pin ldr.w. So started adding nops between the two and found that it won't work with one, but will work with 2 or more. The non-working one looks like this (this is with 1 nop, 0 no is the same without that):

   reg->PIN_CNF[pin_number] = ((uint32_t)dir << GPIO_PIN_CNF_DIR_Pos)
   2647e:       f04f 43a0       mov.w   r3, #1342177280 ; 0x50000000
   26482:       2200            movs    r2, #0
   //------------INTERESTING PART FROM HERE---------------
   **26484:       f8c3 271c       str.w   r2, [r3, #1820] ; 0x71c
        __asm__("nop");
   **26488:       bf00            nop
    return p_reg->IN;
   **2648a:       f8d3 2510       ldr.w   r2, [r3, #1296] ; 0x510
        if (nrf_gpio_pin_read(ID_PIN1)==0)
   2648e:       f012 0f80       tst.w   r2, #128        ; 0x80
        {
                NRF_LOG_INFO("ID1(P0.%d) is LOW.",ID_PIN1);
   26492:       6862            ldr     r2, [r4, #4]
        if (nrf_gpio_pin_read(ID_PIN1)==0)
   26494:       d10f            bne.n   264b6 <board_init+0x42>

And the working one is:

 26484:       f8c3 271c       str.w   r2, [r3, #1820] ; 0x71c
        __asm__("nop");
   26488:       bf00            nop
        __asm__("nop");
   2648a:       bf00            nop
    return p_reg->IN;
   2648c:       f8d3 2510       ldr.w   r2, [r3, #1296] ; 0x510
        if (nrf_gpio_pin_read(ID_PIN1)==0)
   26490:       f012 0f80       tst.w   r2, #128        ; 0x80
        {
                NRF_LOG_INFO("ID1(P0.%d) is LOW.",ID_PIN1);
   26494:       6862            ldr     r2, [r4, #4]
        if (nrf_gpio_pin_read(ID_PIN1)==0)
   26496:       d10f            bne.n   264b8 <board_init+0x44>

So it seems that if the direction change and the read gets within 3 clock cycles it will return the wrong value. An obvious fix of course is to make the assembly longer eg. by creating a volatile uint32_t zero=0 and make the if use that instead of "0", but I think this could be fixed in a nicer way (especially since this is not bullet proof). Is this a known behavior of the nRF52832? Should it be fixed in the SDK by simply adding 2 nops to the direction change static inline function?

Thanks Levente

edit retag flag offensive close delete report spam

Comments

I suppose your 10k resistor and the nRF52's pad capacitance can slow down the voltage rise time on the input just enough so that it doesn't reach a logic '1' fast enough for nrf_gpio_pin_read().

But before I report this to the SDK team, is your nRF52's supply voltage also 1.8V? To register as a logic 1 the input voltage must be higher than 0.7*Vdd (electrical GPIO specs).

Martin Børs-Lind ( 2017-09-14 13:08:18 +0100 )editconvert to answer

Yes, it is a 1.8V system. I mentioned above that we have suspected the capacitance so we have tried to tie the gpio directly to 1.8V as well. If you want we could try to replicate this on one of the nrf52 dks as well, but that might take some time.

Tamisoft ( 2017-09-14 15:12:16 +0100 )editconvert to answer

Can you try to add a "dummy instruction" after nrf_gpio_cfg_input()? Something like this:

nrf_gpio_cfg_input(7,NRF_GPIO_PIN_NOPULL);
(void)NRF_GPIO->OUT;
if (nrf_gpio_pin_read(7)==0)
    [....]

You can also use __ISB() eller __DMB(). The theory being that it might be because the nRF52's peripherals run at 16 MHz while the CPU runs at 64 MHz. It might be worth a shot to use one of these instructions to ensure that the CPU buffers are empty. Here is a somewhat sparse documentation of the issue.

Martin Børs-Lind ( 2017-09-19 08:19:09 +0100 )editconvert to answer

Thanks for the possible solutions, but would it be better to incorporate these into the SDK where it can cause undeterministic output? I mean everywhere else this just manifests as a slight latency (or jitter in it), but in the scenario where IO changes direction it leads to a wrong value read under certain conditions (like optimized enough code generated by the compiler that the two instructions follow closely). The idea about NRF_GPIO->OUT is the same as nops, one is not enough :). ISB and DMB could be a solution, I will test it later, but still I think it should be in the SDK, so from the user's point of view it is consistent.

Tamisoft ( 2017-09-19 08:48:23 +0100 )editconvert to answer

I tend to agree regarding incorporation in the SDK. However, the argument against it is that fixing this corner case will affect other implementations and that we should rather improve our documentation to help people decide on an implementation themselves. I have notified the SDK team so that they can look into it.

Please keep me posted.

Martin Børs-Lind ( 2017-09-19 10:39:10 +0100 )editconvert to answer

Will do! Do you want me to close this ticket, or the SDK team will write their response here?

Tamisoft ( 2017-09-19 11:10:48 +0100 )editconvert to answer

You can just leave it open and we will see what happens.

Martin Børs-Lind ( 2017-09-19 12:17:47 +0100 )editconvert to answer