Arduino 1.0

mgros · Post by **mgros** » Thu Dec 01, 2011 8:50 pm

There is a new arduino IDE version 1.0.

But compiling Multiwii with this new version I have this error. :?:

core.a(HardwareSerial.cpp.o): In function `__vector_26':
C:\Temp\arduino-1.0-windows (1)\arduino-1.0\hardware\arduino\cores\arduino/HardwareSerial.cpp:190: multiple definition of `__vector_26'
MultiWii_1_9.cpp.o:C:\Users\Mariano\AppData\Local\Temp\build3554363568454917800.tmp/MultiWii_1_9.cpp:3325: first defined here

A new task for C++ experts

mon_lolo_fr · Post by **mon_lolo_fr** » Thu Dec 01, 2011 11:44 pm

Hello,

same issue than you trying to compile a v1.9 on arduino 1.0 :-(

Will try to find out what's going on, you probably see that there was some change in the integration of the software serial library...

mgros · Post by **mgros** » Fri Dec 02, 2011 12:21 am

the problem is related with this interrupt:

Code: Select all

// ***********************************
// Interrupt driven UART transmitter for MIS_OSD
// ***********************************
static uint8_t tx_ptr;
static uint8_t tx_busy = 0;

ISR_UART {
  UDR0 = s[tx_ptr++];           /* Transmit next byte */
  if ( tx_ptr == point ) {      /* Check if all data is transmitted */
    UCSR0B &= ~(1<<UDRIE0);     /* Disable transmitter UDRE interrupt */
    tx_busy = 0;
  }
}

any solution???

timecop · Post by **timecop** » Fri Dec 02, 2011 5:34 am

Serial in Arduino is now interrupt based.
This is why.

frank26080115 · Post by **frank26080115** » Fri Dec 02, 2011 5:48 am

Does MultiWii plan on simply switching to using Serial.write to easily integrate with Arduino 1.0?

timecop · Post by **timecop** » Fri Dec 02, 2011 6:31 am

The sensible solution would be moving off Serial class alltogether especially with the conflicts it caused in spektrum input as well. Just import needed bits and modify. I don't think using new serial as-is will work, the buffering and stuff would need to be changed for serialize8/16 and OSD output.

Alexinparis · Post by **Alexinparis** » Sat Dec 03, 2011 4:57 pm

Hi,
I did some changes and uploaded a new dev snapshot compatible with arduino 1.0.
http://code.google.com/p/multiwii/downloads/list

There is however something I need to understand:
With this version of Arduino, the quality of my RC signal is worse (some minor variation).
I think is has something to do with an interrupt in the loop, which is handle differently (maybe the New Serial TX which is now interrupt driven)

timecop · Post by **timecop** » Sat Dec 03, 2011 5:12 pm

Yes, they have more interrupts running in background now. It can only get worse.

mgros · Post by **mgros** » Sat Dec 03, 2011 7:31 pm

Alex, with your new version activating SERVO TILT we have a compilation error.

mbrak · Post by **mbrak** » Sat Dec 03, 2011 8:12 pm

Hi

the Error in the Output section when compiling with the servo tilt option should be fixed with the following change:

line 352:

Code: Select all

 if ((rcOptions1 & activate1[BOXCAMSTAB]) || (rcOptions2 & activate2[BOXCAMSTAB])) {

rcOption was not declared. but you declared rcOptions1 and rcOption2

@alex please confirm that

Alexinparis · Post by **Alexinparis** » Sat Dec 03, 2011 10:04 pm

mbrak wrote:Hi

the Error in the Output section when compiling with the servo tilt option should be fixed with the following change:

line 352:
Code: Select all
 if ((rcOptions1 & activate1[BOXCAMSTAB]) || (rcOptions2 & activate2[BOXCAMSTAB])) {
rcOption was not declared. but you declared rcOptions1 and rcOption2

@alex please confirm that

Yes, there are still rcOption typo errors.
I didn't check servo tilt option with the 2 new AUX possibilities.

Alexinparis · Post by **Alexinparis** » Wed Dec 07, 2011 11:47 pm

dongs wrote:The sensible solution would be moving off Serial class alltogether especially with the conflicts it caused in spektrum input as well. Just import needed bits and modify. I don't think using new serial as-is will work, the buffering and stuff would need to be changed for serialize8/16 and OSD output.

This is exactly what I've just done (new upload in the trunk repository).
I won't rely anymore on Arduino Serial functions which are too much high level and not optimized.

Like I2C, there are now dedicated Serial functions in multiwii code.
The code compile fine with Arduino 1.0 and is jitter free on RC channels like before.

Hamburger · Post by **Hamburger** » Sat Dec 10, 2011 10:00 pm

Alex,
the new Serial code does induce jitter/glitches/instability on motor output (or originally on sensor reads?) when sending serial data for telemetry. See my other thread for more info, please. We diagnosed it down to the new Serial code pretty sure now. But I cannot fix it any further now.
Cheers, Hamburger

PatrikE · Post by **PatrikE** » Sun Dec 11, 2011 3:59 pm

My ATAVRSBIN1 produces 1000 I2C errors/sec...?
It works with 1.9 before all i2c_stop(); was introduced.

Iw'e teasted all combinatinons with internal pullups and I2C speed.
It's conected via an LLC without Diod on D12 .

It was flying almost perfect on 1.9 before!

Whats happend.?
/Patrik

Danal · Post by **Danal** » Fri Dec 16, 2011 7:56 pm

Sorry guys, i've been totally offline with some family issues. I'm back.

As of the introduction of the Spektrum code, which is interrupt based, the Multiwii sketch contains a copy of the Arduino-0022 serial library in the serial tab. This was required because Arduino core has ABSOLUTELY no way to override or share an interrupt. Therefore, we copied the entire core library and added #ifdefs to suppress the core interrupt handlers when certain things are defined in Multiwii.

This means that, when a new Arduino release ships, the core serial library must be re-copied into the Multiwii serial tab and the #ifdefs put in place again. To make this as easy as possible, my rule is to NEVER change the library code at all. Not even one line or one character. Just add the #ifdefs to fully remove entire blocks of code when appropriate.

So, now that I am back, I will do this in shared.

Danal

ziss_dm · Post by **ziss_dm** » Sat Dec 17, 2011 4:39 am

Hi Danal,

Why we need interrupt based handler for Spectrum? As I understand it sends 16 bytes frames every 11ms?
It would be simpler to handle this from main loop (Same as SBUS).

regards,
ziss_dm

Danal · Post by **Danal** » Sat Dec 17, 2011 5:57 pm

ziss_dm wrote:Hi Danal,

Why we need interrupt based handler for Spectrum? As I understand it sends 16 bytes frames every 11ms?
It would be simpler to handle this from main loop (Same as SBUS).

regards,
ziss_dm

The spektrum data stream does not contain any for of "sync" byte, and really doesn't contain any byte or bytes that are consistent in any way. It is not possible to figure out where the "start of frame" is via any type of data content pattern matching. Instead, exact timing is used to figure out the start of frame. Using any form of built-in interrupt handler, or polling the USART for bytes, 'hides' the timing.

Additionally, interrupt handling is more efficient and elegant.

And, last thought... Arduino 1.0 might have a better way to have user sketch interrupts. Maybe. I'll check.

ziss_dm · Post by **ziss_dm** » Sat Dec 17, 2011 9:38 pm

Hi,

You receiving frames every 11ms, our loop is 2-3ms. Yo have plenty of time to implement pause sync.

Additionally, interrupt handling is more efficient and elegant.

Efficient? The interrupt handler takes ~20mks to exit. For each byte..

Elegant? Copy-paste HardwareSerial is elegant?

regards,
ziss_dm

captaingeek · Post by **captaingeek** » Sun Dec 18, 2011 12:30 am

which version of arduino does work with 1.8?

http://arduino.cc/en/Main/Software

PatrikE · Post by **PatrikE** » Sun Dec 18, 2011 12:36 am

22 & 23

Danal · Post by **Danal** » Sun Dec 18, 2011 2:18 am

ziss_dm wrote:Hi Danal,

Why we need interrupt based handler for Spectrum? As I understand it sends 16 bytes frames every 11ms?
It would be simpler to handle this from main loop (Same as SBUS).

regards,
ziss_dm

Ummm... ziss, you do realize the SBUS code in RX using "SerialAvailable" and "SerialRead". These are defined in the Multiwii Serial tab, and use serialBufferRX. In turn, this is populated by:

Code: Select all

SIGNAL(USART_RX_vect){
  uint8_t i = (serialHeadRX[0] + 1) % SERIAL_RX_BUFFER_SIZE;
  if (i != serialTailRX[0]) {serialBufferRX[serialHeadRX[0]][0] = UDR0; serialHeadRX[0] = i;}
}

SIGNAL is the Arduino keyword for an interrupt service routine.

in other words, the SBUS code uses an interrupt receive. Reading the buffer is called from the main loop, but the actual receive is performed by the interrupt code shown above. So that 22mks for each byte... that happens for every SBUS byte as well. On the exact same interrupt vector used by the Spektrum code, namely "USART_RX_vect".

So, what was the problem again, exactly?

ziss_dm · Post by **ziss_dm** » Sun Dec 18, 2011 3:50 am

Hi,

I know, that serial library is buffered and this buffering is quite efficient. This handler finishes in 3mks. I just want to clarify, that there no obvious reason to implement frame deserialization inside interrupt handler. The current implementation has number of disadvantages:
1) It allocates additional buffer (64 bytes serial library + 16 bytes spectrum specific). In main loop, with state machine, you can parse it directly to the rawRC
2) Really long interrupt handler. As a result soft PWM is jittering around 40mks, which is quite noticeable.
3) too many conditional defines + copy-pasted hardware serial.
4) overcomplicated SYNC logic. In main loop you can sync really easy: if (!SerialAvailable) state = START;
5) etc..

regards,
ziss_dm

Danal · Post by **Danal** » Mon Dec 19, 2011 2:15 am

ziss_dm wrote:Hi,

I know, that serial library is buffered and this buffering is quite efficient. This handler finishes in 3mks. I just want to clarify, that there no obvious reason to implement frame deserialization inside interrupt handler. The current implementation has number of disadvantages:
1) It allocates additional buffer (64 bytes serial library + 16 bytes spectrum specific). In main loop, with state machine, you can parse it directly to the rawRC

Since the serial port cannot be used for spektrum and anything else at the same time, I can use the same buffer. No RAM overhead. I will change this.

2) Really long interrupt handler. As a result soft PWM is jittering around 40mks, which is quite noticeable.

This interrupt handler is not de-serializing or anything like that. It is simply setting some flags for the non-interrupt code, which does the serialization later. Perhaps it is the call to micros() that causes the issue. I'll see if there is another way

3) too many conditional defines + copy-pasted hardware serial.

Agreed. I find it interesting that the SerialAvailable and so forth are able to steal the USART_RX_vect vector without doing this... at the time I wrote the spektrum support, suppressing the core library (by copying and #ifdef-ing it) was the ONLY way to override an interrupt. I will investigate how the non-spektrum code is over riding these vectors. If there is a better way, I will use it.

4) overcomplicated SYNC logic. In main loop you can sync really easy: if (!SerialAvailable) state = START;

If this works with precise enough timing, I will do it this way. I will experiment. If this does work, obviously, the comments above mostly go away, as everything would be using the same buffers, the same interrupts, etc, etc.

ziss_dm · Post by **ziss_dm** » Mon Dec 19, 2011 4:39 am

Hi Danal,

As I undestand, something like this should work (Assuming serial is buffered and buffer >= 16 bytes and you call it from main loop every 1-8ms):

Code: Select all

#if defined(SPECTRUM)
#define SPEK_STATE_START     0x00
#define SPEK_STATE_PAYLOAD00 0x10
#define SPEK_STATE_PAYLOAD01 0x30
void  readSpectrum(){
  static uint8_t state = SPEK_STATE_START;
  static uint8_t val0;
  uint8_t avail = SerialAvailable(1);
  if (avail) {
    do {
      uint8_t val1 = SerialRead(1);
      switch (state & 0xF0) {
        case SPEK_STATE_START: 
          if (val1 == 0x01) {
            state = SPEK_STATE_PAYLOAD00; 
            // ToDo: Read status/RSI
            #if defined(FAILSAFE)         
              if(failsafeCnt > 20) failsafeCnt -= 20; else failsafeCnt = 0;   
            #endif 
          } else  
            val0  = val1;
          break;
        case SPEK_STATE_PAYLOAD00:  
          val0   = val1;
          state |= SPEK_STATE_PAYLOAD01;
          break;
        case SPEK_STATE_PAYLOAD01:  
          uint8_t C = 0x0F & (val0 >> SPEK_CHAN_SHIFT);
          if (C < 10)
          #if (SPEKTRUM == 1024)
            rcValue[C] = 988  +  (((uint16_t(val0 & SPEK_CHAN_MASK)  << 8)) | val1);
          #endif       
          #if (SPEKTRUM == 2048)   
            rcValue[C] = 988  + (((uint16_t(val0 & SPEK_CHAN_MASK)  << 8)) | val1) >> 1);
          #endif      
          if (((state++) & 0x0F) == 0x06)
            state = SPEK_STATE_START;
          else  
            state = (state & 0x0F) | SPEK_STATE_PAYLOAD00;
          break;
      }
      avail = SerialAvailable(1);
    } while (avail);
  } else
   state = SPEK_STATE_START;
}
#endif

It also should work for 9 channel receiver.

BTW: According to the rcg, second byte in the header is SYNC byte. And looks like this is true.. So, pause sync can be removed (first if..elese).

regards,
ziss_dm

Danal · Post by **Danal** » Mon Dec 19, 2011 4:13 pm

I get the philosophic idea behind the sample code. And, it has some neat features like the flags in the upper half of a byte, and the frame position (or pair position) in the lower half of the same byte. There are more examples that make it a nice bit of code.

However... The sample code has several issues when evaluated for its ability to parse what a Spektrum satellite physically does in the real world.

1) The phrase if (val1 == 0x01) {state = SPEK_STATE_PAYLOAD00; ... } will not work for all satellite types and bind modes. There is no sync byte. I have seen header bytes of 00 01, 03 12, 00 12, 00 00 with different firmware levels in the sats, and/or different bind modes. And that is all before the first signal hit... if a frame is lost by the satellite, all the example headers change.

Of course, this is easily fixed with the sample code's state machine: Just read and discard two bytes when in the start state and before transitioning to payload0 state. Easy fix.

2) The code as given can get out of sync with a frame. If there is nothing in the buffer when the code is called, everything works. If there is a full frame in the buffer when the code is called, everything works.

But... if the first byte has arrived at the exact instant this code is called, the code can "get ahead" of the arrival rate of the remainder of the buffer. Calculating from the Baud Rate indicates each byte takes 0.00008681 of a second to arrive, or approx 86 Microseconds. This is time for the 16Mhz processor to execute about 1,280 instructions. I haven't dis-assembled the example code, but it is possible it is fewer instructions than that. Yet it gets worse. The satellite does NOT send each serial byte exactly back-to-back. There is a small inter-byte delay. I've seen it on an O-Scope. I didn't write it down, because it wasn't important to how I was parsing the stream. So more instructions get executed than straight baud rate based math would indicate. If the code "gets ahead" of the arrival rate and exits the DO WHILE because there is no SerialAvailable, it (correctly) sets the start state. The next invocation of the code, 2 to 6 ms later, will start in a mid frame. 50/50 shot if the payload0 and payload1 is now in sync. This will result in corrupted values. And, yes, there are values that will pass the "C<10" check, but still be wrong.

Adding delays to this code to allow the bytes to arrive would cause this code to stay out of the main loop longer. To put the same idea a different way, the balancing act between being "non-blocking" and at the same time ensuring that a full frame is processed, that balancing act is not well served with any form of do while SerialAvailable that runs while bytes are arriving. Getting the full frame before checking it and parsing is much safer.

3) There are circumstances where the satellite will start to send a buffer but not send all 16 bytes. These should be discarded. This code directly updates "as it goes" and would not discard these.

And so on...

Having said all of that, your point about the time in the existing Spektrum interrupt handler is well taken. I will note there was no interrupt-based serial handler in the MultiWii code at the point in time when I originally wrote the Spektrum support. That's why a separate routine was written.

Things have changed.

Now that there is interrupt based serial receive directly in MultiWii, and now that Arduino 1.0 allows user sketches to override individual interrupt vectors, I agree that t there are ways to use the existing (fast) MultiWii serial interrupt handler. And to check for complete Spektrum frames, ways to be certain that the correct number of ms have elapsed to reliably detect a 'start', etc, etc, etc. Code coming soon...

Danal · Post by **Danal** » Mon Dec 19, 2011 6:15 pm

Danal wrote:I will note there was no interrupt-based serial handler in the MultiWii code at the point in time when I originally wrote the Spektrum support. That's why a separate routine was written.

I just realized that the change for the total elimination of any use of the core "Serial." libraries occured post-1.9. Now that the 'shared' branch makes no use of core serial, I will re-write the Spektrum support to use the new "Serial" (no dot) stuff that has been built in.

Danal · Post by **Danal** » Mon Dec 19, 2011 8:23 pm

Code ready. MUCH cleaner to use the Serial-no-dot stuff that is built in.

Can't upload because the 'shared' branch has a mix of .ino and .pde. Waiting for Hamburger to fix the repository.

Code is untested... so probably OK that I wait.

ziss_dm · Post by **ziss_dm** » Mon Dec 19, 2011 9:45 pm

Hi,

1) The phrase if (val1 == 0x01) {state = SPEK_STATE_PAYLOAD00; ... } will not work for all satellite types and bind modes. There is no sync byte. I have seen header bytes of 00 01, 03 12, 00 12, 00 00 with different firmware levels in the sats, and/or different bind modes. And that is all before the first signal hit... if a frame is lost by the satellite, all the example headers change.

As I said - according to RCG. From my test it always 0x01 even after lost frames. But this can be easily changed, to use only pause sync.

But... if the first byte has arrived at the exact instant this code is called, the code can "get ahead" of the arrival rate of the remainder of the buffer. Calculating from the Baud Rate indicates each byte takes 0.00008681 of a second to arrive, or approx 86 Microseconds. This is time for the 16Mhz processor to execute about 1,280 instructions. I haven't dis-assembled the example code, but it is possible it is fewer instructions than that. Yet it gets worse. The satellite does NOT send each serial byte exactly back-to-back. There is a small inter-byte delay. I've seen it on an O-Scope. I didn't write it down, because it wasn't important to how I was parsing the stream. So more instructions get executed than straight baud rate based math would indicate. If the code "gets ahead" of the arrival rate and exits the DO WHILE because there is no SerialAvailable, it (correctly) sets the start state. The next invocation of the code, 2 to 6 ms later, will start in a mid frame. 50/50 shot if the payload0 and payload1 is now in sync. This will result in corrupted values. And, yes, there are values that will pass the "C<10" check, but still be wrong.

Not sure I understand your point. The state machine has persistent state (sate and val0 are static). In case buffer is empty, it exits and continues next cycle. The "C<10" check is more to filter "FF" in additional frames for 9ch transmitters.

3) There are circumstances where the satellite will start to send a buffer but not send all 16 bytes. These should be discarded. This code directly updates "as it goes" and would not discard these.

Could you please explain those circumstances? And why this frames should be discarded.

And so on...

Like?

Slightly optimized version:

Code: Select all

#define SPEK_STATE_START     0x00
#define SPEK_STATE_HEADER01  0x10
#define SPEK_STATE_PAYLOAD00 0x20
#define SPEK_STATE_PAYLOAD01 0x30
void  readSpectrum(){
  static uint8_t g_state = SPEK_STATE_START;
  static uint8_t val0;
  register uint8_t state = g_state & 0xF0;
  register uint8_t cnt_  = g_state & 0x0F;
  if (SerialAvailable(1)) {
    do {
      uint8_t val1 = SerialRead(1);
      switch (state) {
        case SPEK_STATE_START:  
          val0  = val1;
          state = SPEK_STATE_HEADER01;
          break;
        case SPEK_STATE_HEADER01: 
            state = SPEK_STATE_PAYLOAD00; cnt_ = 0;
            // ToDo: Read status/RSI
            #if defined(FAILSAFE)         
              if(failsafeCnt > 20) failsafeCnt -= 20; else failsafeCnt = 0;   
            #endif 
          break;
        case SPEK_STATE_PAYLOAD00:  
          val0  = val1;
          state = SPEK_STATE_PAYLOAD01;
          break;
        case SPEK_STATE_PAYLOAD01:  
          state = SPEK_STATE_PAYLOAD00;
          if ((cnt_++) == 0x06) state = SPEK_STATE_START;
          uint8_t C = (val0 >> SPEK_CHAN_SHIFT) & 0x0F;
          if (C < 10)
          #if (SPEKTRUM == 1024)
            rcValue[C] = (((val0 & SPEK_CHAN_MASK)  << 8) | val1) + 988;
          #endif       
          #if (SPEKTRUM == 2048)   
            rcValue[C] = (((val0 & SPEK_CHAN_MASK)  << 8) | val1) >> 1) + 988;
          #endif      
          break;
      }
    } while (SerialAvailable(1));
  } else state = SPEK_STATE_START;
  g_state = state | cnt_; 
}

Flash: ~120 bytes
SRAM: 2 bytes

regards,
ziss_dm

Danal · Post by **Danal** » Mon Dec 19, 2011 11:42 pm

Most of the information on RCGroups regarding Spektrum Satellite data streams is wrong and/or incomplete. I read the RCG threads at first, did some experiments with a bunch of sats, TXs, and an O-Scope, and gradually realized I couldn't trust most of what was said. For example: The header bytes I gave are all from physical captures; reading RCG, there is only a small subset of the things that occur in captures. Regarding your tests, I'm predicting you are seeing the 0x01 on a particular satellite firmware level, and bound to a particular TX, and in a particular mode (1024 v 2048). One easy modification to your test: Bind in 2048 mode. Your header bytes will become 0x00 0x00 (prior to first frame loss). Anyway... At some point, when I have more time, I plan to write up ALL the captures, and my analysis,on RCG.

Regarding the other issues: Explaining these issues takes a lot of time. Developing code is frequently about style, and I don't code in your style (and would never expect you to code in my style). We agree about eliminating the special interrupt routine now that there is an existing interrupt based receive routine to re-use; we agree about using the same buffers... and so forth.

I've coded to my choices about time/pause based sync, and have eliminated the things we agree to eliminate. I am about to test. I'll publish as soon as possible.

ziss_dm · Post by **ziss_dm** » Tue Dec 20, 2011 12:05 am

Hi,

Regarding the other issues: Explaining these issues takes a lot of time. Developing code is frequently about style, and I don't code in your style (and would never expect you to code in my style). We agree about eliminating the special interrupt routine now that there is an existing interrupt based receive routine to re-use; we agree about using the same buffers... and so forth.

Is this main point of discussion?

I've coded to my choices about time/pause based sync, and have eliminated the things we agree to eliminate. I am about to test. I'll publish as soon as possible.

It would be nice, if you can avoid calls to micros() which disables interrupts and int32 operations, which are expensive on avr.

Also, discarding "invalid" frames would require additional buffer. It would be nice to have justification for that.

Another question: Do we still need averaging in computeRC()? Looks like there is no visible jitter in the stream, so we can safelly remove that. And parse directly into rcData[]. This would economize another 80 bytes of SRAM!

regards,
ziss_dm

Danal · Post by **Danal** » Tue Dec 20, 2011 1:57 am

Calls to micros(): Already removed. Many other things removed, as discussed above.

Code coming soon...

ziss_dm · Post by **ziss_dm** » Tue Dec 20, 2011 2:24 am

Hi,

Cool! Now we are talking.

So, what do you think about averaging?

regards,
ziss_dm

Danal · Post by **Danal** » Tue Dec 20, 2011 5:05 am

Really good question. I tend to agree that averaging is not needed for an all-digital stream.

The code I just posted is sort of halfway. It is still integrated into computeRC and therefore into averaging. Maybe take that out later.

daniel · Post by **daniel** » Wed Dec 21, 2011 7:27 pm

Danal wrote:Most of the information on RCGroups regarding Spektrum Satellite data streams is wrong and/or incomplete.

Maybe this helps...

http://www.rcgroups.com/forums/showpost ... tcount=309

Is not the protocol used by a satellite, is the stream sent to a Spektrum RF transmitter, but I think I heard somewhere that's the same between a satellite and his main receiver..

The stream speed is 125000,8,N,1

Danal · Post by **Danal** » Wed Dec 21, 2011 11:53 pm

That's very valuable data about what the Transmitter sends in those two bytes. The Satellite does other things. Partially depending on how it was bound. I have a full table of traces with many sats and many TXs in many modes. Will publish later when I get time.

Danal · Post by **Danal** » Fri Dec 23, 2011 6:16 am

I just regressed out the Spektrum code rewrite that I put in R420. I did extensive testing and it was quiet unreliable. Reading a stream that can ONLY be synched by time stamp... reading that via "polling" just flatly doesn't work reliably.

Having said that, there are still a lot of things to be cleaned up in the existing interrupt handler. Using the same buffer that is now used for serial ports. The copy of the library (already gone). Getting rid of the call to micros(). And much more. I will do all of this... but for now, the 'shared' branch has been regressed to spektrum code that, while not ideally optimized, works reliably.

ziss_dm · Post by **ziss_dm** » Sat Dec 24, 2011 1:23 am

Hi Danal,

just flatly doesn't work reliably

Can you be more specific?

this is debug output to the o-scope:
Cyan: Serial Data
Yellow: pin toggle every cycle, phase toggle - header received.

SPK-Stream-debug.JPG: (19.91 KiB) Not downloaded yet

Looks quite healthy to me..

regards,
ziss_dm

Danal · Post by **Danal** » Sat Dec 24, 2011 9:45 am

The most blatant failure mode: Something delays the read. The serial buffer fills. The way the serial handler is currently written (not your code, the USARTn_RX_vect handlers in the serial tab), it stops reading UDRn when the buffer is about to wrap. Unfortunately, the ATmega hardware requires a UDRn read to clear the interrupt. Therefore, when the serial handler exits having not read this register, the interrupt re-occurs immediately (well, technically, one instruction later, by ATmega hardware spec). This repeats and the sketch comes to a grinding halt because all that is really happening is the USARTn_RX_vect interrupt over and over.

This is easy to create in MultiWii: Move the TX sticks in such a way as to enter LCD config. (of course, LCD Config has to be #defined, but you don't actually have to have an LCD to see the failure. To avoid serial conflicts with the Spektrum satellite, either use for an ATmega, or config a Pro Mini for an Eagle Tree Power Panel, which is i2c connected.) With any code that depends on periodically reading the buffers that are filled by the existing serial code, once you move the sticks to LCD config entry position, everything stops.

Danal · Post by **Danal** » Sat Dec 24, 2011 9:46 am

We started all this by saying that the new serial code offers an opportunity to accomplish several things:

Save memory by using the same buffer as the new serial code.
Have a faster interrupt routine by avoiding micros()
Have a faster interrupt routing by avoiding some of the serialization work
Make the interrupt routing fast enough to reduce or eliminate PWM jitter that exists with the old Spektrum interrupt routine.
Originally, re-use the Serial interrupt routine, which would meet the above four goals. The current status of that routine, hang the sketch if not read often enough, makes it unacceptable.
Save code space (this is a very low priority to me)
Have fewer #ifdefs (also a low priority)

Most of these things are on the way in some code I'm working right now. Not all of them, because what we didn't say was that the code had to be utterly reliable. I'm willing to trade a few bytes and a few clocks for that reliability. Not so many as to jitter PWM... but a few.

Danal · Post by **Danal** » Sat Dec 24, 2011 11:56 am

Oh, and after watching the GUI a bunch during debugging, with averaging on and off, I am VERY convinced to turn OFF averaging on these all digital feeds. I'll re-work computeRC a little bit so that only the "semi-analog" stuff, like PWM and Serial PPM, is averaged. The digital feeds we will leave alone.

Good suggestion.

Cheers,
Danal

ziss_dm · Post by **ziss_dm** » Sat Dec 24, 2011 12:18 pm

Hi Danal,
To overrun 64 bytes buffer you need to delay poll by 55ms. If it would happen in main cycle, you most likely crash anyway.

The LCD configuration loop can be easily adjusted, to avoid: delay(1000).

Or even better, to run in context of main cycle (to read real voltage and sensor data, instead of snapshot.) The soft serial for lcd delays only by 33ms.

The interrupt handler should be just fixed.

This is easy to create in MultiWii: Move the TX sticks in such a way as to enter LCD config. (of course, LCD Config has to be #defined, but you don't actually have to have an LCD to see the failure. To avoid serial conflicts with the Spektrum satellite, either use for an ATmega, or config a Pro Mini for an Eagle Tree Power Panel, which is i2c connected.) With any code that depends on periodically reading the buffers that are filled by the existing serial code, once you move the sticks to LCD config entry position, everything stops.

After removing delay() and calling readSpectrum() every 2ms, looks like it is working fine.

Have a faster interrupt routine by avoiding micros()
Have a faster interrupt routing by avoiding some of the serialization work

It would be hard to implement something more efficient than simple circular buffer.

The current status of that routine, hang the sketch if not read often enough, makes it unacceptable.

I think, this is because you implemented something different, than simple state machine.

Save code space (this is a very low priority to me)

I would prefer to have space in flash and sram for other features, like autonomous flight, etc..

regards,
ziss_dm

Alexinparis · Post by **Alexinparis** » Sat Dec 24, 2011 12:30 pm

Danal wrote:This is easy to create in MultiWii: Move the TX sticks in such a way as to enter LCD config. (of course, LCD Config has to be #defined, but you don't actually have to have an LCD to see the failure. To avoid serial conflicts with the Spektrum satellite, either use for an ATmega, or config a Pro Mini for an Eagle Tree Power Panel, which is i2c connected.) With any code that depends on periodically reading the buffers that are filled by the existing serial code, once you move the sticks to LCD config entry position, everything stops.

I don't understand the problem here.
There is only one LCD that rely on Serial RX: LCD_TEXTSTAR.
- Every read is protected by SerialAvailable => no blocking state is possible with a Read call.
- This test is done once per loop, and this LCD only send one char per key pressed => I don't see how it's possible to reach the buffer limit which is 64 by default.

Danal · Post by **Danal** » Sat Dec 24, 2011 6:04 pm

Alexinparis wrote:
Danal wrote:This is easy to create in MultiWii: Move the TX sticks in such a way as to enter LCD config. (of course, LCD Config has to be #defined, but you don't actually have to have an LCD to see the failure. To avoid serial conflicts with the Spektrum satellite, either use for an ATmega, or config a Pro Mini for an Eagle Tree Power Panel, which is i2c connected.) With any code that depends on periodically reading the buffers that are filled by the existing serial code, once you move the sticks to LCD config entry position, everything stops.

I don't understand the problem here.
There is only one LCD that rely on Serial RX: LCD_TEXTSTAR.
- Every read is protected by SerialAvailable => no blocking state is possible with a Read call.
- This test is done once per loop, and this LCD only send one char per key pressed => I don't see how it's possible to reach the buffer limit which is 64 by default.

It is not the serial for the LCD itself that crashes. It is the serial arriving on a different port, for SBUS or Spektrum. Any code that delays the read of the other port will cause the crash the way the interrupt handler currently works.

Any interrupt handler should clear the interrupt that calls it. As written, these serial handlers sometimes leave UDRn unread, which causes an interrupt 'loop'. The fix is easy, and brings the code more into line with the Arduino core. After the buffer is full, do you prefer discarding the oldest or newest data? See viewtopic.php?f=8&t=1034 for more.

Danal · Post by **Danal** » Sat Dec 24, 2011 8:18 pm

ziss_dm wrote:Hi Danal,
To overrun 64 bytes buffer you need to delay poll by 55ms. If it would happen in main cycle, you most likely crash anyway.

44ms. The buffer holds 63 bytes, not 64. It won't take the 4th frame.

Most receivers have to drop a lot more than 4 frames to go into "lockout" or "failsafe" or whatever.

Danal · Post by **Danal** » Sat Dec 24, 2011 8:26 pm

The fundamental way to sync with Spektrum frames is timing. Any type of polled read does not know the exact time of arrival of each byte. This is too risky for me. Example: You said to take the delays out of LCD Config. At the moment, LCD config uses blinkLED in several places, and the way it is used causes enough delay to blow up a polled read. If you take the delays caused by the blinkLED out, then LCD Config 'skips' or 'jumps; when adjusting parameters. Of course, LCD config could be rewritten... but this is just one example.

It is possible to write a very efficient interrupt handler, one that actually uses a SMALLER buffer than the existing serial handler, for timestamping Spektrum frames. Saves bytes. Code on the way... standby...

ziss_dm · Post by **ziss_dm** » Sat Dec 24, 2011 11:15 pm

Hi Danal,

The buffer holds 63 bytes, not 64

64. Simple case head=0 tail=63 - holds 64 bytes, next byte will be rejected to prevent head=tail (which is empty condition).

The fundamental way to sync with Spektrum frames is timing.

Yes, but time can be discrete and measured in cycles/ticks/etc.

The simplest solution for LCD, without rewriting anything(computeRC() - parsing stream and puts result in rcData[]):

Code: Select all

for (i = 0; i < 50; i++) {computeRC(); delay(2)};
for (i = ROLL; i < THROTTLE; i++) {lcdStickState[i] = (rcData[i] < MINCHECK) | ((rcData[i] > MAXCHECK) << 1);};
...

This ensures, that we parsed at least 1 valid frame before checking stick state, in expense of small delay, which is not noticeable.

regards,
ziss_dm

Danal · Post by **Danal** » Sun Dec 25, 2011 5:38 am

ziss_dm wrote:Hi Danal,
The buffer holds 63 bytes, not 64

64. Simple case head=0 tail=63 - holds 64 bytes, next byte will be rejected to prevent head=tail (which is empty condition).

Your example has 1 byte.

Code: Select all

uint8_t SerialAvailable(uint8_t port) {
  return (SERIAL_RX_BUFFER_SIZE + serialHeadRX[port] - serialTailRX[port]) % SERIAL_RX_BUFFER_SIZE;
}

(64 + 0 - 63) % 64
(1) %64
1 Available.

The correct example is: Tail = 0, Head = 63.
(64 + 63 - 0) % 64
(127) % 64
63 Available.

Or, consider this. Tail = 0, Head = 63. Can't receive a new character because head would wrap to zero. Start reading characters and tail advances. When it hits 62, a character will be read, when it hits 63, no char read because head=tail which means empty. So, 0 through 62 tail means 63 characters read.

Or, Call 'SerialAvailable" and put the result in a variable that is displayed in a debug slot in the GUI. Inject data. The largest number you'll see is 63. This not an index... it is a count... and it never passes 63.

ziss_dm · Post by **ziss_dm** » Sun Dec 25, 2011 6:47 am

Yes,

You right, it keeps one byte "open" to distinct between full and empty. I missed that.

regards,
ziss_dm