cleanflight / cleanflight

Clean-code version of the baseflight flight controller firmware
http://cleanflight.com
GNU General Public License v3.0
2.61k stars 1.39k forks source link

SPRacingF3EVO SPI mag driver causes stuttering motors. #2022

Closed hydra closed 8 years ago

hydra commented 8 years ago

Until this is fixed disable the mag on the SPRacingF3EVO using the following cli commands

set mag_hardware=1
save

The issue can be seen in the GUI too - the looptime will occaisionally jump to around 24000 10 times a second (the frequency of the updateCompass task).

The issue is the delay in reading from the I2C slave in the compass_ak8963.c file in the ak8963SPIRead method.

UPDATE:

Firmware with a fix for this issue is here:

http://cleanflight.memoryleaks.org/builds/cleanflight_cleanflight/2016-04-08/3451/3451_4706dea_travis_cleanflight_SPRACINGF3EVO.hex

Any build after 3451 contains a fix. Note that this binary contains many other changes and will also require latest developent configurator to view the 'configuration' page.

MJ666 commented 8 years ago

We have seen this problem with the AlienFlight prototypes. The comunication to I2C via SPI is not ideal and will give even add more delay than native I2C. I was not able to reduce this delay much and on the AlienFlight F4 we have even more problems and comunicatin is not workig reliable yet. We have decided to go for the MPU6500 for newer versions and use an seperate mag sensor if needed in the future. For the AlienFlight F3 the mag is disabled by default already.

hydra commented 8 years ago

i have a fix for this, working code here. needs cleanup. basically it adds a state machine into the mag read code and queues a read up, waits for the time to have elapsed and then reads the i2c buffer from the mpu9250.

i found it was possible to reduce it to two reads, one for the status, and another for the data and the status2 register.

hydra commented 8 years ago

the general idea is this:

bool ak8963Read(int16_t *magData)
{
    bool ack;
    uint8_t buf[7];

    static ak8963ReadState_e state = CHECK_STATUS;

    switch (state) {
        case CHECK_STATUS:
            ak8963SPIStartRead(AK8963_MAG_I2C_ADDRESS, AK8963_MAG_REG_STATUS1, 1, &buf[0]);
            state++;
            return false;

        case WAITING_FOR_STATUS: {
            uint32_t timeRemaining = ak8963SPIQueuedReadTimeRemaining();
            if (timeRemaining) {
                return false;
            }

            ack = ak8963SPICompleteRead();

            uint8_t status = buf[0];

            if (!ack || (status & STATUS1_DATA_READY) == 0) {
                return false;
            }

            // read the 6 bytes of data and the status2 register
            ak8963SPIStartRead(AK8963_MAG_I2C_ADDRESS, AK8963_MAG_REG_HXL, 7, &buf[0]);

            state++;

            return false;
        }

        case WAITING_FOR_DATA: {
            uint32_t timeRemaining = ak8963SPIQueuedReadTimeRemaining();
            if (timeRemaining) {
                return false;
            }

            ack = ak8963SPICompleteRead();

            uint8_t status2 = buf[6];
            if (!ack || (status2 & STATUS2_DATA_ERROR) || (status2 & STATUS2_MAG_SENSOR_OVERFLOW)) {
                return false;
            }

            magData[X] = -(int16_t)(buf[1] << 8 | buf[0]) * magGain[X];
            magData[Y] = -(int16_t)(buf[3] << 8 | buf[2]) * magGain[Y];
            magData[Z] = -(int16_t)(buf[5] << 8 | buf[4]) * magGain[Z];

            state = CHECK_STATUS;

            return ak8963config.write(AK8963_MAG_I2C_ADDRESS, AK8963_MAG_REG_CNTL, CNTL_MODE_ONCE); // start reading again
        }
    }

    return false;
}
hydra commented 8 years ago

there still needs to be some integration with the scheduler, so the updateCompass task can ask the driver if action needs to be taken or not.

borisbstyle commented 8 years ago

@hydra Why is spracingf3evo missing exti definition in initialisation.c?

ledvinap commented 8 years ago

@hydra: is the branch with rest of code anywhere?

Why not use MPU slave handling to do all the work - just read registers containing data and wait for DRDY?

hydra commented 8 years ago

@ledvinap i'm still looking into it at the moment, but thanks for the hints. no the code isn't anywhere but on my laptop. I'm waiting for my 4x4 to be fixed in the garage, limited internet access...

hydra commented 8 years ago

@borisbstyle not sure what happened to that. perhaps it was lost in the merge to master. fixed in ca843a84ad85e0d489a7c6f3c45b06f54a6d4c90

hydra commented 8 years ago

I've pushed 4706dea to master. It fixes the problem and doesn't affect other targets right now. a properly solution and cleanup still required.

hydra commented 8 years ago

improvement PR's welcome, i have to attend to other areas of code right now.

hydra commented 8 years ago

closing this issue. I will open a new issue for the cleanup that is needed.