bad coding in sensor loop...

[csurf]

The following code structure exists in the state machine for the sensor loop...

Code: Select all

      if(taskOrder>4) taskOrder-=5;
      switch (taskOrder) {
        case 0:
          taskOrder++;
#if MAG
          if (Mag_getADC() != 0 ) break; // 320 µs
#endif
        case 1:
          taskOrder++;
#if BARO
          if (Baro_update() != 0 ) break; // for MS baro: I2C set and get: 220 us  -  presure and temperature computation 160 us
#endif
        case 2:
          taskOrder++;
#if BARO
          if (getEstimatedAltitude() !=0) break; // 280 us
#endif    
        case 3:
          taskOrder++;
#if GPS
          if(GPS_Enable) GPS_NewData();  // I2C GPS: 160 us with no new data / 1250us! with new data 
          break;
#endif
        case 4:
         taskOrder++;
#if SONAR
          Sonar_update(); //debug[2] = sonarAlt;
#endif
#ifdef LANDING_LIGHTS_DDR
          auto_switch_landing_lights();
#endif
#ifdef VARIOMETER
          if (f.VARIO_MODE) vario_signaling();
#endif
          break;
        }
    }

questions...
- why is the taskOrder variable increment statement redundantly included within each case block of the switch statement? It only needs to be coded once, either before or after the switch/case statement. It actually unnecessarily bloats the generated assembly code and simply looks ugly...

- why THE HELL is a subtraction op performed on the taskOrder variable in order to keep it within the range of 0-4:

Code: Select all

if(taskOrder>4) taskOrder-=5;

That does nothing for code clarity and only adds more overhead to the resultant machine code...
All that needs to be done is to set the variable to '0' and be done with it... Stuff like this can actually cause some pretty nasty bugs too.

I'd really like to know who wrote that, because this is just plain bad coding, and it hurts my eyes every time I look at it... Now that I'm diving more into the MWC code, I'm seeing a lot of bad coding practices, some small & others pretty big/dangerous...

Anyway, what I personally did was make the following changes...

Code: Select all

static uint8_t taskOrder=0; // never call all functions in the same loop, to avoid high delay spikes
switch (taskOrder) {
        case 0:
#if MAG
          if (Mag_getADC() != 0 ) break; // 320 µs
#endif
        case 1:
...
}
taskOrder = (++taskOrder > 4) ? 0 : taskOrder;

one line of code takes care of everything; no more redundant, nested increment statements, no more subtraction.
Anyway, perhaps one of the MWC code maintainers might want to consider cleaning up that section of code...

[QuadBow]

It appears to me that you have not understood the coding completely. That is no a shame.
But, lowering the performance of the developers by using words like BAD, HELL, HURT, etc sounds very arrogant...
By the way: your proposed changes seem to demonstate your skills, in no way there are clearer or more readable!

[csurf]

It appears to me that you have not understood the coding completely. That is no a shame.
But, lowering the performance of the developers by using words like BAD, HELL, HURT, etc sounds very arrogant...
By the way: your proposed changes seem to demonstate your skills, in no way there are clearer or more readable!

Hmmm, so sad, it also appears to me that you do not understand how to read code or how this particular switch case statement works... the taskOrder variable is incremented upon each loop iteration, so there's no point in it being down within each case. Not arrogant, it's just common sense. take it or leave it...

BTW, a ternary operator statement is a very basic coding structure (as is the use of the pre-increment operator). So, if people can't read/understand that, then perhaps they shouldn't be writing multi-rotor flight control code...

[timecop]

The "reason" the code does ++ in each switch() is because when there's no mag for example, it will just get incremented and fallthrough to the next (where it also might just get incremented etc). If there's no mag or baro or whatever, your version of taskorder will only increment by 1-2 points and > 4 condition will not work.

Anyway, not defending the original code, it is ugly as shit, as most of mwc code is.

[csurf]

The "reason" the code does ++ in each switch() is because when there's no mag for example, it will just get incremented and fallthrough to the next (where it also might just get incremented etc). If there's no mag or baro or whatever, your version of taskorder will only increment by 1-2 points and > 4 condition will not work.

Anyway, not defending the original code, it is ugly as shit, as most of mwc code is.

That *almost* makes sense in this context but it's still very bad form, and also doesn't explain the retarded 'taskOrder-=5' statement, but oh well. I'm starting to see that mwc is definitely not done by real coders, and there's a bunch of pasted toy code all over the place; looks like a lost cause trying to suggest anything since people get all touchy and butt-hurt about it...

[timecop]

Maybe you should join the CLAN then? Stop by #Multiwii on freenode and we can talk shit about mwc code all day long
(and maybe you'll move your constructive criticism over to baseflight...

[timecop]

I think -= was there because % is more expensive...
-=5 is a single? instruction in avr thats fast, foo % 5 is slower.. or someshit, who knows. its all fucking retarded. when you have to sacrifice code readability to do this kinda shit, its just a waste of time.

[csurf]

I think -= was there because % is more expensive...
-=5 is a single? instruction in avr thats fast, foo % 5 is slower.. or someshit, who knows. its all fucking retarded. when you have to sacrifice code readability to do this kinda shit, its just a waste of time.

% would be a lot slower since it's division...
I don't see the point of the math op in general when you can just say 'if(x > 4) x=0;' That seems to be the goal anyway, to set the upper bound of the iterator variable and have it go back to start. The weird, rigged switch/case should take care of handling all cases anyway...

Maybe you should join the CLAN then? Stop by #Multiwii on freenode and we can talk shit about mwc code all day long
(and maybe you'll move your constructive criticism over to baseflight...

hmmm, not sure if that's meant to be serious or sarcastic... I wouldn't mind working on on FC code if the vibe was right and attitudes were checked at the door. So far, from what I've experienced on RCG, DIYD, and here, that doesn't always seem to be the case. I'm just trying to get one of these stupid AVR-based FC's to work without crashing or flying off...

[nicog]

then stop by the channel, there are fools and bad people. but mainly funny people.

[sismeiro]

That *almost* makes sense in this context but it's still very bad form, and also doesn't explain the retarded 'taskOrder-=5' statement, but oh well. I'm starting to see that mwc is definitely not done by real coders, and there's a bunch of pasted toy code all over the place; looks like a lost cause trying to suggest anything since people get all touchy and butt-hurt about it...

Hi,

For starters, I am not a coder but I follow some projects for a few years. What usual happens is an ego war between some involved sooner or later. Even when the coders are at the same skill level, some technical decisions are made by the most influential one and some of them are political.

Most of the times is not the project with the "real coders" and where the best technical decisions are made that wins, it's the project that in the end is most useful. In the case of MultiWii, even with the "pasted toy code", it flies really well in most situations where we only want to have fun.

Now imagine if the great talented coders that can contribute MultiWii, joined in an coordinated effort to clean all the code and make it better? That would be the best of two worlds, a multicopter firmware that works very well and that is the best technical made. This message it to all of the people interested in MultiWii, not specific to the people on this thread. So lets all make an effort to contribute to the code and the project the best way we can.

Contribute with the patches and advocate them in a technical and polite way, don't expect them to be accepted at the first time. Some people are not looking at the forum everyday and some of them are just distracted. Others simply don't understand the code and others don't even bother or care. The sources are open, someone can always start a new branch, prove it's capability and contribute to the may tree later.

Regards,
Luis Sismeiro

[copterrichie]

The mindset here is to compete not collaborate.

compete: strive to gain or win something by defeating or establishing superiority over others who are trying to do the same

collaborate: work jointly on an activity, esp. to produce or create something

That is all.

[Alexinparis]

The "reason" the code does ++ in each switch() is because when there's no mag for example, it will just get incremented and fallthrough to the next (where it also might just get incremented etc). If there's no mag or baro or whatever, your version of taskorder will only increment by 1-2 points and > 4 condition will not work.

Anyway, not defending the original code, it is ugly as shit, as most of mwc code is.

That *almost* makes sense in this context but it's still very bad form, and also doesn't explain the retarded 'taskOrder-=5' statement, but oh well. I'm starting to see that mwc is definitely not done by real coders, and there's a bunch of pasted toy code all over the place; looks like a lost cause trying to suggest anything since people get all touchy and butt-hurt about it...

timecop gave you the exact reason about "++ in each switch".
nothing is almost true in a code, it is true or it is not and your code is not equivalent.
please explain us why it is bad to do like this.

about "if(taskOrder>4) taskOrder-=5;" I admit "=0" is probably cleaner
I personally don't feel "taskOrder = (++taskOrder > 4) ? 0 : taskOrder;" is easier to read
But thank you anyway because you make me think of another shortcut we can do in this part of code.

[csurf]

The "reason" the code does ++ in each switch() is because when there's no mag for example, it will just get incremented and fallthrough to the next (where it also might just get incremented etc). If there's no mag or baro or whatever, your version of taskorder will only increment by 1-2 points and > 4 condition will not work.

Anyway, not defending the original code, it is ugly as shit, as most of mwc code is.

That *almost* makes sense in this context but it's still very bad form, and also doesn't explain the retarded 'taskOrder-=5' statement, but oh well. I'm starting to see that mwc is definitely not done by real coders, and there's a bunch of pasted toy code all over the place; looks like a lost cause trying to suggest anything since people get all touchy and butt-hurt about it...

timecop gave you the exact reason about "++ in each switch".
nothing is almost true in a code, it is true or it is not and your code is not equivalent.
please explain us why it is bad to do like this.

about "if(taskOrder>4) taskOrder-=5;" I admit "=0" is probably cleaner
I personally don't feel "taskOrder = (++taskOrder > 4) ? 0 : taskOrder;" is easier to read
But thank you anyway because you make me think of another shortcut we can do in this part of code.

I see the change that you recently made, and, in that case, there's really no point in incrementing taskOrder when you can just set it to the next value at each step.

ternary statement looks clear enough to me. In fact, there are various ternary statements already scattered throughout the mwc...

I personally think that the intentional fall-through nature of the switch/case within the state machine is a bad idea; I already have a very good idea of how that could be better setup & to cleanly allow for conditionally defined sensor reads... But, I really don't care to pursue this anymore. I personally wouldn't have setup that routine in such a convoluted way, yet I give up trying to help out here since I see that mwc people are very reluctant to change and assistance. So, i'll just keep my potential improvements to myself. as copterrichie stated, people here prefer to compete than collaborate...

Anyways, I'm done with this particular issue and moving on...

[krebbi]

...same story like the new PID-controller...

[-ralf-]

...same story like the new PID-controller...

Yes .... btw the AlexK works since published .....