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.

Sign up for GitHub

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

Jump to bottom

samples: sensor: access_trig: add double tap detection. #83818

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

samples: sensor: access_trig: add double tap detection. #83818

wants to merge 1 commit into from

Conversation

pcurt
Copy link
Contributor

@pcurt pcurt commented Jan 10, 2025

The LIS2DW12 is an advanced 3-axis linear accelerometer.
The driver exists in Zephyr but it is not very easy to configure it and use with trigger detection.

So I added a samples that configure the accelerometer to detect a double tap, the example is running on a stm32f3_disco board with a steval-mki190v1 adapter board

avisconti
avisconti requested changes Jan 10, 2025
View reviewed changes
Copy link
Collaborator

@avisconti avisconti left a comment

Choose a reason for hiding this comment

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

Just few comments.
Also note that you have a typo in the commit message ("aslo" instead of "also")

samples/sensor/lis2dw12/README.rst Outdated Show resolved Hide resolved
samples/sensor/lis2dw12/src/main.c Outdated Show resolved Hide resolved
samples/sensor/lis2dw12/src/main.c Outdated Show resolved Hide resolved
@pcurt pcurt force-pushed the feat_lis2dw12_sample branch from 2e17e21 to 16f1526 Compare January 10, 2025 16:32
@pcurt
Copy link
Contributor Author

pcurt commented Jan 10, 2025

Just few comments. Also note that you have a typo in the commit message ("aslo" instead of "also")

Fixed, thanks

@pcurt pcurt requested a review from avisconti January 10, 2025 16:32
avisconti
avisconti previously approved these changes Jan 10, 2025
View reviewed changes
MaureenHelm
MaureenHelm requested changes Jan 17, 2025
View reviewed changes
Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

We're trying to get away from driver-specific sensor samples. There are other sensor drivers that support the same SENSOR_TRIG_DOUBLE_TAP and SENSOR_CHAN_ACCEL_XYZ channels, so if you'd like to add a sample demonstrating this functionality please make it independent of the specific driver implementation.

@pcurt
Copy link
Contributor Author

pcurt commented Jan 19, 2025

We're trying to get away from driver-specific sensor samples. There are other sensor drivers that support the same SENSOR_TRIG_DOUBLE_TAP and SENSOR_CHAN_ACCEL_XYZ channels, so if you'd like to add a sample demonstrating this functionality please make it independent of the specific driver implementation.

I also think that it is better to have generic samples for multiple hardware. I can integrate the SENSOR_TRIG_DOUBLE_TAP in the accel_trig example (https://docs.zephyrproject.org/latest/samples/sensor/accel_trig/README.html)
What do you think about it ?

@MaureenHelm
Copy link
Member

We're trying to get away from driver-specific sensor samples. There are other sensor drivers that support the same SENSOR_TRIG_DOUBLE_TAP and SENSOR_CHAN_ACCEL_XYZ channels, so if you'd like to add a sample demonstrating this functionality please make it independent of the specific driver implementation.

I also think that it is better to have generic samples for multiple hardware. I can integrate the SENSOR_TRIG_DOUBLE_TAP in the accel_trig example (docs.zephyrproject.org/latest/samples/sensor/accel_trig/README.html) What do you think about it ?

Sounds good, thanks.

@pcurt pcurt force-pushed the feat_lis2dw12_sample branch from 16f1526 to 113d10c Compare January 26, 2025 16:10
@pcurt pcurt changed the title samples: sensor: lis2dw12: add sample with double tap detection. samples: sensor: access_trig: add double tap detection. Jan 26, 2025
@pcurt
Copy link
Contributor Author

pcurt commented Jan 26, 2025

@MaureenHelm I have integrated the tap detection in accel_trig example

@pcurt pcurt requested a review from MaureenHelm January 26, 2025 16:13
@pcurt
Copy link
Contributor Author

pcurt commented Jan 26, 2025

This is the console output with TAP detection enabled

TAP detected
     lis2dw12@19 [m/s^2]:    (   -1.899901,   -12.550355,    -2.742174)
TAP detected
     lis2dw12@19 [m/s^2]:    (   12.349357,   -18.125630,     6.015556)
TAP detected
     lis2dw12@19 [m/s^2]:    (  -11.385050,    -7.274181,    -9.229117)
TAP detected
     lis2dw12@19 [m/s^2]:    (    9.214760,    -9.286545,     2.311466)
TAP detected
     lis2dw12@19 [m/s^2]:    (   10.090533,   -17.391034,    12.320643)
TAP detected
     lis2dw12@19 [m/s^2]:    (   -0.478564,     2.390429,    15.876378)
TAP detected
     lis2dw12@19 [m/s^2]:    (   -5.668596,   -13.138989,     0.741775)
TAP detected
     lis2dw12@19 [m/s^2]:    (   -2.385644,   -10.559526,     9.899107)
TAP detected
     lis2dw12@19 [m/s^2]:    (    7.537391,    -8.551948,    16.740187)

yperess
yperess requested changes Jan 27, 2025
View reviewed changes
Copy link
Collaborator

@yperess yperess left a comment

Choose a reason for hiding this comment

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

Can you also update the README file to reflect the feature?

@pcurt pcurt force-pushed the feat_lis2dw12_sample branch from 113d10c to b3448ff Compare January 27, 2025 08:35
@pcurt
samples: sensor: accel_trig: add sample with double tap detection.
8e0409b 
Add the tap detection trigger to the existing example.
For this example it uses lis2dw12 accelerometer to detect a double
tap.

Signed-off-by: Pierrick Curt <pierrickcurt@gmail.com>
@pcurt pcurt force-pushed the feat_lis2dw12_sample branch from b3448ff to 8e0409b Compare January 27, 2025 08:38
@pcurt
Copy link
Contributor Author

pcurt commented Jan 27, 2025

Can you also update the README file to reflect the feature?

Yes, I have updated it

@pcurt pcurt requested a review from yperess January 27, 2025 08:40
kartben
kartben reviewed Jan 27, 2025
View reviewed changes
samples/sensor/accel_trig/src/main.c
@@ -41,10 +55,19 @@ int main(void)
return 0;
}

#if DT_NODE_HAS_PROP(DT_ALIAS(accel0), tap_mode)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't seem to be generic enough since only ST sensors have this property. You want this generic sample to also work for the other sensors that support tap trigger (fxos8700, icm42605...), no?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kartben kartben kartben left review comments

@yperess yperess yperess approved these changes

@avisconti avisconti avisconti left review comments

@asemjonovs asemjonovs Awaiting requested review from asemjonovs

@jeppenodgaard jeppenodgaard Awaiting requested review from jeppenodgaard

@nashif nashif Awaiting requested review from nashif

@teburd teburd Awaiting requested review from teburd

@tristan-google tristan-google Awaiting requested review from tristan-google

@ubieda ubieda Awaiting requested review from ubieda

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

Requested changes must be addressed to merge this pull request.

Assignees

@MaureenHelm MaureenHelm

Labels
area: Samples Samples area: Sensors Sensors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pcurt @MaureenHelm @avisconti @kartben @yperess @zephyrbot