-
Notifications
You must be signed in to change notification settings - Fork 8k
drivers: sensor: st: lis2du12: add SENSOR_TRIG_DELTA support #96376
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
base: main
Are you sure you want to change the base?
drivers: sensor: st: lis2du12: add SENSOR_TRIG_DELTA support #96376
Conversation
39d31b5
to
d015a4f
Compare
d015a4f
to
69001a1
Compare
Fixed SonarQube's "discards 'const' qualifiers" warnings. |
b83b6bf
to
79c8a20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems not bad. Please look at my notes. The only doubt I have is that we will need sooner or later to migrate to new read and decode APIs, which means also a different way to handle triggers.
This number represents which of the two interrupt pins | ||
(INT1 or INT2) the delta (wake-up) line is attached to. This property is not | ||
mandatory and if not present it defaults to 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it defaults to 2? Because you want to keep drdy on int1 and interrupts on int2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avisconti That is influenced by the LIS2DH sensor. Per default, INT1 is used there for data-ready and INT2 for motion interrupts, so I chose these defaults here as well, even if one could use only one line for both interrupts here.
Maybe choosing a default of 1 would be a little bit more robust here, because this will work with one or two interrupt lines connected (if we assume a user would connect INT1 at first)?
If you want, I could change It to 1 as default, but personally I see no real advantage for this one or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no, your choice is fine. I was thinking to add more of such details in the description itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avisconti :D ok, now I get you. I will add the additional information that one could use both interrupts together on one line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avisconti I have added the additional explanations.
(INT1 or INT2) the delta (wake-up) line is attached to. This property is not | ||
mandatory and if not present it defaults to 2. | ||
enum: [1, 2] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to add wakeup threshold and duration also here, to configure it at compile time. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avisconti I have thought about that as well, but my conclusion was that the sensor API has attributes for these values and assumes that they are set through the API because the documentation of sensor_trigger_type states:
SENSOR_TRIG_DELTA:
Trigger fires when the selected channel varies significantly.
This includes any-motion detection when the channel is acceleration or gyro. If detection is based on slope between successive channel readings, the slope threshold is configured via theSENSOR_ATTR_SLOPE_TH
andSENSOR_ATTR_SLOPE_DUR
attributes.
And with that in mind, my conclusion was that I don't wanted to introduce settings with "no effect" to the device-tree, only that a user could get those from there manually to pass it to the sensor_attr_set()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooook (with still some thinking :) )
@avisconti Thank you for your quick response, I really appreciate it! Don't know if I get you wrong, but you wrote:
But wouldn't this have an effect on the most of the sensor drivers and is more of a general problem of deprecation of API (here the fetch and get API)? I think this extension could help someone (at least me/us) as long as the fetch and get API is valid. Everything else has to be tackled by future PRs, after the fetch and get API is deprecated. |
Yes, that is also correct. I think that I was confused by the fact that it is not possible to submit NEW drivers using fetch_and_get APIs, but I guess it would be OK to maintain drivers even based on old solution. I'm not sure if I have been clear enough, but I would ask @teburd to comment on it. |
@avisconti and maybe @teburd
I have read https://docs.zephyrproject.org/latest/hardware/peripherals/sensor/index.html#implementing-sensor-drivers before and there was no hint that it is not possible to add new drivers with fetch and get API. Maybe there should be a note or at least a hint if the information is documented somewhere else. I cross the fingers that maintenance of existing drivers is still allowed 🤞😅 |
Extend the LIS2DU12 accelerometer driver with SENSOR_TRIG_DELTA support. The detection is based on the slope between successive channel readings. Support for setting SENSOR_ATTR_SLOPE_TH and SENSOR_ATTR_SLOPE_DUR is added as well. In line with other sensors, SENSOR_ATTR_SLOPE_TH is configured in SI units (m/s^2) and SENSOR_ATTR_SLOPE_DUR in samples relative to the ODR. The new trigger can be mapped either to the same GPIO as the data-ready interrupt or to a dedicated one. Signed-off-by: Jan Kablitz <[email protected]>
79c8a20
to
9f39f6f
Compare
|
Tomorrow I'll take a look again to your new push. |
This PR extends the LIS2DU12 accelerometer driver with support for SENSOR_TRIG_DELTA.
The trigger detects changes based on the slope between successive channel readings.
This patch also adds support for configuring:
SENSOR_ATTR_SLOPE_TH (threshold, in SI units m/s²)
SENSOR_ATTR_SLOPE_DUR (duration, in samples relative to the ODR)
The new trigger can be routed either to the same GPIO used for the data-ready interrupt or to a dedicated interrupt line.
The changes were tested using a nucleo_l433rc_p connected to the LIS2DU12 via I²C and confirmed to work reliably.