drivers: sensor: adxl34x: Add sensor driver for ADXL3x motion sensors #77275
Conversation
chaim-zax
force-pushed
the
adxl34x_accel_driver
branch
2 times, most recently
from
211b1fa to
79a8e06
Compare
The current adxl345 driver doesn't support the extended driver features Zephyr has to offer, and only support the sensor sample_fetch and channel_get API. It also doesn't support adxl344 and adxl346 sensors. This new driver is a full replacement of the existing driver (using a compatibility flag) and, apart from the full driver API (adding support for attributes, triggers, RTIO and decoders), also support access to all registers of the adxl343, adxl344, adxl345 and adxl346 sensors. It also has power management support and comes with a full set of regression tests. The adxl345 driver API has an additional (unofficial) feature which uses the FIFO to return multiple values using a single sensor_sample_fetch command. To make the adxl34x driver a drop in replacement of the adxl345 driver an additional compile flag was introduced which mimics this behavior (default off). Both the adxl345 and adxl34x drivers have been tested using the various (application) use-cases to check functionality and behavior. Source code is available in a different repository. Signed-off-by: Chaim Zax <[email protected]>
chaim-zax
force-pushed
the
adxl34x_accel_driver
branch
from
79a8e06 to
1fb5da9
Compare
zephyrbot
requested review from
MaureenHelm,
avisconti,
galak,
jeppenodgaard,
microbuilder,
ozersa,
teburd,
tristan-google,
ttmut,
ubieda and
yperess
|
@MaureenHelm can you have a look at this pull request? I'm still a bit unfamiliar with the Zephyr contribution process, so please let me know if there is still anything I need to (or can) do. |
| sensor_ug_to_ms2(ug, &ms2); | ||
| const int64_t q31_temp = ((int64_t)ms2.val1 * INT64_C(1000000) + ms2.val2) * | ||
| ((int64_t)INT32_MAX + 1) / ((1 << shift) * INT64_C(1000000)); | ||
| *out = CLAMP(q31_temp, INT32_MIN, INT32_MAX); |
There was a problem hiding this comment.
This should be a simple mulitply rather than requiring division, shifting, and 64 bit math. The raw value is 16bit, you are converting to a q1.31 with a new scale.
See #76968
There was a problem hiding this comment.
Thanks for the feedback @teburd, I was starting to worry if anyone would take the time for this review.
I am new with the q31 notation so I mainly looked at existing drivers for examples. I see what you're getting at. No problem, the driver already uses some lookup tables for conversion and it shouldn't be a problem to extend this in favor of having these calculations done runtime.
| #elif defined(CONFIG_ADXL34X_DATA_TYPE_SENSOR_VALUE) | ||
| struct sensor_value *out = (struct sensor_value *)data_out; | ||
|
|
||
| #elif defined(CONFIG_ADXL34X_DATA_TYPE_DOUBLE) |
There was a problem hiding this comment.
Doubles aren't supported from the driver itself through this API, its always possible to convert a q31 to a double.
There was a problem hiding this comment.
The Zephyr documentation indicates the q31 notation is experimental, so I wasn't sure what the best approach would be. Therefore I made the output configurable compile time. I have no problem removing this 'feature' and completely switch to q31 notation for the decoder. Shall I completely remove the 'double' and 'sensor_value' support?
| enum pm_device_state pm_state; | ||
| int rc; | ||
|
|
||
| rc = pm_device_state_get(dev, &pm_state); |
There was a problem hiding this comment.
It's really nice we are able to do this automatic power management potentially with read+decode, @JordanYates might be interested or have some comments here on this.
| int rc; | ||
|
|
||
| /* Read accel x, y and z values. */ | ||
| rc = config->bus_read_buf(dev, ADXL34X_REG_DATA, rx_buf, buf_size); |
There was a problem hiding this comment.
This looks to be a blocking read, any particular reason not to trigger an async read here?
Imagine wishing to start the read periodically directly from a timer interrupt for example.
There was a problem hiding this comment.
No particular reason. I had some issues finding a good reference driver and started with a relative simple approach (using blocking calls). The icm42688 driver is the only one using the RTIO system to do the actual I2C commands as well for streaming data, which does use this async mechanism (as far as I can tell). I can do the same here.
|
@ubieda the sensor driver implements several optional interfaces. I've split up the implementation of these interfaces into various files. So if needed, I can limit the initial pull request to the basic 'fetch' and 'get' interface, and add additional pull requests by adding these files one by one (more or less) for the remaining interfaces. This is relatively simple. Would this be the preferred way of working? |
Starting simple sounds good. As far a splitting commits, I'd suggest:
If possible, leave the RTIO support on a separate PR so we don't block the rest iterating on this. Does it sound like a good plan? |
Sounds perfectly fine! I'll leave out the RTIO support initially, so there will be a total of four pull requests. I'll keep this one open and will use it for the first pull request. Thanks for the feedback. |
No need to have 4-PRs. As long as they're separate commits: you can have
|
Ah, sorry, my mistake. I misread, so two pull requests, splitting the first one up in several commits. Thanks! |
|
Just a few days ago PR #80004 was merged. This PR extends the existing ADXL345 driver with RTIO, decoder and trigger features. Initially this PR was meant to replace the existing ADXL345 driver. Now with the changes to the ADXL345 driver I'm wondering if we should merge the two. Anyone any thoughts on this? |
Update the existing ADXL345 driver to support ADXL343/344/346 variants, then add the other triggers and emulator support in separate commits or PRs. Want to avoid having two drivers for the same hardware. |
Agree, no sense in having two similar drivers. Sounds like a fine plan. I'll add the test-cases as well. |
|
@chaim-zax , I am changing the milestone to 4.1 as we are now only merging bug fixes and documentation updates for 4.0 release. |
|
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
6 participants
The current adxl345 driver doesn't support the extended driver features Zephyr has to offer, and only support the sensor sample_fetch and channel_get API. It also doesn't support adxl344 and adxl346 sensors. This new driver is a full replacement of the existing driver (using a compatibility flag) and, apart from the full driver API (adding support for attributes, triggers, RTIO and decoders), also support access to all registers of the adxl343, adxl344, adxl345 and adxl346 sensors. It also has power management support and comes with a full set of regression tests.
The adxl345 driver API has an additional (unofficial) feature which uses the FIFO to return multiple values using a single sensor_sample_fetch command. To make the adxl34x driver a drop in replacement of the adxl345 driver an additional compile flag was introduced which mimics this behavior (default off).
Both the adxl345 and adxl34x drivers have been tested using the various (application) use-cases to check functionality and behavior. Source code is available in a different repository.