Bug - MultiWii.ino lines 832-852 needs rcDelayCommand

doughboy · Post by **doughboy** » Tue Oct 09, 2012 7:20 pm

In MultiWii.ino lines 832-852, the if blocks are missing check for rcDelayCommand==20.
This bug cost me two propellers yesterday, as the stick combo for acc calibration is not triggered but the second if condition below got triggered instead. no wonder I had a hard time flying level.

} else if (rcData[PITCH] > MAXCHECK) {
conf.angleTrim[PITCH]+=2;writeParams(1);
#if defined(LED_RING)
blinkLedRing();
#endif
} else if (rcData[PITCH] < MINCHECK) {
conf.angleTrim[PITCH]-=2;writeParams(1);
#if defined(LED_RING)
blinkLedRing();
#endif
} else if (rcData[ROLL] > MAXCHECK) {
conf.angleTrim[ROLL]+=2;writeParams(1);
#if defined(LED_RING)
blinkLedRing();
#endif
} else if (rcData[ROLL] < MINCHECK) {
conf.angleTrim[ROLL]-=2;writeParams(1);
#if defined(LED_RING)
blinkLedRing();
#endif
} else {

I think there is a flaw in the rcDelayCommand logic as the last stick combo that gets rcDelayCommand==20 gets triggerred. Say if the first 19 you had throttle max and yaw min and you slightly moved yaw past mincheck, then the wrong action is triggered. I think it should be 20 consecutive pass with the same stick combo (like debouncing a switch).

we can use a byte variable with 2 bits assigned to one stick and 00=min, 11=max, 01=mid, then on each loop detect the position of all 4 sticks and set that variable, then if it is not the same as previous loop, to reset rcDelayCommand counter to 0. if it reaches 20 consecutive passes with the same value, then do a switch case to perform the selected action. stick positions that are irrelevant (can be min max or mid) can be treated as a wildcard. With this setup, adding a new stick combo action is as simple as adding a case block. perhaps can even be configurable like the aux assignment in gui.

jevermeister · Post by **jevermeister** » Tue Oct 09, 2012 7:44 pm

already fixed. acc calibration has been moved before thw code stated by you. it was a bug introduced by the implementation of parameter switch. sorry to hear about you props mate.

doughboy · Post by **doughboy** » Tue Oct 09, 2012 8:14 pm

this is a different issue not fixed by your check in today. before an action is triggered, it must satisfy rcDelayCommand==20 condition, for which those 4 if conditions under throttle>maxcheck section did not do.

Alexinparis · Post by **Alexinparis** » Tue Oct 09, 2012 9:05 pm

doughboy wrote:this is a different issue not fixed by your check in today. before an action is triggered, it must satisfy rcDelayCommand==20 condition, for which those 4 if conditions under throttle>maxcheck section did not do.

It is expected.
The trigger delay is not based on rcDelayCommand == 20, but on the delay of blinkLED(15,20,1) (in writeParams(1);)

jevermeister · Post by **jevermeister** » Tue Oct 09, 2012 9:11 pm

what to do now?

doughboy · Post by **doughboy** » Tue Oct 09, 2012 9:14 pm

Alexinparis wrote:
doughboy wrote:this is a different issue not fixed by your check in today. before an action is triggered, it must satisfy rcDelayCommand==20 condition, for which those 4 if conditions under throttle>maxcheck section did not do.

It is expected.
The trigger delay is not based on rcDelayCommand == 20, but on the delay of blinkLED(15,20,1) (in writeParams(1);)

it is supposed to execute RC stick command action after 20 passes in the 50 hz if block no?
for the 4 if blocks under throttle>maxcheck, they will execute writeParams on first pass, which is not correct. If I had the sticks positioned for acc calibration, it will still execute the parameter setting action, which is not what I wanted.

Sebbi · Post by **Sebbi** » Tue Oct 09, 2012 9:21 pm

Does this code really execute the trim every cycle when calibrating what is level? No wonder I overshoot so often when setting the trim ...

P.S.: It's true what doughboy is writing. When you executed the mag or acc calibration via stick command and release the yaw stick before the pitch stick the pitch will get trimmed unintentionally. At least in the current _shared branch.

jevermeister · Post by **jevermeister** » Tue Oct 09, 2012 9:36 pm

thank you for the testong and feedback. I will try to fix the code asap.

Alexinparis · Post by **Alexinparis** » Tue Oct 09, 2012 9:46 pm

it is supposed to execute RC stick command action after 20 passes in the 50 hz if block no?

No, not for acc trimming. It is executed every 20ms as long as you maintain the stick, and only if the previous blinkLED is finished.

for the 4 if blocks under throttle>maxcheck, they will execute writeParams on first pass, which is not correct. If I had the sticks positioned for acc calibration, it will still execute the parameter setting action, which is not what I wanted.

It is coded this way. What bother you is the fact the eeprom is written at each trim tick ?

P.S.: It's true what doughboy is writing. When you executed the mag or acc calibration via stick command and release the yaw stick before the pitch stick the pitch will get trimmed unintentionally. At least in the current _shared branch.

Yes it's true, it can occur. But just for one tick (0.2 deg) except if you are really slow to move the sticks

doughboy · Post by **doughboy** » Tue Oct 09, 2012 9:50 pm

the quick fix is to just add rcDelayCommand==20 in the if condition like this

} else if (rcData[PITCH] > MAXCHECK && rcDelayCommand==20) {
conf.angleTrim[PITCH]+=2;writeParams(1);
#if defined(LED_RING)
blinkLedRing();
#endif
} else if (rcData[PITCH] < MINCHECK && rcDelayCommand==20) {
conf.angleTrim[PITCH]-=2;writeParams(1);
#if defined(LED_RING)
blinkLedRing();
#endif
} else if (rcData[ROLL] > MAXCHECK && rcDelayCommand==20) {
conf.angleTrim[ROLL]+=2;writeParams(1);
#if defined(LED_RING)
blinkLedRing();
#endif
} else if (rcData[ROLL] < MINCHECK && rcDelayCommand==20) {
conf.angleTrim[ROLL]-=2;writeParams(1);
#if defined(LED_RING)
blinkLedRing();
#endif
} else {

but as I described in the first post, I think the logic of using rcDelayCommand has a flaw. Because in theory, you can switch different stick combo before the 20th, and the 20th will tirgger the first action that satisfies the stick position. I think the intent is for all 20 passes to have exactly the same stick positions. meaning, you are holding the sticks in position for 0.4 seconds.

jevermeister · Post by **jevermeister** » Tue Oct 09, 2012 9:55 pm

What about this:

Code: Select all

    // end of failsafe routine - next change is made with RcOptions setting
    if (rcData[THROTTLE] < MINCHECK) {
      errorGyroI[ROLL] = 0; errorGyroI[PITCH] = 0; errorGyroI[YAW] = 0;
      errorAngleI[ROLL] = 0; errorAngleI[PITCH] = 0;
      rcDelayCommand++;
      if (rcData[YAW] < MINCHECK && !f.ARMED) {            // THTOTTLE min, YAW left 
        if (rcData[PITCH] > MAXCHECK && rcData[ROLL] > MAXCHECK && rcDelayCommand == 20){
          #if defined(INFLIGHT_ACC_CALIBRATION) 
            if (AccInflightCalibrationMeasurementDone){                // trigger saving into eeprom after landing
              AccInflightCalibrationMeasurementDone = 0;
              AccInflightCalibrationSavetoEEProm = 1;
            }else{ 
              AccInflightCalibrationArmed = !AccInflightCalibrationArmed; 
              #if defined(BUZZER)
                if (AccInflightCalibrationArmed)  notification_toggle = 2;
                else notification_toggle = 3;
              #endif
            }
          #endif
        } 
       else if(rcData[PITCH] < MINCHECK && rcDelayCommand == 20) {                              // PITCH down -> GYRO cal
          calibratingG=400;
          #if GPS 
            GPS_reset_home_position();
          #endif
        }
        else if(rcData[ROLL]  < MINCHECK && rcData[PITCH] > 1300 && rcData[PITCH] < 1700 && rcDelayCommand == 20) i=1;    // ROLL left  -> SET 1
        else if(rcData[PITCH] > MAXCHECK && rcData[ROLL]  > 1300 && rcData[ROLL]  < 1700 && rcDelayCommand == 20) i=2;    // PITCH up   -> SET 2
        else if(rcData[ROLL]  > MAXCHECK && rcData[PITCH] > 1300 && rcData[PITCH] < 1700 && rcDelayCommand == 20) i=3;    // ROLL right -> SET 3
        else i=0;
        if(i) {
          global_conf.currentSet = i-1;
          writeGlobalSet(0);
          readEEPROM();
          blinkLED(2,40,i);
        }
      }

doughboy · Post by **doughboy** » Tue Oct 09, 2012 9:56 pm

Alexinparis wrote:No, not for acc trimming. It is executed every 20ms as long as you maintain the stick, and only if the previous blinkLED is finished.

ok, I never mentioned acc trim anywhere, I said acc calibration (or Calib Stable Acc). stick combo throttle max, yaw min, pitch min.
what you are referring to as acc trim, is completely different with its own stick combo. throttle max, and pitch min. two different things.
the logic is flawed. ever wondered why there are so many posts about stick combo not working? because it is not working!

Alexinparis · Post by **Alexinparis** » Tue Oct 09, 2012 9:58 pm

Because in theory, you can switch different stick combo before the 20th, and the 20th will tirgger the first action that satisfies the stick position. I think the intent is for all 20 passes to have exactly the same stick positions. meaning, you are holding the sticks in position for 0.4 seconds.

The intent is not to check if the stick is maintained in the same position for 0.4s, but rather to wait for a minimum delay before the stick combo is executed.
With no special care about how rcDelayCommand == 20 is reached.

doughboy · Post by **doughboy** » Tue Oct 09, 2012 10:00 pm

Alexinparis wrote:
Because in theory, you can switch different stick combo before the 20th, and the 20th will tirgger the first action that satisfies the stick position. I think the intent is for all 20 passes to have exactly the same stick positions. meaning, you are holding the sticks in position for 0.4 seconds.

The intent is not to check if the stick is maintained in the same position for 0.4s, but rather to wait for a minimum delay before the stick combo is executed.
With no special care about how rcDelayCommand == 20 is reached.

which is exactly my point, you can end up triggering the wrong action.

doughboy · Post by **doughboy** » Tue Oct 09, 2012 10:06 pm

ok, let's say I take your logic behind the design.
There is still a usability issue, as once the user sees the lights blink, they will think they got the acc calib command in, but it is actually blinking for a different action (writeparams). why the need to write eeprom every 20ms?

Alexinparis · Post by **Alexinparis** » Tue Oct 09, 2012 10:22 pm

doughboy wrote:ok, let's say I take your logic behind the design.
There is still a usability issue, as once the user sees the lights blink, they will think they got the acc calib command in, but it is actually blinking for a different action (writeparams). why the need to write eeprom every 20ms?

I agree, the current implementation can lead to confusing situation.
If I take your example:
1) let's consider the PITCH MIN condition is detected (before PITCH MIN + YAW MIN) at first as a pitch acc trim.
2) write+blinkled
3) if the stick combo is still maintained, PITCH MIN + YAW MIN is then detected, leading to a calib ACC + zeroing acc trim.
not clean, but the result is here.
I'm open to other implementations, if the code size resultant is similar.

There is no need to write eeprom every 20ms, it's just a simple way to save changes at each tick.
A proper implementation should wait for the end of stick combo and then write to EEPROM, but it works...

jevermeister · Post by **jevermeister** » Tue Oct 09, 2012 10:23 pm

anybodyy noticed my last post?

Alexinparis · Post by **Alexinparis** » Tue Oct 09, 2012 10:43 pm

what is the purpose of moving
if (rcData[PITCH] > MAXCHECK && rcData[ROLL] > MAXCHECK && rcDelayCommand == 20){
outside of
#if defined(INFLIGHT_ACC_CALIBRATION)
?

jevermeister · Post by **jevermeister** » Tue Oct 09, 2012 10:52 pm

so i can check every other combo with if else to ensure that only one is executed. this would not be possible if the first if statement would be inside the define

Alexinparis · Post by **Alexinparis** » Tue Oct 09, 2012 10:57 pm

jevermeister wrote:so i can check every other combo with if else to ensure that only one is executed

I don't understand how other combos could be executed at the same time.
could you give an example ?

jevermeister · Post by **jevermeister** » Tue Oct 09, 2012 11:21 pm

if someone wants tu use parts of the two stick combo to switch something else. e.g. pitch forward roll right. it is now possible to do that.

what do you think of the rest?

it's just a proposed idea. If you think it is not good, thats no problem at all. Just my two cents. :-)

Mis · Post by **Mis** » Wed Oct 10, 2012 1:56 am

we can use a byte variable with 2 bits assigned to one stick and 00=min, 11=max, 01=mid, then on each loop detect the position of all 4 sticks and set that variable, then if it is not the same as previous loop, to reset rcDelayCommand counter to 0. if it reaches 20 consecutive passes with the same value, then do a switch case to perform the selected action. stick positions that are irrelevant (can be min max or mid) can be treated as a wildcard.

This is good idea and can even save the codespace. But bits for stick should be like 10 11 01 for use simple masks.
I can try make some machine state for stick combos detection.

Hamburger · Post by **Hamburger** » Wed Oct 10, 2012 8:49 am

Is it worth the trouble? Does the current implementation - even if introduces room for ambiguity - pose a possible threat or is it difficult to use as is?

Mis · Post by **Mis** » Wed Oct 10, 2012 4:53 pm

This is pretty easy

Now done in r1180
Code is about 300 bytes smaller, and more,more better readable.
All checks is done in full acuracy - position of all 4 sticks are checked.
Old stuff still present, but commented out.

jevermeister · Post by **jevermeister** » Wed Oct 10, 2012 6:03 pm

cool. will have a look tonight.

great work mis

jevermeister · Post by **jevermeister** » Wed Oct 10, 2012 9:42 pm

Hi,
I tested you code and it is working, *edit* No it is not working!

I have a few problems and I am not sure if they are connected to your change:

If I arm the motor via aux the software checked if throttle was in lowest position, this is not the case now, I can arm with full throttle, that would propably end in a disaster. I can switch off motors while still applying throttle, that wasn'T possible in 2.1 either.
Failsafe is detected at once if copter is not armed but takes a lot of time to activate while copter is armed, if I reenable tx while failsafe landing is active, the copter accepts the current state of the aux channels, but it should not do that, is should stay armed until throttle is low if aux is set to not armed while reconnect.

Nils

Alexinparis · Post by **Alexinparis** » Wed Oct 10, 2012 9:49 pm

Mis wrote:This is pretty easy Now done in r1180
Code is about 300 bytes smaller, and more,more better readable.
All checks is done in full acuracy - position of all 4 sticks are checked.
Old stuff still present, but commented out.

nice !
there are some other small bugs to mention:

Code: Select all

    if (rcSticks & 0xC0 == THR_LO) {          // THROTTLE at minimum

is never reached

Code: Select all

        if      (rcSticks == THR_HI + YAW_CE + PIT_HI + ROL_CE) {conf.angleTrim[PITCH]+=2; i=1;}

we loose the repeat ability. maybe zeroing rcDelayCommand inside {} could do the job.

jevermeister · Post by **jevermeister** » Wed Oct 10, 2012 9:56 pm

Okay,
I uploaded the code before your change and noticed the following crucial differences to after the change:

- I am unable to arm copter while throttle is applied
- I am unable to disarm copter while throttle is applied
- failsafe is kicking in instantly regardles if armed or not
- arm status after reconnoect is maintained as before regardless of aux position if throttle is applied.

Please accept this feedback, I like your solution very much but the above has to implemented to be safe.
Thank you for the excellent solution and the grat work Mis
Nils

Mis · Post by **Mis** » Thu Oct 11, 2012 12:35 am

Thank's for feedback. Most of bugs is fixed in r1181. I'm not sure only about failsafe kick.

jevermeister · Post by **jevermeister** » Thu Oct 11, 2012 8:07 am

Hi,
I took a quick look on you changes, seems that you catched the bugs.

I cannot test it right now, because I am in the office.
I will do a testrun tonight.

Why are you using blinkLED(2,800,1); ?

The function is working with delays, it is not good to use it anymore.

Maybe this was the reason why the failsafe was delayed while the copter was armed, I only tested it without hardware on the flyduino but I think this would have crashed the copter.

Nils

ps.: Thank you for the quick fix!

Mis · Post by **Mis** » Thu Oct 11, 2012 10:42 am

The blinkLLED is executed only if you try motors arming, and arming is not possible for some reasons eg failsafe active, ACC not calibrated ect. In this case you see led blink and hear error beep instead motors arming.

jevermeister · Post by **jevermeister** » Thu Oct 11, 2012 10:50 am

Hi,
in the r1180 it was executed inflight if failsavecnt>1. I think that caused the delay.

if you want to use the buzzer, you need to set a warning flag.
you may use notification_toggle = 1 or 2or 3 to do the desired beepcount or define an alarm pattern in alarms.ino

blinkLED won't beep since 2.1

Code: Select all

/********************************************************************/
/****                         LED Handling                       ****/
/********************************************************************/
//Beware!! Is working with delays, do not use inflight!

void blinkLED(uint8_t num, uint8_t ontime,uint8_t repeat) {
  uint8_t i,r;
  for (r=0;r<repeat;r++) {
    for(i=0;i<num;i++) {
      #if defined(LED_FLASHER)
        switch_led_flasher(1);
      #endif
      #if defined(LANDING_LIGHTS_DDR)
        switch_landing_lights(1);
      #endif
      LEDPIN_TOGGLE; // switch LEDPIN state
      delay(ontime);
      #if defined(LED_FLASHER)
        switch_led_flasher(0);
      #endif
      #if defined(LANDING_LIGHTS_DDR)
        switch_landing_lights(0);
      #endif
    }
    delay(60); //wait 60 ms
  }
}

Mis · Post by **Mis** » Thu Oct 11, 2012 12:07 pm

Code before r1181 have some more bugs if you use ARM BOX instead arming via stick command. If you accidentally move throttle stick to minimum during flight, the headFreeModeHold variable is updated with current heading. In r1181 this is eliminated by adding if(!armed) condition in go_arm() function.
BTW From r1147 I restore buzzer signalization inside blinkLED because I need it in profile selectting procedure. This not interferent with alarm or buzzer handlers in alarms.ino because the handlers is not execuded during dealys, and not called from blinkLED.

jevermeister · Post by **jevermeister** » Thu Oct 11, 2012 8:19 pm

Hi,

I did some tests with my Flyduino and sensor board with WMP,ACC,MAG and BARO

on the first glance everything seems to work properly. The Failsafe is working as it should now <-- very good!

But I have some issues, maybe you can think about that.

1. The beeps of the blinkLED function interfere with the buzzerhandler. The beeps sound afwul (and distorted on the pilotlamp - it is not designed to use a buzz created by a delay function)and they interfere with the beeps of the buzzer handler.
Calibrating gyro brings me 6 beeps in two different patterns, the blinkled function was stripped of the buzzercode and the beeps were implemented by setting a global variable in each occasion where blinkled is executed, we should decide for a way, buzzerhandler or blinkled.

2. the copter cannot arm if it is not level.
the acc is showing a leveled copter but the mag is indicating a tilted copter (it is not calibrated) this is superdangerous, I fly stunts where I unarm the copter in midair and rearm in freefall,

preventing the copter to arm while not level is a nice feature. but is should be a "feature" to turn on and off via define
also the mag should not be taken into the angle calculation because of magnetic interference.

Hope my feedback is helping.

Thanks again for the fix.

Sebbi · Post by **Sebbi** » Thu Oct 11, 2012 8:41 pm

I'd like to know where the mag is used to calculate angles - besides heading - in the code. I know that this would theoretically possible, but does MultiWii really use this?

jevermeister · Post by **jevermeister** » Thu Oct 11, 2012 8:46 pm

the copter model was upside down and tilted until i switched off mag.

I assumed it was used for that...

jevermeister · Post by **jevermeister** » Fri Oct 12, 2012 9:42 am

Hi,
I fixed the buzzer bug by removing the beeps from blinkLED() and appended a notification_confirmation after each blinkled() call in sourcecode.

You deleted them in eeprom but forgot to delete them in lcd etc. this lead to distorted and double buzzer signals.
the current implementation works the same.
BlinkLED will be changed in the future to be handled exactly like buzzer.

If you use a piezo speaker or a pilot light the use of buzzer with delay is not possible, because they are working with timers.

Nils

Mis · Post by **Mis** » Fri Oct 12, 2012 1:21 pm

OK, but your change have some issues
1. No current active profile signalisation after power-up (only led blinks, no buzzer confirmation)
2. Buzzer and leds is out of sync. In old way the buzzer beeps synchronized with led's, now first the leds blinks, then after a while buzzer is activating.

The alarm handler cannot use two or more resources at the same time (eg buzzer and led's), and is limited only to few blinks/beeps. Additionally handler is complicated use more program space, and not work outside main loop and during delay instruction is executed. Any loop outside main loop prevent alarms handler from working too.
I don't like current alarms handler implementation.

jevermeister · Post by **jevermeister** » Fri Oct 12, 2012 1:24 pm

Okay,
then remove it.

Nils

Mis · Post by **Mis** » Fri Oct 12, 2012 1:43 pm

I think, the alarm handler should be rewrited for handling simultanously more than one channel (every channel should have one bit mask in resource variable) and maybe changed for easest using eg maybe in this way: "notification(resource, notification pattern)". Resource can be defined as eg "LED" or "BUZZER" or "LED + BUZZER", notification pattern can be a 16 bit pattern (simmilar led flasher pattern) eg 0b111100011110000 create two long beep , or 0b1000000000000000 create one short beep.
Maybe some other way ? For now I can't get better idea.