MultiWii

Hamburger wrote: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)?

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.

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.

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.

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.

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

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

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.

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.

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.

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

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.

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

everything with underscore or all upercase is const or a define

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

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.

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

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.

EOSBandi wrote: Your bitmask implementation (which is NOT a bitmask anyway) is actually reinventing bitfields, with the unnecessary code obfuscation.

Hamburger wrote:From past experience with protocols in other projects, I propose to include one new item of information to the protocol, namely the serial protocol version number. Currently, we have only MSP_IDENT which iincludes copter type and mwc version number.

For gui implementations it will become helpful to know which version of the protocol the mwc side speaks, for the protocol will evolve and change over time. This has advantages in cases of
- added/removed messages
- changed messages (like int8 -> int16, added vars)

With a protocol version number, gui coders can
- verify if their gui code speaks the same protocol version as the mwc and decide whether the gui is capable to interpret the mwc side on a solid basis, or tell the user the gui has to take a guess
- preserve compatibility for several protocol versions if they want and do a switch based on protocol number.

This 'serial protocol version number' should be completely independent from other version numbers (mwc version, eeprom version). It gets incremented with each change to the protocol.
What do you think?

Hamburger wrote:This 'serial protocol version number' should be completely independent from other version numbers (mwc version, eeprom version). It gets incremented with each change to the protocol.
What do you think?

Tommie wrote:As a side note, I tried implementing self describing PID items; it kind of works, but takes up quite a bunch of flash memory:

https://github.com/wertarbyte/multiwii- ... ...pidconf
https://github.com/wertarbyte/multiwii- ... ...pidconf

#define P_PREC_1         0x00   //00 00 00 00   0-255
#define P_PREC_10          0x40   //01 00 00 00   0.0 - 25.5
#define P_PREC_100         0x80   //10 00 00 00   0.00 - 2.55
#define P_PREC_1000         0xc0   //11 00 00 00   0.00 - 0.255

#define I_PREC_1         0x00   //00 00 00 00
#define I_PREC_10          0x01   //00 01 00 00
#define I_PREC_100          0x02   //00 10 00 00
#define I_PREC_1000         0x03   //00 11 00 00

#define D_PREC_1            0x00   //00 00 00 00
#define D_PREC_10           0x04   //00 00 01 00
#define D_PREC_100          0x08   //00 00 10 00
#define D_PREC_1000         0x0c   //00 00 11 00

#define I_VISIBLE           0x02   //00 00 00 10   
#define D_VISIBLE           0x01   //00 00 00 01


#define PID_ROLL            P_PREC_10+I_PREC_1000+D_PREC_1+I_VISIBLE+D_VISIBLE
#define PID_MAG             P_PREC_10
#define P_POSHOLD_RATE      P_PREC_100+I_PREC_100+I_VISIBLE

EOSBandi wrote:Alex,
Do you have any plans on freeze of serial protocol for 2.1 release ? I would like to move ahead with the WinGUI but i postponed it until the serial protocol is not stabilised.

copterrichie wrote:

EOSBandi wrote:Alex,
Do you have any plans on freeze of serial protocol for 2.1 release ? I would like to move ahead with the WinGUI but i postponed it until the serial protocol is not stabilised.

It would be nice if the serial routine was contained in a .dll file so that the entire GUI does not have to wait.

EOSBandi wrote:For example i'm not a fan of the self describing serial protocol but if Alex decides to go there then the gui have to adopt.

kos wrote:in fact we should add a toggle flag for all requestable . so we can construct any osd , any gui with any refresh rate for any components

we just have to use the unused payload as the toggle flag

Code: Select all

no payload 
   // send reply and stop
else
  payload = read8
 // payload == 5 ->send reply @5 hz

Pyrofer wrote:I second this request. In fact I would like it as a simple compile time option to force GUI data out.

That way a GPS could send its data into the serial port while the GUI received the telemetry out at the same time. This would make life MUCH easier for me

The GPS doesn't need to receive anything so other than not having the option to configure PID etc while doing this there is no problem. I would take that minor issue over having to choose between GUI/GPS at the moment.