New Multiwii Serial Protocol

This forum is dedicated to software development related to MultiWii.
It is not the right place to submit a setup problem.
Software download
User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: New Multiwii Serial Protocol

Post by kos »

the is only related to how the gui was flooding the fc ;)

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

via USB? So you are using an ATMega32U4?

copterrichie
Posts: 2261
Joined: Sat Feb 19, 2011 8:30 pm

Re: New Multiwii Serial Protocol

Post by copterrichie »

USB Serial.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

So just a plain serial interface, just like the rest of us uses (via USB, via Bluetooth, or even to the COM port).
The problem is that the GUI is sending far more requests than the copter can handle; that's why we are reducing the request frequency, to give the copter some time to transmit the answer.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:Serialize32 and serialize16 are no more equivalent in the current code

Of course not, they never were. One serializes uin32_t, the other uint16_t, and both can be reduced to variants of serialize8.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:3) then uncheck SERIAL_GPS
based on r885:
code size impossible: 31420
based on 0606:
code size: 30550

Due to renaming, I cannot produce a proper.diff to locate the key differences; but why is a file size of 31420 byte impossible? With 32kB of flash memory, that should be flashable. And before you reply, I am aware of the Arduino bootloader. But if you build a quadcopter for quite some money, why not invest in a proper ISP programmer device and get rid of the Arduino bootloader? That way, you save roughly 2kB of flash memory, without having to abandon any feature. At ~8€, USB programmers are probably one of the cheapest components of a multi rotor vehicle.

By the way: I just compiled your configuration (QUADX, BOARD_PROTO_1, GPS_SERIAL) with the latest version - SERIAL_GPS does not exist, so I think you are implying GPS_SERIAL? Although configuration cos claim it is only possible on MEGA boards?

Code: Select all

stefan@exa:~/git/multiwii-firmware [alex_size…] $ avr-size build/MultiWii.hex 
   text      data       bss       dec       hex   filename
      0     26388         0     26388      6714   build/MultiWii.hex
stefan@exa:~/git/multiwii-firmware [alex_size…] $


Looks more than OK to me. Can you find out the compiler settings Arduino uses? The above is compiled using -Os (size optimization), and even with -O1 it still within limits:

Code: Select all

stefan@exa:~/git/multiwii-firmware [alex_size…] $ avr-size build/MultiWii.hex 
   text      data       bss       dec       hex   filename
      0     27486         0     27486      6b5e   build/MultiWii.hex
stefan@exa:~/git/multiwii-firmware [alex_size…] $


Even if I use -O2, which is known to produce big code, the result is still suitable for the 328p:

Code: Select all

stefan@exa:~/git/multiwii-firmware [alex_size…] $ avr-size build/MultiWii.hex 
   text      data       bss       dec       hex   filename
      0     28772         0     28772      7064   build/MultiWii.hex
stefan@exa:~/git/multiwii-firmware [alex_size…] $


// edit: I did some research, Arduino uses -Os
Last edited by Tommie on Sun Jun 10, 2012 10:02 am, edited 1 time in total.

User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: New Multiwii Serial Protocol

Post by kos »

atmega2560 use also --relax

Code: Select all

   
    // For atmega2560, need --relax linker option to link larger
    // programs correctly.
    String optRelax = "";
    String atmega2560 = new String ("atmega2560");
    if ( atmega2560.equals(boardPreferences.get("build.mcu")) ) {
        optRelax = new String(",--relax");
    }
 List baseCommandLinker = new ArrayList(Arrays.asList(new String[] {
      avrBasePath + "avr-gcc",
      "-Os",
      "-Wl,--gc-sections"+optRelax,
      "-mmcu=" + boardPreferences.get("build.mcu"),
      "-o",
      buildPath + File.separator + primaryClassName + ".elf"
    }));

    for (File file : objectFiles) {
      baseCommandLinker.add(file.getAbsolutePath());
    }

    baseCommandLinker.add(runtimeLibraryName);
    baseCommandLinker.add("-L" + buildPath);
    baseCommandLinker.add("-lm");

    execAsynchronously(baseCommandLinker);

    List baseCommandObjcopy = new ArrayList(Arrays.asList(new String[] {
      avrBasePath + "avr-objcopy",
      "-O",
      "-R",
    }));

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Now serial is broken again.The ATMega firmware cannot keep up with the number and speed of request that arrive at once; so the TX buffer fills and overwrites itself. This is why the checksums fail.

So how do you explain with a 256 buffer, there is no more error of checksum ?
(I did the test with the last rev and the old serial code)

With both buffers at 256 byte, I am down to 185 bytes,

based on the last rev with the Arduino IDE compiler only, please indicate the opions to check and where to put the checking sequence to obtain this result ?

Another thing: before adding your patch, did you see there is already some code to check the free mem (DEBUG case F:)

Due to renaming, I cannot produce a proper.diff to locate the key differences; but why is a file size of 31420 byte impossible?

It's not a question of bootloader size or code problem.
It's very tied to multiwii: 99% users will use Arduino to configure and upload the code on the board.


By the way: I just compiled your configuration (QUADX, BOARD_PROTO_1, GPS_SERIAL) with the latest version - SERIAL_GPS does not exist, so I think you are implying GPS_SERIAL? Although configuration cos claim it is only possible on MEGA boards?

I meant GPS_SERIAL, but right it's in fact GPS_PROMINI_SERIAL xyz which is here to be able to connect a serial GPS device on a 328p FC board. The doc is not up to date right now.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

once again in the last rev, the GUI is very laggy (3 or 4 update per second)

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:
Now serial is broken again.The ATMega firmware cannot keep up with the number and speed of request that arrive at once; so the TX buffer fills and overwrites itself. This is why the checksums fail.

So how do you explain with a 256 buffer, there is no more error of checksum ?
(I did the test with the last rev and the old serial code)

I think this is obvious? Because a large buffer will fill less quickly. You can push more bytes into it before the ring buffer bites its own tail.
With both buffers at 256 byte, I am down to 185 bytes,

based on the last rev with the Arduino IDE compiler only, please indicate the opions to check and where to put the checking sequence to obtain this result ?

I placed the checks in two locations, main loop and tinygps_query(). And this is the minimum value. If I put more checks into it, it will only get lower.
Another thing: before adding your patch, did you see there is already some code to check the free mem (DEBUG case F:)

Since I do not own an LCD, I did not look there. It's also not really useful to place that check in the LCD routine itself, since it will only yield the amount of free memory in that stack location. To acquire a valid result, this call has to be sprinkled all over the code, perhaps even via an ISR.
It's very tied to multiwii: 99% users will use Arduino to configure and upload the code on the board.

And how many of those will run into problems because they use a small controller while loading their copter with every sensor they can find? You can't have your cake and eat it, too.
Do you think they will be happier if they can flash the slightly smaller firmware, only to watch their copter disappear out of sight after they lost control due to heap/stack corruption?
Last edited by Tommie on Sun Jun 10, 2012 10:47 am, edited 1 time in total.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:once again in the last rev, the GUI is very laggy (3 or 4 update per second)

Can't reproduce. I tried it this night, it was quite responsive.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

I repeat the question:
Based on the last rev with the Arduino IDE compiler only, please indicate the options to check and where to put the checking sequence to obtain this result ?

Do you think they will be happier if they can flash the slightly smaller firmware, only to watch their copter disappear out of sight after they lost control due to heap/stack corruption?

I'm currently not convinced by the heap/stack corruption argument in the current code.


Since I do not own an LCD, I did not look there. It's also not really useful to place that check in the LCD routine itself, since it will only yield the amount of free memory in that stack location. To acquire a valid result, this call has to be sprinkled all over the code, perhaps even via an ISR.

RAM size instantaneous consumption can be evaluated I would say in a gap +/- 50 bytes.
So this method is good for me.
I see no functions in multiwii that could explain a sudden +100 bytes consumption due to local instance.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie wrote:
Alexinparis wrote:
Now serial is broken again.The ATMega firmware cannot keep up with the number and speed of request that arrive at once; so the TX buffer fills and overwrites itself. This is why the checksums fail.

So how do you explain with a 256 buffer, there is no more error of checksum ?
(I did the test with the last rev and the old serial code)

I think this is obvious? Because a large buffer will fill less quickly. You can push more bytes into it before the ring buffer bites its own tail.

that would not explain that in the long run (1 or 2 minutes), there is no errors at all

User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: New Multiwii Serial Protocol

Post by kos »

please do no revert all the changes regarding the serial code , you can do whatever with the internal handling ..

but the boxname and pidname and some other enhancement (unknown msp and pid desc ) can help leverage the gui and third party software .

also we should have a full java gui that do not lag very soon ;)

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:I repeat the question:
Based on the last rev with the Arduino IDE compiler only, please indicate the options to check and where to put the checking sequence to obtain this result ?

As Arduino uses avr-gcc, there is no difference. I cannot provide you my config for the moment, but unless you know of any options that drastically shrink the RAM use, I do not consider it significant; it is a valid configuration that can be flashed and works - although it crashes the copter violently and unspectingly after some time due to RAM constraints.

I already did the math for you in a few postings before. Stack and heap are scarce. And you do not notice until it is too late.

Do you think they will be happier if they can flash the slightly smaller firmware, only to watch their copter disappear out of sight after they lost control due to heap/stack corruption?

I'm currently not convinced by the heap/stack corruption argument in the current code.

I am kind of flattered. You are talking as if this were a completely abstract problem. I have some very concrete scratches in my furniture that tell otherwise. Reducing the buffer sizes and removing variables fixed that major malfunction. So what do you think was the cause of my rcData arrays getting overwritten?

RAM size instantaneous consumption can be evaluated I would say in a gap +/- 50 bytes.
So this method is good for me.
I see no functions in multiwii that could explain a sudden +100 bytes consumption due to local instance.

Do you know what's not good for me? A copter plowing through my living room - but at least it has a lot of free flash memory!

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:that would not explain that in the long run (1 or 2 minutes), there is no errors at all

Sure it does. The receive buffer has to bridge the time between two calls to serialCom. The send buffer has too be big enough to hold all the reply data that has been generated by the messages processed until they data has been transmitted.

Increasing the buffer buys time to empty it; if the buffer fills fast than being transmitted, we get into trouble, especially, if we have sudden bursts of big messages (BOXNAMES, PIDNAMES).

User avatar
Hamburger
Posts: 2578
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: New Multiwii Serial Protocol

Post by Hamburger »

Tommie wrote:
Hamburger wrote:You may not know this, but _shared is meant as a common basis for working code as in work in progress. It is not your personal playgorund. It is not ok to leave your basic coding errors for others to stumble upon.

Right, because code from _shared has always been completely reliable and never ever failed before. Like the time MAG calibration and debug data (along others) in the GUI did not work _for_weeks_. No one noticed, except me while tinkering with the sonar device, so I submitted a patch.
Or by the other time, when merging smashed two functions in the GPS code into each other, yielding a compiler error everytime GPS was included?
Or by the time some of the GPS changes made by EOSBandi were garbled by merging process, which only got noticed after I refactored some of it yesterday to remove compiler warnings?

irrelevant hogwash.
This is about your error. You submitting a patchset that fails on basic inspection, something you obviously never did.
You are way too convinced of yourself to be taken seriously.

If I was your parent I'd say 'an apology is in order'. Having followed your arrogance on others' work and your ignorance disguised as friendly 'lets discuss' while ignoring arguments presented leads me to question your qualities as a contributor to an ongoing open group effort.

"I do not like it" is not a technical argument. It's the kind of subjective statement I like to ignore.[/quote]
I leave to to the reader to revert back to the technical arguments given.
You have just given further proove of your attitude. Very sad.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie wrote:
Alexinparis wrote:I repeat the question:
Based on the last rev with the Arduino IDE compiler only, please indicate the options to check and where to put the checking sequence to obtain this result ?

As Arduino uses avr-gcc, there is no difference. I cannot provide you my config for the moment, but unless you know of any options that drastically shrink the RAM use, I do not consider it significant; it is a valid configuration that can be flashed and works - although it crashes the copter violently and unspectingly after some time due to RAM constraints.

I ask for a valid configuration with the current code. Not a specific code which we are not aware of.
I only ask for a valid config.h we can discuss on to evaluate if the ram size is an issue.
Without this argument, I'm still not convinced about the issue and some mods won't be clearly included.
Arduino uses gcc, right. But Arduino IDE gcc version and options is the reference I use.

There was ram memory problems in the past mainly due to powermeter array and LCD strings.
It was solved with progmem statement and 16 bits instead of 32 bits variables for powermeter.
Until now, I've never heard of other RAM problems.
I don't say there aren't, but I want to have an evidence based on a factual case.

I also want to understand why a RAM problem could "randomly" be the source of a malfunction.
If it's ok for a lot of cycles, it should be ok forever.

RAM size instantaneous consumption can be evaluated I would say in a gap +/- 50 bytes.
So this method is good for me.
I see no functions in multiwii that could explain a sudden +100 bytes consumption due to local instance.

Do you know what's not good for me? A copter plowing through my living room - but at least it has a lot of free flash memory!

For your specific developments, I don't know. But for the code presently in _shared this is my understanding.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:I ask for a valid configuration with the current code. Not a specific code which we are not aware of.

I am using the current HEAD of MultiWii_Shared. Yes, I also tried disabling my own specific modifications.
Here are the changes applied to config.h:

Code: Select all

#define QUADX
#define I2C_SPEED 400000L
#define WMP
#define BMA020
#define BMP085
#define HMC5883
#define MAG_ORIENTATION(X, Y, Z)  {magADC[ROLL]  = X; magADC[PITCH]  = Y; magADC[YAW]  = -Z;}
#define RCAUXPIN8
#define LED_FLASHER
#define LED_FLASHER_DDR DDRB
#define LED_FLASHER_PORT PORTB
#define LED_FLASHER_BIT PB4
#define LED_FLASHER_SEQUENCE ( (uint8_t) 0 )
#define LED_FLASHER_SEQUENCE_ARMED ( (uint8_t) (1<<0 | 1<<2) )
#define LED_FLASHER_SEQUENCE_MAX 0xFF
#define LANDING_LIGHTS_DDR DDRC
#define LANDING_LIGHTS_PORT PORTC
#define LANDING_LIGHTS_BIT PC0
#define LANDING_LIGHTS_AUTO_ALTITUDE 50
#define DISABLE_POWER_PIN
#define TINY_GPS
#define TINY_GPS_SONAR


I only ask for a valid config.h we can discuss on to evaluate if the ram size is an issue.
Without this argument, I'm still not convinced about the issue and some mods won't be clearly included.
Arduino uses gcc, right. But Arduino IDE gcc version and options is the reference I use.

And so do I.

Code: Select all

ii  arduino-core         1:1.0.1+dfsg-1       Code, examples, and libraries for the Arduino platform
ii  avr-libc             1:1.8.0-2            Standard C library for Atmel AVR development
ii  gcc-avr              1:4.7.0-2            The GNU C compiler (cross compiler for avr)

Compiler and libc are default dependencies of arduino-core. No manual installation of fancy version.

Until now, I've never heard of other RAM problems.
I don't say there aren't, but I want to have an evidence based on a factual case.

I'd gladly invite you for a beer, some flying and the inspection of the scratches in my living room furniture. It's real, I did not spend the last two days hunting down RAM because its fun, but because of pure necessity.
I also want to understand why a RAM problem could "randomly" be the source of a malfunction.
If it's ok for a lot of cycles, it should be ok forever.

That's why It's called a race condition, something that may/will occur in concurrent systems, very nasty, very hard to debug.
It might be running without traouble for hours, until an ISR gets triggered while the copter is executing some deeply nested function with enough local variables allocated to nearly touch the heap; then the additional stack frame of the ISR concludes the collision.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Hamburger wrote:irrelevant hogwash.

Sure. Other mistakes are irrelevant.
But firmware and GUI not being in sync for 2 hours due to my patches is a major international incident. Quick, get the red phone!

I am not willing to delve deeper into this; in my opinion, your postings have left the ground of technical discussion some time ago. I'd rather spend more time coding than feeding your personal agenda and/or vendetta.
If you have better a better solution for handling the serial communication, please step forward. If you want to improve the current implementation to meet your goal of perfection, feel free to do so. But don't complain about people climbing trees instead of going to the moon, especially if you are sitting on the park bench while doing so.

copterrichie
Posts: 2261
Joined: Sat Feb 19, 2011 8:30 pm

Re: New Multiwii Serial Protocol

Post by copterrichie »

I have a suggestion, why not split the Serial.ino file into two files, one for the GUI and a second for all other communications that their parties can summit as plug ins for their applications.

Edited: Provide a C Class structure and let the their party do what they wish in transmitting that to their application.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Here are the changes applied to config.h:

error: 'PB4' was not declared in this scope.
error: 'PC0' was not declared in this scope.

The code must compile with arduino IDE, not only with your gcc environment.

To evaluate this, let's for the moment put 4 instead of PB4 and 0 instead of PC0.

testing get_free_memory()
in main loop:
=> 576 bytes free

inside tinygps_query(void)
=> 455 bytes free

with 256 bytes rx&tx serial buffer:
in main loop:
=> 256 bytes free

inside tinygps_query(void)
=> 134 bytes
with a tricopter, it's even worse: 96 bytes

in the main loop with GPS_PROMINI_SERIAL instead of tynygps:
=> 237 bytes free

So, ok I understand now the concern. The example of 96 is too critic.
looking in the serial code:
things like:

Code: Select all

  if (headTX == sizeof(bufTX)-1) {
    headTX = 0;
  } else {
    headTX++;
  }

could be optimized (at least in size), 2 or 3 occurences noted:

Code: Select all

headTX++;
if (headTX == sizeof(bufTX)) {
  headTX = 0;
}


replacing serialize32 and serialize16 by not using serialize8 (which caused unusfull calls to UCSR0B |= (1<<UDRIE0);) can also shrink the code size.


So 2 things I'm still not ok with:
- FLAG mod which is not good at all for cycle time performances. 16 RAM bytes saved is really nothing regarding the tradeoff on performances.
- the laggy GUI on my side. I don't understand the need for a thread. a proper frame spacing can be managed inside the draw function. Anyway, I'm not against any GUI change as long as it works ;)

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:
Here are the changes applied to config.h:

error: 'PB4' was not declared in this scope.
error: 'PC0' was not declared in this scope.

The code must compile with arduino IDE, not only with your gcc environment.

To evaluate this, let's for the moment put 4 instead of PB4 and 0 instead of PC0.

Weird, perhaps an issue of compiler version. I noticed that the LCD code does not compile with any current GCC either, so there probably have been some changes.
So, ok I understand now the concern. The example of 96 is too critic.

Good, I'm glad we are on the same page now.

Code: Select all

replacing serialize32 and serialize16 by not using serialize8 (which caused unusfull calls to UCSR0B |= (1<<UDRIE0);) can also shrink the code size.


No, actually it will increase code size, because every function and every call to the buffer will have to include the boundary check.
So 2 things I'm still not ok with:
- FLAG mod which is not good at all for cycle time performances. 16 RAM bytes saved is really nothing regarding the tradeoff on performances.

Do you know that there is an actual impact? What is the difference between the same firmware with plain old flag variables and the same one with bitmask? And does it really matter? There are people running their copters at 8 Mhz, so their processing takes double the time, and they still fly. 16 bytes are not that much, but it can mean the difference between flying and crashing.

//EDIT: OK, it seems gcc is not smart enough to optimize it thoroughly. I just tried this macro which reduces flash size again:
#define get_flag(f) ( (flag_mask[f/8] & (1<<f%8)) != 0 )

//EDIT2: we can also force gcc to inline the function:

Code: Select all

uint8_t inline get_flag(enum mwc_flag f) __attribute__((always_inline));

Takes a little bit more flash, but provides type safety.

since f is constant, the compiler can unravel the entire construct during compilation. I cannot test it right now (because my copter is unavailable, but it looks promising.


- the laggy GUI on my side. I don't understand the need for a thread. a proper frame spacing can be managed inside the draw function.

No. Doing I/O in the GUI thread is something that every Java guide frowns upon. And for good reason: Why should the speed of your GUI responses be limited by the transmission speed of your modem?
What if your webbrowser would handle things like that? As long as data is transmitted, the GUI freezes. Good times. That's why nearly every software written in the last 10 years does not use blocking IO.

User avatar
Hamburger
Posts: 2578
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: New Multiwii Serial Protocol

Post by Hamburger »

Tommie - move over.

_shared is called _shared for a reason. Your actions have turned it into a lottery.
If you want to do major conversions, use the branches section and invite others to verify.

It is tiresome to have to follow your 'Tommie knows better, Tommie against the rest of the world' agenda.
The MultiWii project is a joint group effort and not well suited as your personal ego booster.
You may not have realized it yet, but your actions are a hindrance to others and annoying.

Now cut the crap, do whatever coding you want to do, and stop monopolizing development in _shared.

User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: New Multiwii Serial Protocol

Post by kos »

the lottery was there , waiting for a long serial burst to appears.. there is not much work left before completion

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie wrote:Do you know that there is an actual impact? What is the difference between the same firmware with plain old flag variables and the same one with bitmask? And does it really matter? There are people running their copters at 8 Mhz, so their processing takes double the time, and they still fly. 16 bytes are not that much, but it can mean the difference between flying and crashing.


I would say around 0.3ms, ie 10% with a 10DOF sensor.
Yes it really matters because our goal was always to reduce as possible this critical loop time.
And again, flash memory is a precious resource.
It's hard to evaluate the differences in flight for a small delay like this. A lot of things are empirical ans generally speaking, a smaller loop offers better performances.
A WMK+NK combination offers a 6ms delay, and the difference is very noticeable in flight.
We can fly with a 8MHz proc like does kos, but probably not with a great stability using a 10DOF sensor.

So I suggest a rollback on this feature.

For the GUI: I have a 32 bit windows XP, and until now I never encountered any GUI latency problem since the beginning of the project. I tried to reduce "static int request_interval = 0;" just for testing, and it's better.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Hamburger wrote:Tommie - move over.

_shared is called _shared for a reason. Your actions have turned it into a lottery.
If you want to do major conversions, use the branches section and invite others to verify.

It is tiresome to have to follow your 'Tommie knows better, Tommie against the rest of the world' agenda.
The MultiWii project is a joint group effort and not well suited as your personal ego booster.
You may not have realized it yet, but your actions are a hindrance to others and annoying.

Now cut the crap, do whatever coding you want to do, and stop monopolizing development in _shared.


Hi Hamburger,

I understand your feeling.
Things have been "forced", and it was not the right way to do it whatever the quality of the contribution.
I was nice enough to not make a whole rollback **this time** because we need to finalize the serial protocol for the 2.1 and I saw good things added.
But you can be confident, things won't be handled like this anymore.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:And again, flash memory is a precious resource.
It's hard to evaluate the differences in flight for a small delay like this. A lot of things are empirical ans generally speaking, a smaller loop offers better performances.
A WMK+NK combination offers a 6ms delay, and the difference is very noticeable in flight.
We can fly with a 8MHz proc like does kos, but probably not with a great stability using a 10DOF sensor.

So I suggest a rollback on this feature.

Please don't, replacing all those variables was extremly tiresome. Have you tried the macro I posted? It reduces flash size again und should be quite easy to optimize for the compiler, boiling down to or single XOR.
For the GUI: I have a 32 bit windows XP, and until now I never encountered any GUI latency problem since the beginning of the project. I tried to reduce "static int request_interval = 0;" just for testing, and it's better.

I'll try another thing; instead of sending requests at a fixed interval, the GUI can keep track of how many requests are still being processed by the copter; so if we receive a reply, we can transmit the next request immediatly. This way, the firmware buffers only have to deal with one answer at a time, while transmitting the data as fast as possible.

User avatar
kos
Posts: 286
Joined: Thu Feb 16, 2012 4:51 am
Location: Fr

Re: New Multiwii Serial Protocol

Post by kos »

ok, the macro is better than the inline ,

now i am back in the 2800 ~ 3650 cycletime , before that it was 3150 ~ 3950 and never under 3000. speed improvement .

cycletime_serial.png


time are best and worst , not average .. with the latest shared , average with 10dof is ~3005

User avatar
Hamburger
Posts: 2578
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: New Multiwii Serial Protocol

Post by Hamburger »

Alexinparis wrote:
Hamburger wrote:Tommie - move over.

_shared is called _shared for a reason. Your actions have turned it into a lottery.
If you want to do major conversions, use the branches section and invite others to verify.

It is tiresome to have to follow your 'Tommie knows better, Tommie against the rest of the world' agenda.
The MultiWii project is a joint group effort and not well suited as your personal ego booster.
You may not have realized it yet, but your actions are a hindrance to others and annoying.

Now cut the crap, do whatever coding you want to do, and stop monopolizing development in _shared.


Hi Hamburger,

I understand your feeling.
Things have been "forced", and it was not the right way to do it whatever the quality of the contribution.
I was nice enough to not make a whole rollback **this time** because we need to finalize the serial protocol for the 2.1 and I saw good things added.
But you can be confident, things won't be handled like this anymore.


Hi Alex,
thank you. That is good news.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

OK, I did some modifications to the GUI as well as the firmware. It's looking good, and Kos already tested the changes last night.

What has been done to the GUI:

- serial communication is handled in a separate thread, so drawing the GUI and exchanging messages with the copter do not block or limit each other
- requests are queued, so that only one request is issued to the copter at a time; once a reply is received, the next request is transmitted; therefore the copter firmware does not need big buffers to accumulate multiple requests and answers
- entire chunks of data are read from the serial port instead of singly bytes; reading single byte (g_serial.read()) yields terrible performance depending in your OS and serial hardware; I know of people whose bluetooth transmitters made the GUI unusable. now we read as much as possible with each read-Request to limit performance impacts

Future plan:

get rid of the inBuf[] array in the firmware; the payload data of incoming packets is stored there, but since the data is also in the rx_buffer, we do not actually have to store them twice; a lot of wasted memory (and possibly flash) can be freed that way.

Slight update to the protocol:

Each request (gui->copter) starts with

$M<

after that, everything is included into a XOR checksum.

The next byte contains the payload size of the request (0-255, although currently the copter cannot process more than 64 bytes),

followed by the command id (1 byte).

If the declared payload size was greater than 0, the data follows next.

The packet is ended byte one byte of XOR checksum.

The reply starts with $M>; if the copter does not understand the requested message size, the header changes to $M!; the remaining data is structured like to request above.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie,

- I checked the last flag, and the performance impact is much better. From this point it’s ok. But it consumes around 200 bytes of flash mem. (just realize you exchange just 16 RAM bytes against 200 bytes flash mem, the business is not very fair)
- get rid of the inBuf[] array in the firmware is a nice purpose. Let’s see how it could be handle with no tradeof on performance.
- I’m not ok to add acknowledgment messages. It complexifys the protocol. And it consumes AGAIN the flash mem.
- I will test the GUI tonight


About flash size, take this as a reference:
- 328p board + Arduino 1.0 IDE
- #define QUADX
- one 10 DOF board with MS(FREEIMUv035_MS or BOARD_PROTO_1)
- #define GPS_PROMINI_SERIAL 57600

It was possible in version 0606
It is no more possible, and it is my need.

So please invest your brain also to reduce the flash size.
For instance,, optimize the flash size footprint of this:
int32_t dT = ms561101ba_ctx.ut.val - ((uint32_t)ms561101ba_ctx.c[5] << 8);
int64_t off = ((uint32_t)ms561101ba_ctx.c[2] <<16) + (((int64_t)dT * ms561101ba_ctx.c[4]) >> 7);
int64_t sens = ((uint32_t)ms561101ba_ctx.c[1] <<15) + (((int64_t)dT * ms561101ba_ctx.c[3]) >> 8);
temperature = 2000 + (((int64_t)dT * ms561101ba_ctx.c[6])>>23);


And you will be a hero

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:- I checked the last flag, and the performance impact is much better. From this point it’s ok. But it consumes around 200 bytes of flash mem. (just realize you exchange just 16 RAM bytes against 200 bytes flash mem, the business is not very fair)

I cannot confirm this. Using the macros, the compiler completely boils down the functions to a static XOR comparison; are you sure that there's no tother difference casing this?
- get rid of the inBuf[] array in the firmware is a nice purpose. Let’s see how it could be handle with no tradeof on performance.

Since we do not need to copy bytes around anymore, I even assume that it will increase performance.
- I’m not ok to add acknowledgment messages. It complexifys the protocol. And it consumes AGAIN the flash mem.

It does not complexify the protocol, it simplifies it. If you issue a request, you will receive a reply. Simple. And since most messages are answered anyway, it only affects a small bunch of messages, e.g. the ones saving PID values or launching calibration. You could even indicate success or errors with that, not something I'D like to dismiss.
About flash size, take this as a reference:
- 328p board + Arduino 1.0 IDE
- #define QUADX
- one 10 DOF board with MS(FREEIMUv035_MS or BOARD_PROTO_1)
- #define GPS_PROMINI_SERIAL 57600

It was possible in version 0606
It is no more possible, and it is my need.

Code: Select all

stefan@exa:~/git/multiwii-firmware [test_alex…] $ avr-size build/MultiWii.hex 
   text      data       bss       dec       hex   filename
      0     25958         0     25958      6566   build/MultiWii.hex


So even smaller than my personal firmware and well within the limits of an ATMega328p, even with bootloader. Which version of avr-gcc is your distribution of Arduino using?

At first, I was unable to compile your configuration with the Arduino IDE at first due to this errors:

Code: Select all

MultiWii.cpp:2448:9: error: ‘prog_char’ does not name a type
MultiWii.cpp:2449:9: error: ‘prog_char’ does not name a type


Looks like some errors creeped into the LCD.ino code, I'll see what I can do about that: https://code.google.com/p/arduino/issues/detail?id=795

// edit this was a glitch, see below
Last edited by Tommie on Mon Jun 11, 2012 4:03 pm, edited 1 time in total.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

OK, I fixed the errors in LCD.ino, so I can compile the configuration using the Arduino IDE; just a second...

//EDIT The LCD was not enabled in your config, I accidently tried compiling an old configuration I had still lying round. Fixed the errors regardless :-)

Your configuration yields a file of <27k, using the standard Arduino "compile" button:
Binäre Sketchgröße: 26.970 Bytes (von einem Maximum von 32.256 Bytes)

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Actually, flash usage has gone down. I just compiled a firmware image from the code before introducing get_flag/set_flag:

Code: Select all

#before
stefan@exa:~/git/multiwii-firmware [old_test] $ avr-size build/MultiWii.hex
   text      data       bss       dec       hex   filename
      0     27060         0     27060      69b4   build/MultiWii.hex
#current
stefan@exa:~/git/multiwii-firmware [tupper] $ avr-size build/MultiWii.hex
   text      data       bss       dec       hex   filename
      0     26636         0     26636      680c   build/MultiWii.hex

So more than 400 bytes of flash memory have been saved!

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie wrote:Actually, flash usage has gone down. I just compiled a firmware image from the code before introducing get_flag/set_flag:


So if you can't compare it properly, I do


comment:

Code: Select all

enum mwc_flag {
  FLAG_OK_TO_ARM,
  FLAG_ARMED,
  FLAG_I2C_INIT_DONE,
  FLAG_ACC_CALIBRATED,
  FLAG_NUNCHUKDATA,
  FLAG_ACC_MODE,
  FLAG_MAG_MODE,
  FLAG_BARO_MODE,
  FLAG_GPS_HOME_MODE,
  FLAG_GPS_HOLD_MODE,
  FLAG_HEADFREE_MODE,
  FLAG_PASSTHRU_MODE,

  FLAG_GPS_FIX,
  FLAG_GPS_FIX_HOME,

  FLAG_SMALL_ANGLES_25,

  FLAG_CALIBRATE_MAG,

  FLAG_CNT
};
uint8_t flag_mask[FLAG_CNT+7 / 8] = {0};
#define set_flag(f, v) ( (v) ? ( flag_mask[f/8] |= 1<<(f%8) ) : ( flag_mask[f/8] &= ~(1<<(f%8)) ) )
#define get_flag(f) ( !!(flag_mask[f/8] & (1<<f%8)) )


replace with:

Code: Select all

uint8_t FLAG_OK_TO_ARM;
uint8_t FLAG_ARMED;
uint8_t FLAG_I2C_INIT_DONE;
uint8_t FLAG_ACC_CALIBRATED;
uint8_t FLAG_NUNCHUKDATA;
uint8_t FLAG_ACC_MODE;
uint8_t FLAG_MAG_MODE;
uint8_t FLAG_BARO_MODE;
uint8_t FLAG_GPS_HOME_MODE;
uint8_t FLAG_GPS_HOLD_MODE;
uint8_t FLAG_HEADFREE_MODE;
uint8_t FLAG_PASSTHRU_MODE;
uint8_t FLAG_GPS_FIX;
uint8_t FLAG_GPS_FIX_HOME;
uint8_t FLAG_SMALL_ANGLES_25;
uint8_t FLAG_CALIBRATE_MAG;

#define set_flag(f, v) ( f=v )
#define get_flag(f) (f)



here is the result:

with the flag statement
23202

with the code equivalent to no flag
23032

23202-23032 = 170

170 bytes spent in flash mem, just for saving 15 bytes in RAM.
Do you really thing it's a good choice ?
Do you really think the code looks clearer with this mod ?

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

I cannot reproduce your 170 byte; I only get a difference of 26648-26544=104 bytes, which is more than I thought. And consider the circumstances of this modification: I was hunting down every single byte to keep my copter from wreaking havoc on its own ;-)

In this light, I have to admit that saving the flash memory should probably be given priority, although I like the definition of FLAGS in an enumeration; it makes the flags stand out from the code and accumulates them at one location for easy reference.
I'll do some more tests to come up with a solution that combines both approaches and takes the best of both with it.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie wrote:
About flash size, take this as a reference:
- 328p board + Arduino 1.0 IDE
- #define QUADX
- one 10 DOF board with MS(FREEIMUv035_MS or BOARD_PROTO_1)
- #define GPS_PROMINI_SERIAL 57600

It was possible in version 0606
It is no more possible, and it is my need.

Code: Select all

stefan@exa:~/git/multiwii-firmware [test_alex…] $ avr-size build/MultiWii.hex 
   text      data       bss       dec       hex   filename
      0     25958         0     25958      6566   build/MultiWii.hex


So even smaller than my personal firmware and well within the limits of an ATMega328p, even with bootloader. Which version of avr-gcc is your distribution of Arduino using?


I don't use avr-gcc, I use only Arduino IDE.

using a fresh r909

1) uncomment #define QUADX
2) uncomment #define FREEIMUv035_MS
3) uncomment #define GPS_PROMINI_SERIAL 57600

Binary sketch size: 31 050 bytes (of a 30 720 byte maximum)

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:I don't use avr-gcc, I use only Arduino IDE.

Which in turn uses an included version of? avr-gcc.
using a fresh r909

1) uncomment #define QUADX
2) uncomment #define FREEIMUv035_MS
3) uncomment #define GPS_PROMINI_SERIAL 57600

Binary sketch size: 31 050 bytes (of a 30 720 byte maximum)

As I said, when using the Arduino IDE 1.0.1 and the latest _shared, I receive this:
"Binäre Sketchgröße: 27.128 Bytes (von einem Maximum von 32.256 Bytes)"

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

It does not complexify the protocol, it simplifies it. If you issue a request, you will receive a reply. Simple. And since most messages are answered anyway, it only affects a small bunch of messages, e.g. the ones saving PID values or launching calibration. You could even indicate success or errors with that, not something I'D like to dismiss.

With another view, ok.
Each message will get a response.
It should allow some mutualization in the request, for instance tailSerialReply(); and maybe the beginning of headSerialReply()

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Alexinparis wrote:Each message will get a response.
It should allow some mutualization in the request, for instance tailSerialReply(); and maybe the beginning of headSerialReply()

Can you explain further what you have in mind?

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie wrote:
Alexinparis wrote:I don't use avr-gcc, I use only Arduino IDE.

Which in turn uses an included version of? avr-gcc.
using a fresh r909

1) uncomment #define QUADX
2) uncomment #define FREEIMUv035_MS
3) uncomment #define GPS_PROMINI_SERIAL 57600

Binary sketch size: 31 050 bytes (of a 30 720 byte maximum)

As I said, when using the Arduino IDE 1.0.1 and the latest _shared, I receive this:
"Binäre Sketchgröße: 27.128 Bytes (von einem Maximum von 32.256 Bytes)"


I think it is avr-gcc-4.3.2 which is also in the linux version of Arduino IDE

fiendie
Posts: 151
Joined: Fri Apr 20, 2012 4:22 pm

Re: New Multiwii Serial Protocol

Post by fiendie »

Alexinparis wrote:I think it is avr-gcc-4.3.2 which is also in the linux version of Arduino IDE

Some distributions decouple the compiler and use a separate package.
Arduino 1.0.1 on Mac OS X bundles 4.3.2 too, though.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

No, most linux distributions supply their own version of avr-gcc, just like Apple provides its own Java.

People have already filed bugs against the Arduino project for sticking to such an old compiler which generates big code, and I don't quite understand why they are not upgrading. Saving 4kB without changing a single line of code would be a huge win, but I guess we are hostages of Arduino guys here.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie wrote:People have already filed bugs against the Arduino project for sticking to such an old compiler which generates big code, and I don't quite understand why they are not upgrading. Saving 4kB without changing a single line of code would be a huge win, but I guess we are hostages of Arduino guys here.

Yes we are... Arduino guys should have a good reason behind
I don't say it's a good thing, I just say it's the reference for multiwii.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: New Multiwii Serial Protocol

Post by Alexinparis »

Tommie,
Please do not modify the content of the MSP message.
If you feel that transmitting the head reference is useful, I prefer to add another message type.
Attitude message is something which is called very rapidly.
It should remain small and not include something that does not vary a lot.

Have you got another ideas in mind about the message content and message type ?
My purpose is to finalize this protocol definition asap so that OSD & GUI can have a reliable basis.

Tommie
Posts: 438
Joined: Sun Apr 08, 2012 9:50 am

Re: New Multiwii Serial Protocol

Post by Tommie »

Adding new message types should not break anything, but neither should appending new data to existing ones; I refrained from adding another message type for performance reasons, since transmitting two additional bytes in this message is essentially free, while adding an additional message is not. It also ensures that heading and reference heading are always in sync.

User avatar
Hamburger
Posts: 2578
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: New Multiwii Serial Protocol

Post by Hamburger »

about the flags:

before code looked like this:

Code: Select all

if (rcOptions[BOXGPSHOLD]) {
  if !GPSModeHold & !GPSModeHome) {
    GPSModeHold = 1;

which has turned into

Code: Select all

if (rcOptions[BOXGPSHOLD]) {
  if (!get_flag(FLAG_GPS_HOLD_MODE) & !get_flag(FLAG_GPS_HOME_MODE)) {
    set_flag(FLAG_GPS_HOLD_MODE, 1);

Now that is less readable and looks - not aesthetically good.
Also, following our latest coding style agreements it still needs fixing and will end up as

Code: Select all

if (rcOptions[BOXGPSHOLD]) {
  if (!GET_FLAG(FLAG_GPS_HOLD_MODE) & !GET_FLAG(FLAG_GPS_HOME_MODE)) {
    SET_FLAG(FLAG_GPS_HOLD_MODE, 1);

Is there any benefit in this that could justify such eye sore (with or without syntax highlighting)?

timecop
Posts: 1880
Joined: Fri Sep 02, 2011 4:48 pm

Re: New Multiwii Serial Protocol

Post by timecop »

All I have to say about this is, freakin move off 8bit junk already :)

</flame>
Cost, availability, level of entry, etc, hasn't been an issue for years.
Normal end-user should NEVER have to "compile" firmware, you make everything dynamic and have tons of flash/ram/etc left over.

Pyrofer
Posts: 180
Joined: Sat Apr 14, 2012 2:55 pm

Re: New Multiwii Serial Protocol

Post by Pyrofer »

Dongs, sure, love to.
Post me a new all in one board with 32bit cpu and IMU and I will use it.

Sadly, a lot of people are limited by cost and thus stuck with the 8bit.
I applaud Alex for wanting to keep the 8bit option open to people with low budgets or existing hardware.

Post Reply