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

Sensor BMI160: set of undersampling mode is not working #32774

Closed
vinceWii4 opened this issue Mar 2, 2021 · 1 comment · Fixed by #33006
Closed

Sensor BMI160: set of undersampling mode is not working #32774

vinceWii4 opened this issue Mar 2, 2021 · 1 comment · Fixed by #33006
Labels
area: Sensors Sensors bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@vinceWii4
Copy link

Problem description
In low power mode, the accelerometer current consumption is to high. After investigation I discovered that the undersampling mode wasn't corretly set in BMI160_REG_ACC_CONF.

To Reproduce
Steps to reproduce the behavior:

  1. Add in prj.conf:
  • CONFIG_BMI160=y
  • CONFIG_BMI160_ACCEL_PMU_LOW_POWER=y
  • CONFIG_BMI160_GYRO_PMU_SUSPEND=y
  1. Modify drivers/sensor/bmi160/bmi160.c
int bmi160_reg_field_update(struct device *dev, u8_t reg_addr,
			    u8_t pos, u8_t mask, u8_t val)
{
	u8_t old_val, new_val, val;

	if (bmi160_byte_read(dev, reg_addr, &old_val) < 0)
		return -EIO;
	val = (old_val & ~mask) | ((val << pos) & mask);
	if (bmi160_byte_write(dev, reg_addr, &val) < 0)
		return -EIO;
	if (bmi160_byte_read(dev, reg_addr, &new_val) < 0)
		return -EIO;

	if (reg_addr == BMI160_REG_ACC_CONF)
		printk("BMI160_REG_ACC_CONF - old: %d, value to write "
			  "in reg: %d, new val: %d", old_val, val, new_val);

	return 0;
}
  1. Run a simple program
  2. See that new value in BMI160_REG_ACC_CONF has not the expected value

Expected behavior
BMI160_REG_ACC_CONF: bit 7 of new_val should be enabled.

Impact
High current consumption in accelerometer low power mode

Solution
bmi160_pmu_set calls bmi160_reg_field_update() with a wrong pos parameter (shift of 128 instead of 7).

git diff drivers/sensor/bmi160/bmi160.h
@@ -170,7 +170,8 @@
 #define BMI160_ACC_CONF_ODR_MASK	0xF
 #define BMI160_ACC_CONF_BWP_POS		4
 #define BMI160_ACC_CONF_BWP_MASK	(0x7 << 4)
-#define BMI160_ACC_CONF_US		BIT(7)
+#define BMI160_ACC_CONF_US_POS		7
+#define BMI160_ACC_CONF_US_MASK		BIT(7)

 /* BMI160_REG_GYRO_CONF */
 #define BMI160_GYR_CONF_ODR_POS	0
git diff drivers/sensor/bmi160/bmi160.c
@@ -163,7 +163,7 @@ static int bmi160_pmu_set(struct device *dev, union bmi160_pmu_status *pmu_sts)
 	/* set the undersampling flag for accelerometer */
 	return bmi160_reg_field_update(dev, BMI160_REG_ACC_CONF,
-				       BMI160_ACC_CONF_US, BMI160_ACC_CONF_US,
+				       BMI160_ACC_CONF_US_POS, BMI160_ACC_CONF_US_MASK,
 				       pmu_sts->acc != BMI160_PMU_NORMAL);
 }
@vinceWii4 vinceWii4 added the bug The issue is a bug, or the PR is fixing a bug label Mar 2, 2021
@nashif nashif added priority: low Low impact/importance bug area: Sensors Sensors labels Mar 2, 2021
@gudnimg
Copy link
Contributor

gudnimg commented Mar 6, 2021

@vinceWii4 I created a pull request with the changes you suggested. The fix makes sense to me. 😃

@jenmwms jenmwms added the has-pr label Mar 6, 2021
carlescufi pushed a commit that referenced this issue Mar 9, 2021
Fix #32774

Bit position BIT(7) is 1000 0000 in binary which is actually the
bit positon 128. BIT(7) can be used as a mask, but we need to define
the position specifically as the integer 7.

Signed-off-by: Guðni Már Gilbert <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Sensors Sensors bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants