Betaflight rebase - Githubissues

pqueiros commented 8 years ago

Merged betaflight 'v2.2.0'

compiles and runs on naze32(on the bench and as stand alone)
test on tricopter still needed
marked with TODO some changes that need carefull review

Bengt-M commented 8 years ago

I tried to compile and got a lot of warnings. The data structure for CLI has changed so it needs something like this:

--- a/src/main/io/serial_cli.c
+++ b/src/main/io/serial_cli.c
@@ -576,3 +576,3 @@
-    { "tri_servo_angle_at_max",     VAR_INT16  | MASTER_VALUE, &masterConfig.mixerConfig.tri_servo_angle_at_max, 0, 500 },
-    { "tri_tail_motor_thrustfactor",VAR_INT16  | MASTER_VALUE, &masterConfig.mixerConfig.tri_tail_motor_thrustfactor, TAIL_THRUST_FACTOR_MIN, TAIL_THRUST_FACTOR_MAX },
-    { "tri_tail_servo_speed",       VAR_INT16  | MASTER_VALUE, &masterConfig.mixerConfig.tri_tail_servo_speed, 0, 1000 },
+    { "tri_servo_angle_at_max",     VAR_INT16  | MASTER_VALUE, &masterConfig.mixerConfig.tri_servo_angle_at_max, .config.minmax = { 0, 500 } },
+    { "tri_tail_motor_thrustfactor",VAR_INT16  | MASTER_VALUE, &masterConfig.mixerConfig.tri_tail_motor_thrustfactor, .config.minmax = { TAIL_THRUST_FACTOR_MIN, TAIL_THRUST_FACTOR_MAX } },
+    { "tri_tail_servo_speed",       VAR_INT16  | MASTER_VALUE, &masterConfig.mixerConfig.tri_tail_servo_speed, .config.minmax = { 0, 1000 } },
@@ -592,2 +592,2 @@
-    { "tpa_yaw_rate",               VAR_UINT8  | CONTROL_RATE_VALUE, &masterConfig.controlRateProfiles[0].tpa_yaw_rate, 0, CONTROL_RATE_CONFIG_YAW_TPA_MAX},
-    { "tpa_yaw_breakpoint",         VAR_UINT16 | CONTROL_RATE_VALUE, &masterConfig.controlRateProfiles[0].tpa_yaw_breakpoint, PWM_RANGE_MIN, PWM_RANGE_MAX},
+    { "tpa_yaw_rate",               VAR_UINT8  | CONTROL_RATE_VALUE, &masterConfig.controlRateProfiles[0].tpa_yaw_rate, .config.minmax = { 0, CONTROL_RATE_CONFIG_YAW_TPA_MAX } },
+    { "tpa_yaw_breakpoint",         VAR_UINT16 | CONTROL_RATE_VALUE, &masterConfig.controlRateProfiles[0].tpa_yaw_breakpoint, .config.minmax = { PWM_RANGE_MIN, PWM_RANGE_MAX } },

I think a lot of people, like me, trust that the master branch is always good for flying. As GitHub explains: "There's only one rule: anything in the master branch is always deployable". This huge request is quite a bit to digest and probably too much to verify with just code review.

lkaino commented 8 years ago

This is not in master. Did you try to compile the master or this PR?

Bengt-M commented 8 years ago

I did a local experiment. Branched the master and then merged in the PR and compiled. I would not advice doing that in the official master.

pqueiros commented 8 years ago

Hi Bengt thanks for spotting that I will take a look into it.

But before, @lkaino does it make sense to put effort into merge betaflight into triflight?

@Bengt-M, deployable does not mean bug free ;). This change set (as far as I remember) compiles and actually 'runs' on the naze32 so I would actually say is deployable although most likely not the most stable ;). Nevertheless I agree that this needs more work, testing and review.

/pq

Bengt-M commented 8 years ago

I also thought about that but cannot quite get my head around it. With a risk of going slightly OT I share some thoughts:

We have at least three well-known good forks now, Cleanflight, Betaflight and Triflight. Maybe there are more. I see how they started and can appreciate the rationale but I don't see the roadmap and how they should relate in the future. Without that it's hard to see what is worth an effort.

In my experience from following other open source projects I would say it's a good strategy to push as much as possible upstream and minimise the remaining special parts. Otherwise, the maintenance burden can turn the hobby into boring unpaid work. As one example Planet CCRMA have survived largely because they pushed almost everything up to Fedora; the only things remaining are some esoteric and realtime stuff a normal desktop user don't want. This way the PM could manage his day-job too.

The PM for BF stated something similar in his initial announcement but it is not easy. Things are moving fast and CF already have lots of issues and PR's open. CF also prioritise features while I think most TF users prioritise behaviour and performance with a given (small) configuration. Maybe a future strategy could be to merge everything from CF, unless it creates unwanted issues, and merge from BF only when it solves real problems. There are many ways and maybe some refactoring will be needed to make it work smoothly.

But things may need to stabilise first. It is still early days and prio 1, I hope, is to get something that is working well first. With predictable and understandable behaviour so all pilot excuses are eliminated.

I really admire the clever work being done here and all the good support we users get on the RCE forums. Without it I would probably have chosen a Quad with CF instead of the MiniTri. I would like to give back and offer my support but for the much needed flight testing I am still useless.

BR /Bengt

lkaino commented 8 years ago

@Bengt-M: Those are wise words!

This started out as a hobby project, and it still is. Main goal is to improve the flight behavior for tricopters, and a separate fork gives free hands to work on it. Now that the user base has grown, Cleanflight related issues start to show up. Maintaining those CF related issues is not what I enjoy doing on my freetime. I have a day job that gives me enough of that :).

Triflight is currently based on Betaflight, which is a good and a bad thing. Betaflight has superior flight performance (well, some versions of it does). It is also a fork where Boris and others are testing out features, something that is not a good base for improving the tricopter experience. I will attach snips from mail I wrote to @pqueiros last week to end of this comment.

Target now is to get the performance to a level that satisfies most tricopter pilots. Then create a PR and get at least some of this stuff to CF if we can get Dominic to agree. The unit test project that @pqueiros started with is one step towards that.

Here's the snips from the mail: The original idea was to merge back to cleanflight at some point, the changes in Triflight are not dependent on betaflight features at all. I just based it on betaflight because Boris and co are doing awesome stuff with the overall flight performance. The problem with betaflight is that it's constantly changing, due to the beta testing nature of the fork. Nowadays betaflight has better release cycle with well tested stable releases, though. I want Triflight to be as stable platform as possible for tricopters that you can just flash and go flying. That being said I want the defaults to work well for most tricopters.

In Triflight the goal is to improve the yaw performance and it is challenging to compare effects of different fixes and/or improvements if there are always new betaflight features to test at the same time.

So to conclude, we do full betaflight merges but only to stable betaflight releases.

Triflight is very close to being ready for what comes to the yaw performance. Main concerns currently are:

Feedback! I feel that I'm not getting enough feedback from people, positive or negative. I think the people are missing the point that this all is a work in progress and a good feedback from community is more important than anything.
Squash any bugs in the tricopter mixer logic. This means that unit tests would be required. I have tried to split the functionality into small testable components. Do you have experience in unit testing? Do you feel that you could set up at least a unit test project for the mixer_tricopter.c module? Good unit tests might require splitting the code even more. This is also something that is needed before merge back to cleanflight.
Do you have a blackbox? Now that we have feedback possibility from the servo it is possible to improve the virtual servo code to better mimic a real world servo. It doesn't have to be perfect but closer. This means that we would need to implement light-weight model of the servo taking into account the slowness in braking/acceleration (mass). Keep it simple, I don't want this model to take up all processing power. The feedback branch is not yet in github but I will push it soon.
I term windup protection. The current system works very well, but as you might have noticed I'm using a hardcoded DPS limit to disable integrator growth. Although it works, I'm experimenting with other possibilities, such as detecting when the actuator (servo) is saturated and maybe feeding the saturation back to the integrator.

Bengt-M commented 8 years ago

Your statement of target makes sense to me.

Maybe you should consider more specific requests for feedback. Maybe as a new forum top(?). I'm sure there are lots of pilots out there, without snow and minus, that would like to help but it's not so easy to understand how. If tests can be made easy to perform I guess a lot would try it. Look at the responses about thrust and servo speed. If a test needs a pro the potential test crew is more narrow but anyway. If you want something you have much better chances if you ask for it loud and clear.

I started to read about test and just couldn't stop. See separate PR. For what it's worth...

/B

Bengt-M commented 8 years ago

Guys, here are some more food for thoughts.

My Mrs is on holliday and nothing on TV. I have spent a few hours browsing and comparing the code to get some idea of the effort required to align Triflight with Cleanflight. It is huge. And I see CF now contains a lot of really nice stuff that we probably all want. Modern math, filtered gyro, scheduler, better filters, fixed drivers; to pick just a few. Improved performance can be expected from all this. A lot of files have changed and all unit tests are up to par too. They still have a long list of PR's but with 35+ commits this year alone I'd say the pace is good.

Maybe there is an alternative strategy to reach the goal. Have you considered a full restart of the Triflight project; to start at current Cleanflight and again add the changes you have been working with?

I looked in the GIT log to find files changed by @lkaino commits and I found about a dozen. The rest of the log are stuff from other people, i.e. Betaflight, and the most important parts are probably already in Cleanflight.

I'd say the effort this way is some hours; it's not days or weeks. To test that estimate I did a practical experiment. I took CF, added mixer_tricopter and updated the makefile. Then I modified mixer to use mixer_tricopter and then I cherrypicked changes until it could build clean. That took me less than one hour.

The list of files from the commits are below.

/Bengt

src/main/blackbox/blackbox.c
src/main/config/config.c
src/main/config/runtime_config.h
src/main/flight/mixer.c
src/main/flight/mixer_tricopter.h
src/main/flight/mixer_tricopter.c
src/main/flight/pid.c
src/main/mw.c
src/main/io/rc_controls.h
src/main/io/rc_controls.c
src/main/io/serial_cli.c
src/main/io/serial_msp.c
Makefile

lkaino commented 8 years ago

Now that you mentioned it, I started looking at the cleanflight log. Almost all key features of betaflight are already there as you said. Only thing missing is the airmode mixer, but that is easily merged in if wanted.

I think you made me to change the course. It makes more sense to align with CF. The best way to do it is as you said. Take current CF and apply changes there. I "get" to clean some stuff and squash commits while at it.

Do you have idea if replacing the current master with CF master would work? Would I be able to do PRs from that?

Bengt-M commented 8 years ago

Here is what I did. First I renamed the existing Triflight folder. Party as backup and partly so I could compare using other tools. Then I did this:

cd git
git clone https://github.com/lkaino/Triflight.git
cd Triflight
git remote add cf https://github.com/cleanflight/cleanflight.git
git fetch cf
git branch cf_master cf/master
git checkout cf_master
git branch mynewbranch
git checkout mynewbranch

...and then started to play. After that you are on the same commit status as CF master and have all the history and I think that means you can do anything that you could do with a fresh new fork of CF. The best part is that all the Triflight history is also in the the repo so you can use git tools to compare and selectively merge branches. (I suppose git is very powerful for that but it's on the border for my git skills so I also used Beyond Compare.)

lkaino commented 8 years ago

Okay I finally had time to do the merge: https://github.com/lkaino/Triflight/commits/cleanflight_merge.

Only compilation tested so far, need to do flight testing at some point.

Did it by cherry-picking the commits to cleanflight master.

lkaino commented 8 years ago

I think I'll wait for Boris to create PR or the air mode mixer before starting flight testing. Boris mentioned that he will do it at some point. There's already two PRs from Boris active.

lkaino / Triflight

Betaflight rebase #5