Flag handling/coding style/committing rules and behavior

[jevermeister]

Hi,

I just want to give my two cents here.
First of all: I am not a native speaker in english so please excuse my improper use of common terms etc.

1. I do not like the overall tone the discussion about flags has. So please watch your language.
2. As a long term contributor to multiwii I saw recent changes in the shared trunk that were very fundamental and affected nearly every part of the code. I think changes like this needed to be approved by the coding community in a democratic style- one contributor must not change the code by force because he think it is working better now if the majority of contributors do not approve.

This is Alexinparis brainchild so we should first of all respect his opinion and take it very very serious. If the majority of the contributors do not approve a change or a feature add-on we should consider to remove the code in question. As a contributor I want to understand and folow the code.

Tommie: please get used to accept constructive critique and be open to reconsider your decisions.

I followed the recent changes and I do not see an advantage that justifies a radical change like this either. The flag thing is do hard to read.

Everybody has his own programming style - I for example am programming Windpower Turbines with PLC controller in ST and this is a whole other world but you cannot force changing the whole style like this.

I use a lot code changes that I add to my private code after downloading the code but I am not that arrogant to think that I can force my opinion onto the community - if my changes are not liked I will not add them to the trunk. EOL!

Multiwii has been a growing codebase for years and we are a very open community regarding optimization, but the recent development is not ok and I think we should stick our heads together and rethink this issue.

Let's work together on this mates!!!!

Nils

ps.: About the whole 8bit/16bit theme: I think this is stuff for another multicopter project - the advantage of multiwii has always been the easy programming, the open software, the cheap boards etc. If we take this away we would raise the entry step height a lot and would loose a large amount of growth.

If you want amxed out hardware etc - get a Mikrokopter and pay a fortune!

[shikra]

+1

I've been with Multiwii since beginning and its only in the last couple of weeks we have had such negative comments. (Multiwii thread 2nd birthday in 3 days guys...)

Put your code up for critique. Let the feedback roll in. If its accepted great, if not move on to the next. Maybe don't waste too much time on detailed coding unless concept is agreed as good

Alex ultimately decides...

[EOSBandi]

Okay folks,

I really not into this war, but what i misses here is some clear and concise comparation, so I made some. Ultimately it's up to Alex to decide, but I hope this comparison will helps him.

We are talking about consolidating flags to save SRAM, but meanwhile trying not to increase code size.

Tommie's proposal is an enum plus some inline macros to do bitwise operations.
pro : saves sram bytes by packing flags into bits
cons: increases code size (see below detailed comparision), makes code obfuscated, we have to use a macro which looks like a function at every point where a flag is set or get.

Original Method : every flag stored as an uint_8
pro: easier to read, less code space
con: wasting 1 byte sram for each and every flags.

There is a third way, which is abvious, but often overlooked: Bitfields
Packing flags into a bitfield struct,
for example :

Code: Select all

struct flags_struct {
uint8_t OK_TO_ARM:1 ;
uint8_t ARMED:1 ;
uint8_t I2C_INIT_DONE:1 ;
uint8_t ACC_CALIBRATED:1 ;
uint8_t NUNCHUKDATA: 1;
uint8_t ACC_MODE:1 ;
uint8_t MAG_MODE:1 ;
uint8_t BARO_MODE: 1;
...
 } f

f.OK_TO_ARM = 1;
if (f.OK_TO_ARM) { arming.....}

Pros: saves sram bytes by packing flags into bits, keeps the code as tidy as it was with the original method
Cons: have exactly the same code size impact at Tommie's proposal.


Code size check :
I was very suprised, when readed the thread, that nobody checked the actual assembly code which is generated from the above flags implementations. So I did it, here are the results. (please note, I took out optimalisations which are depends on actual sourrunding code)

Code: Select all

Tommie's proposal
set_flag(FLAG_OK_TO_ARM,0);
    90 91 00 02    lds   r25, 0x0200
    9e 7f          andi   r25, 0xFE   ; 254
   90 93 00 02    sts   0x0200, r25
   
bitfield
 flags.OkToArm = 0;
 80 91 01 02    lds   r24, 0x0201
 8e 7f          andi   r24, 0xFE   ; 254
 80 93 01 02    sts   0x0201, r24   
 
 Old method : flags are stored in uint8_t variables
 FLAG_OK_TO_ARM = 0;
 81 e0          ldi   r24, 0x00   ; 0
 80 93 00 02    sts   0x0200, r24
 
 
 Tommie's proposal
 if (get_flag(FLAG_SMALL_ANGLES_25) ) 
 80 91 01 02    lds   r24, 0x0201
 84 fd          sbrs   r24, 6
 
 Bitfield
 if (f.OkToArm)  
 80 91 00 02    lds   r24, 0x0200
 84 ff          sbrs   r24, 4
 
 Original method
 if ( FLAG_SMALL_ANGLES_25 )  
 80 91 01 02    lds   r24, 0x0201
 88 23          and   r24, r24
 21 f0          breq   .+8         ; 0x27a <loop+0x14>

As you can see the bitfield method is identical to Tommie's proporsal, and setting flags are plus 4 byte (one instruction)
However checking a flag is two byte larger when a flag is stored in an uint8.
Of course the compiller optimalisation can change these (for example clearing an uint8 flag is often even less code, since there are usually a register which contains zero).

There are other methods to decrease code and sram usage further, (using binded registers or use an unused SFR) but these methods take resources from the compiller otimalisation and eventually ends with larger code.

So if the decision is that the we really need that ~14 byte more RAM, even at the cost of the larger code size (we are talking about 100-200 bytes) then we should use a bitfield struct, this keeps code readable and have all other benefits that Tommie's proposal brought.

EOSBandi

[Hamburger]

+2.
The two former posts sum it up pretty good.

thanks to EOSBandi, for providing an unbiased technical view on the flags issue.

For those who want to rehash what jevermeister was referring to, you can find some of it buried in these threads here
viewtopic.php?f=8&t=1516&p=16139#p16129
viewtopic.php?f=8&t=1830
viewtopic.php?f=8&t=1516&start=80#p15828

Hamburger

[ziss_dm]

Hi
You also can switch between old and new method just by adjusting struct declaration and no need to adjust code.
Ziss_dm

[ronco]

hi,

i first liked tommies idea .. i like optimizon more then adding stuff
but if it just saves ~14 bytes i think its not worth it .. good code readability is also a important thing fore me.

so +1

@ tommie. i would like to say that i think the problems that we have here now are not realted to your ideas.. it is the way you implement them ..
pushing things without taking care what the community thinks or wants inst a fine way. and the way you respond to criticism is also not kind.
so if you acting without respect you should no be suprised if you also get no respect.

regards felix

[copterrichie]

Kudos to you jevermeister for bringing this public. I made a similar statement in a private statement but in retrospect, I should have done what you did here.

[jevermeister]

@ tommie. i would like to say that i think the problems that we have here now are not realted to your ideas.. it is the way you implement them ..
pushing things without taking care what the community thinks or wants inst a fine way. and the way you respond to criticism is also not kind.
so if you acting without respect you should no be suprised if you also get no respect.

I fully agree with your statement ronco!

@copterrichie: Thank you!

@eosbandi: thank you for your neutral and professional statement mate.

So what can we do? is there a way to exfiltrate the mess and bring the code to a good solution!? I added some stuff using tommie's new style so a simple revert would not work

Nils

[copterrichie]

We need a code review process before anything is committed to the shared / main trunk. IMO, there is just no way around this. Disagree is actually a good thing in its proper usage.

[EOSBandi]

Alex.
I recommend to roll back to r852 or somewhere around and revoke Tommmi's commit right. One disgruntled guy is better than a disgruntled community.
A

[PatrikE]

I'we keept in the background in this discussion because i think it look like a kindergaden war...
And i don't like the tone of the discussions.

Just follow the rules stated by the Head developer conserning contributors....
http://code.google.com/p/multiwii/source/browse/trunk/README.txt

From that view its qite clea for me.
Make your personal Branch as a playground and get some betaTesters to confirm functionality before you comitt to trunk.
Keep in mind Alex can remove _shared and use what he tink is suitable When he think so..
The shared is just a way to make implmentation easier for Alex so lets keep it that way.

The project have grown a lot since he opend the _shared.

As for the Flags i see only one advantage.
It's easier to find them in the code.

Otherwise i think they makes code less readable.

I don't want to ban anyone..
All idés are good idés... At the right time!..

And aways try to keep a Nice tone in dicussions.

/PatrikE

[copterrichie]

For the record, I don't believe in Good or Bad Ideas, just subjective views about issues. The problem here is not about Good or Bad ideas but the unwillingness to work as a team!

[Tommie]

Pros: saves sram bytes by packing flags into bits, keeps the code as tidy as it was with the original method
Cons: have exactly the same code size impact at Tommie's proposal.

I thought about using bifields, but I refrained from it for flexbility reasons; as you pointed out, the generated code is more or less the same, but by using struct syntax (flag.foo), you have nailed yourself down to that method. Using a macro, one can always shift between a bitor uint8_t based approach without having to change a single line of code.

It's often questioned whether sarificing 100-200kB of flash memory con be justified by freeing ~15 bytes of RAM; that depends on your configuration and use case. I had lots of flash to spare, but my copter was going berzerk after a while because of insufficient RAM. In this case, reducing variables was a clear choice; reclaiming 15 bytes of RAM can make quite a difference if your stack keeps trashing your heap.

By using a macro, everyone can pick their own poison; either grant each variable its own 8 bit, or smash them all together by using bitmasks.

[EOSBandi]

Pros: saves sram bytes by packing flags into bits, keeps the code as tidy as it was with the original method
Cons: have exactly the same code size impact at Tommie's proposal.

I thought about using bifields, but I refrained from it for flexbility reasons; as you pointed out, the generated code is more or less the same, but by using struct syntax (flag.foo), you have nailed yourself down to that method. Using a macro, one can always shift between a bitor uint8_t based approach without having to change a single line of code.

It's often questioned whether sarificing 100-200kB of flash memory con be justified by freeing ~15 bytes of RAM; that depends on your configuration and use case. I had lots of flash to spare, but my copter was going berzerk after a while because of insufficient RAM. In this case, reducing variables was a clear choice; reclaiming 15 bytes of RAM can make quite a difference if your stack keeps trashing your heap.

By using a macro, everyone can pick their own poison; either grant each variable its own 8 bit, or smash them all together by using bitmasks.

nope,
changing between 8bit or 1bit is simply remove the bit denominator from the struct, code will remain intact, and readable

[Tommie]

changing between 8bit or 1bit is simply remove the bit denominator from the struct, code will remain intact, and readable

I have to agree, this works great and is quite intriguing. I'm playing around with it and will publish a branch for comparison in a few minutes...

//edit: And here we are, just a few sed calls later: https://github.com/wertarbyte/multiwii- ... s_bitfield

The definition inside the flag struct could still be simplified; e.g.

Code: Select all

#ifdef USE_BITFLAGS
  #define BITFLAG(v) uint8_t v : 1
#else
  #define BITFLAG(v) uint8_t v
#endif

struct _flag_t {
  BITFLAG(OK_TO_ARM);
  BITFLAG(ARMED);
...
} flag;

[Hamburger]

Alex,

I recommend to roll back to r852 or somewhere around and revoke Tommmi's commit right. One disgruntled guy is better than a disgruntled community.

my thoughts entirely.
Hamburger

[Tommie]

I recommend to roll back to r852 or somewhere around and revoke Tommmi's commit right. One disgruntled guy is better than a disgruntled community.

my thoughts entirely.

And what will be gained by that?
https://github.com/wertarbyte/multiwii- ... les_bucket
Take a deep breath, look at a diff between r852 and r922ff and evaluate the code. And then explain why all these fixed compiler warnings, removed limitations and added features should be removed from the firmware. I'm not claiming that every patch in there is perfect, but you are proposing to get rid of all of that. I fail to comprehend the necessity for that; what damage does the outcome of these changes do? Which unfixable problems are still present in the codebase at the end that can only be mitigated by nuking several days of work, testing and coding?

[Alexinparis]

Which unfixable problems are still present in the codebase at the end that can only be mitigated by nuking several days of work, testing and coding?

Are you sure ?

Code: Select all

if ( (!get_flag(FLAG_GPS_HOME_MODE) && !get_flag(FLAG_GPS_HOME_MODE)) || !get_flag(FLAG_GPS_FIX_HOME) ) {

=> it's impossible you tested GPS POS HOLD with this code

GPS_FROM_OSD does not compile

Your own LED code on a genuine Arduino IDE does not compile (still the PB4 problem after we told you).

BUZZER+RCOPTIONSBEEP does not compile (static uint8 last_rcOptions[CHECKBOXITEMS];)

the GUI is still very laggy, and not only on my computer after your several attempts to fix the thread method

I said MultiWii is not a coding exercise. The code is maybe dirty for you, but it worked.

[Alexinparis]

I think jevermeister summarized well the feeling and some feedback I’ve already had from significant contributors ( a feeling I also share)

What I’m going to do:
1) Tommie was removed from the googlecode committer list, as we can’t continue like this. done
2) I’m going to remove some code insertion since 0606 where there is no consensus or no discussion. in progress

Note this is my first act of moderation in this project.

[Tommie]

Your own LED code on a genuine Arduino IDE does not compile (still the PB4 problem after we told you).

After your report, I included <avr/io.h> into MultiWii.ino (r903) before including the config; Using the Arduino IDE, I was unable to reproduce this compile time error and considered it fixed.

The other issues (GPS_FROM_OSD, BUZZER) were typos that just happen. They have already been addressed in my git branch.

Of course it's easy to test every possible configuration and function in this project to avoid such mistakes.

[Hamburger]

I think jevermeister summarized well the feeling and some feedback I’ve already had from significant contributors ( a feeling I also share)

What I’m going to do:
1) Tommie was removed from the googlecode committer list, as we can’t continue like this. done
2) I’m going to remove some code insertion since 0606 where there is no consensus or no discussion. in progress

Alex, thank you. So now we can go back to what we enjoy most.

Note this is my first act of moderation in this project.

Yes. I think you do well as project head . It brings along the power to decide, which is both a freedom and a duty and the wise use such measure sparingly. You did well pulling the plug now.

[copterrichie]

Alex, Not every person can handle being the head guy but you have done the job well. Kudos!

[Hamburger]

I almost forgot:
thank you everybody for taking a stand. It does feel good indeed to know that together we can withstand non-technical disturbances.

[Tommie]

I'd be even more delighted if technical disturbances would be tackled with such energy.