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

BME280 SPI #5538

Merged
merged 15 commits into from
Jan 10, 2024
Merged

BME280 SPI #5538

merged 15 commits into from
Jan 10, 2024

Conversation

apbodrov
Copy link
Contributor

@apbodrov apbodrov commented Oct 16, 2023

What does this implement/fix?

Implements BME280 over SPI.

Breaking change notes: The original bme280 component has been renamed to bme280_i2c for consistency with other platforms. To update your configuration, simply replace bme280 with bme280_i2c. No other changes are required.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): implements esphome/feature-requests#1321

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#3273

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

spi:
  clk_pin: GPIO18
  mosi_pin: GPIO23
  miso_pin: GPIO19

sensor:
  - platform: bme280_spi
    temperature:
      name: "BME280 Temperature"
      oversampling: 16x
    pressure:
      name: "BME280 Pressure"
    humidity:
      name: "BME280 Humidity"
    update_interval: 60s
    cs_pin: GPIO5

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

If user exposed functionality or configuration variables are added/changed:

@probot-esphome
Copy link

Hey there @apbodrov,
Thanks for submitting this pull request! Can you add yourself as a codeowner for this integration? This way we can notify you if a bug report for this integration is reported.
In __init__.py of the integration, please add:

CODEOWNERS = ["@apbodrov"]

And run script/build_codeowners.py

(message by NeedsCodeownersLabel)

@apbodrov
Copy link
Contributor Author

tested two BMEs with config

spi:
  clk_pin: GPIO18
  mosi_pin: GPIO23
  miso_pin: GPIO19

i2c:
  sda: 21
  scl: 22
  scan: true

sensor:
  - platform: bme280_spi
    temperature:
      name: "BME280 SPI Temperature"
      oversampling: 16x
    pressure:
      name: "BME280 SPI Pressure"
    humidity:
      name: "BME280 SPI Humidity"
    update_interval: 60s
    cs_pin: GPIO5


  - platform: bme280
    temperature:
      name: "BME280 I2C Temperature"
      oversampling: 16x
    pressure:
      name: "BME280 I2C Pressure"
    humidity:
      name: "BME280 I2C Humidity"
    update_interval: 60s
    address: 0x76

@apbodrov
Copy link
Contributor Author

apbodrov commented Oct 17, 2023

@OttoWinter @jesserockz hello! can you check my PR? Thanks!

@jesserockz
Copy link
Member

Please don't ping maintainers. Its a good way to get ignored 😉

@apbodrov
Copy link
Contributor Author

apbodrov commented Oct 19, 2023

@jesserockz
I'm following your own guide ;)
изображение

@jesserockz
Copy link
Member

3 days is not lingering yet. It's still fresh out of the oven.

@rossens
Copy link

rossens commented Dec 16, 2023

I am looking forward to trying/using this - I have several SPI BME280 and Pi Pico ready and waiting. I'm fairly new to ESPHome - can anyone advise how long it usually takes for something like this to make it over to Home Assistant?
PS - seems one of the Validate YAML tests failed - "Pin 5 is used in multiple places."?

@kbx81
Copy link
Member

kbx81 commented Dec 18, 2023

@rossens you (or anyone else who might be interested) can test this PR as an external component by adding this to your yaml config:

external_components:
  - source: github://pr#5538
    components: [ bme280_base, bme280_spi ]

@kbx81
Copy link
Member

kbx81 commented Dec 18, 2023

While it means a breaking change, for consistency with other components within the platform where the hardware (sensor, display, etc.) is able to support both I2C and SPI, the I2C component should probably be renamed to bme280_i2c.

@apbodrov
Copy link
Contributor Author

apbodrov commented Dec 19, 2023

While it means a breaking change, for consistency with other components within the platform where the hardware (sensor, display, etc.) is able to support both I2C and SPI, the I2C component should probably be renamed to bme280_i2c.

In fact, the only difference will be another name, but it will take a lot of work to

  1. Refactor the code, tests, and documentation in the repository.
  2. Refactor the alll configs of all users.

I also think that bme280_i2c sounds more logical, but there is not much point in renaming it.

@rossens
Copy link

rossens commented Dec 21, 2023

@rossens you (or anyone else who might be interested) can test this PR as an external component by adding this to your yaml config:

external_components:
  - source: github://pr#5538
    components: [ bme280_base, bme280_spi ]

Thanks that looks much simpler than what I did (I applied this PR as a patch over my installed ESPHome files). I got it working fine though. Will try this better way next time.

Results: Temperature and pressure seem pretty close - but humidity is reading too low. e.g. currently 25% reported vs 50% actual.
Wondering if it could be anything to do with the PR - but assume it is more likely an issue with the cheap sensors I purchased from AliExpress.

I also wonder if it could be relative vs absolute humidity? I am assuming the sensor/code and ESPHome all talk "relative" but I'm not 100% sure. Anyone know?

I will continue to investigate on my end and advise if/when I get correct Humidity - what was the issue.

@apbodrov
Copy link
Contributor Author

@rossens

Results: Temperature and pressure seem pretty close - but humidity is reading too low. e.g. currently 25% reported vs 50% actual. Wondering if it could be anything to do with the PR - but assume it is more likely an issue with the cheap sensors I purchased from AliExpress.

Hello! I had a similar problem, and I also decided that it was the sensor. But since I'm not the only one with the problem, now I'm starting to think that the problem is in the code.

I'll try to fix the bug and synchronize the readings later.

Thanks!

@rossens
Copy link

rossens commented Dec 21, 2023

I'm suspicious the problem may be here:

float BME280Component::read_humidity_(const uint8_t *data, int32_t t_fine) {
  uint16_t const raw_adc = ((data[6] & 0xFF) << 8) | (data[7] & 0xFF); // <<<<<<<<<<<<<<<

Depending on compiler - (data[6] << 8) may always give 0 because uint8_t(x) << 8 = uint8_t(0)

Try this instead... (I also removed the & 0xFF which aren't neccesary)

  uint16_t const raw_adc = (uint16_t(data[6]) << 8) | data[7];

I'll also give this a try later on today myself.

@apbodrov
Copy link
Contributor Author

I'm suspicious the problem may be here:
float BME280Component::read_humidity_(const uint8_t *data, int32_t t_fine)

This is a shared code between I2C and SPI component, it was moved from BME280 I2C. So that fix will affect I2C code (which I do not touched, just move)

Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

As per the message above from @kbx81

While it means a breaking change, for consistency with other components within the platform where the hardware (sensor, display, etc.) is able to support both I2C and SPI, the I2C component should probably be renamed to bme280_i2c.

Please move the main component. This is to avoid confusion in the future and we have done it many times in the past.

@esphome esphome bot marked this pull request as draft December 23, 2023 01:55
@esphome
Copy link

esphome bot commented Dec 23, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@apbodrov apbodrov marked this pull request as ready for review December 23, 2023 03:06
@esphome esphome bot requested a review from jesserockz December 23, 2023 03:06
CODEOWNERS Show resolved Hide resolved
Copy link

@rossens rossens left a comment

Choose a reason for hiding this comment

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

Looks good to me.
I'm successfully using this PR/patch to operate 4x BME280 SPI sensors on a Pi Pico.

@TCB13
Copy link

TCB13 commented Jan 9, 2024

Any updates on merging this? Thank you.

Copy link
Member

@kbx81 kbx81 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@kbx81 kbx81 dismissed jesserockz’s stale review January 10, 2024 04:30

Request implemented

@kbx81 kbx81 merged commit 4b783c0 into esphome:dev Jan 10, 2024
50 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants