Skip to content
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

Correcting BME280 IIR filter setting #1787

Merged
merged 1 commit into from Feb 11, 2017
Merged

Correcting BME280 IIR filter setting #1787

merged 1 commit into from Feb 11, 2017

Conversation

FrankX0
Copy link
Contributor

@FrankX0 FrankX0 commented Feb 9, 2017

Fixes #1786.

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

Parameters "inactive_duration" and "IIR_filter" were incorrectly written to the config register.
@mickley: can you test?
@vsky279: do you agree?

@mickley
Copy link

mickley commented Feb 9, 2017

@FrankX0:

I'm still working on getting a docker setup running smoothly (very limited time here), but I'll be happy to test when it hits the dev branch and I can use nodemcubuild.com

@marcelstoer marcelstoer added this to the 2.0.0-follow-up milestone Feb 9, 2017
@mickley
Copy link

mickley commented Feb 11, 2017

I've gotten docker working and can confirm that this fixes the bug in the BME280 module. Pressures are now correct when the IIR filter parameter is 1.

It's difficult to accurately test the filter itself, but turning the filter off (0), vs using a filter coefficient of 16 (4) show clear differences in data variability so it appears the filter is working correctly.

@marcelstoer
Copy link
Member

I'm still working on getting a docker setup running smoothly

Feel free to leave feedback with https://github.com/marcelstoer/docker-nodemcu-build (assuming you're using that image) if you think documentation should be improved.

@mickley
Copy link

mickley commented Feb 11, 2017

@marcelstoer the docker side of things worked pretty well, though its inability to run on Windows 7 is a real shame.

What irked me more is that it's rather tedious to build, and difficult to figure out. The section on building firmware in the Nodemcu docs is very brief. The location for user_modules.h and user_config.h are not mentioned, and this isn't covered at all from your docker repository's Readme. You might get more individuals interested in helping out if this was more accessible and well-documented.

Editing user_modules.h, user_version.h, and user_config.h (and sometimes more) each time you want to build a new firmware is also rather tedious. Why not have all of that in one file? Better yet would be a way to preserve multiple build configurations (e.g. different sets of modules) without maintaining multiple sets of the user_*****.h files, especially if one could do that via scripting or something.

I find it quite useful that your nodemcu-build.com adds the commit, current date, and included modules into the firmware's version info string. This is really the only way to see which modules a firmware contains. I'm surprised that there's no built-in support for this when building via docker. I've already added the build date automatically, but again this just seems like something that should be built-in at least as an option.

@vsky279
Copy link
Contributor

vsky279 commented Feb 11, 2017

@FrankX0: Yes, you are right. There was a bug in the code. I was misled by the fact that Bit 1 does not have any meaning. The documentation on page 28 says:

Table 26: Register 0xF5 “config”

Register 0xF5 “config” Name Description
Bit 7, 6, 5 t_sb[2:0] Controls inactive duration t_standby in normal mode. See Table 27 for settings and chapter 5.3.4 for details.
Bit 4, 3, 2 filter[2:0] Controls the time constant of the IIR filter. See Table 27 for settings and chapter 5.4.4 for details.
Bit 0 spi3w_en[0] Enables 3-wire SPI interface when set to ‘1’. See chapter 8.3 for details.

I think we can merge.

@marcelstoer
Copy link
Member

The section on building firmware in the Nodemcu docs is very brief.

True, could you maybe document what you were missing and prepare a PR to that effect?

The location for user_modules.h and user_config.h are not mentioned

There's some info at https://github.com/nodemcu/nodemcu-firmware#build-options but I agree it doesn't really belong there (historical left-over).

and this isn't covered at all from your docker repository's Readme.

Indeed but I argue it doesn't belong there but here.

Editing user_modules.h, user_version.h, and user_config.h (and sometimes more) each time you want to build a new firmware is also rather tedious. Why not have all of that in one file?

Yep #1223.

I'm surprised that there's no built-in support for this when building via docker.

I am surprised I had to build that myself for the cloud builder. Again, I argue it should be a standard firmware feature; #1739.

@marcelstoer marcelstoer merged commit 4dfa5cd into nodemcu:dev Feb 11, 2017
@marcelstoer
Copy link
Member

Thanks Frank.

eiselekd pushed a commit to eiselekd/nodemcu-firmware that referenced this pull request Jan 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants