ARMmbed / mbed-os

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

WIZwiki-W7500 timing issues #1524

Closed ghost closed 7 years ago

ghost commented 8 years ago

Hello!

I'm having issues performing correct timing for 1-Wire operations (such as reading a DS18B20 temperature sensor) on WIZwiki-W7500 platform. Other bitbanged protocols like the one in use for DHT11/22 sensors also seem to fail.

Take this code as example, since it explicits microseconds time delays: https://developer.mbed.org/forum/mbed/topic/101/?page=1#comment-829

On STM32 NUCLEO-F446RE and F303K8 it works just fine and the temperature is read correctly. Timing is also spot on, see this logic analyzer trace:

screenshot from 2016-02-03 17 36 31

On WIZwiki-W7500 I can't really say the same: screenshot from 2016-02-03 17 38 52

Could this be related to the choice of SystemCoreClock and internal Xtal in #1237, which has not been fixed?

Thank you for your help.

VladimirAkopyan commented 8 years ago

I have this problem too, OneWire protocol is unusable

ghost commented 8 years ago

Glad to know it's not just my problem. cc @hjjeon0608 ?

hjjeon0608 commented 8 years ago

Add the below code into top of main, and test one more plz. I think also because of clock speed. (volatile uint32_t )(0x41001014) = 0x0060100; //clock setting 48MH wait_ms(100);

Willem23 commented 8 years ago

Adding the above code to main will change the SystemCoreClock to the desired value. Note however that any objects that depend on this clock and that were declared as global before main() will now have incorrect timing settings. You need for example to explicitly call serial.baud(9600) to get the correct baudrate again, same for I2C, SPI bitrates etc. This wont be necessary when the systemclock is set to 48MHz as part of the regular system init which is automatically called before anything else.

hjjeon0608 commented 8 years ago

Serial clock is always hold, so you don't care of serial baud rate. I2C is used by GPIO, so I2C bit rates is dependent on operation clock not I2C setting clock. SPI bitrate is changed by setting function so don't care of operating clock.

hjjeon0608 commented 8 years ago

And I will change global clock as maximum clock ASAP.

ghost commented 8 years ago

Hello, I've run this simple code and I've taken a few more measurements:

#include "mbed.h"
DigitalOut out(D8);
int main(void) {
    while(1) {
        out = !out;
        wait_us(5);
    }
}

At 20 MHz SystemCoreClock a wait_us(5) is actually ~21uS: screenshot from 2016-02-18 11 10 11

At 48 MHz SystemCoreClock a wait_us(5) is ~10uS: screenshot from 2016-02-18 11 11 59

On a different platform, such as STM32F446RE, wait_us(5) is ~5uS: screenshot from 2016-02-18 11 14 52

I haven't digged through the code implementation of that delay (yet) but in my opinion it either means that:

In any case, bitbanged protocols (1-Wire, DHT sensors...) still fail.

Let me know if you need more tests.

VladimirAkopyan commented 8 years ago

Thanks for runnign tests lcafaro, I don't have a logic analyser, so all I can tell is that it doesn't work, but I don't have any way of knowing why. Could you sample different delays to know how the board reacts? In your original screenshot the board takes extra 71µs to react, but here it's 15 and 5, or a factor of 4 and 2. So it seems to be neither a constant offset, nor a liner overhead. What the hell?

hjjeon0608 commented 8 years ago

I had tested. First, I explain about wait function.

void wait_us(int us) {
    uint32_t start = us_ticker_read();
    while ((us_ticker_read() - start) < (uint32_t)us);
}

and us_ticker_read() is

uint32_t us_ticker_read() {
    if (!us_ticker_inited) us_ticker_init();
    return (TIMER_1->TCR);
}

TIMER_1-> TCR is timer register and equal to PWM_CH1->TCR.

And I made and tested absolute delay about function. Below function is equal(more fast) to the wait_us() function.

void wiz_wait_us(uint32_t us) {
    int start = PWM_CH1->TCR;
    while( (PWM_CH1->TCR - start) < us );
}

And I use value of us as 0. So the while is performed 1 time. main code is

int main() {
    *(volatile uint32_t *)(0x41001014) = 0x0060100; //clock setting 48MH
    while(1) {
        out = !out;
        wiz_wait_us(0);
    }
}

The result is 4.45us 2016-02-22 09 42 09

I think 4.45us is absolute delay. If use wait_us(100), the wait time is 105us. If use wait_us(50), the wait time is 55us. I think It is because gpio toggling time or performed time of instruction or access register. W7500 has M0 but STM32F446RE has M4. M4 performs faster than M0. So STM32F446RE can implemented more detailed.

ghost commented 8 years ago

@hjjeon0608 thanks for clarifying that.

In fact, I've done some more testing and I've found out that as you say the main issue is not the timer itself, but the GPIO toggling time, and the change in direction register.

1-Wire data is written by "tri-stating" the output so that the external resistor can pull up the line and make a logic high state, like this:

int main() {
  while(true) {
      *(volatile uint32_t *)(0x41001014) = 0x0060100;
      DigitalOut pin_out(D8);
      pin_out = 0;
      DigitalIn pin_in(D8);
    }
}

What's really slow is the re-declaration of DigitalOut and DigitalIn, which takes ~30uS. Using a single DigitalInOut is even slower (~48uS):

int main() {
  while(true) {
      *(volatile uint32_t *)(0x41001014) = 0x0060100;
      DigitalInOut pin(D8);
      pin.output();
      pin = 0;
      pin.input();
    }
}

Since the W7500 even at 48 MHz can't keep up with that, I guess our best option for faster GPIO toggling is to perform the operations directly via the gpio_mode(), gpio_dir() and gpio_set() primitives.

So either we re-write the code or find a simpler way out, such as hardware offloading the communication by the use of some peripheral that the W7500 has, such as SPI or I2C (I've tested the latter and it works just fine). This would mean replacing DS18B20/DHTxx sensors with something different and possibly more expensive though.

hjjeon0608 commented 8 years ago

pin.output() and pin.input() call the function of gpio_dir(). So both function is equal.

void output() {
    gpio_dir(&gpio, PIN_OUTPUT);
}
void input() {
    gpio_dir(&gpio, PIN_INPUT);
}

And DigitalOut is

DigitalOut(PinName pin) : gpio() {
    gpio_init_out(&gpio, pin);
}

And gpio_init_out(); is

static inline void _gpio_init_out(gpio_t* gpio, PinName pin, PinMode mode, int value)
{
    gpio_init(gpio, pin);
    if (pin != NC) {
        gpio_write(gpio, value);
        gpio_dir(gpio, PIN_OUTPUT);
        gpio_mode(gpio, mode);
    }
}

So

int main() {
  while(true) {
      *(volatile uint32_t *)(0x41001014) = 0x0060100;
      DigitalInOut pin(D8);
      pin.output();
      pin = 0;
      pin.input();
    }
}

it should be faster than

int main() {
  while(true) {
      *(volatile uint32_t *)(0x41001014) = 0x0060100;
      DigitalOut pin_out(D8);
      pin_out = 0;
      DigitalIn pin_in(D8);
    }
}
hjjeon0608 commented 8 years ago

If you want to up the speed, use W7500 driver not mbed library. #include "W7500x_gpio.h" To set output : void GPIO_OutEnSet(GPIO_TypeDef* GPIOx, uint16_t GPIO_Pin) To set input : void GPIO_OutEnClr(GPIO_TypeDef* GPIOx, uint16_t GPIO_Pin)

Example GPIO_OutEnSet(PAD_PA, GPIO_Pin_4); GPIO_OutEnClr(PAD_PC, GPIO_Pin_12); PAD group is A,B,C and D. But at mbed, D is not used. Pin number is 0 ~ 15.

VladimirAkopyan commented 8 years ago

Ok, thanks. And how fast is that?

hjjeon0608 commented 8 years ago

I don't check exact time but gpio_dir() function is

void gpio_dir(gpio_t *obj, PinDirection direction)
{
    MBED_ASSERT(obj->pin != (PinName)NC);
    obj->direction = direction;

    pin_function(obj->pin, WIZ_PIN_DATA(obj->direction, obj->mode, 1));
}

And pin_function() is

void pin_function(PinName pin, int data) {
    MBED_ASSERT(pin != (PinName)NC);
    // Get the pin informations
    uint32_t mode  = WIZ_PIN_MODE(data);
    uint32_t pupd  = WIZ_PIN_PUPD(data);
    uint32_t afnum = WIZ_PIN_AFNUM(data);

    uint32_t port_num = WIZ_PORT(pin);
    uint32_t pin_index  = WIZ_PIN_INDEX(pin);

    GPIO_TypeDef *gpio;

    // Configure Alternate Function
    // Warning: Must be done before the GPIO is initialized
    switch (afnum) {
        case 0:
            HAL_PAD_AFConfig((PAD_Type)port_num, (uint16_t)pin_index, (PAD_AF_TypeDef)Px_AFSR_AF0);
            break;
        case 1:
            HAL_PAD_AFConfig((PAD_Type)port_num, (uint16_t)pin_index, (PAD_AF_TypeDef)Px_AFSR_AF1);
            break;
        case 2:
            HAL_PAD_AFConfig((PAD_Type)port_num, (uint16_t)pin_index, (PAD_AF_TypeDef)Px_AFSR_AF2);
            break;
        case 3:
            HAL_PAD_AFConfig((PAD_Type)port_num, (uint16_t)pin_index, (PAD_AF_TypeDef)Px_AFSR_AF3);
            break;
        default:
            break;
    }

    if(mode == WIZ_MODE_AF)
        return;

    // Configure GPIO
    gpio = (GPIO_TypeDef *)Get_GPIO_BaseAddress(port_num);

    GPIO_InitTypeDef GPIO_InitStructure;
    GPIO_InitStructure.GPIO_Pin       = pin_index;
    GPIO_InitStructure.GPIO_Mode      = (GPIOMode_TypeDef)mode;
    GPIO_InitStructure.GPIO_Pad       = (GPIOPad_TypeDef)pupd;
    HAL_GPIO_Init(gpio, &GPIO_InitStructure);
}

So I guess just one register access time is faster. Maybe the time is a little fast.

VladimirAkopyan commented 8 years ago

So, has anybody tested if directly editing the register is an improvement? @hjjeon0608 , aren't interested to find out where it's getting delayed for unusually long amount of time?

ciarmcom commented 8 years ago

ARM Internal Ref: IOTMORF-220