lkaino / Triflight

Triflight flight controller firmware for tricopters
http://rcexplorer.se
GNU General Public License v3.0
50 stars 19 forks source link

Improvements #46

Closed Bengt-M closed 7 years ago

Bengt-M commented 7 years ago

First commit is to fix a bug (yaw_deadband) and remove some unused functions.

Second commit is to change the gyro limit during tail tune dynamically. Start tight and open up slowly. No reason at all that the bigger tri's shall use the same criteria as needed for the flimsy baby tri. Everyone should get the best tuning possible. This way every size should get the best possible using the same code.

Try it! I have left two line for debugging, so the final gyro limit can be easily visible in the configurator. (I did fly with this in the 0.5 version but not yet with 0.6. Too much snow now now.)

In the same commit I've also replaced a lot of calls to "millis()" with a local variable "now". It saves some bytes in the hex, and it's bad style to read the same HW multiple times in the same function call.

lkaino commented 7 years ago

Nice!

I was wondering why we closed the earlier PR.

I will test it out on weekend if weather allows. Seems good.

It's a good idea to only fetch the time once per function. However, I don't fancy having to declare a variable with specific name in order to use the macro. Additional macros for declaring the now variable and getting the value would be better imo. What do you think?

Bengt-M commented 7 years ago

Better to remove the macro. It is so routine and short that it doesn't actually add anything. We should normally not program in directives when we can do it in C. Along the same lines we should change all those globals defined for the preprocessor with proper typesafe const's. Usually the code size will not grow by that.

lkaino commented 7 years ago

I disagree with the first one. I have seen experienced developers making mistakes in delay calculations, mostly because of the used data types and integer overflow. In most cases I favor writing c or functions instead of macros, but not in this case. Eases up maintenance and code review.

Macros are fine to use for consts, although not the way done here. They would require literal type suffix to be more typesafe: e.g. (900) -> (900UL). I do like the idea of using const vars instead of macros though.

The code would need general cleaning up before merging to Cleanflight. I think I will remove some functionality also.

Bengt-M commented 7 years ago

In Java or C++ there are clever ways of doing this. C is more limited. You can't have a macro to set a global, because the compiler will not detect if you forget to use it. So the macro would have to set a local. Something like: `

define GetCurrentTime() (uint32_t now = millis())

` It's a matter of taste but I don't think it will make it more readable and less error prone.

I will not fight for this but when I saw that the change I did actually reduced the hex size a little I was happy.

If you want I can change into const vars and cleanup the code to the coding style. It's trivial but it will change a lot of lines.

lkaino commented 7 years ago

I won't fight for stuff in non-profit projects either :). You do have valid points.

Yes it would need to be a macro that declares and sets a local var. The macro could be called InitDelayMeasurement_ms() and the now variable should be called const uint32_t _now_ms. Do it however you like, remove the macros altogether if you want, I will adapt :).

It would be awesome if you could help with the clean up. I saw you wrote the style guide for CF so you're probably the best person to do it :).

We got snow here in Finland also and wife has plans for the weekend, so the testing will be postponed for next week.

Bengt-M commented 7 years ago

Just did a little testing. type safe const vars cannot be used to initialise a static var in C. It's the proper way in C++ but not allowed in C. Static is good so I take that comment back.