Add support for IST8310 compass

[haslinghuis]

https://intofpv.com/attachment.php?aid=8104

[github-actions]

Do you want to test this code? You can flash it directly from Betaflight Configurator:

• Simply put #12917 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

[McGiverGim]

I did a try in the past #5397 but the i2c bus of Betaflight was not reliable in the timing. For inav I did similar code and it worked.

[haslinghuis]

Thanks @McGiverGim did not know. Also cannot test without hardware.

[McGiverGim]

I've some, I will try to test it but I will be out until monday.

[McGiverGim]

Confirmed, I've an IST8310 and an IST8308, but I need to do some soldering to test it.
I will do it the next week.

[blckmn]

AUTOMERGE: (FAIL)

• github identifies PR as mergeable -> FAIL
• assigned to a milestone -> PASS
• cooling off period lapsed -> PASS
• commit count less or equal to three -> FAIL
• Don't merge label NOT found -> PASS
• at least one RN: label found -> PASS
• Tested label found -> PASS
• assigned to an approver -> PASS
• approver count at least three -> FAIL

[McGiverGim]

I've informed by Discord, but I think is good to have here the problems I've found. Maybe others can help here and not in Discord.
I've tested it. The MAG is not found with this code. But adding a set mag_i2c_address = 12 did the trick.
I can't find in your code where you use this "default" address in the detection routine.
But detecting it is not enough, it seems not to be reading any value, they are always zero.
I think remember the detection/reading of my old code worked in the past, simply the data read had wrong values.

[McGiverGim]

With the latest changes it's going better. Now it's detected correctly. But it does not read any value from the mag (the values are always zero).

[McGiverGim]

Tested latest code. The mag is detected but again no values read.

[haslinghuis]

@McGiverGim IST8310 supports standard speed (100kHz) and fast speed (400Khz).

Default we use a clock speed of 800Khz, please try:

set i2c1_clockspeed_khz = 100

We could also try getting axis data from their respective registers (having a low and high byte)

[McGiverGim]

I tested all the different i2c speed with your earlier code, without luck, but I will recheck it later.

[McGiverGim]

Strange. Using 100 and 400 clockspeed makes the MAG and the BARO not found. Pushing it at 800 again makes them found again.

[McGiverGim]

The return false, if I'm not wrong, is to say that there is not new data in the MAG variables. When returns true, is because there is new data.

[ledvinap]

Yes. But it also means 'try again 1ms later'.
busReadRegisterBuffer waits for transfer competition, so buf is available when it returns. Splitting it into two parts makes some sense (busWriteRegister(dev, IST8310_REG_CNTRL1, IST8310_ODR_SINGLE); is called in on next invocation, reducing latency), but is not necessary for testing.

You want IST8310_ODR_SINGLE just before returning true, so that MAG has enough time to prepare data (100ms)

[McGiverGim]

Some tests:

• Tested reading from 0x02: glitches in the first byte of X axis, but no problem with the second one.
• Tested the actual code of @haslinghuis, adding simply the fix for the static buffer. IT WORKS. It seems to work without the hack of reading the buffer ahead. The only problem is that is slower than the three states version:

# tasks
Task list             rate/hz  max/us  avg/us maxload avgload  total/ms   late    run reqd/us
16 - (        COMPASS)     10     124      73    0.1%    0.0%       220    858   1716      46

with the three states:

Task list             rate/hz  max/us  avg/us maxload avgload  total/ms   late    run reqd/us
16 - (        COMPASS)     10      46      15    0.0%    0.0%        56      0   1972      44

So what are the next steps? start with the two states version and move it to three states to fix the times? Or start with the three states and look for a way to read the data without the hack?

EDIT: I forgot that the problem is not the states, is that this version does not have the read without blocking....

[McGiverGim]

Tested the current @haslinghuis version with the static fix, adding the PULSE_NORMAL (but it seems it does not affect) and the asynchronous buffer read and... IT WORKS!
And the times far better:

Task list             rate/hz  max/us  avg/us maxload avgload  total/ms   late    run reqd/us
16 - (        COMPASS)     10      46      24    0.0%    0.0%        37      0   1170      44

I will do a review with the changes I have :)

[McGiverGim]

Done a tests using the async write too (we don't care about the result so I suppose it will be better) and it works too:

Task list             rate/hz  max/us  avg/us maxload avgload  total/ms   late    run reqd/us
16 - (        COMPASS)     10       7       4    0.0%    0.0%         4      0    566       6

The times are far better...

[McGiverGim]

Some are optional, but with this seems to work and use very few times :)

[McGiverGim]

Asyncrhonous

Suggested change

	busReadRegisterBuffer(dev, IST8310_REG_DATA, buf, sizeof(buf));
	busReadRegisterBufferStart(dev, IST8310_REG_DATA, buf, sizeof(buf));

[ledvinap]

But code shall check that operation is finished before using received data

[McGiverGim]

I suppose that the worst that can happen is that it will contain old data, that is the same that will happen if the data is not ready.
I don't know neither how to know if the read has finished. Do we have some method that gives this information?

[ledvinap]

Worst case is that there is race between I2C and data processing, mixing low and high byte from different measurements. Probability of this is very low ..

Sorry, my knowledge of I2C driver is very limited. Maybe grep code for busReadRegisterBufferStart and see how odher drivers handle it

[haslinghuis]

Returns status from (drivers/stm32/bus_i2c_hal.c):

// Non-blocking read
bool i2cReadBuffer(I2CDevice device, uint8_t addr_, uint8_t reg_, uint8_t len, uint8_t* buf)
{
    if (device == I2CINVALID || device >= I2CDEV_COUNT) {
        return false;
    }

    I2C_HandleTypeDef *pHandle = &i2cDevice[device].handle;

    if (!pHandle->Instance) {
        return false;
    }

    HAL_StatusTypeDef status;

    status = HAL_I2C_Mem_Read_IT(pHandle, addr_ << 1, reg_, I2C_MEMADD_SIZE_8BIT, buf, len);

    if (status == HAL_BUSY) {
        return false;
    }

    if (status != HAL_OK) {
        return i2cHandleHardwareFailure(device);
    }

    return true;
}

[McGiverGim]

Maybe grep code for busReadRegisterBufferStart and see how odher drivers handle it

Other drivers seem to ignore it, when they use "Start" method. At least the QMC5883 that is very similar. I don't say that this is correct, but is what is being done at this moment.

[ledvinap]

@McGiverGim : Ideally implement nonblocking version. 2 states may do:

1. Process received data from 2. + start new trigger (IST8310_ODR_SINGLE) (return true) (10Hz timer started)
2. start I2C data receive, return false (1ms timeout started)

Some check shall make sure that receive is finished (1ms should be enough at 400kHz) and some state handling may be necessary to handle startup (no data received yet)

[ledvinap]

Problem is that this will fail if some other transaction is pending.
Maybe just :

Suggested change

	busReadRegisterBufferStart(dev, IST8310_REG_DATA, buf, sizeof(buf));
	if (!busReadRegisterBufferStart(dev, IST8310_REG_DATA, buf, sizeof(buf))) return false;

[haslinghuis]

@McGiverGim @ledvinap function HAL_I2C_Mem_Read_IT is not really non-blocking: https://community.st.com/t5/stm32-mcu-products/hal-i2c-mem-read-it-blocks-mcu/td-p/112861

[McGiverGim]

Tested with the cloud build from the Configurator, and now it works, as is. It can go better, but now it works.

[McGiverGim]

I have an IST8308 sample too, if you want to start again :P

[ledvinap]

Looks good enough. Someone shall look at asynchronous reads eventually, it may cause hard-to-catch bugs (for example, it will probably fail is DRDY interrupt is used and I2C collision occurs)