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

Add mpu6050 interrupt support #6827

Draft
wants to merge 10 commits into
base: dev
Choose a base branch
from
Draft

Conversation

oarcher
Copy link
Contributor

@oarcher oarcher commented May 29, 2024

What does this implement/fix?

This PR implements interrupt support for the MPU6050 accelerometer, allowing the 'INT' pin to be used, for example, to wake up the device upon detected motion.

This is a breaking change, as the original MPU6050 component only handled platform sensors. The code has been refactored to split the component into a base component and a platform sensor.

Here is a summary of the changes:

  • Moved and renamed the old platform sensor component from mpu6050/ to mpu6050/sensor/
    • Removed some base power management configuration (now in the base component)
    • Removed i2c inheritance (now in the base component and accessed via this->parent_)
  • Added a new base component with a new __init__.py
    • Handles base power management
    • Handles i2c
    • Handles interrupts

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): fixes

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

Test Environment

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

Example entry for config.yaml:

#  added base component
mpu6050:
  id: accel
  address: 0x68
  i2c_id: i2c_bus
  interrupt:  # optionnal (False by default, use defaults values if True, or a schema)
    threshold: 2
    duration: 2ms

sensor:
  - platform: mpu6050
      # address: 0x68  # removed
      # i2c_id: i2c_bus # removed
      mpu6050_id: accel  # added
      accel_x:
        name: "MPU6050 Accel X"

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:

Sorry, something went wrong.

@codecov-commenter
Copy link

codecov-commenter commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.88%. Comparing base (4d8b5ed) to head (27a9ec9).
Report is 688 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #6827      +/-   ##
==========================================
+ Coverage   53.70%   53.88%   +0.17%     
==========================================
  Files          50       50              
  Lines        9408     9619     +211     
  Branches     1654     1698      +44     
==========================================
+ Hits         5053     5183     +130     
- Misses       4056     4112      +56     
- Partials      299      324      +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oarcher oarcher marked this pull request as ready for review May 29, 2024 21:49
@@ -12,8 +12,7 @@
UNIT_DEGREE_PER_SECOND,
UNIT_CELSIUS,
)

DEPENDENCIES = ["i2c"]
Copy link
Member

Choose a reason for hiding this comment

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

THis should change to DEPENDENCIES = ["mpu6050"] now that it has a dependency on a parent component.

CONFIG_SCHEMA = cv.Schema(
{
cv.GenerateID(): cv.declare_id(MPU6050Sensor),
cv.Required(CONF_MPU6050_ID): cv.use_id(MPU6050Component),
Copy link
Member

Choose a reason for hiding this comment

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

This will automatically get the id if there is only 1 parent in config

Suggested change
cv.Required(CONF_MPU6050_ID): cv.use_id(MPU6050Component),
cv.GenerateID(CONF_MPU6050_ID): cv.use_id(MPU6050Component),

CONFIG_SCHEMA = cv.Schema(
{
cv.GenerateID(): cv.declare_id(MPU6050Component),
cv.Optional(CONF_INTERRUPT, default=False): validate_interrupt,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cv.Optional(CONF_INTERRUPT, default=False): validate_interrupt,
cv.Optional(CONF_INTERRUPT): validate_interrupt,

Comment on lines +12 to +25
def validate_interrupt(value):
schema = None
if isinstance(value, bool):
if value:
schema = {CONF_THRESHOLD: 1, CONF_DURATION: cv.positive_time_period("1ms")}

else:
schema = cv.Schema(
{
cv.Required(CONF_THRESHOLD): cv.positive_not_null_int,
cv.Required(CONF_DURATION): cv.positive_time_period,
}
)(value)
return schema
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def validate_interrupt(value):
schema = None
if isinstance(value, bool):
if value:
schema = {CONF_THRESHOLD: 1, CONF_DURATION: cv.positive_time_period("1ms")}
else:
schema = cv.Schema(
{
cv.Required(CONF_THRESHOLD): cv.positive_not_null_int,
cv.Required(CONF_DURATION): cv.positive_time_period,
}
)(value)
return schema
INTERRUPT_SCHEMA = cv.Schema(
{
cv.Required(CONF_THRESHOLD): cv.positive_not_null_int,
cv.Required(CONF_DURATION): cv.positive_time_period,
}
)
def validate_interrupt(value):
if isinstance(value, bool):
if value:
return INTERRUPT_SCHEMA({
CONF_THRESHOLD: 1,
CONF_DURATION: cv.positive_time_period("1ms")
})
else:
return value
return INTERRUPT_SCHEMA(value)


if conf_interrupt := config[CONF_INTERRUPT]:
if conf_interrupt:
cg.add_define("USE_MPU6050_INTERRUPT")
Copy link
Member

Choose a reason for hiding this comment

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

We dont use defines for things like this.

Suggested change
cg.add_define("USE_MPU6050_INTERRUPT")

Comment on lines +42 to +43
if conf_interrupt := config[CONF_INTERRUPT]:
if conf_interrupt:
Copy link
Member

Choose a reason for hiding this comment

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

A falsey value will return false in the below statement already

Suggested change
if conf_interrupt := config[CONF_INTERRUPT]:
if conf_interrupt:
if conf_interrupt := config[CONF_INTERRUPT]:

this->mark_failed();
return;
// return;
Copy link
Member

Choose a reason for hiding this comment

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

?

@esphome esphome bot marked this pull request as draft October 7, 2024 02:34
@esphome
Copy link

esphome bot commented Oct 7, 2024

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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants