Skip to content

Conversation

@matiasilva
Copy link

This PR adds a new example for the BMP280 temperature and pressure sensor via I2C. It takes care of reading sensor values and also the necessary conversion for the values to make sense. I'm not sure how much "cleaning up" is needed, for now all code is in one file for simplicity but a few definitions want to be in their own header file. Fritzing diagram and file included, accompanying documentation too.

@aallan aallan requested a review from kilograham June 25, 2021 13:18
@aallan aallan added the enhancement New feature or request label Jun 25, 2021
Copy link
Contributor

@kilograham kilograham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like this a lot even though i did make a bunch of comments

@lurch
Copy link
Contributor

lurch commented Jun 28, 2021

Perhaps it's worth changing your wiring diagram to use a half-size breadboard (as you've only got a very limited number of wires), like the other examples in Appendix A of https://datasheets.raspberrypi.org/pico/raspberry-pi-pico-python-sdk.pdf ?

EDIT: Ah, oops, I was looking at the wrong PDF! 🤣 A bunch of the examples in https://datasheets.raspberrypi.org/pico/raspberry-pi-pico-c-sdk.pdf do use full-size breadboards, whereas it looks like they could get away with using half-size breadboards 😉

Also @aallan I've just noticed that (most of) the wiring-diagrams in the Python SDK PDF have the Pico's USB port facing to the right, whereas (most of) the wiring-diagrams in the C/C++ SDK PDF have the Pico's USB port facing to the left 😉

@matiasilva
Copy link
Author

Perhaps it's worth changing your wiring diagram to use a half-size breadboard (as you've only got a very limited number of wires), like the other examples in Appendix A of https://datasheets.raspberrypi.org/pico/raspberry-pi-pico-python-sdk.pdf ?

I've done so anyway and I think it looks quite nice!

@lurch
Copy link
Contributor

lurch commented Jun 28, 2021

I know this is intended to be a standalone example, but I wonder if there's any value in trying to keep the "I2C BMP280" and "SPI BME280" examples vaguely similar-ish? Just as one example, the SPI BME280 example does:

printf("Humidity = %.2f%%\n", humidity / 1024.0);
printf("Pressure = %dPa\n", pressure);
printf("Temp. = %.2fC\n", temperature / 100.0);

whereas the I2C BMP280 example does:

    float converted_temp = bmp280_convert_temp(&temp, &params) * 0.01f;
    float converted_pressure = bmp280_convert_pressure(&pressure, &temp, &params) * 0.001f;
    printf("Temperature: %.2f °C  Pressure: %.3f kPa\n", converted_temp,
      converted_pressure);

@lurch
Copy link
Contributor

lurch commented Jun 28, 2021

On an additional side-note @aallan , I also just noticed that the Raspberry Pi Pico PCB images in Appendix A of https://datasheets.raspberrypi.org/pico/raspberry-pi-pico-c-sdk.pdf look less detailed than their counterparts in Appendix A of https://datasheets.raspberrypi.org/pico/raspberry-pi-pico-python-sdk.pdf ?

@aallan
Copy link

aallan commented Jun 28, 2021

Yes. They need replacing with the new Fritzing part. Most of them were done pre-launch.

@matiasilva matiasilva changed the title Additional BMP280 I2C example Add BMP280 I2C example Jul 15, 2021
@matiasilva
Copy link
Author

I've only realized now after some head scratching on another board, that the SDK's i2c_write_blocking assumes 7-bit addresses and adds the read/write bits for us. Last commit reflects this.

@lurch
Copy link
Contributor

lurch commented Jul 15, 2021

I've only realized now after some head scratching on another board, that the SDK's i2c_write_blocking assumes 7-bit addresses and adds the read/write bits for us. Last commit reflects this.

I know very little about I2C, but does the same thing affect #143 ? (please leave an appropriate comment on that PR if it does)

@matiasilva
Copy link
Author

I know very little about I2C, but does the same thing affect #143 ? (please leave an appropriate comment on that PR if it does)

Done!

@lurch
Copy link
Contributor

lurch commented Jul 16, 2021

I added a comment about README.md but as it's in a "resolved" section, GitHub is helpfully auto-hiding it! 😕 So here it is again:

Actually, while it makes sense for the files listed in CMakeLists.txt to be sorted alphabetically, it probably makes sense for the files listed in README.md to be sorted in order of increasing complexity? 🤔
In which case, bus_scan should indeed be listed before bmp280_i2c - sorry for changing my mind! 😆

@matiasilva
Copy link
Author

I added a comment about README.md but as it's in a "resolved" section, GitHub is helpfully auto-hiding it! confused So here it is again:

Actually, while it makes sense for the files listed in CMakeLists.txt to be sorted alphabetically, it probably makes sense for the files listed in README.md to be sorted in order of increasing complexity? thinking
In which case, bus_scan should indeed be listed before bmp280_i2c - sorry for changing my mind! laughing

Ah, yeah I didn't see that, thanks for bringing it up again. I agree, and perhaps we should indicate this in a little comment too?

@aallan
Copy link

aallan commented Aug 3, 2021

Where are we on this, are we ready to pass it to @kilograham for a final review? @lurch opinions?

@JamesH65
Copy link
Collaborator

JamesH65 commented Sep 2, 2021

There is an outstanding change request flagged, but I believe it has been fixed but I cannot see a way of flagging it has been fixed. Otherwise, this LGTM.

@aallan
Copy link

aallan commented Sep 2, 2021

There is an outstanding change request flagged, but I believe it has been fixed but I cannot see a way of flagging it has been fixed.

I think we'd just have to "re-request review" from @kilograham. There seems to be no way to get rid of the flag otherwise.

@aallan aallan changed the base branch from develop to merge-intern-examples October 14, 2021 09:17
@aallan aallan merged commit 4caac4d into raspberrypi:merge-intern-examples Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants