New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I have added new BMP280 barometer support in cleanflight #975

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
@rlehey

rlehey commented Jun 1, 2015

It builds

@JohnieBraaf

This comment has been minimized.

Show comment
Hide comment
@JohnieBraaf

JohnieBraaf Jun 1, 2015

Contributor

Why did you add it to makefile and target.h for all the boards? Should you not just add it only to the boards that actually have this sensor?

Contributor

JohnieBraaf commented Jun 1, 2015

Why did you add it to makefile and target.h for all the boards? Should you not just add it only to the boards that actually have this sensor?

@rlehey

This comment has been minimized.

Show comment
Hide comment
@rlehey

rlehey Jun 1, 2015

I follow the bmp085 which is a similar sensor.
It could be a replacement.
There is no target specific code, so it doesn't hurt to build it.

rlehey commented Jun 1, 2015

I follow the bmp085 which is a similar sensor.
It could be a replacement.
There is no target specific code, so it doesn't hurt to build it.

@rlehey

This comment has been minimized.

Show comment
Hide comment
@rlehey

rlehey Jun 1, 2015

Also it can connect by breakout board to any target with i2c. This is how I test it.

rlehey commented Jun 1, 2015

Also it can connect by breakout board to any target with i2c. This is how I test it.

@JohnieBraaf

This comment has been minimized.

Show comment
Hide comment
@JohnieBraaf

JohnieBraaf Jun 1, 2015

Contributor

I would say you only define the sensors actually present on the target.
Otherwise you should also add all the other ones no? Although CC3D has multiple indeed.

Contributor

JohnieBraaf commented Jun 1, 2015

I would say you only define the sensors actually present on the target.
Otherwise you should also add all the other ones no? Although CC3D has multiple indeed.

@rlehey

This comment has been minimized.

Show comment
Hide comment
@rlehey

rlehey Jun 1, 2015

See my comment just before your. It can connect to any target using I2C breakout.

rlehey commented Jun 1, 2015

See my comment just before your. It can connect to any target using I2C breakout.

@JohnieBraaf

This comment has been minimized.

Show comment
Hide comment
@JohnieBraaf

JohnieBraaf Jun 1, 2015

Contributor

Yeah, that makes sense

Contributor

JohnieBraaf commented Jun 1, 2015

Yeah, that makes sense

@JohnieBraaf

This comment has been minimized.

Show comment
Hide comment
@JohnieBraaf

JohnieBraaf Jun 1, 2015

Contributor

How do you make cleanflight pick the right sensor?

Contributor

JohnieBraaf commented Jun 1, 2015

How do you make cleanflight pick the right sensor?

@rlehey

This comment has been minimized.

Show comment
Hide comment
@rlehey

rlehey Jun 1, 2015

It's automatically detected, the case-switch in sensors/initialization.c, if not detected then it will try another or else disable that type of sensor. So without it connected also still works.

rlehey commented Jun 1, 2015

It's automatically detected, the case-switch in sensors/initialization.c, if not detected then it will try another or else disable that type of sensor. So without it connected also still works.

@rlehey

This comment has been minimized.

Show comment
Hide comment
@rlehey

rlehey Jun 13, 2015

What's the status on this?

rlehey commented Jun 13, 2015

What's the status on this?

@developingchris

This comment has been minimized.

Show comment
Hide comment
@developingchris

developingchris Jun 18, 2015

I have a board with a bmp280, that I'd love to get support for. What needs to happen to this, for it to be in the next release?
@JohnieBraaf ?

developingchris commented Jun 18, 2015

I have a board with a bmp280, that I'd love to get support for. What needs to happen to this, for it to be in the next release?
@JohnieBraaf ?

@hydra

This comment has been minimized.

Show comment
Hide comment
@hydra

hydra Jun 18, 2015

Contributor

There are older existing pull requests that need to be addressed first, there's quite a backlog.

Bugs are prioritized over new features too.

Contributor

hydra commented Jun 18, 2015

There are older existing pull requests that need to be addressed first, there's quite a backlog.

Bugs are prioritized over new features too.

@hydra

This comment has been minimized.

Show comment
Hide comment
@hydra

hydra Jun 18, 2015

Contributor

Oh, and i need to be able to test the code before its merged but i dont have a board with a BMP280 here. Unit tests would be good too.

Contributor

hydra commented Jun 18, 2015

Oh, and i need to be able to test the code before its merged but i dont have a board with a BMP280 here. Unit tests would be good too.

@rlehey

This comment has been minimized.

Show comment
Hide comment
@rlehey

rlehey commented Jun 18, 2015

@developingchris

This comment has been minimized.

Show comment
Hide comment
@developingchris

developingchris Jun 18, 2015

@hydra being rather new to this space, is there anything I can do to help with those? Can I tag the ones with bad travis builds or something, in a way to triage bugs that can be paid attention to?

developingchris commented Jun 18, 2015

@hydra being rather new to this space, is there anything I can do to help with those? Can I tag the ones with bad travis builds or something, in a way to triage bugs that can be paid attention to?

@hydra

This comment has been minimized.

Show comment
Hide comment
@hydra

hydra Jun 18, 2015

Contributor

Yes, submittig PRs that fix the bugs would help immensely.

Contributor

hydra commented Jun 18, 2015

Yes, submittig PRs that fix the bugs would help immensely.

@stronnag

This comment has been minimized.

Show comment
Hide comment
@stronnag

stronnag Jun 20, 2015

Contributor

@rlehey , you may also consider that somewhat less disingenuously describing the target of this patch may help merging into main line. Specifically, rather than spamming all targets, creating a dedicated target (such as DODOF3) may more accurately identify the real purpose of the PR and thus hasten its acceptance. I would council against the terms such as SERIOUSLY or WOW in creating such a target if you intend the PR to be taken, dare I say it, seriously.

Contributor

stronnag commented Jun 20, 2015

@rlehey , you may also consider that somewhat less disingenuously describing the target of this patch may help merging into main line. Specifically, rather than spamming all targets, creating a dedicated target (such as DODOF3) may more accurately identify the real purpose of the PR and thus hasten its acceptance. I would council against the terms such as SERIOUSLY or WOW in creating such a target if you intend the PR to be taken, dare I say it, seriously.

@rlehey

This comment has been minimized.

Show comment
Hide comment
@rlehey

rlehey Jun 20, 2015

the sensor can be used by other targets with breakout boards.

the same way there is BMP 085/180 support now. anyone making a commercial
for profit board with that?
On Jun 21, 2015 2:41 AM, "stronnag" notifications@github.com wrote:

@rlehey https://github.com/rlehey , you may also consider that somewhat
less disingenuously describing the target of this patch may help merging
into main line. Specifically, rather than spamming all targets, creating a
dedicated target (such as DODOF3) may more accurately identify the real
purpose of the PR and thus hasten its acceptance. I would council against
the terms such as SERIOUSLY or WOW in creating such a target if you intend
the PR to be taken, dare I say it, seriously.


Reply to this email directly or view it on GitHub
#975 (comment)
.

rlehey commented Jun 20, 2015

the sensor can be used by other targets with breakout boards.

the same way there is BMP 085/180 support now. anyone making a commercial
for profit board with that?
On Jun 21, 2015 2:41 AM, "stronnag" notifications@github.com wrote:

@rlehey https://github.com/rlehey , you may also consider that somewhat
less disingenuously describing the target of this patch may help merging
into main line. Specifically, rather than spamming all targets, creating a
dedicated target (such as DODOF3) may more accurately identify the real
purpose of the PR and thus hasten its acceptance. I would council against
the terms such as SERIOUSLY or WOW in creating such a target if you intend
the PR to be taken, dare I say it, seriously.


Reply to this email directly or view it on GitHub
#975 (comment)
.

@rlehey

This comment has been minimized.

Show comment
Hide comment
@rlehey

rlehey Jun 21, 2015

my PR adds support for the barometer.
I have no idea what dedicated target you are talking about. I've added it
into every target that had i2c headers where external board can be
attached. last I checked committing support for new hardware didn't require
having commercial interest in a particular target.
On Jun 21, 2015 2:41 AM, "stronnag" notifications@github.com wrote:

@rlehey https://github.com/rlehey , you may also consider that somewhat
less disingenuously describing the target of this patch may help merging
into main line. Specifically, rather than spamming all targets, creating a
dedicated target (such as DODOF3) may more accurately identify the real
purpose of the PR and thus hasten its acceptance. I would council against
the terms such as SERIOUSLY or WOW in creating such a target if you intend
the PR to be taken, dare I say it, seriously.


Reply to this email directly or view it on GitHub
#975 (comment)
.

rlehey commented Jun 21, 2015

my PR adds support for the barometer.
I have no idea what dedicated target you are talking about. I've added it
into every target that had i2c headers where external board can be
attached. last I checked committing support for new hardware didn't require
having commercial interest in a particular target.
On Jun 21, 2015 2:41 AM, "stronnag" notifications@github.com wrote:

@rlehey https://github.com/rlehey , you may also consider that somewhat
less disingenuously describing the target of this patch may help merging
into main line. Specifically, rather than spamming all targets, creating a
dedicated target (such as DODOF3) may more accurately identify the real
purpose of the PR and thus hasten its acceptance. I would council against
the terms such as SERIOUSLY or WOW in creating such a target if you intend
the PR to be taken, dare I say it, seriously.


Reply to this email directly or view it on GitHub
#975 (comment)
.

@thenickdude

This comment has been minimized.

Show comment
Hide comment
@thenickdude

thenickdude Jun 29, 2015

Contributor

AfroMini revision 3 is now shipping with this BMP280 barometer.

Contributor

thenickdude commented Jun 29, 2015

AfroMini revision 3 is now shipping with this BMP280 barometer.

@FenomPL

This comment has been minimized.

Show comment
Hide comment
@FenomPL

FenomPL Jul 8, 2015

@hydra Any chance to merge it soon?

FenomPL commented Jul 8, 2015

@hydra Any chance to merge it soon?

@theantnest

This comment has been minimized.

Show comment
Hide comment
@theantnest

theantnest Jul 12, 2015

Hey guys, would appreciate this to be finalised/ fixed/ merged. Thanks for all your hard work

theantnest commented Jul 12, 2015

Hey guys, would appreciate this to be finalised/ fixed/ merged. Thanks for all your hard work

@hydra

This comment has been minimized.

Show comment
Hide comment
@hydra

hydra Jul 12, 2015

Contributor

The servo/mixer code and related configurator changes to support the tilt flight modes are currently the highest priority.

Some unit tests for this PR wouls help immensely.

Contributor

hydra commented Jul 12, 2015

The servo/mixer code and related configurator changes to support the tilt flight modes are currently the highest priority.

Some unit tests for this PR wouls help immensely.

@theantnest

This comment has been minimized.

Show comment
Hide comment
@theantnest

theantnest Jul 12, 2015

Please let me know if there's any way I can help out (in any way - not just related to this pull request). I know screw all about coding, but have a pretty decent lab kitted up for hardware, and am no noob about firmware and protocols.

theantnest commented Jul 12, 2015

Please let me know if there's any way I can help out (in any way - not just related to this pull request). I know screw all about coding, but have a pretty decent lab kitted up for hardware, and am no noob about firmware and protocols.

@thenickdude

This comment has been minimized.

Show comment
Hide comment
@thenickdude

thenickdude Jul 13, 2015

Contributor

@theantnest unit tests that check for datasheet-specified conversions between barometer raw readings and the output pressures/temperatures would be the main testable component here. The preexisting baro_bmp085_unittest.cc can be used as a template.

Contributor

thenickdude commented Jul 13, 2015

@theantnest unit tests that check for datasheet-specified conversions between barometer raw readings and the output pressures/temperatures would be the main testable component here. The preexisting baro_bmp085_unittest.cc can be used as a template.

@add1ct3dd

This comment has been minimized.

Show comment
Hide comment
@add1ct3dd

add1ct3dd Jul 16, 2015

Contributor

Any progress on this? Could do with this being in the next release (imo).

Contributor

add1ct3dd commented Jul 16, 2015

Any progress on this? Could do with this being in the next release (imo).

@leothehuman

This comment has been minimized.

Show comment
Hide comment
@leothehuman

leothehuman Jul 26, 2015

I have checked this out on afromini rev3, the code works, nothing is broken as far as I can see. The values it outputs look correct, but very imprecise.
Changes look like a port from baseflight.
I would suggest to limit the impact to naze boards, as attaching separate bmp280 seems unlikely.
I would consider baro not working on afromini rev3 as a bug and give it a priority.

leothehuman commented Jul 26, 2015

I have checked this out on afromini rev3, the code works, nothing is broken as far as I can see. The values it outputs look correct, but very imprecise.
Changes look like a port from baseflight.
I would suggest to limit the impact to naze boards, as attaching separate bmp280 seems unlikely.
I would consider baro not working on afromini rev3 as a bug and give it a priority.

@xoxota99

This comment has been minimized.

Show comment
Hide comment
@xoxota99

xoxota99 Aug 30, 2015

What's blocking this merge? Is it just the backlog of pull requests? Something else?

xoxota99 commented Aug 30, 2015

What's blocking this merge? Is it just the backlog of pull requests? Something else?

@leothehuman

This comment has been minimized.

Show comment
Hide comment
@leothehuman

leothehuman Sep 1, 2015

@hydra please, consider accepting this. It works and helps people with rev3 afrominis.

leothehuman commented Sep 1, 2015

@hydra please, consider accepting this. It works and helps people with rev3 afrominis.

@flacjacket

This comment has been minimized.

Show comment
Hide comment
@flacjacket

flacjacket Sep 17, 2015

Contributor

I've taken this code and added the requisite unit tests, see #1316.

Contributor

flacjacket commented Sep 17, 2015

I've taken this code and added the requisite unit tests, see #1316.

@hydra

This comment has been minimized.

Show comment
Hide comment
@hydra

hydra Oct 6, 2015

Contributor

merged.

Contributor

hydra commented Oct 6, 2015

merged.

@hydra hydra closed this Oct 6, 2015

martinbudden pushed a commit to martinbudden/cleanflight that referenced this pull request Aug 11, 2016

sambas pushed a commit to sambas/cleanflight that referenced this pull request Dec 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment