Closed mozzbozz closed 7 years ago
Hi @mozzbozz (Torben) ,
I believe I've discovered a problem with both the main library and your extensions (which are very nice, by the way, thanks for doing them, I look forward to seeing them in the library).
It has to do with operations in Normal mode (set by default in the main lib and your code). See the Bosch data sheets about Shadow Registers usage (BMP280 sections 3.9,3.10 and BME280 sections 4,4.1). To get consistent data, you need to read the registers together in burst mode, meaning that after you lower and before you raise the CS line, you have to read all the registers in order from 0xF7 to either 0xFC (T+P) or 0xFE (+H for BME) and then assemble the 6 or 8 bytes into the two or three values for the uncompensated T and P (20 bit), and H (16 bit). Then you can apply the appropriate compensations.
I was having problems with jittery data until I did this. It seems that the Temp reading that goes with the Press reading would not come from the same sample if you had settings that caused it to finish a new sample while you were reading the last one. But if you read in a burst, this problem is handled internally by the chip by keeping the new values on hold in shadow registers, and releasing them to be read when the CS line goes high again to stop the current transaction.
The solution is simple enough, based on a function like read24(), maybe call it readAll(). Just keep reading and assembling all 6 or 8 register bytes, since the chip will auto-increment the register number in read mode, before deactivating the chip select pin. That's why you can't skip the Pressure registers if you're only interested in a Humidity reading. You can stop after 6 bytes if only doing T and P, though, if that's a useful optimization.
Let me know what you think. I believe I can work up a PR myself for this, either based on the old code or the new. But the existing implementations to read Pressure or Humidity using separate calls to read24() are wrong and have this rather subtle problem in all implementations that use Normal mode (which is everything out there, it looks like). Should I raise this as an issue as well or instead, before I try to submit the PR? (The Temperature-only code should work fine, since it does 3 bytes in a burst already.)
It may only cause very intermittent reading errors, but that depends on the data rate ODR, which depends on all the other settings (t_standby, osrs_x, filter, etc.). So folks may not notice it much until they start using your code. But they have the problem on the 00/B7 setting in the default lib since t_standby is lowest, so the data is generated as fast as possible, even if it is only read once a minute. This is because the Config register is never set, the default is 00, and that says to use 0.5ms standby time, which means ODR is maximized.
I also wrote a C/C++ function to calculate all those timing values shown in BME datasheet Appendix B, section 9.1-9.4, if anyone's interested. It helped me a lot to figure all this out. FYI, the datasheet notes in Table 17, section 5.2 (page 24), that the only real difference in settings between BMP and BME (other than the chip ID) is the meaning of the last 2 t_standby codes, which go from high values for BMP (2 or 4 sec) to low for BME (10 or 20 msec). And that if you turn on the filtering, you always get 20-bit resolution, in spite of the osrs_x settings. I don't think that would affect use of the code, however.
Does this make sense?
Best regards, Mike Mehr (@mmehr2)
Hello Mike (@mmehr2)!
I also stumbled across these notes in the datasheet and I even implemented the burst read first. But then I decided to remove it for the PR again as my implementation (maybe there are better ways of doing it, I only implemented it the "quick and dirty" way for testing purposes) broke too much of the API of this library here...
I'm not 100% sure if I got you right and you are saying the same as I think the datasheet says? As far as I understood the datasheet, the only disadvantage of not doing the burst read is that your temp / humidity / pressure values can be from different samples (e.g. the temperature is from the last measurement and humidity and pressure are the newer value already). When doing the burst read, you can be sure that all readings come from the same sample. When you only read the single values, they are correct and valid values, just maybe from another sample - did I get this right? Or can even the single values be broken (I don't think so)? If not, I don't see a lot of cases, where someone needs to assure that the readings are from the same sample (or at least those people probably wouldn't use a random library from the internet) (of course I could be wrong here).
The problem with implementing your suggestion (as mentioned before, I already tried it :wink: ) is the following: You'd need to completely change the way the readTemperature()
, ... functions work. Currently in normal
mode, they just read "their" register. If my interpretation above is correct, you wouldn't get any advantage by burst-reading the registers in each of this functions - as you would do a separate read in all three functions, you could still read the values of three different samples. So, the solution would be to implement a function like readAllRegisters()
and save the readings for later. If you then call e.g. readTemperature()
, you could simply return that value. However, this means, that you have to call the readAllRegisters()
function before calling the existing read-functions. This is, where the current API would be broken and existing code would need to be changed...
Now, you could say "well, let's just take a timestamp and see if we already need to read the registers again". Not a very good idea as seen in the DHT-library by Adafruit which uses this kind of concept. It will break when you send the microcontroller to sleep between the readings as millis()
won't increase then. You could then add a "force" parameter (which has been introduced to the DHT-lib for this reason...) which I think is quite hacky...
The best solution (in my opinion) would be to introduce a "update" / "updateValues" (or whatever you want to name it) parameter to the readTemperature()
, ... functions which defaults to true
(so old code wouldn't break)... However, maybe you can think of a even better solution?
But maybe you should consider having a look at the Adafruit unified sensors thing and implement a unified version of this library, which kind of uses the readAllRegisters()
-concept mentioned at the beginning (the function is called getEvent()
there for whatever reason, maybe this name is derived from the Android sensor model which they used as an inspiring example). That work would certainly be very nice as the unified libs "seem to be the future". And this way you wouldn't need to do any dirty hacks or break existing API -> this is the possibility to do a clean implementation without any legacy API issues. When I removed the burst read code again before submitting the PR, I deferred it until later until I would have the motivation and time to do the unified stuff (but currently it doesn't look like I get to do it any time soon, so you are welcome to do it :+1: ). I don't even think this is too much work, implementing the unified stuff could be done in a couple of hours, I think (I just needed the sleep functionality etc., so I focused on that).
Please let us know if you started working on implementing one of the suggestions above!
Should I raise this as an issue as well or instead, before I try to submit the PR?
I'd go the PR way, you never know how long someone from Adafruit needs to react...
I believe I can work up a PR myself for this, either based on the old code or the new.
Not sure, but I'd go with the new code here to prevent merge conflicts etc. (or is there even a way to do a PR on a PR?)...
EDIT: I've had another look at the datasheet:
To read out data after a conversion, it is strongly recommended to use a burst read and not address every register individually. This will prevent a possible mix-up of bytes belonging to different measurements and reduce interface traffic.
As the code at least burst-reads all registers belonging to one value (e.g. it uses read24
for temp and press; read16
for hum), the byte-mix-up won't appear here and you might only get old but not wrong readings. This would only be the case if you'd e.g. read 0xFD
and 0xFE
individually when reading the humidity register (this is the same for temp and pressure, just with other addresses).
Or is there something which I misunderstood?
Hi Torben,
Thanks for the detailed reply! I'm glad to hear I'm not too far out on my efforts here.
I read the data sheet first as if the readings could be corrupted if a new sample became ready while you were in the middle of a register read sequence. They state that they only do the register shadowing in between CS line changes. Which to me would imply a problem if the scenario developed and you weren't bursting it (no shadow registers, kind of like double-buffering). But it would also seem to me that more folks using the existing code would have encountered the occasional issue out in the field if this were the behavior, so the fact that no one is reporting an issue (are they?) did put me off. I suppose one could write to the engineers at Bosch for a clarification.
And I believe the P and H readings are always compensated for with T values (the call to readPressure() calls readTemperature() internally), so they always have to read the T first to get the P and/or H. But yeah, I don't think the time stamp thing would be a good idea. Stateless always wins over stateful in flexibility and multithreading. :)
Actually, one wouldn't need to break the existing API. I envisioned an internal/private function and assocaited types: enum { READTYPE_TEMP = 0x01, READTYPE_PRESS = 0x02, READTYPE_HUMID = 0x04 }; // combinable flags using '|' typedef struct uncompensatedRegisters_t { uint32_t temp, press, humid; } uncompRegs; uncompRegs readAllRegistersInternal(uint8_t regStart, uint8_t type); and a family of compensation functions, also internal: float compensateTemperature( const uncompRegs ur ); // this also sets the t_fine "global" member, or we could add it to 'ur' and make the ref non-const (or play with const under the hood) float compensatePressure( const uncompRegs ur ); // these contain the original 32-bit integer compensation code in the library now float compensateHumidity( const uncompRegs ur ); This just divides up the code differently. Then you could change the readPressure() and readHumidity() functions to something like this: float readPressure(void) { uncompRegs ur = readAllRegistersInternal( BME280_REGISTER_TEMPDATA , READTYPE_TEMP | READTYPE_PRESS ); compensateTemperature(ur); // must be done first to set t_fine return compensatePressure(ur); } (Sorry for my C coding style! I'm currently working in C by porting the library to the NPC LPC1769 board and LPCExpresso IDE for a class project, and all the work is in C. Normally I'm a C++ guy!)
That way the same functionality would exist, it just wouldn't be broken in Normal mode. Would this also work for Forced mode? I admit I don't understand the difference as well as I think I should. But if it did, this could be the only low level implementation needed.
And you could write a new function that would return all three floats in a new struct, if folks wanted to get more than one data value back from the burst read. typedef struct bmpeValues_s { float temperature, pressure, humidity; } bmpeSensorValues; bmpeSensorValues readAllRegisters(void) { bmpeSensorValues result; uncompRegs ur; uint8_t type = (READTYPE_TEMP | READTYPE_PRESS); if (typeBME) type |= READTYPE_HUMID; ur = readAllRegistersInternal( BME280_REGISTER_TEMPDATA , type ); result.temperature = compensateTemperature(ur); // must be done first result.pressure = compensatePressure(ur); result.humidity = 0; if (typeBME) result.humidity = compensateHumidity(ur); return result; } (The internal variable typeBME would contain the results of querying the ID register in the begin() function. It could be exposed with a public function 'bool hasHumidity(void);' as well.) Users of the code wouldn't have to know that burst reads were being used internally. Plus, I think it would be more efficient reading of the registers this way (save some time on extra pin writes for CS, admittedly not much). The downside timing wise would be the extra pressure read in the case of someone only calling readHumidity().
Of course one could leave it up to the users, and only introduce the new function to handle the burst mode 3-way read as an option. I think I will write to Bosch for the clarification, just so I understand the issue better.
I'm willing to look into doing the unified implementation. And I did want to mention that if you're okay with waiting, I can check your SPI code, because that was my project, to get all this working on the hardware SPI functions in the LPC-1769 (Cortex M3). It's my first experience with ARM, Cortex, and this type of code (my early career was spent doing bare-metal programming on Z80 assembler, Forth, and C/C++, but then I drifted off into software for a couple decades lol). My Arduino class starts in about a month, but I'm porting some other stuff (I have a 1.3" OLED display that I'm trying to tease up), and will hopefully have that working soon, also on SPI for the LPC1769 project. The SPI sensor reader was demo'ed last week successfully using a Beagle protocol debugger to verify proper transactions, and it's due today, so I can vouch for it functioning properly with bursts. And the burst reading seemed to fix my altitude-diff function that was otherwise broken (it takes a reference pressure on startup and then uses that as "seaLevel" for the altitude calculation on demand). If I can get the instructor to tell me which Arduino they will use, I can buy it early and get started, otherwise I'll have to wait to test the SPI functions for BME/BMP on Arduino until early October.
I'll let you know what I find out from Bosch. Feel free to comment back at your leisure. I'm having lots of fun with this, so I hope I can keep doing it.
Regards, Mike Mehr General Manager AzuResults LLC www.azuresults.com 650-224-4254
-----Original Message----- From: Torben Woltjen [mailto:notifications@github.com] Sent: Thursday, September 1, 2016 05:20 AM To: 'adafruit/Adafruit_BME280_Library' Cc: 'Michael L. Mehr', 'Mention' Subject: Re: [adafruit/Adafruit_BME280_Library] Implemented sensor modes and settings among other improvements (#13)
Hello Mike (@mmehr2)! I also stumbled across these notes in the datasheet and I even implemented the burst read first. But then I decided to remove it for the PR again as my implementation (maybe there are better ways of doing it, I only implemented it the "quick and dirty" way for testing purposes) broke too much of the API of this library here... I'm not 100% sure if I got you right and you are saying the same as I think the datasheet says? As far as I understood the datasheet, the only disadvantage of not doing the burst read is that your temp / humidity / pressure values can be from different samples (e.g. the temperature is from the last measurement and humidity and pressure are the newer value already). When doing the burst read, you can be sure that all readings come from the same sample. When you only read the single values, they are correct and valid values, just maybe from another sample - did I get this right? Or can even the single values be broken (I don't think so)? If not, I don't see a lot of cases, where someone needs to assure that the readings are from the same sample (or at least those people probably wouldn't use a random library from the internet) (of course I could be wrong here). The problem with implementing your suggestion (as mentioned before, I already tried it π ) is the following: You'd need to completely change the way the readTemperature(), ... functions work. Currently in normal mode, they just read "their" register. If my interpretation above is correct, you wouldn't get any advantage by burst-reading the registers in each of this functions - as you would do a separate read in all three functions, you could still read the values of three different samples. So, the solution would be to implement a function like readAllRegisters() and save the readings for later. If you then call e.g. readTemperature(), you could simply return that value. However, this means, that you have to call the readAllRegisters() function before calling the existing read-functions. This is, where the current API would be broken and existing code would need to be changed... Now, you could say "well, let's just take a timestamp and see if we already need to read the registers again". Not a very good idea as seen in the DHT-library by Adafruit which uses this kind of concept. It will break when you send the microcontroller to sleep between the readings as millis() won't increase then. You could then add a "force" parameter (which has been introduced to the DHT-lib for this reason...) which I think is quite hacky... The best solution (in my opinion) would be to introduce a "update" / "updateValues" (or whatever you want to name it) parameter to the readTemperature(), ... functions which defaults to true (so old code wouldn't break)... However, maybe you can think of a even better solution? But maybe you should consider having a look at the Adafruit unified sensors thing and implement a unified version of this library, which kind of uses the readAllRegisters()-concept mentioned at the beginning (the function is called getEvent() there for whatever reason, maybe this name is derived from the Android sensor model which they used as an inspiring example). That work would certainly be very nice as the unified libs "seem to be the future". And this way you wouldn't need to do any dirty hacks or break existing API -> this is the possibility to do a clean implementation without any legacy API issues. When I removed the burst read code again before submitting the PR, I deferred it until later until I would have the motivation and time to do the unified stuff (but currently it doesn't look like I get to do it any time soon, so you are welcome to do it π ). I don't even think this is too much work, implementing the unified stuff could be done in a couple of hours, I think (I just needed the sleep functionality etc., so I focused on that). Please let us know if you started working on implementing one of the suggestions above! Should I raise this as an issue as well or instead, before I try to submit the PR? I'd go the PR way, you never know how long someone from Adafruit needs to react... I believe I can work up a PR myself for this, either based on the old code or the new. Not sure, but I'd go with the new code here to prevent merge conflicts etc. (or is there even a way to do a PR on a PR?)... β You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/adafruit/Adafruit_BME280_Library","title":"adafruit/Adafruit_BME280_Library","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/adafruit/Adafruit_BME280_Library"}},"updates":{"snippets":[{"icon":"PERSON","message":"@mozzbozz in #13: Hello Mike (@mmehr2)!\r\n\r\nI also stumbled across these notes in the datasheet and I even implemented the burst read first. But then I decided to remove it for the PR again as my implementation (maybe there are better ways of doing it, I only implemented it the \"quick and dirty\" way for testing purposes) broke too much of the API of this library here...\r\n\r\nI'm not 100% sure if I got you right and you are saying the same as I think the datasheet says? As far as I understood the datasheet, the only disadvantage of not doing the burst read is that your temp / humidity / pressure values can be from different samples (e.g. the temperature is from the last measurement and humidity and pressure are the newer value already). When doing the burst read, you can be sure that all readings come from the same sample. When you only read the single values, they are correct and valid values, just maybe from another sample - did I get this right? Or can even the single values be broken (I don't think so)? If not, I don't see a lot of cases, where someone needs to assure that the readings are from the same sample (or at least those people probably wouldn't use a random library from the internet) (of course I could be wrong here).\r\n\r\nThe problem with implementing your suggestion (as mentioned before, I already tried it :wink: ) is the following: You'd need to completely change the way the readTemperature()
, ... functions work. Currently in normal
mode, they just read \"their\" register. If my interpretation above is correct, you wouldn't get any advantage by burst-reading the registers in each of this functions - as you would do a separate read in all three functions, you could still read the values of three different samples. So, the solution would be to implement a function like readAllRegisters()
and save the readings for later. If you then call e.g. readTemperature()
, you could simply return that value. However, this means, that you have to call the readAllRegisters()
function before calling the existing read-functions. This is, where the current API would be broken and existing code would need to be changed...\r\n\r\nNow, you could say \"well, let's just take a timestamp and see if we already need to read the registers again\". Not a very good idea as seen in the DHT-library by Adafruit which uses this kind of concept. It will break when you send the microcontroller to sleep between the readings as millis()
won't increase then. You could then add a \"force\" parameter (which has been introduced to the DHT-lib for this reason...) which I think is quite hacky... \r\n\r\nThe best solution (in my opinion) would be to introduce a \"update\" / \"updateValues\" (or whatever you want to name it) parameter to the readTemperature()
, ... functions which defaults to true
(so old code wouldn't break)... However, maybe you can think of a even better solution?\r\n\r\nBut maybe you should consider having a look at the Adafruit unified sensors thing and implement a unified version of this library, which kind of uses the readAllRegisters()
-concept mentioned at the beginning (the function is called getEvent()
there for whatever reason, maybe this name is derived from the Android sensor model which they used as an inspiring example). That work would certainly be very nice as the unified libs \"seem to be the future\". And this way you wouldn't need to do any dirty hacks or break existing API -\u003e this is the possibility to do a clean implementation without any legacy API issues. When I removed the burst read code again before submitting the PR, I deferred it until later until I would have the motivation and time to do the unified stuff (but currently it doesn't look like I get to do it any time soon, so you are welcome to do it :+1: ). I don't even think this is too much work, implementing the unified stuff could be done in a couple of hours, I think (I just needed the sleep functionality etc., so I focused on that).\r\n\r\nPlease let us know if you started working on implementing one of the suggestions above!\r\n\r\n\u003e Should I raise this as an issue as well or instead, before I try to submit the PR?\r\nI'd go the PR way, you never know how long someone from Adafruit needs to react...\r\n\r\n\u003eI believe I can work up a PR myself for this, either based on the old code or the new. \r\nNot sure, but I'd go with the new code here to prevent merge conflicts etc. (or is there even a way to do a PR on a PR?)..."}],"action":{"name":"View Pull Request","url":"https://github.com/adafruit/Adafruit_BME280_Library/pull/13#issuecomment-244061616"}}}
And I believe the P and H readings are always compensated for with T values (the call to readPressure() calls readTemperature() internally), so they always have to read the T first to get the P and/or H
Ok, I forgot about this. As you'd need to read most registers every time anyway, you are right, there is no real reason to not burst read all values every time...
That way the same functionality would exist, it just wouldn't be broken in Normal mode. Would this also work for Forced mode? I admit I don't understand the difference as well as I think I should. But if it did, this could be the only low level implementation needed.
I have to admit, I didn't understand all of your cited code (it's very difficult to read due to the formatting). However, I can say the following: Reading the registers and calculating the values are the same for normal and forced mode. While in normal mode, the BME280 takes new measurements every n
ms and you read them whenever you want, it works different in forced mode: You can manually trigger such a measurement and then read the values from the registers. It's implemented the following way: BME is in sleep state -> you set it to forced mode -> BME takes measurements -> BME goes to sleep state again -> you can read the values and set the BME to forced mode again, if you want a new measurement. E.g. for a battery powered weather station where you want to take measurements every minute or so, you could either set the BME to normal mode and an interval of 1 second (which is the maximum) and just read the registers every 60 seconds - not very nice as we waste power on unused measurements. So just use the forced mode and trigger a "manual" measurement every 60 seconds and then read the registers. TL;DR: As far as I understand your intention, I don't think, there will be any problems with forced mode.
And you could write a new function that would return all three floats in a new struct, if folks wanted to get more than one data value back from the burst read.
Yep, that would be a really nice addition and prevent the overhead of reading the registers three times over and over again for just getting the three values (which would be the most common use case, I assume).
I can check your SPI code
That would be nice, though I don't think I broke anything here (and it was another reason why I didn't implement the burst mode, because I would have needed to touch the SPI code without having the hardware).
Thanks! mozzbozz
PS: Please use code highlighting next time, your post is very difficult to read at some points... It is very easy thanks to Markdown ;) (this page is linked below the text field here on github -> "Styling with Markdown is supported")
I do apologize, @mozzbozz . I was using a reply in my email and of course it knows nothing about formatting or markdown. Not that I'd be better off learning MD better anyway, but I wasn't even trying to use it. The code looked fine in the UI of the email client lol.
I'm still learning my way around Github. But I will try to put the code in as a PR and see if that will provide food for thought to more people. I have a request in to Bosch, but no reply yet. But reading the data sheet again, I believe that the read24() call effectively is a short burst, so data will not be corrupted, it will just come from different samples if a new one becomes available between the T and P reads. It probably isn't too big of an issue datawise, but bursting the reads may provide more benefits that I'm not thinking about. Probably worthy of discussion, at least.
Thanks for braving my pseudocode, and I'll try not to let that happen again. Will reply directly on Github from now on, it's so much easier.
Best wishes, Mike
Guys.
Its an interesting discussion.
I've written my own library for this to run on the nRF51, by looking at this library, as the official Bosch code seems overly complicated.
Considering that to read the pressure, the temperature registers need to be read and intermediate values calculated, I think it would make sense to always read all the registers into a buffer in one transfer, and then just access that buffer from the functions that need to read the temperature and pressure etc
(In my code reading in bursts was unavoidable as the nRF51 SDK API only transfers into buffers so I have to do it that way)
Transferring 18 bytes over SPI is not going to take much time at all, and is probably faster than the current code which has to make separate calls to spi.transfer for each byte, so it would be faster to do it that way. Albeit, there would need to be a permanent buffer for those bytes, but 18 bytes is very little
It's a good idea that was pointed out by @mmehr2 a few comments above !
I've experimented with doing:
Wire.requestFrom(i2cAddr, (byte)8); // 0xF7 to 0xFE => 8 bytes
int8_t buffer[8];
for (int i = 0; i < sizeof(buffer) / sizeof(buffer[0]); ++i)
{
buffer[i] = Wire.read();
}
pres = (buffer[0] << 12)
| (buffer[1] << 4)
| (buffer [2] >> 4);
temp = (buffer[3] << 12)
| (buffer[4] << 4)
| (buffer[5] >> 4);
hum = (buffer[6] << 8)
| buffer[7];
But I've only got garbled data and I gave up at this point because I didn't experience the same jitterness @mmehr2 described.
Note that I also gave up on using bitfields for configuring the BME280 and instead I just push 3x uint8_t for the registers which I pre-compute. The pseudo code goes something like this:
void setRegisters(uint8_t ctrlMeas, uint8_t ctrlHum, uint8_t config)
{
if (read8(CONFIG) & POWER_MODE_FLAG != POWER_SLEEP)
{
write8(RESET_REG, RESET_VAL);
delay(3); // ms
}
write8(CONTROL_HUM, ctrlHum);
write8(CONTROL_CONF, config);
write8(CONTROL_MEAS, ctrlMeas);
}
hey so this required some refactoring - original code didnt compile. i put the code in a new function called setSampling() and redid the examples - can people try it out and if it works out ill close, bump the default arduino library version thx!
I've added the possibility to
normal
vsforced
)Also:
takeForcedMeasurement()
function which is meant to be used in forced mode - when called, it tells the sensor to update the sensor values, i.e. to take a new measurementNAN
now if sensor is deactivatedI've tested my changes intensively, but of course I cannot guarantee that there are no bugs. I'd suggest, that someone tests my code with SPI, because my breakout board only has I2C and therefore I couldn't test SPI (but I've tried not to touch any of the SPI stuff).
Thanks also for the PR by @julien-lebot. My code is heavily based on his nice work :+1: (this PR is a complete replacement for his PR)