search

multiwii / baseflight

32 bit fork of the MultiWii RC flight controller firmware
500 stars 356 forks source link

GPS fixes and GPS soft serial support #206

Closed jaahaavi closed 9 years ago

jaahaavi commented 9 years ago

Added possibility to use both Serial RX (Spektrum satellite/SBUS/SUMD) and GPS at the same time.

trollcop commented 9 years ago

I'm out of office until 24th to properly review this but I see many style violations. Also I would personally prefer disabled SBAAS to go to zero, and other numbers shifted upwards. Because if another one is added, 'disable' index will now have to move.

Other issues: I don't want UBX running at 9600baud as thats not enoughf for 10Hz output, which is why it didn't have an init structure. please careafully read through the original code and understand waht it does, first.

On Fri, Nov 21, 2014 at 8:52 PM, Jaakko Haavisto notifications@github.com wrote:

Added possibility to use both Serial RX (Spektrum satellite/SBUS/SUMD) and GPS at the same time.

  • When features: feature SERIALRX, feature SOFTSERIAL and feature GPS are enabled Serial RX is using UART and pins 3&4 and GPS is using soft serial and pins 5&6.
  • Fixed GPS code, so that now it's reconnecting if connection to GPS is lost.
  • GPS tab on Chrome Configurator now shows GPS signal strength.
  • Possibility to disable SBAS (ground assistace type). (SBAS causes occasional severe altitude calcuation errors)

You can merge this Pull Request by running

git pull https://github.com/jaahaavi/baseflight new_features

Or view, comment on, or merge it at:

https://github.com/multiwii/baseflight/pull/206 Commit Summary

  • Spektrum satellite binding improvement
  • Formatting correction
  • Merge pull request #2 from multiwii/master
  • GPS fixes and GPS soft serial support
  • Revert "Formatting correction"
  • Revert "Spektrum satellite binding improvement"

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206.

trollcop commented 9 years ago

That being said, I still had no time to deal wiht spektrum bind. It is on todo list.

On Fri, Nov 21, 2014 at 9:19 PM, timecop timecop@gmail.com wrote:

I'm out of office until 24th to properly review this but I see many style violations. Also I would personally prefer disabled SBAAS to go to zero, and other numbers shifted upwards. Because if another one is added, 'disable' index will now have to move.

Other issues: I don't want UBX running at 9600baud as thats not enoughf for 10Hz output, which is why it didn't have an init structure. please careafully read through the original code and understand waht it does, first.

On Fri, Nov 21, 2014 at 8:52 PM, Jaakko Haavisto <notifications@github.com

wrote:

Added possibility to use both Serial RX (Spektrum satellite/SBUS/SUMD) and GPS at the same time.

  • When features: feature SERIALRX, feature SOFTSERIAL and feature GPS are enabled Serial RX is using UART and pins 3&4 and GPS is using soft serial and pins 5&6.
  • Fixed GPS code, so that now it's reconnecting if connection to GPS is lost.
  • GPS tab on Chrome Configurator now shows GPS signal strength.
  • Possibility to disable SBAS (ground assistace type). (SBAS causes occasional severe altitude calcuation errors)

You can merge this Pull Request by running

git pull https://github.com/jaahaavi/baseflight new_features

Or view, comment on, or merge it at:

https://github.com/multiwii/baseflight/pull/206 Commit Summary

  • Spektrum satellite binding improvement
  • Formatting correction
  • Merge pull request #2 from multiwii/master
  • GPS fixes and GPS soft serial support
  • Revert "Formatting correction"
  • Revert "Spektrum satellite binding improvement"

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206.

jaahaavi commented 9 years ago

Hi, some comments:

-For "That being said, I still had no time to deal wiht spektrum bind. It is on todo list.": Good to know that it's coming. After that I can write detailed binding instructions to baseflight wiki.

And some other comments: I have been testing UBLOX MAX-7 gps and now baseflight gps is working quite fine. I't stable and no longer loses sync with naze32. Also that gps is know for resetting it's setting back to default values (don't know if it's firmware issue or some other bad design), but it doesn't matter because naze32 reconnects and sets new settings when sync is lost. Also works if gps is disconnected and then reconnected. And one question. Have you ever thought of redesigning the soft serial implementation? Now soft serial has only low baud rates and both ports has to use same baud rates. Naze32 with that processor could do better than that.

fiendie commented 9 years ago

@jaahaavi it has to be in baseflight first, then @cTn-dev can implement it in configurator. The master branch is allowed to be out of sync with baseflight-configurator. Releases are tagged and if you want to flash unstable/untested firmware you better know what you're doing ;)

We could probably do a version comparison a la openLRSng @trollcop?

trollcop commented 9 years ago

another option would be -1 probably preferred actually On Nov 21, 2014 10:53 PM, "Andreas Tacke" notifications@github.com wrote:

@jaahaavi https://github.com/jaahaavi it has to be in baseflight first, then @cTn-dev https://github.com/cTn-dev can implement it in configurator. The master branch is allowed to be out of sync with baseflight-configurator. Releases are tagged and if you want to flash unstable/untested firmware you better know what you're doing ;)

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-63972348.

jaahaavi commented 9 years ago

Update: Changed that SBAS order. Used 0 for disabled value. (using -1 for disabled value would required changing functions like read8 and so on because those was handling values as unsigned).

trollcop commented 9 years ago

Pretty sure there's example of -1 being done with read8

On Sat, Nov 22, 2014 at 12:58 AM, Jaakko Haavisto notifications@github.com wrote:

Update: Changed that SBAS order. Used 0 for disabled value. (using -1 for disabled value would required changing functions like read8 and so on because those was handling values as unsigned).

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-63990573.

jaahaavi commented 9 years ago

Found example with negative values used... Do you want it so that disabled value is -1?

trollcop commented 9 years ago

I think so, that would require the least changes and won't affect (immediately) the old(er) firmware/configurator.

On Sat, Nov 22, 2014 at 1:35 AM, Jaakko Haavisto notifications@github.com wrote:

Found example with negative values used... Do you want it so that disabled value is -1?

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-63996981.

trollcop commented 9 years ago

Also while you're changing that, rangecheck where cfg.sbaas is set when doing read8(), cuz if it's -1 or > SBAAS_LAST, it'll be array out of bounds

On Sat, Nov 22, 2014 at 1:36 AM, timecop timecop@gmail.com wrote:

I think so, that would require the least changes and won't affect (immediately) the old(er) firmware/configurator.

On Sat, Nov 22, 2014 at 1:35 AM, Jaakko Haavisto <notifications@github.com

wrote:

Found example with negative values used... Do you want it so that disabled value is -1?

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-63996981.

jaahaavi commented 9 years ago

Made the change that SBAS disabled is -1.

jaahaavi commented 9 years ago

Corrections done and splitted SBAS stuff to next PR.

jaahaavi commented 9 years ago

@trollcop Any estimate when you are going to check those corrections?

trollcop commented 9 years ago

where is separate PR for sbaas? On Nov 26, 2014 5:30 PM, "Jaakko Haavisto" notifications@github.com wrote:

@trollcop https://github.com/trollcop Any estimate when you are going to check those corrections?

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-64529659.

trollcop commented 9 years ago

looks OK except for that one style part.fix and I'll merge

jaahaavi commented 9 years ago

What style part I didn't fix? ps. I'll commit that SBAS PR after I can get this completed.

trollcop commented 9 years ago

oops, looks like my phone munged adding notes. i re-added them basically else { must be on same line and if () { is also same line. 3 places as commented.

jaahaavi commented 9 years ago

Ok, now I see. It's somehow hard to adjust my eyes for the new style when I used to use "Nokia" style at work for 13 years. Now I have corrected those issues.

trollcop commented 9 years ago

I think you misunderstood re: } else { formatting, you had it correct in one place but wrong in other. The new change is wrong. Anyway, after this its all good, so I can either merge or you can squash them all into a single commit.

jaahaavi commented 9 years ago

Ok, I misunderstood it. Now it should be correct. btw in that codingstyle wikipage there aren't case how the braces and spaces should be in this if else case when if has braces

trollcop commented 9 years ago

Actually, it does

OK, so looks good. Squash them all into one?

On Wed, Nov 26, 2014 at 6:19 PM, Jaakko Haavisto notifications@github.com wrote:

Ok, I misunderstood it. Now it should be correct. btw in that codingstyle wikipage there aren't case how the braces and spaces should be in this if else case when if has braces

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-64535103.

jaahaavi commented 9 years ago

"- Curly brace on same line for everything except function definitions" Ok, my fault, maybe there should be example for dummies that haven't used for using K&R style ;)

"Squash them all into one?" Do you mean this that I should add SBAS code to this now?

trollcop commented 9 years ago

No, I mean turn ~12 commits into a single one. I dunno how git does it. I'm sure there's a way.

On Wed, Nov 26, 2014 at 6:27 PM, Jaakko Haavisto notifications@github.com wrote:

"- Curly brace on same line for everything except function definitions" Ok, my fault, maybe there should be example for dummies that haven't used for using K&R style ;)

"Squash them all into one?" Do you mean this that I should add SBAS code to this now?

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-64536245.

schugabe commented 9 years ago

The easiest way is to create a new branch and merge the squashed commit:

git checkout master
git checkout -b tmpsquash
git merge --squash new_features
git commit -a -m "insert commit message..."

I never tried it but I think the new branch can be fore pushed into your new_features branch and replaces the other commits in this PR.

readerror67 commented 9 years ago

@fiendie taught me an easier way as well, you basically need to create a patch on your branch vs. master, then create another branch of master and apply it which will allow you to have a single commit.

I always seem to screw up the rebase stuff..

trollcop commented 9 years ago

nah whatever ill just commit this shit

On Wed, Nov 26, 2014 at 6:37 PM, Johannes notifications@github.com wrote:

The easiest way is to create a new branch and merge the squashed commit:

git checkout master git checkout -b tmpsquash git merge --squash new_features git commit -a -m "insert commit message..."

I never tried it but I think the new branch can be fore pushed into your new_features branch and replaces the other commits in this PR.

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-64537353.

trollcop commented 9 years ago

I can't merge this because I think gps.c newlines aren't matching the rest of the project. whoever can fix this, please do.

readerror67 commented 9 years ago

@jaahaavi

Was able to use some merge tool to get it to go successfully in #209 (put in to see if merge is possible, you should do it for the Github credits though) I have no idea what I'm doing so I don't know how it worked exactly so I cant explain the fix ;/ It was TortoiseGitMerge tool.

jaahaavi commented 9 years ago

Yesterday there were major problem in Internet at Finland. Only one fiber cable was broken and and all of major ISP international data was down. Not much redundancy...

I think @trollcop can close this PR and merge that @readerror67 #209. I installed that Git for windows and then that Tortoise Git for my computer, but the jump from my "retarded" Github for windows to tortoise is so big that it'll take some time for me to learn to use it or maybe I start to use SmartGit. It's better to get this done before I accidentally delete my fork with new git tool ;)

fiendie commented 9 years ago

TortoiseGit is every bit as retarded as Github for Windows ;) I can recommend Atlassian SourceTree on your way to learn how the stuff works on the command line ;P

trollcop commented 9 years ago

I agree, tortoisegit is garbage. On Nov 27, 2014 4:16 PM, "Andreas Tacke" notifications@github.com wrote:

TortoiseGit is every bit as retarded as Github for Windows ;) I can recommend Atlassian SourceTree on your way to learn how the stuff works on the command line ;P

— Reply to this email directly or view it on GitHub https://github.com/multiwii/baseflight/pull/206#issuecomment-64753945.

readerror67 commented 9 years ago

Woh now, I don't use TurdisGit, the merge tool on the other hand was helpful in fixing gps.c