BUG - SERIAL_SUM_PPM channel mappiing is incorrect

[doughboy]

Its a little bit convoluted, but I can confirm this is a bug in multiwii code.
phenolic is correct tthat the mapping is reversed.

so lets say you defined SERIAL_SUM_PPM as YAW, PITCH, THROTTLE, ROLL (we'll just do the first 4 for simplicity)
as your channel order. and rcChannel array is initialized as follows
rcChannel[0] = 2 //YAW is 2
rcChannel[1] = 1 //PITCH is 1
rcChannel[2] = 3 // THROTTLE is 3
rcChannel[3] = 0 //ROLL is 0

so interrupt service handler will assign values to
rcValue[0] ->yaw value
rcValue[1]-> pitch value
rcValue[2]=>throttle value
rcValue[3]->roll value


so to get the rcData for THROTTLE channel, we actually need rcValue[2]

RX.ino code to get throttle value is

Code: Select all

uint16_t readRawRC(uint8_t chan) {
  uint16_t data;
  #if defined(SPEKTRUM)
    readSpektrum();
    if (chan < RC_CHANS) {
      data = rcValue[rcChannel[chan]];
    } else data = 1500;
  #else
    uint8_t oldSREG;
    oldSREG = SREG; cli(); // Let's disable interrupts
    data = rcValue[rcChannel[chan]]; // Let's copy the data Atomically
    SREG = oldSREG;        // Let's restore interrupt state
  #endif
  return data; // We return the value correctly copied when the IRQ's where disabled
}

for which if you request data for THROTTLE channel, it will return
rcValue[rcChannel[3]]
which evaluates to rcValue[0]
which is the value for ppm channel 0, and is the value for yaw.
so you can see that you requested data for throttle but got YAW instead.
proof the multiwii code is incorrect.

The fix is actually quite simple. change this

Code: Select all

    data = rcValue[rcChannel[chan]]; // Let's copy the data Atomically

to this

Code: Select all

    data = rcValue[rcChannel[rcChannel[chan]]]; // Let's copy the data Atomically

so in the throttle example, the result will now be

rcValue[rcChannel[rcChannel[3]]]
since rcChannel[3] is 0, which is ROLL then
rcValue[rcChannel[0]]
and rcChannel[0] is 2, which is YAW
so we now get the correct value
rcValue[2]
which is the 3rd ppm channel as defined in SERIAL_SUM_PPM
and is the correct value for throttle

Someone should make this fix in RX.ino, and give the proper warning to define SERIAL_SUM_PPM to the actual RX channel order instead of the current trial and error method to get the right combination.

[doughboy]

I tried a few more combination and the previous suggested fix won't work for other combination.
like THROTTLE, YAW, PITCH, ROLL order

I think that's why it works for some as is, but not for others. In the above sequence, the original code works as is.
I think the actual fix should be to reverse the value to array index.

something like

Code: Select all

uint8_t rxchans[RC_CHANS] = {SERIAL_SUM_PPM};
statoc uint8_t rcChannels[RC_CHANS];
for (int chan=0;chan<RC_CHANS;chan++) {
   rcChannel[rxchans[chan]] = chan ;
}

[doughboy]

another possible fix to make this more intuitive and easier to understand. This is consistent with PWM code, where ROLLPIN is defined and is assigned the pin number.

in config.h

Code: Select all

    //#define SERIAL_SUM_PPM
     //assign rx ppm channel number to function
      #define PPMROLL     3 
      #define PPMPITCH    2
      #define PPMYAW      1
      #define PPMTHROTTLE 0
      #define PPMAUX1     4
      #define PPMAUX2     5
      #define PPMAUX3     6
      #define PPMAUX4     7

in RX.ino

Code: Select all

#if defined(SERIAL_SUM_PPM) //Channel order for PPM SUM RX Configs
    static uint8_t rcChannel[RC_CHANS] = {PPMROLL, PPMPITCH, PPMYAW, PPMTHROTTLE, PPMAUX1, PPMAUX2, PPMAUX3, PPMAUX4, 8,9,10,11};

[johu]

Although I'm aware of the fact this is a really ancient thread, I still feel the need for a bump...

It seems to me this SERIAL_SUM_PPM bug is still present in the 2.3 and also dev version of the project.

I used the MultiWii 2.2 and 2.3 code with a PPM-sum-signal.
Setting up the channel order for the #define SERIAL_SUM_PPM line was always a trial-and-error process.
Up till last weekend when I got really bothered about this and tried to figure out why it doesn't work as expected.

First I'll explain what's going on and why it's going wrong, after I present a simple fix.

=====
In RC.cpp the "rcValue"-array holds the PPM-values of the received signal.

Let's assume I'm sending out a four channel PPM signal, in the order 1:Rudder, 2:Throttle, 3: Elevator, 4: Aileron
Also assume some values, Rudder: 1000, Throttle: 1100, Elevator: 1200, Aileron: 1300
This will yield rcValue[4] = {1000,1100,1200,1300}

Now assuming the following line in config.h
#define SERIAL_SUM_PPM YAW,THROTTLE,PITCH,ROLL // Signal order of my TX

Recalling line 5 in types.h

Code: Select all

  enum rc {ROLL,PITCH,YAW,THROTTLE,AUX1,AUX2,AUX3,AUX4,AUX5,AUX6,AUX7,AUX8}
//evaluated: 0 ,  1  ,  2,    3   ,  4 ,  5 ,  6 ,  7 ,  8 ,  9 , 10 , 11

In config.h, my line would evaluate into:

Code: Select all

#define SERIAL_SUM_PPM   2,3,1,0

Also assuming #define RC_CHANS 4, then the pre-processor will substitute in RC.cpp:line 29

Code: Select all

rcChannel[RC_CHANS]={SERIAL_SUM_PPM}
// Will result into
rcChannel[4] = {2,3,1,0}

So far so good right?

Now during runtime, at a certain moment computeRC() is executed.
It will iterate through the 4 channels and it should fill the "rcData"-array with data in the order Roll,Pitch,Yaw,Throttle
<< This is the internal channel ordering for the MultiWii project right? >>

Evaluating the first for-loop cycle (chan=0)
Ignoring the fact that there is some averaging going on here.
Following the lines, readRawRC goes into rcData4Values which goes into rcDataMean which goes into rcData, thus:
rcData[0] = readRawRC(0) is the endresult.

In the readRawRC(0) function, the line:
rcValue[rcChannel[0]]
would evaluate, for the above example (rcChannel index 0 has value 2), into:
rcValue[2]
Given the rcValue line from the example, index 2 has value 1200
rcValue[2] has value 1200

Manually checking the other for-loop cycles for chan=1,chan=2,chan=3 the rcData-array becomes:
rcData = {1200, 1300, 1100, 1000}

Now let's label this computed data rcData_computed, so:
rcData_computed = {1200,1300,1100,1000}
To verify this with the intended values, we must reorder the rcValue to match the correct order.
We had: rcVlaue = {1000,1100,1200,1300} with order YAW,THROTTLE,PITCH,ROLL
Reordering this into ROLL,PITCH,YAW,THROTTLE this would become:
rcData_intended = {1300,1200,1000,1100}

Now you can clearly see an inconsistency:
rcData_computed = {1200,1300,1100,1000}
rcData_intended = {1300,1200,1000,1100}

///// Proposed fix: /////
Change in file: RC.cpp line: 388 (altough this is for SPEKTRUM)

Code: Select all

      data = rcValue[rcChannel[chan]];

into

Code: Select all

      data = rcValue[chan];

And also for line: 393 (this is for SERIAL_SUM_PPM)
data = rcValue[rcChannel[chan]]; // Let's copy the data Atomically
into:
data = rcValue[chan]; // Let's copy the data Atomically


Also change line: 440-441

Code: Select all

          if ( rcDataMean < (uint16_t)rcData[chan] -3)  rcData[chan] = rcDataMean+2;
          if ( rcDataMean > (uint16_t)rcData[chan] +3)  rcData[chan] = rcDataMean-2;

into:

Code: Select all

          if ( rcDataMean < (uint16_t)rcData[rcChannel[chan]] -3)  rcData[rcChannel[chan]] = rcDataMean+2;
          if ( rcDataMean > (uint16_t)rcData[rcChannel[chan]] +3)  rcData[rcChannel[chan]] = rcDataMean-2;

///// End of fix /////

After this fix the channel order list for the pre-defined "#define SERIAL_SUM_PPM" lines in the config.h file need to be figured out.

Maybe someone can confirm this fix, and then incorporate this fix into the current dev-version of MultiWii.

[haydent]

bump, this is why we need a bug tracker. things like this get lost ... !

[Plüschi]

This seems to work in mwii.
Basically swap the table to the other side

Code: Select all

// instead of
rcData[chan] = readRawRC(rcChannel[chan]);
// do this
rcData[rcChannel[chan]] = readRawRC(chan);

Checked with graupner mx12 and futaba sg14. This swap seems to work fine on mwii, but i am still confused about.

[haydent]

so are you going to commit it, or does it need further dev or testing ?

[Plüschi]

I cant commit anything here.

[haydent]

ok, ill get some attention who can.

[Alexinparis]

Hi,

I understand the channel mapping for PPM is counter intuitive in the config.h section.

However we can't really speak about a bug, as the current relation in SERIAL_SUM_PPM is bijective and does the job.
I'm open to introduce any possible modification matching more the real PPM channel stream, but only if it doesn't introduce more computation time.
do you have any suggestion ?

[haydent]

will have to see, so yeah not so much a bug, but counter intuitive, and even just a note of such in the config.h next to that section would save users time and confusion, and take up no code space

[Plüschi]

Would be cool if someone comes up with a compile-time solution for this