drivers: sensor: adxl372: Updated driver with RTIO stream functionality #78941

vladislav-pejic · 2024-09-24T13:09:29Z

Updated ADXL372 driver with RTIO stream functionality.
RTIO stream is using both FIFO threshold and FIFO full triggers.
Together with RTIO stream, RTIO async read is also implemented.

Signed-off-by: Vladislav Pejic [email protected]

github-actions · 2024-09-24T13:12:17Z

Hello @vladislav-pejic, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

teburd

Looks like a really nice driver, very happy to see another device taking advantage of this functionality. Now I need to get an analog adxl372 on my bench it seems 💯

teburd · 2024-09-24T13:54:33Z

drivers/sensor/adi/adxl372/adxl372.h

+	uint8_t has_z: 1;
+	uint16_t accel_odr: 4;
+	uint16_t fifo_byte_count: 12;
+} __attribute__((__packed__));


Since its packed, its likely worth asserting the size is aligned to 4 bytes with a build assert here.

BUILD_ASSERT(sizeof(struct adxl372_fifo_data) % 4 == 0, "struct should be word aligned");

Or something along these lines, otherwise unaligned access is likely to bite someone as soon as the struct is changed.

I agree and changes are in the latest code. I needed to do some rearranging to align it and should be ok now.

teburd · 2024-09-24T13:55:39Z

drivers/sensor/adi/adxl372/adxl372_decoder.c

+
+#ifdef CONFIG_ADXL372_STREAM
+
+static uint32_t accel_period_ns[] = {


possible to make const?

Yes. Changes are in the latest code.

teburd · 2024-09-24T13:57:02Z

drivers/sensor/adi/adxl372/adxl372_decoder.c

+	[ADXL372_ODR_6400HZ] = UINT32_C(100000000) / 6400,
+};
+
+static void adxl372_accel_convert_q31(q31_t *out, uint16_t data_in)


This should be doable with a constant multiplier that is pre-calculated, and the function can be inlined in that case.

Can you check the latest code to see if this is what you had in mind?

teburd · 2024-09-24T13:59:01Z

drivers/sensor/adi/adxl372/adxl372_decoder.c

+{
+	struct sensor_value *out = (struct sensor_value *) data_out;
+
+    if (*fit > 0) {


looks like a spacing thing, missing a tab?

teburd · 2024-09-24T14:00:49Z

drivers/sensor/adi/adxl372/adxl372_rtio.c

+
+	__ASSERT_NO_MSG(req);
+
+	rtio_work_req_submit(req, iodev_sqe, adxl372_submit_helper);


Is the work queue usage really needed here? Ideally you'd use RTIO with the spi bus as well to allow for the gpio ISR to directly start a spi transfer and avoid the context swaps.

It seems like the fetch vs stream path are using different methods, which is fine but in the stream case the work queue can be bypassed.

It is not necessary. I updated the implementation in the latest code.

Updated ADXL372 driver with RTIO stream functionality. RTIO stream is using both FIFO threshold and FIFO full triggers. Together with RTIO stream, RTIO async read is also implemented. Signed-off-by: Vladislav Pejic <[email protected]>

zephyrbot · 2024-09-30T08:48:04Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

teburd

Looks pretty good, some minor things I noticed and commented on

teburd · 2024-09-30T16:36:44Z

drivers/sensor/adi/adxl372/adxl372_stream.c

+	if (fifo_samples > sample_set_size/2) {
+		fifo_samples -= sample_set_size/2;
+	} else {
+		LOG_ERR("fifo sample caunt error %d\n", fifo_samples);


s/caunt/count I think? or caught?

drivers/sensor/adi/adxl372/adxl372_stream.c

teburd · 2024-09-30T16:39:52Z

drivers/sensor/adi/adxl372/adxl372_stream.c

+
+	uint32_t buf_avail = buf_len;
+
+	memcpy(buf, &hdr, sizeof(hdr));


Could avoid the memcpy here perhaps by treating the buffer as a *hdr and initializing from there, though that requires that everything is word aligned.

teburd · 2024-09-30T16:43:59Z

drivers/sensor/adi/adxl372/adxl372_stream.c

+	/* Flush completions */
+	struct rtio_cqe *cqe;
+
+	do {


It's likely worth checking error codes here and bailing with rtio_iodev_sqe_err() on a bus error if one occurs.

Can you give me more information? rtio_cqe_consume returns NULL when there is no CQE and that is cowered, and rtio_cqe_release is void func.
Did you mean for this part:
struct rtio_sqe *write_fifo_addr = rtio_sqe_acquire(data->rtio_ctx); ?

When there is a completion queue event from the bus request, its result may be -errno, e.g.

struct rtio_cqe *cqe; int res = 0; do { cqe = rtio_cqe_consume(...); if (cqe != NULL) { if(cqe->result < 0 && res == 0) { /* handle spi/i2c bus error here */ LOG_ERR("first bus error: %d", cqe->result); res = cqe->result; } rtio_cqe_release(cqe); } } while (cqe != NULL); /* Bail/cancel attempt to read sensor on any error */ if (res != 0) { rtio_iodev_sqe_err(...); return; }

Fixed some formating issues. Signed-off-by: Vladislav Pejic <[email protected]>

zephyrbot added area: Sensors Sensors platform: ADI Analog Devices, Inc. labels Sep 24, 2024

zephyrbot requested review from avisconti, galak, jeppenodgaard, MaureenHelm, microbuilder, ozersa, teburd, tristan-google, ttmut, ubieda and yperess September 24, 2024 13:10

zephyrbot assigned MaureenHelm Sep 24, 2024

vladislav-pejic force-pushed the adi_adxl372_rtio_stream branch from e7d3a57 to 3d9f64b Compare September 24, 2024 13:48

teburd reviewed Sep 24, 2024

View reviewed changes

vladislav-pejic force-pushed the adi_adxl372_rtio_stream branch 4 times, most recently from de34935 to 80a5167 Compare September 25, 2024 13:58

vladislav-pejic force-pushed the adi_adxl372_rtio_stream branch from 80a5167 to 43e0d80 Compare September 30, 2024 08:46

vladislav-pejic requested review from anangl, mmahadevan108, dleach02, ssekar15, MulinChao, ChiHuaL and XenuIsWatching as code owners September 30, 2024 08:46

vladislav-pejic requested review from carlescufi, de-nordic, nordicjm and nandojve as code owners September 30, 2024 08:46

zephyrbot added manifest manifest-hal_silabs manifest-hostap manifest-zephyr-lang-rust manifest-nrf_hw_models manifest-hal_espressif manifest-hal_intel manifest-sof DNM This PR should not be merged (Do Not Merge) labels Sep 30, 2024

vladislav-pejic force-pushed the adi_adxl372_rtio_stream branch from 43e0d80 to c6b545a Compare September 30, 2024 10:49

zephyrbot removed manifest manifest-nrf_hw_models manifest-hal_silabs manifest-sof manifest-hal_espressif manifest-hal_intel manifest-hostap manifest-zephyr-lang-rust DNM This PR should not be merged (Do Not Merge) labels Sep 30, 2024

vladislav-pejic force-pushed the adi_adxl372_rtio_stream branch from c6b545a to 18bf686 Compare September 30, 2024 12:57

teburd reviewed Sep 30, 2024

View reviewed changes

MaureenHelm mentioned this pull request Oct 1, 2024

boards: shields: Add EVAL-ADXL372-ARDZ accelerometer shield #79233

Open

vladislav-pejic force-pushed the adi_adxl372_rtio_stream branch from e844bef to 53cdb2c Compare October 1, 2024 20:23

driver: sensor: adxl372: Formating fixed

fe0e73a

Fixed some formating issues. Signed-off-by: Vladislav Pejic <[email protected]>

vladislav-pejic force-pushed the adi_adxl372_rtio_stream branch from 53cdb2c to fe0e73a Compare October 4, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: sensor: adxl372: Updated driver with RTIO stream functionality #78941

drivers: sensor: adxl372: Updated driver with RTIO stream functionality #78941

vladislav-pejic commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

teburd left a comment

teburd Sep 24, 2024

vladislav-pejic Sep 25, 2024

teburd Sep 24, 2024

vladislav-pejic Sep 25, 2024

teburd Sep 24, 2024

vladislav-pejic Sep 25, 2024

teburd Sep 24, 2024

vladislav-pejic Sep 25, 2024

teburd Sep 24, 2024 •

edited

Loading

vladislav-pejic Sep 25, 2024

zephyrbot commented Sep 30, 2024 •

edited

Loading

teburd left a comment

teburd Sep 30, 2024

vladislav-pejic Oct 1, 2024

teburd Sep 30, 2024

vladislav-pejic Oct 1, 2024

teburd Sep 30, 2024

vladislav-pejic Oct 1, 2024

teburd Oct 4, 2024 •

edited

Loading


		#ifdef CONFIG_ADXL372_STREAM

		static uint32_t accel_period_ns[] = {


		__ASSERT_NO_MSG(req);

		rtio_work_req_submit(req, iodev_sqe, adxl372_submit_helper);


		uint32_t buf_avail = buf_len;

		memcpy(buf, &hdr, sizeof(hdr));

drivers: sensor: adxl372: Updated driver with RTIO stream functionality #78941

Are you sure you want to change the base?

drivers: sensor: adxl372: Updated driver with RTIO stream functionality #78941

Conversation

vladislav-pejic commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zephyrbot commented Sep 30, 2024 • edited Loading

teburd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teburd Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

teburd Sep 24, 2024 •

edited

Loading

zephyrbot commented Sep 30, 2024 •

edited

Loading

teburd Oct 4, 2024 •

edited

Loading