MultiWii

Posted: **Sun Jun 10, 2012 3:04 pm**

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.

Posted: **Sun Jun 10, 2012 4:10 pm**

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

Posted: **Sun Jun 10, 2012 5:07 pm**

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.

Posted: **Sun Jun 10, 2012 5:51 pm**

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.

Posted: **Sun Jun 10, 2012 5:57 pm**

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

Posted: **Sun Jun 10, 2012 7:19 pm**

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.

Posted: **Sun Jun 10, 2012 7:30 pm**

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.

Posted: **Sun Jun 10, 2012 7:40 pm**

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.

Posted: **Sun Jun 10, 2012 10:42 pm**

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 .

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

Posted: **Sun Jun 10, 2012 11:01 pm**

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.

Posted: **Mon Jun 11, 2012 1:19 pm**

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.

Posted: **Mon Jun 11, 2012 3:03 pm**

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

Posted: **Mon Jun 11, 2012 3:28 pm**

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

Posted: **Mon Jun 11, 2012 3:45 pm**

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)

Posted: **Mon Jun 11, 2012 4:02 pm**

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!

Posted: **Mon Jun 11, 2012 9:27 pm**

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 ?

Posted: **Mon Jun 11, 2012 9:46 pm**

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.

Posted: **Mon Jun 11, 2012 9:56 pm**

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)

Posted: **Mon Jun 11, 2012 10:07 pm**

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)"

Posted: **Mon Jun 11, 2012 10:12 pm**

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()

Posted: **Mon Jun 11, 2012 10:18 pm**

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?

Posted: **Mon Jun 11, 2012 10:26 pm**

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

Posted: **Mon Jun 11, 2012 10:33 pm**

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.

Posted: **Mon Jun 11, 2012 10:36 pm**

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.

Posted: **Tue Jun 12, 2012 7:11 am**

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.

Posted: **Tue Jun 12, 2012 7:18 am**

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.

Posted: **Tue Jun 12, 2012 8:34 am**

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.

Posted: **Tue Jun 12, 2012 9:06 am**

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)?

Posted: **Tue Jun 12, 2012 9:09 am**

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.

Posted: **Tue Jun 12, 2012 9:19 am**

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.

Posted: **Tue Jun 12, 2012 9:22 am**

Hamburger wrote:Now that is less readable and looks - not aesthetically good.

But at least now it's plain obvious that these are boolean flags; something that wasn't the case with the old variable names.

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);

I disagree. Just because the functions have turned into macros their name shouldn't need to change.

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

It allows an easy retransition to a more efficient way of storing the flags, should the need arise to shift the balance between flash and RAM use.

Posted: **Tue Jun 12, 2012 9:22 am**

http://code.google.com/p/afrodevices/wiki/AfroFlight32

:-D

Posted: **Tue Jun 12, 2012 9:47 am**

Pyrofer wrote: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.

Stm32 from good luck buy is 23$ shipped.
Tarduino pro mini is what, 19.95?
Reuse your current imu.
I see no reason to keep 8bit

Posted: **Tue Jun 12, 2012 9:50 am**

Tommie wrote: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.

That's not my view. Reference heading is something that do not need to be refreshed at the same speed as attitude data.
attitude data message is something to represent the 3D orientation of the copter. nothing else.

Posted: **Tue Jun 12, 2012 9:53 am**

This is getting a bit off topic Dongs, so lets try not to hog the thread but,
"Stm32 from good luck buy is 23$ shipped.
Tarduino pro mini is what, 19.95?
Reuse your current imu.
I see no reason to keep 8bit"
My Controller board was £50 with arduino328p and IMU all IN ONE.
Do you see that last bit? ALL IN ONE. I cannot "keep" my IMU unless I keep my Arduino.
Plus, it doesn't matter how cheap the stm32 is compared to the Arduino because the point is I ALREADY have the Arduino.
If you GIVE me the stm32 and IMU to replace what I have already paid for at zero cost to me I will happily use them.

Expecting me to replace hardware and buy new is not going to work.

If I was coming at this new, with a fresh budget I WOULD use a stm32.
I am however, like many others already flying a quad with hardware. I would like to improve the FW rather than replace that hardware.

Posted: **Tue Jun 12, 2012 9:56 am**

It has to be refreshed at the same speed if you wish to deduce which reference frame is used for control inputs. ATTITUDE transfers the 3D orientation of the copter, as well as the direction it considers its "bow"; this is important if you send control commands to the device.

Posted: **Tue Jun 12, 2012 11:23 am**

De soldering avr and leaving sensor bits is trivial. Infact, you gain removal of stupid transistor based level shifting and other baggage related to no longer needing 5v i/o. The world moved off 5v i/o long time ago. It just sounds like you're making excuses, much like the rest of guys in this thread.

Posted: **Tue Jun 12, 2012 12:03 pm**

Dongs, I could discuss this with you for ages, but we are derailing the thread.
Instead PM me some links to good cheap stm32 boards.

Posted: **Tue Jun 12, 2012 4:13 pm**

Doug, with all due respect, the NAZE32 is a good board and when tuned correctly, it performs very well on my Bicopter however, the 8bit Promini previously installed on the copter performed equally as well. So going to a stm32 is purely subjective. As for MWC, if people want all of the newest features, then moving to an ATMega2560 is the answer in my opinion. Other flight controller have traveled this same path.

http://www.rcgroups.com/forums/showthread.php?t=1669324

Posted: **Tue Jun 12, 2012 10:28 pm**

Tommie wrote:
Hamburger wrote:Now that is less readable and looks - not aesthetically good.

But at least now it's plain obvious that these are boolean flags; something that wasn't the case with the old variable names.
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);
I disagree. Just because the functions have turned into macros their name shouldn't need to change.

You are entitled to your opinion. But it still is against the coding style that was agreed on recently. In case you cannot find it viewtopic.php?f=8&t=1412 And before you get on your high horse, it was not _my_ decision but consensus.

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

It allows an easy retransition to a more efficient way of storing the flags, should the need arise to shift the balance between flash and RAM use.

you want us to live with this monstrous code on the offhand chance that in an unforseeable future the code might need just this change? With that same argument you could justify about everything, like making all vars float or whatever.

I can not see any good reason to trade off the former concise version of

Code: Select all

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

against the code you proposed (see above to compare)

Now if we take a look at how other project handle this; from random linux drivers section they do the bitmasking in situ, so adopting their style would yield something like

Code: Select all

if (rcOptions[BOXGPSHOLD]) {
  if !(GPSflags & GPS_HOLD_MODE) & !(GPSflags & GPS_HOME_MODE) {
    GPSflags |= GPS_HOLD_MODE;

you get the idea. I do not advocate this, I do not know about effects on time and resources; just wanted to share another aspect.
Converting the current proposal in _shared to this could be done with some regexp.

Question remains: should we trade concise old style against any of the other flavours which bring
+ flags contain FLAG in their name
- source code increase
- reduced readability
- increased size
- no execution time improvement (or time penalty?)

Bottom line: does not look good. The old style has its merits still.
Take your pick

Posted: **Tue Jun 12, 2012 10:48 pm**

Hamburger wrote:You are entitled to your opinion. But it still is against the coding style that was agreed on recently. In case you cannot find it viewtopic.php?f=8&t=1412 And before you get on your high horse, it was not _my_ decision but consensus.

Actually, you made that proposal and no one disagreed; probably because there werent actually any function style macros in the code at that time.

you want us to live with this monstrous code on the offhand chance that in an unforseeable future the code might need just this change? With that same argument you could justify about everything, like making all vars float or whatever.

Invalid example. Actually using a #define or typedef to wrap the variable type is common practive to avoid error prone changes during refactoring.

Now if we take a look at how other project handle this; from random linux drivers section they do the bitmasking in situ, so adopting their style would yield something like
Code: Select all
if (rcOptions[BOXGPSHOLD]) {
  if !(GPSflags & GPS_HOLD_MODE) & !(GPSflags & GPS_HOME_MODE) {
    GPSflags |= GPS_HOLD_MODE;
you get the idea. I do not advocate this, I do not know about effects on time and resources; just wanted to share another aspect.
Converting the current proposal in _shared to this could be done with some regexp.

It's the same code, only more verbose and error prone. Since all arguments are constant, the compiler boils the macros down to exactly this.

- increased size
- no execution time improvement (or time penalty?)

Neither.

Posted: **Tue Jun 12, 2012 11:04 pm**

Tommie wrote:
Hamburger wrote:You are entitled to your opinion. But it still is against the coding style that was agreed on recently. In case you cannot find it viewtopic.php?f=8&t=1412 And before you get on your high horse, it was not _my_ decision but consensus.

Actually, you made that proposal and no one disagreed; probably because there werent actually any function style macros in the code at that time.

how ignorant do you want to play? viewtopic.php?f=8&t=1412&start=10#p11231 :

kos wrote:
Hamburger wrote:use Camel style (cycleTimeMax) or get an all capitals prefix for the realm like GPS_latitude ( I found BaroAlt difficult to identify, rather prefer BAROalt or BARO_alt)

everything with underscore or all upercase is const or a define, it is ok to use upper case for acronym or short word [1]

int GPSMaxSpeed = ok
define GPS_ALT_MAX 33000 = ok
int BAROalt = ok

int BARO_alt = bad
void INIT() = bad
int GPSHOME_latitude = bad

1 : http://en.wikipedia.org/wiki/CamelCase# ... _computing

Posted: **Tue Jun 12, 2012 11:19 pm**

So where does it state that functions turned into macros for performance reasons must use ALLCAPS? This does not make sense from a semantic point of view. Using ALLCAPS for #defined constants is common practice because they are literals; you cannot assign them or do anything else of the stuff usually done with variables, while function like #defines behave like functions in most common use cases.

everything with underscore or all upercase is const or a define

Check you propositional calculus. Implication does not imply equivalence.
A->B !=> B->A
A->B != A<->B

Posted: **Wed Jun 13, 2012 7:51 am**

Hi,

Generally speaking, reasons to mark all preprocessor macroses with all caps are outlined here:
http://www.cprogramming.com/tutorial/cpreprocessor.html

regards,
ziss_dm

Posted: **Wed Jun 13, 2012 8:12 am**

ziss_dm wrote:Generally speaking, reasons to mark all preprocessor macroses with all caps are outlined here:
http://www.cprogramming.com/tutorial/cpreprocessor.html

What exactly are you referring to?

"Typically, constants and macros are written in ALL CAPS to indicate they are special (as we will see)."

I do not see the need or advantage of using ALLCAPS for a macro that behaves exactly as the function it replaced.

Posted: **Wed Jun 13, 2012 9:52 am**

Hi,

Just to indicate that it is macro.. For example, it is quite tempting to do something like this:

Code: Select all

set_flag(FLAG_GPS_HOLD_MODE, !get_flag(GPS_HOLD_MODE) && !get_flag(FLAG_GPS_HOME_MODE));

and it would not work as expected..

If I know, that set_flag/get_flag are macroses, first thing I would check is parentneses in macro definition.

regards,
ziss_dm

Posted: **Wed Jun 13, 2012 10:05 am**

ziss_dm wrote:Hi,

Just to indicate that it is macro.. For example, it is quite tempting to do something like this:
Code: Select all
set_flag(FLAG_GPS_HOLD_MODE, !get_flag(GPS_HOLD_MODE) && !get_flag(FLAG_GPS_HOME_MODE));
and it would not work as expected.. If I know, that set_flag/get_flag are macroses, first thing I would check is parentneses in macro definition.

Oh, I see what you are getting at. But I'd consider that a bug in the macro definition, just like a faulty function may have unintended side effects.

Posted: **Wed Jun 13, 2012 10:39 am**

Tommie,

how many people does it take to change your opinion on something.

This is a community not a contest.

Please be cooperative and try not to force all you ideas into the code at once, this annoys people. Just slow down a little and do it bit by bit, so everyone else can follow and understand your changes.

I think the flags are causing more confusion and errors than advantages right now.

How about using your own branch for now, and then we compare the trunk to you branch.

Posted: **Wed Jun 13, 2012 10:54 am**

jevermeister wrote:How about using your own branch for now, and then we compare the trunk to you branch.

I've been placing bug reports and improvements with diffs (to my private branch) into this forum for quite some time now, and I made the experience that no one cares. If one isn't affected by a specific issue, it tends to be ignored.

Consider the RAM issue, which was the original trigger for me hunting down every spare byte of memory I could find, and finally yielded the bitmask implementation of flags as well as the reduction of the serial buffers; how long did it take until the existence of this actual major problem was actually acknowledged? I also reported multiple cases of undefined behaviour inside the code; no one cared, for their copters where flying. There are other examples, and it is quite frustrating to find and debug an error, create a fix, present it - only to let it rot away while the code moves on and your carefully crafted patch does not apply anymore and you can start over.

Posted: **Wed Jun 13, 2012 11:19 am**

Tommie wrote:
jevermeister wrote:How about using your own branch for now, and then we compare the trunk to you branch.

I've been placing bug reports and improvements with diffs (to my private branch) into this forum for quite some time now, and I made the experience that no one cares. If one isn't affected by a specific issue, it tends to be ignored.

Consider the RAM issue, which was the original trigger for me hunting down every spare byte of memory I could find, and finally yielded the bitmask implementation of flags as well as the reduction of the serial buffers; how long did it take until the existence of this actual major problem was actually acknowledged? I also reported multiple cases of undefined behaviour inside the code; no one cared, for their copters where flying. There are other examples, and it is quite frustrating to find and debug an error, create a fix, present it - only to let it rot away while the code moves on and your carefully crafted patch does not apply anymore and you can start over.

Tommie,
Your bitmask implementation (which is NOT a bitmask anyway) is actually reinventing bitfields, with the unnecessary code obfuscation. See my comparision in the relevant thread : viewtopic.php?f=8&t=1840
EOS

MultiWii

New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol

Re: New Multiwii Serial Protocol