external i2c fail (P)C0/1

[csBlueChip]

Describe the bug.

EVERY transaction on the i2c bus MUST be wrapped with

furi_hal_i2c_acquire();
furi_hal_i2c_ACTION();
furi_hal_i2c_release();

Where ACTION = {device_ready, tx, rx, trx}

If you attempt a second transaction without this wrapping, it will timeout and fail.

This is especially bad for furi_hal_i2c_trx(); which performs TWO transactions {Tx(), Rx()}, and therefore ALWAYS fails. The workaround for this is trivial, just call Tx() and Rx() yourself - do NOT use the TRx() wrapper.

Reproduction

Write a plugin that will call a function when the OK button is pressed
Run the plugin and press the OK button

The function may be:

FuriHalI2cBusHandle*  bus     = &furi_hal_i2c_handle_external;
uint8_t               addr    = 0x52 <<1;
uint32_t              timeout = 1000;
uint8_t               cmd[]   = {0xf0, 0x00};
uint8_t               res;

void fail_tx (void) {
    furi_hal_i2c_acquire(bus);
        furi_hal_i2c_tx(bus, addr, cmd, sizeof(cmd), timeout);
        furi_hal_i2c_tx(bus, addr, cmd, sizeof(cmd), timeout);
    furi_hal_i2c_release(bus);
}

void pass_tx (void) {
    furi_hal_i2c_acquire(bus);
        furi_hal_i2c_tx(bus, addr, cmd, sizeof(cmd), timeout);
    furi_hal_i2c_release(bus);

    furi_hal_i2c_acquire(bus);
        furi_hal_i2c_tx(bus, addr, cmd, sizeof(cmd), timeout);
    furi_hal_i2c_release(bus);
}

void fail_trx (void) {
    furi_hal_i2c_acquire(bus);
        furi_hal_i2c_trx(bus, addr, cmd, sizeof(cmd), &res, sizeof(res), timeout);
    furi_hal_i2c_release(bus);
}

void pass_trx (void) {
    furi_hal_i2c_acquire(bus);
        furi_hal_i2c_tx(bus, addr, cmd, sizeof(cmd), timeout);
    furi_hal_i2c_release(bus);

    furi_hal_i2c_acquire(bus);
        furi_hal_i2c_rx(bus, addr, &res, sizeof(res), timeout);
    furi_hal_i2c_release(bus);
}

Target

Flipper git head as of posting

Logs

n/a

Anything else?

There is clearly something missing in firmware/targets/f7/furi_hal/furi_hal_i2c.c ...I've found a workaround, but I'm stumped how to fix this properly :(

[csBlueChip]

These wrapper functions (notice the W in front of i2c) will fix i2c ...it's ugly, but until a proper fix is available - it works :)

static inline
bool furi_hal_Wi2c_is_device_ready (FuriHalI2cBusHandle* const bus,  const uint8_t addr,  const uint32_t tmo)
{
    furi_hal_i2c_acquire(bus);
    bool rv = furi_hal_i2c_is_device_ready(bus, addr, tmo);
    furi_hal_i2c_release(bus);
    return rv;
}

static inline
bool furi_hal_Wi2c_tx (FuriHalI2cBusHandle* const bus,  const uint8_t addr,
                       const void* buf,  const size_t len,  const uint32_t tmo )
{
    furi_hal_i2c_acquire(bus);
    bool rv = furi_hal_i2c_tx(bus, addr, buf, len, tmo);
    furi_hal_i2c_release(bus);
    return rv;
}

static inline
bool furi_hal_Wi2c_rx (FuriHalI2cBusHandle* const bus,  const uint8_t addr,
                       void* buf,  const size_t len,  const uint32_t tmo )
{
    furi_hal_i2c_acquire(bus);
    bool rv = furi_hal_i2c_rx(bus, addr, buf, len, tmo);
    furi_hal_i2c_release(bus);
    return rv;
}

static inline
bool furi_hal_Wi2c_trx (FuriHalI2cBusHandle* const bus,  const uint8_t addr,
                        const void* tx, const size_t txlen,
                              void* rx,  const size_t rxlen,  const uint32_t tmo )
{
    bool    rv = furi_hal_Wi2c_tx(bus, addr, tx, txlen, tmo);
    if (rv) rv = furi_hal_Wi2c_rx(bus, addr, rx, rxlen, tmo);
    return rv;
}

[GitChris3004]

Hey, i wrote an I²C-Scanner for the flipper and had no problems using the furi_hal_i2c_is_device_ready() function multiple times inside one aquire and release block. Have you tried bigger timeouts?

You can have a look at my code here:
I2C-Scanner Pull Request
applications/gpio/gpio_i2c_scanner_control.c

[csBlueChip]

Hmm. Yes. I can verify your specific case! ...Curiouser and curiouser ...Are you able to verify/contradict my multiple tx/rx() case? [be nice to have another mind on this]

My timeout is 1000mS - so it's not that.

I did a bit more digging last night ...it seems LL_I2C_IsActiveFlag_STOP(handle->bus->i2c) never returns true (the second time around)
https://github.com/flipperdevices/flipperzero-firmware/blob/dev/firmware/targets/f7/furi_hal/furi_hal_i2c.c#L85
...but the STM datasheet is pretty light on developer info, so the LowLevel driver code is still being a elusive.

Something in the acquire/release() code must resolve the issue ...in lieu of the person who wrote the i²c code, I guess I just keep guessing ...Problem is, an ignorant fix is a problem waiting to happen.

I am also VERY curious as to why it's all wrapped in a do { ... } while(0) construct ...maybe this function was once a macro? https://github.com/flipperdevices/flipperzero-firmware/blob/dev/firmware/targets/f7/furi_hal/furi_hal_i2c.c#L65

bool furi_hal_i2c_tx(
    FuriHalI2cBusHandle* handle,
    uint8_t address,
    const uint8_t* data,
    uint8_t size,
    uint32_t timeout) {
    furi_check(handle->bus->current_handle == handle);
    furi_assert(timeout > 0);

    bool ret = true;
    uint32_t timeout_tick = furi_get_tick() + timeout;

bool e = (handle==&furi_hal_i2c_handle_external);
    do {
        while(LL_I2C_IsActiveFlag_BUSY(handle->bus->i2c)) {
            if(furi_get_tick() >= timeout_tick) {
                ret = false;
                break;
            }
        }

        if(!ret) {
            break;
        }

        LL_I2C_HandleTransfer(
            handle->bus->i2c,
            address,
            LL_I2C_ADDRSLAVE_7BIT,
            size,
            LL_I2C_MODE_AUTOEND,
            LL_I2C_GENERATE_START_WRITE);
if (e) FURI_LOG_I("i2c", "---Ready");

        while(!LL_I2C_IsActiveFlag_STOP(handle->bus->i2c) || size > 0) {
if (e) FURI_LOG_I("i2c", "!= STOP || %d > 0", size);
            if(LL_I2C_IsActiveFlag_TXIS(handle->bus->i2c)) {
if (e) FURI_LOG_I("i2c", "== TXIS %02X", *data);
                LL_I2C_TransmitData8(handle->bus->i2c, (*data));
                data++;
                size--;
            }

            if(furi_get_tick() >= timeout_tick) {
                ret = false;
                break;
            }
        }

        LL_I2C_ClearFlag_STOP(handle->bus->i2c);
if (e) FURI_LOG_I("i2c", "Clr STOP");
    } while(0);
if (e) FURI_LOG_I("i2c", "} %d", ret);

    return ret;
}

[GitChris3004]

I didn't looked deeply in the hardware abstraction layer but i tried your failed "Reproduction" code and for me all of them is working. I'm not into writing plugins yet so maybe that might differ from my approach.

My setup:

• connected an LPS22HH Pressure sensor to the I²C-GPIOs of the Flipper (SDA: Pin 15, SCL: Pin 16, GND and 3v3)
• one 4k7 ohms resistor from SDA to 3V3 and one from SCL to 3V3
• used my I²C-Scanner application as base but put the following code into the gpio_i2c_scanner_run_once function of applications/gpio/gpio_i2c_scanner_control.c file (one after another)

TRX works (pressure sensor returns WHO_AM_I register and state is true)

void gpio_i2c_scanner_run_once(I2CScannerState* i2c_scanner_state) {
    i2c_scanner_state->items = 0;
    furi_hal_i2c_acquire(&furi_hal_i2c_handle_external);

    uint32_t response_timeout_ticks = furi_ms_to_ticks(5.f);
    uint8_t addr = 0x5c <<1;
    uint8_t cmd[] = {0x0f};
    uint8_t res = 0;

    bool state = furi_hal_i2c_trx(&furi_hal_i2c_handle_external, addr, cmd, sizeof(cmd), &res, sizeof(res), response_timeout_ticks);

    i2c_scanner_state->responding_address[i2c_scanner_state->items] = res+=state;
    i2c_scanner_state->items++;  
    
    furi_hal_i2c_release(&furi_hal_i2c_handle_external);
}

TX RX works (pressure sensor returns WHO_AM_I register and state is true)

void gpio_i2c_scanner_run_once(I2CScannerState* i2c_scanner_state) {
    i2c_scanner_state->items = 0;
    furi_hal_i2c_acquire(&furi_hal_i2c_handle_external);

    uint32_t response_timeout_ticks = furi_ms_to_ticks(5.f);
    uint8_t addr = 0x5c <<1;
    uint8_t cmd[] = {0x0f};
    uint8_t res = 0;
    bool state = furi_hal_i2c_tx(&furi_hal_i2c_handle_external, addr, cmd, sizeof(cmd), response_timeout_ticks);
    state &= furi_hal_i2c_rx(&furi_hal_i2c_handle_external, addr, &res, sizeof(res), response_timeout_ticks);

    i2c_scanner_state->responding_address[i2c_scanner_state->items] = res+=state;
    i2c_scanner_state->items++;  
    
    furi_hal_i2c_release(&furi_hal_i2c_handle_external);
}

Double TX works (pressure sensor returns WHO_AM_I register and state is true)

void gpio_i2c_scanner_run_once(I2CScannerState* i2c_scanner_state) {
    i2c_scanner_state->items = 0;
    furi_hal_i2c_acquire(&furi_hal_i2c_handle_external);

    uint32_t response_timeout_ticks = furi_ms_to_ticks(5.f);
    uint8_t addr = 0x5c <<1;
    uint8_t cmd[] = {0x0f};
    uint8_t res = 0;
    bool state = furi_hal_i2c_tx(&furi_hal_i2c_handle_external, addr, cmd, sizeof(cmd), response_timeout_ticks);
    state &= furi_hal_i2c_tx(&furi_hal_i2c_handle_external, addr, cmd, sizeof(cmd), response_timeout_ticks);
    state &= furi_hal_i2c_rx(&furi_hal_i2c_handle_external, addr, &res, sizeof(res), response_timeout_ticks);

    i2c_scanner_state->responding_address[i2c_scanner_state->items] = res+=state;
    i2c_scanner_state->items++;  
    
    furi_hal_i2c_release(&furi_hal_i2c_handle_external);
}

[csBlueChip]

Hmmm. Then I am baffled. I will hopefully release my code within the next week-or-so, then I will be able to pick up the thread with an "IRL" example.
Thanks for your help :)

[csBlueChip]

Here is the promised example: https://github.com/csBlueChip/FlipperZero_WiiEC

You will need a Wii Nunchuck (or any other Wii Extension Controller) to see it work ...I figure they're more ubiquitous than any other random i2c chip I might have laying around.

Then you can change this line https://github.com/csBlueChip/FlipperZero_WiiEC/blob/main/i2c_workaround.h#L48 to #define ENABLE_WORKAROUND 0 and see it stop working :-/

It also demonstrates the need for an i2c read function that pauses between the tx and the rx ...In this case to allow the uC in the device a few microseconds to take 5or6 A2D readings

The i2c init and read functions are here : https://github.com/csBlueChip/FlipperZero_WiiEC/blob/main/wii_i2c.c#L126

[Mywk]

Can replicate it, it wasn't working properly for me either without using it as you described:
Acquire -> Action -> Release

Is this by design?

Example code for a HTU21D / Si7021 temperature sensor using I2c in my repository, code is here.

[skotopes]

I somehow forgot about this issue.
Acquire+Release performs full i2c subsystem reset, so each time you start clean and fresh, that's why it's magically fixes transaction behavior. Most likely furi_i2c_hal not clearing up flags for next transaction. I'll fix it and will try to pack into next release.

[skotopes]

I've added unit tests that covers transmission failures: #2089
I can not reproduce your issue, please provide minimum code example that fails.

[skotopes]

The best option if you reproduce it on internal bus. If your issue is only reproducible on external bus please provide schematics.

[csBlueChip]

I don't really fancy ripping apart my FZ so I can connect to the internal bus, sorry. And the last time I played with the internal i2c bus [while trying to fix this bug], I killed the power management subsystem and had to DFU flash the device ...Mostly, I think it would be nice to be able to say "external i2c sensors/devices CAN be used with FZ [without hacky code]".

I think an Official Nintendo Wii Nunchuck Extension Controller is an ideal candidate: Cheap (£3.50 boxed as new on my high street [CEX]); easy-to-acquire or borrow; high and consistent manufacturing standard.

The wiring schematic for this (and all Wii-Extension and Mini-(S)NES Controllers) is FOUR wires:
Vcc <--> Vcc [3v3]
Gnd <--> Gnd
SCL <--> SCL [C0]
SDA <--> SDA [C1]
...which can be seen as a graphic, a photo, and a table here: https://github.com/csBlueChip/FlipperZero_WiiEC#screen-wait ...along with connectivity, electrical safety considerations, etc.

This function: https://github.com/csBlueChip/FlipperZero_WiiEC/blob/main/wii_i2c.c#L165
has comprehensive trace and debug code, along with comments as to what is happening ...the code currently works around the bug, by use of this file: https://github.com/csBlueChip/FlipperZero_WiiEC/blob/main/i2c_workaround.h ...and you can disable the workaround by setting ENABLE_WORKAROUND to 0 here: https://github.com/csBlueChip/FlipperZero_WiiEC/blob/main/i2c_workaround.h#L48

At the end of the day, if you can't fix it, it's not the end of the world. That workaround.h gets things running ...The only down-side is that i2c readings take an estimated 3..5 times longer than they should - which would only really be relevant in scenarios where timing is important.

BC

[skotopes]

@csBlueChip By "reproduce on internal bus" I meant using internal i2c devices(LED controller, charger, gauge) to reproduce issue. For example unit tests currently working with LED controller.

As for your code, it's too big for me to dive into, please provide smallest example possible. Ensure that your can reproduce issue with that code and then attach it here.

Your starting post is kinda puzzling me: there is a chance that you either got problems on physical layer or incorrectly implement software part that works with furi_hal_i2c.
So lets recap couple things:

• acquire / release are essential for anything you do in-between them(that is how bus shared between multiple threads)
• acquire / transaction / release are only valid if you do them in the same thread
• you can have as many transaction in-between acquire / release as you want/need
• since ext i2c GPIO are multipurpose there are no pull-ups, you must ensure that your bus got correct pull-ups(in your case: either on controller side or adapter board)

[csBlueChip]

My code is a single thread - so unless it is being interrupted by the OS in the middle of a function, this is not a threading issue. I would be especially surprised if trx() had threading problems [qv. OP].

I would be surprised if an absence of pull-ups would allow single transactions to work, but break multiple transactions - in a reliable and repeatable way ...I plan to pull a Nunchuck apart and reverse engineer the board when I have time to spare - mostly to check for polarity protection, but I can take a peek at the bus when I get there and confirm that Nintendo have wired up the i2c bus correctly.

I will strip out all the debug and trace code and post what's left here at some point, but I'm lacking free time at the moment, so no promises when.

I have no desire to try and reproduce this problem with other devices on other busses which you assure me work fine - I'd rather just take you at your word.

AIS, for as long as you're not doing anything timing-critical [like trying to control a drone], there's a simple workaround, so there's no rush to fix it :)

BC