Skip to content

drivers/lis2dux12: add read_and_decode APIs support #88598

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

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

Conversation

avisconti
Copy link
Collaborator

Add RTIO async and RTIO stream functionalities that enables, among all the other things, the sensor data streaming from FIFO.

RTIO stream supports following triggers:

  • SENSOR_TRIG_FIFO_WATERMARK
  • SENSOR_TRIG_FIFO_FULL
  • SENSOR_TRIG_DATA_READY

Following FIFO parameters has to be defined in device tree to correctly stream sensor data:

  • fifo-watermark
  • accel-fifo-batch-rate

Currently the driver can decode FIFO content with Accelerometer 16-bit samples.

@avisconti avisconti force-pushed the add-lis2dux12-rtio branch from 0b9f18a to 855b284 Compare April 15, 2025 08:38
@avisconti avisconti requested a review from ct-lt April 15, 2025 14:54
@avisconti avisconti force-pushed the add-lis2dux12-rtio branch from 855b284 to ae53446 Compare April 15, 2025 15:21
},
.fifo_count = fifo_count,
.accel_batch_odr = lis2dux12->accel_batch_odr,
.accel_odr = lis2dux12->odr,
Copy link

Choose a reason for hiding this comment

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

We could store fifo_status[0] (also in rx_data->int_status instead of fifo_status[1]) in .int_status and then use that in the decoder like

static bool lis2dux12_decoder_has_trigger(const uint8_t *buffer, enum sensor_trigger_type trigger)
{
	const struct lis2dux12_fifo_data *data = (struct lis2dux12_fifo_data *)buffer;
	const struct lis2dux12_decoder_header *header = &data->header;

	if (header->is_fifo) {
		switch (trigger) {
		case SENSOR_TRIG_FIFO_WATERMARK:
			return data->int_status & 0x80;
		case SENSOR_TRIG_FIFO_FULL:
			return data->int_status & 0x40;
		default:
			return false;
		}
	}

	return false;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it seems to me a good idea. There are so many things still left to be implemented. I appreciate your help!

Copy link

Choose a reason for hiding this comment

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

I'm happy to help as I really appreciate the driver!
The FIFO triggers are easy as we configure and read the correct registers anyway.
The others like (in)activity and tap recognition require some more configuration and registers to be read, so maybe it would also make sense to put the whole trigger topic to a different PR. Then we could replace int_status with a bitfield for the detected triggers or something like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just pushed a new version. Nevertheless I saw a furter review from your side just now. So, I think there will be another one later on

Copy link

Choose a reason for hiding this comment

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

Yeah sorry for the single comments, should've batched them in a review from the beginning. But I'm done for now

return false;
}

SENSOR_DECODER_API_DT_DEFINE() = {
Copy link

Choose a reason for hiding this comment

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

#define DT_DRV_COMPAT st_lis2dux12 is missing before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had forgotten it. Changed, tested and re-pushed

Copy link

@ct-lt ct-lt left a comment

Choose a reason for hiding this comment

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

Thanks for that driver!
I tested the following on a custom board with an nRF5340 and the LIS2DUX12:

  • Streaming and decoding with various sample rates and ranges using samples/sensor/stream_fifo
  • Fetch + Get of temperature
  • One-shot reads of temperature and three-axis data
  • Accelerometer batching (we currently need to enable temperature batching for that due to the underlying ST driver)

if (header->is_fifo) {
return lis2dux12_decode_fifo(buffer, chan_spec, fit, max_count, data_out);
} else {
return lis2dux12_decode_sample(buffer, chan_spec, fit, max_count, data_out);
Copy link

Choose a reason for hiding this comment

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

I'd remove the else branch as it is the same as the return below, but that may just be my personal preference

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it makes sense. The code would be more compacted

#include <zephyr/rtio/rtio.h>

int lis2dux12_gbias_config(const struct device *dev, enum sensor_channel chan,
enum sensor_attribute attr,
Copy link

Choose a reason for hiding this comment

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

I guess these are leftovers from the other driver?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ooops, yes. The lsm6dsv16x drivers also support sensor fusion stuff, while this one doesn't.
Thanks again for the rview.

@avisconti avisconti force-pushed the add-lis2dux12-rtio branch 2 times, most recently from 97d443a to acd37f1 Compare April 17, 2025 12:33
@avisconti
Copy link
Collaborator Author

Thanks for that driver!
I tested the following on a custom board with an nRF5340 and the LIS2DUX12:

Streaming and decoding with various sample rates and ranges using samples/sensor/stream_fifo
Fetch + Get of temperature
One-shot reads of temperature and three-axis data
Accelerometer batching (we currently need to enable temperature batching for that due to the underlying ST driver)

Very good! I appreciate your help. It really motivates me.
I am working at a new commit for this driver which will support 3 types of FIFO content:

 - 0x0 # 1x Accelerometer @12bit and 1x temperature @12bit samples
 - 0x1 # 1x Accelerometer @16bit sample
 - 0x2 # 2x Accelerometer @8bit samples (previous and current)

This will be selectable from a new DT prop: fifo-mode-sel.
Please note that fifo-mode-sel = <1> is matching the FIFO content of this current PR.

Add RTIO async and RTIO stream functionalities that enables,
among all the other things, the sensor data streaming from FIFO.

RTIO stream supports following triggers:

  - SENSOR_TRIG_FIFO_WATERMARK
  - SENSOR_TRIG_FIFO_FULL
  - SENSOR_TRIG_DATA_READY

Following FIFO parameters has to be defined in device tree to
correctly stream sensor data:

  - fifo-watermark
  - accel-fifo-batch-rate

Currently the driver can decode FIFO content with Accelerometer
16-bit samples.

Signed-off-by: Armando Visconti <[email protected]>
@avisconti avisconti force-pushed the add-lis2dux12-rtio branch from acd37f1 to 9866e50 Compare April 22, 2025 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants