Bug/unintended delays (version 2.0+)

[pm1]

Hi,
is it clear to everyone, that the following code limits the time resolution for every step to about 15ms?

MultiWii.ino

Code: Select all

    static uint8_t taskOrder=0; // never call all functions in the same loop, to avoid high delay spikes
    switch (taskOrder++ % 5) {
      case 0:
        #if MAG
          Mag_getADC();
        #endif
        break;
      case 1:
        #if BARO
          Baro_update();
        #endif
        break;
      case 2:
        #if BARO
          getEstimatedAltitude();
        #endif
        break;
...

For baro it means, that you get a new value about each 75 ms. If you dont' believe, measure the time between 2 consecutive BaroAlt calculations. The BMP085 is capable of deliver 8 measurements in about 30 ms (8*oversampling), the code now calculates 4 values (4*oversampling) in 75 ms. Instead of stacking optimizations without knowing exactly the effect of any of them the basics have to be fixed..

[Crashpilot1000]

I know it, and that's why i don't do it this way anymore. Besides this the baroreadout is problematic on the timingbase as well (both ms & bmp - it's even worse on bmp085). The later processing, after reading out the uncomprehensible delayed values (like you correctly stated), is also associated with double readings and dataloss due to lacking synchronization of data gathering and evaluation. I am no expert but that can't be good. Have a look at my corrected version viewtopic.php?f=8&t=2371&start=110#p22860 the changes are written here in human readable form (no "diff"): http://fpv-community.de/showthread.php? ... 4nderungen and here in the chapter "Sensors": http://fpv-community.de/showthread.php? ... post189823. I wonder why this isn't already discussed in the "Altitude Hold improvement solution" thread - where i posted it already - perhaps i stink in some way.

So long

Kraut Rob

[Hamburger]

perhaps i stink in some way.

Kraut Rob

that coming from a Kraut, I say no wonder

It seems people are so happy for the last improvement due to mahowik's code that it will have to settle first before further steps are desired?
And do not forget, the mahowik code is integrated in _shared, so it is easier to get it working, together with all the other latest craze... while your code is isolated - a common problem. Best remedy I can think of is teaming up with Mahowik; I seem to remember he expressed interest in your modifications.

Would not the fixation of cycle time reduce the effect you describe?

[pm1]

Would not the fixation of cycle time reduce the effect you describe?

No, it will get worse (at least with the bad way, how it is implemented now). If you fix the cycle time to 5 ms, the sensors will be called each 25ms, the baro calculation will be each 100ms+. Instead of continously polling if something might have to be done (update baro, mag,...), the cpu cycles are burnt in a wait loop.

[Hamburger]

Would not the fixation of cycle time reduce the effect you describe?

No, it will get worse (at least with the bad way, how it is implemented now).

ok. waiting for your good implementation.

[copterrichie]

Is there an interrupt flag on the Baro that can be polled and if the data is ready, then retrieve it otherwise, just bypass the reading/calculation routine?

I guess not on the BMP085 however, on page 16 of the datasheet, it talks about an end of conversation flag that can be checked for when this process is ready.

[pm1]

Is there an interrupt flag on the Baro that can be polled and if the data is ready, then retrieve it otherwise, just bypass the reading/calculation routine?

I guess not on the BMP085 however, on page 16 of the datasheet, it talks about an end of conversation flag that can be checked for when this process is ready.

Unfortunately this is a pin, which is not connected and the signal is not mapped to a register somewhere. But I don't think, that is a problem.

If you code it the following way, you can ensure, that you don't read the results too early. At the moment, the code relies on the variable current_time, which may reflect a very old timestamp in worst case.

Code: Select all

void Baro_update() {
  if (micros() < bmp085_ctx.deadline) return; 
  TWBR = ((F_CPU / 400000L) - 16) / 2; // change the I2C clock rate to 400kHz, BMP085 is ok with this speed
  switch (bmp085_ctx.state) {
    case 0:
        i2c_BMP085_UT_Start(); 
        bmp085_ctx.state++; 
        bmp085_ctx.deadline = micros() + 5000;
      break;
...

[pm1]

ok. waiting for your good implementation.

You may look in timecops repository. I made the fixation of the cycle time there some months ago.

In short (MUltiWii.ino):

Code: Select all

          Mag_getADC();
          Baro_update();
          getEstimatedAltitude();
        #if GPS
          if(GPS_Enable) GPS_NewData();
        #endif
        #if SONAR
          Sonar_update();debug[2] = sonarAlt;
        #endif
        #ifdef LANDING_LIGHTS_DDR
          auto_switch_landing_lights();
        #endif
  }
  
  if ((micros() - previousTime) > conf.cycletime_fixated) {
      computeIMU();
  
     // Measure loop rate just afer reading the sensors
     currentTime = micros();
     cycleTime = currentTime - previousTime;
     previousTime = currentTime;
     ...
    writeMotors();
  }
}

This way you have read sensors in the loop as fast as you can. IMU and PID calculations, where you need constant timing are made in a fixed interval...

[Alexinparis]

Hi,
is it clear to everyone, that the following code limits the time resolution for every step to about 15ms?

MultiWii.ino

Code: Select all

    static uint8_t taskOrder=0; // never call all functions in the same loop, to avoid high delay spikes
    switch (taskOrder++ % 5) {
      case 0:
        #if MAG
          Mag_getADC();
        #endif
        break;
      case 1:
        #if BARO
          Baro_update();
        #endif
        break;
      case 2:
        #if BARO
          getEstimatedAltitude();
        #endif
        break;
...

For baro it means, that you get a new value about each 75 ms. If you dont' believe, measure the time between 2 consecutive BaroAlt calculations. The BMP085 is capable of deliver 8 measurements in about 30 ms (8*oversampling), the code now calculates 4 values (4*oversampling) in 75 ms. Instead of stacking optimizations without knowing exactly the effect of any of them the basics have to be fixed..

Hi,

I agree the current implementation is far from being perfect.
Baro_update(); must be called at least 4 times and with a minimal delay in order to have a computed result (BaroAlt)
And getEstimatedAltitude(); is called more than one time during BaroAlt refresh cycle.

The main purpose of this is to ensure non critical extra computations are never done at the same time, in order the minimize the cycle time peak. I understand it's not an issue for 32 bit proc.
critical: gyro+acc at every cycle, and rc stuff every 20ms
non critical: everything else in a sequential order when we are not in a rc decoding cycle.

I agree in this condition the BMP085 oversampling choice is not relevant as we loose some resolution.

But, according to the good results we get now (even with this poor implementation),
I don't think it's so important to have the maximal refresh rate for baro.

integrating getEstimatedAltitude(); inside Baro_update() as a 5th state (case 4:) should solve partially the problem.

[pm1]

critical: gyro+acc at every cycle, and rc stuff every 20ms
non critical: everything else in a sequential order when we are not in a rc decoding cycle.

If this is critcal, check between every small step, if it has to be done. The implemetation, that fixing the cycle time changes the whole timing of sensor readouts averages... is not very wise. If you define a fixed cycle time for example of 5 ms, you have about 40 % CPU time left to speed up things. You can readout gyro and ACC more often, if necessary...

btw. If you leave the taskorder switch/case ind and make the looptime fixation my way for example to 5ms, I guess the baro readout will be much faster, since this part of the loop can pbe processed much more often...

[copterrichie]

I would like to question, just how often does the Baro require reading when used in conjunction with the Acc-Z to maintain altitude? Maybe it is just my reasoning however but it would seem not very often, more as a reference to restore any drift.

[pm1]

The averaging array gets a new value very 25 ms. The altitude is updated every 75 ms. The averaging array is therefore populated with only 3 measurements. The internal oversampling of the BMP085 was reduced from 8 to 4 with the reason, that you get more unique samples for averaging assuming new values every 20ms.That I do mean with hardly to understand stacking of optimizations. Maybe it is enough to read out the baro every 500 ms

[timecop]

Actually naza queries MS5611 over SPI at pretty much its max rate of ~60Hz (8.22ms per each temp/baro conversion at max oversample).

[pm1]

I agree the current implementation is far from being perfect.
Baro_update(); must be called at least 4 times and with a minimal delay in order to have a computed result (BaroAlt)
And getEstimatedAltitude(); is called more than one time during BaroAlt refresh cycle.

Code suggestion (works fine for me):

Code: Select all

    static uint8_t taskOrder=0; // never call all functions in the same loop, to avoid high delay spikes
    static uint32_t ts;          
                  
    ts = micros();
    while ((micros() - ts) < 300) {  
      
      switch (taskOrder++ % 5) {
        case 0:
          #if MAG
            Mag_getADC();
          #endif
          break;
        case 1:
          #if BARO
            Baro_update();
          #endif
          break;
        case 2:
          #if BARO
            getEstimatedAltitude();
          #endif
          break;
        case 3:
          #if GPS
            if(GPS_Enable) GPS_NewData();
          #endif
          break;
        case 4:
          #if SONAR
            Sonar_update();
          #endif
          #ifdef LANDING_LIGHTS_DDR
            auto_switch_landing_lights();
          #endif
          break;
      }
    }  

Btw. Gps serial needs more that 4 ms in peak, baro about 1.2 ms, gps i2c 0.9ms, the rest is negligible < 0.3...

[Alexinparis]

the rest is negligible < 0.3

something like adding 10% of the loop time is not negligible

[pm1]

the rest is negligible < 0.3

something like adding 10% of the loop time is not negligible

There are quite a lot of code pieces in the code, which are increasing the loop time >= 300 ms for single loops, so it *is* negligible.

I tried different loop times up to 10 ms. With up to 5 ms i saw no negative effect. In my opinion is is better to have 5 ms preditive for IMU and lower variations for sensor readout as to hold minimum loop time at 2.8 ms.

[pm1]

Serial GPS causes highest peaks of > 4000 ms. CPU hog is here reading the serial interface. The following change reduces the spikes to about 2000 us, SerialAvailable costs a lot...

void GPS_NewData() {
...

#if defined(GPS_SERIAL) || defined(TINY_GPS) || defined(GPS_FROM_OSD)
#if defined(GPS_SERIAL)
uint8_t c = SerialAvailable(GPS_SERIAL);
while (c--) {

[Alexinparis]

Serial GPS causes highest peaks of > 4000 ms. CPU hog is here reading the serial interface. The following change reduces the spikes to about 2000 us, SerialAvailable costs a lot...

void GPS_NewData() {
...

#if defined(GPS_SERIAL) || defined(TINY_GPS) || defined(GPS_FROM_OSD)
#if defined(GPS_SERIAL)
uint8_t c = SerialAvailable(GPS_SERIAL);
while (c--) {

nice catch
It's difficult to understand why a small function like SerialAvailable costs a lot.

[pm1]

It's difficult to understand why a small function like SerialAvailable costs a lot.

165 bytes ublox messages leading to 165 calls in one loop -> For NMEA it could be even more.