W60x-I2C - fixed bug in machine_hard_i2c_transfer()

[e-tobi]

parenthesis-typo caused tls_reg_read() to hang on invalid register

[robert-hh]

Looks better this way.

[robert-hh]

I tested that again and it works. I'm sure that I tested it before. Just as a hint: I use the repository here just as a "backport" place. Usually I maintain a repo here: https://github.com/robert-hh/micropython/tree/w60x
It is linked to the up-to-date version of MicroPython and has for instance uasyncio included, but threading excluded. The latter seems not to work. Still on my to-do list. Anyhow, uasyncio is faster and more efficient.

[e-tobi]

I've created a PR in your fork as well.
What exactly is the "official" MP W60x repository then? Thhis one from WinnerMicro or yours?
uasyncio is cool - I'll test this.

Which SDK version are you using? I test with G3.02. With G3.04 I can't make it compile in the 1M flash version.

Currently I run into problems getting OneWire to work, because the W60x-SDK has no "real" OPEN_DRAIN mode. I'm working on this right now.

[robert-hh]

The "Official one" is the one from Winner Micro. I'm using my fork because I prefer to have a recent Micropython version. I changed it already to match your PR form yesterday.
I am using G3.04. SDK. I have a copy here with a few minor modifications. https://github.com/robert-hh/WM_SDK_W60X. I avoid the 1M flash problem by modifying the loader file. tools/link_w600_1m.ld

  FLASH (rx) : ORIGIN = 0x8010100, LENGTH = 600K /* 600KB-256B */

That way you cannot use the compressed flash method any more. You can then also increase the size of the flash FS.

By modifying the flash script you can also prevent the file system to be erased all the time change:
./${DLIMG} -c ${SERIAL_NAME} -ds 2M -dl "${BUILD}/${TARGET}.fls" -eo all -rs rts
into
./${DLIMG} -c ${SERIAL_NAME} -ds 2M -dl "${BUILD}/${TARGET}.fls" -eo secboot -rs rts

About Onewire: the software version of I2C has the same problems. It uses the macros from include/mphalport.h:

static inline void mp_hal_pin_open_drain(mp_hal_pin_obj_t pin) {
    tls_gpio_cfg(pin, WM_GPIO_DIR_INPUT, WM_GPIO_ATTR_PULLHIGH);
}
static inline void mp_hal_pin_od_low(mp_hal_pin_obj_t pin) {
    tls_gpio_cfg(pin, WM_GPIO_DIR_OUTPUT, WM_GPIO_ATTR_PULLHIGH);
    tls_gpio_write(pin, 0);
}
static inline void mp_hal_pin_od_high(mp_hal_pin_obj_t pin) {
    tls_gpio_cfg(pin, WM_GPIO_DIR_INPUT, WM_GPIO_ATTR_PULLHIGH);
    tls_gpio_write(pin, 1);
}

[robert-hh]

I managed to get some reading from a DS18B20 sensor using a modified modonewire.c file (copy below). That's just the working state. The biggest problem is, that the timing of mp_hal_delay_us() is wrong. As a first hack, I modified the values in modonewire.c to tell, whether it would work then. Next is to fix mp_hal_delay_us(). I moved modonewire.c into the modules folder of the W60x port.
onewire.py and ds18x20.py are the ones from micropython/drivers/onewire. The test script is the one from the docs.

/*
 * This file is part of the MicroPython project, http://micropython.org/
 *
 * The MIT License (MIT)
 *
 * Copyright (c) 2015-2017 Damien P. George
 *
 * Permission is hereby granted, free of charge, to any person obtaining a copy
 * of this software and associated documentation files (the "Software"), to deal
 * in the Software without restriction, including without limitation the rights
 * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
 * copies of the Software, and to permit persons to whom the Software is
 * furnished to do so, subject to the following conditions:
 *
 * The above copyright notice and this permission notice shall be included in
 * all copies or substantial portions of the Software.
 *
 * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
 * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
 * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
 * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
 * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 * THE SOFTWARE.
 */

#include <stdio.h>
#include <stdint.h>

#include "py/obj.h"
#include "py/mphal.h"

#include "mphalport.h"
#include "wm_io.h"
#include "wm_gpio.h"
/******************************************************************************/
// Low-level 1-Wire routines

#define TIMING_RESET1 (190) // (480)
#define TIMING_RESET2 (35) // (70)
#define TIMING_RESET3 (150) // (410)
#define TIMING_READ1 (1) // (5)
#define TIMING_READ2 (1) // (5)
#define TIMING_READ3 (20) // (40)
#define TIMING_WRITE1 (3) / (10)
#define TIMING_WRITE2 (20) // (50)
#define TIMING_WRITE3 (4) // (10)

STATIC int onewire_bus_reset(mp_hal_pin_obj_t pin) {
    mp_hal_pin_od_low(pin);
    mp_hal_delay_us(TIMING_RESET1);
    uint32_t i = mp_hal_quiet_timing_enter();
    mp_hal_pin_od_high(pin);
    mp_hal_delay_us_fast(TIMING_RESET2);
    int status = !mp_hal_pin_read(pin);
    mp_hal_quiet_timing_exit(i);
    mp_hal_delay_us(TIMING_RESET3);
    return status;
}

STATIC int onewire_bus_readbit(mp_hal_pin_obj_t pin) {
    mp_hal_pin_od_high(pin);
    uint32_t i = mp_hal_quiet_timing_enter();
    mp_hal_pin_od_low(pin);
    mp_hal_delay_us_fast(TIMING_READ1);
    mp_hal_pin_od_high(pin);
    mp_hal_delay_us_fast(TIMING_READ2);
    int value = mp_hal_pin_read(pin);
    mp_hal_quiet_timing_exit(i);
    mp_hal_delay_us_fast(TIMING_READ3);
    return value;
}

STATIC void onewire_bus_writebit(mp_hal_pin_obj_t pin, int value) {
    uint32_t i = mp_hal_quiet_timing_enter();
    mp_hal_pin_od_low(pin);
    mp_hal_delay_us_fast(TIMING_WRITE1);
    if (value) {
        mp_hal_pin_od_high(pin);
    }
    mp_hal_delay_us_fast(TIMING_WRITE2);
    mp_hal_pin_od_high(pin);
    mp_hal_delay_us_fast(TIMING_WRITE3);
    mp_hal_quiet_timing_exit(i);
}

/******************************************************************************/
// MicroPython bindings

STATIC mp_obj_t onewire_reset(mp_obj_t pin_in) {
    return mp_obj_new_bool(onewire_bus_reset(mp_hal_get_pin_obj(pin_in)));
}
STATIC MP_DEFINE_CONST_FUN_OBJ_1(onewire_reset_obj, onewire_reset);

STATIC mp_obj_t onewire_readbit(mp_obj_t pin_in) {
    return MP_OBJ_NEW_SMALL_INT(onewire_bus_readbit(mp_hal_get_pin_obj(pin_in)));
}
STATIC MP_DEFINE_CONST_FUN_OBJ_1(onewire_readbit_obj, onewire_readbit);

STATIC mp_obj_t onewire_readbyte(mp_obj_t pin_in) {
    mp_hal_pin_obj_t pin = mp_hal_get_pin_obj(pin_in);
    uint8_t value = 0;
    for (int i = 0; i < 8; ++i) {
        value |= onewire_bus_readbit(pin) << i;
    }
    return MP_OBJ_NEW_SMALL_INT(value);
}
STATIC MP_DEFINE_CONST_FUN_OBJ_1(onewire_readbyte_obj, onewire_readbyte);

STATIC mp_obj_t onewire_writebit(mp_obj_t pin_in, mp_obj_t value_in) {
    onewire_bus_writebit(mp_hal_get_pin_obj(pin_in), mp_obj_get_int(value_in));
    return mp_const_none;
}
STATIC MP_DEFINE_CONST_FUN_OBJ_2(onewire_writebit_obj, onewire_writebit);

STATIC mp_obj_t onewire_writebyte(mp_obj_t pin_in, mp_obj_t value_in) {
    mp_hal_pin_obj_t pin = mp_hal_get_pin_obj(pin_in);
    int value = mp_obj_get_int(value_in);
    for (int i = 0; i < 8; ++i) {
        onewire_bus_writebit(pin, value & 1);
        value >>= 1;
    }
    return mp_const_none;
}
STATIC MP_DEFINE_CONST_FUN_OBJ_2(onewire_writebyte_obj, onewire_writebyte);

STATIC mp_obj_t onewire_crc8(mp_obj_t data) {
    mp_buffer_info_t bufinfo;
    mp_get_buffer_raise(data, &bufinfo, MP_BUFFER_READ);
    uint8_t crc = 0;
    for (size_t i = 0; i < bufinfo.len; ++i) {
        uint8_t byte = ((uint8_t *)bufinfo.buf)[i];
        for (int b = 0; b < 8; ++b) {
            uint8_t fb_bit = (crc ^ byte) & 0x01;
            if (fb_bit == 0x01) {
                crc = crc ^ 0x18;
            }
            crc = (crc >> 1) & 0x7f;
            if (fb_bit == 0x01) {
                crc = crc | 0x80;
            }
            byte = byte >> 1;
        }
    }
    return MP_OBJ_NEW_SMALL_INT(crc);
}
STATIC MP_DEFINE_CONST_FUN_OBJ_1(onewire_crc8_obj, onewire_crc8);

STATIC const mp_rom_map_elem_t onewire_module_globals_table[] = {
    { MP_ROM_QSTR(MP_QSTR___name__), MP_ROM_QSTR(MP_QSTR_onewire) },

    { MP_ROM_QSTR(MP_QSTR_reset), MP_ROM_PTR(&onewire_reset_obj) },
    { MP_ROM_QSTR(MP_QSTR_readbit), MP_ROM_PTR(&onewire_readbit_obj) },
    { MP_ROM_QSTR(MP_QSTR_readbyte), MP_ROM_PTR(&onewire_readbyte_obj) },
    { MP_ROM_QSTR(MP_QSTR_writebit), MP_ROM_PTR(&onewire_writebit_obj) },
    { MP_ROM_QSTR(MP_QSTR_writebyte), MP_ROM_PTR(&onewire_writebyte_obj) },
    { MP_ROM_QSTR(MP_QSTR_crc8), MP_ROM_PTR(&onewire_crc8_obj) },
};

STATIC MP_DEFINE_CONST_DICT(onewire_module_globals, onewire_module_globals_table);

const mp_obj_module_t mp_module_onewire = {
    .base = { &mp_type_module },
    .globals = (mp_obj_dict_t *)&onewire_module_globals,
};

[e-tobi]

Thanks for your work!
I took a different route and implemented OPEN_DRAIN support directly into machine_pin.c. I think this might be the better way, so no libraries calling these functions need to be modified.
I'll provide a PR once I've tested everything.

I've also stumbled upon the timing problem. delay_us is way off - a simple loop with 2 x 500 µs delays resulted in a 390 Hz signal, while 1 kHz is expected. I fixed this by using tls_usleep in mphalport.c (will provide a PR for this as well).

Now I'm trying to get this all together working with the unmodified OneWire code from MP.

[robert-hh]

Implementing OPEN_DRAIN in the PIN class is surely a good thing. You have to store in the pin object the output mode, and then you can decide upon writing a 1, which way to go. That seems straightforward. But at a few files mp_hal_pin_write() is called, which

drivers/bus/softqspi.c
drivers/bus/softspi.c
drivers/memory/spiflash.c
extmod/modonewire.c

For these you have to extend mp_hal_pin_obj_t to carry the output mode too.

Using tls_usleep() instead of delay_us() does not work for me. After a few delays, the micro simply stalls.
B.t.w.: Do you know where delay_us is implemented? I looked for it and did not find it. But I may have bad eyes today.

[e-tobi]

delay_us is exported in the wlan-library (wlan.lib/wlan.a) -

[robert-hh]

OK. No wonder that I did not find it in the sources. And I did not look yet into the linker map (and ignored grep telling it).

[robert-hh1]

This change sneaked in as part of the most recent merges. So the PR can be closed.