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

new driver for the lis2de12 accelerometer #20162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leocordier
Copy link

Contribution description

This new code is based on the existing lisdh12 driver. The lis2dh12 and lis2de12 are very similar.

Testing procedure

I made a test example for this drivers in test/drivers/lis2de12
This test can:

  • configure rate
  • configure scale
  • read acceleration data
  • read temperature (from the internal temperature sensor)

@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration labels Dec 8, 2023
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Welcome and thank you for the PR!
The driver looks good, but static-checks has some suggestions.

It looks like some features (interrupt mode, fifo) alre only halfway implemented. E.g. you configure the interrupt mode and fifo, but never read the fifo or register an interrupt pin for the interrupt line.

Do you plan on adding those features to the PR?

And is lis2dh12 sufficiently different from lis2de12 that a common driver is not feasible?

I see they both even respond with the same WHO_AM_I_VAL response in the driver init, so wouldn't lis2dh12 already work with this device out of the box if you set LIS2DH12_PARAM_RESOLUTION to LIS2DH12_POWER_LOW as it seems that the only difference is that lis2de12 only supports 8-bit mode.

* @{
*
* @file
* @brief Command definition for the LIS2DE12 accelerometer
Copy link
Contributor

Choose a reason for hiding this comment

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

This might as well be in lis2de12_registers.h

Comment on lines +303 to +305
fifo_reg.bit.TR = config->FIFO_set_INT2;
fifo_reg.bit.FM = config->FIFO_mode;
fifo_reg.bit.FTH = config->FIFO_watermark;
Copy link
Contributor

Choose a reason for hiding this comment

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

You are configuring the interrupt on the device side, but there is no interrupt handling in the driver.

/* read sampled data from the device */
_acquire(dev);

float sensitivity = (((_read(dev, REG_CTRL_REG4) >> 4) & 0x3) + 1) * 15.6f; //0: +-2g
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does the 15.6 come from?

int lis2de12_set_scale(lis2de12_t *dev, lis2de12_scale_t scale);
lis2de12_scale_t lis2de12_get_scale(lis2de12_t *dev);

int lis2de12_set_fifo(const lis2de12_t *dev, const lis2de12_fifo_t *config);
Copy link
Contributor

Choose a reason for hiding this comment

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

You are never doing anything with the fifo, you only read the first element with lis2de12_read()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: doc Area: Documentation Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: tests Area: tests and testing framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants