Skip to content

Conversation

ubieda
Copy link
Member

@ubieda ubieda commented Oct 15, 2025

Basic helper to reduce code-duplication.

Note

I'm aware this removes error-handling of failing to obtain the timestamp.

my impression was that even without timestamp, some-data (and the time at which it is received) is better than no data with an error. In both cases, the error can still be detected (e.g: timestamp = 0) and handled accordingly

Although that's my preference, I'm willing to add it back. Feel free to chime in with your thoughts.

@ubieda ubieda added the DNM This PR should not be merged (Do Not Merge) label Oct 15, 2025
Spares from having to do it in a two-step process, typically seen in
sensor drivers.

Signed-off-by: Luis Ubieda <[email protected]>
In order to reduce duplication.

Signed-off-by: Luis Ubieda <[email protected]>
@ubieda ubieda force-pushed the sensor-clock-add-ns-helper branch from ca22cae to d254169 Compare October 15, 2025 20:47
@ubieda ubieda requested a review from avisconti October 15, 2025 20:49
Copy link

Copy link
Contributor

@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.

Please add docs in the decoder that a timestamp of 0 is now an error.

* and converts it directly in nanoseconds, instead of having to do it in
* a two-step process.
*
* @return nanoseconds on succes, zero on failure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @return nanoseconds on succes, zero on failure.
* @return nanoseconds on success, zero on failure.

* @brief Get Current sensor clock in nanoseconds.
*
* This helper function performs the acquisition of sensor time in cycles
* and converts it directly in nanoseconds, instead of having to do it in
Copy link
Contributor

Choose a reason for hiding this comment

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

not a native speaker so i might be wrong

Suggested change
* and converts it directly in nanoseconds, instead of having to do it in
* and converts it directly to nanoseconds, instead of having to do it in

@kartben kartben requested a review from Copilot October 15, 2025 21:57
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a helper function sensor_clock_get_ns() to simplify sensor timestamp retrieval by combining cycle acquisition and nanosecond conversion into a single call. The helper reduces code duplication across multiple sensor drivers and removes error handling for timestamp acquisition, returning 0 on failure instead of propagating errors.

  • Adds sensor_clock_get_ns() helper function to sensor_clock.h
  • Replaces two-step timestamp acquisition pattern across 30+ sensor driver files
  • Removes error handling for timestamp failures, allowing sensors to continue operation with zero timestamp

Reviewed Changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
include/zephyr/drivers/sensor_clock.h Adds new inline helper function sensor_clock_get_ns()
drivers/sensor/tdk/icm45686/icm45686_stream.c Replaces manual timestamp acquisition with helper function
drivers/sensor/tdk/icm45686/icm45686_decoder.c Simplifies timestamp acquisition using new helper
drivers/sensor/tdk/icm4268x/icm4268x_rtio_stream.c Updates to use helper function for timestamp
drivers/sensor/tdk/icm4268x/icm4268x_decoder.c Adopts new helper for timestamp generation
drivers/sensor/st/lsm6dsv16x/lsm6dsv16x_rtio_stream.c Replaces error-prone timestamp code with helper
drivers/sensor/st/lsm6dsv16x/lsm6dsv16x_rtio.c Simplifies timestamp handling
drivers/sensor/st/lis2dux12/lis2dux12_rtio_stream.c Updates timestamp acquisition pattern
drivers/sensor/st/lis2dux12/lis2dux12_rtio.c Uses helper function for cleaner code
drivers/sensor/st/iis3dwb/iis3dwb_stream.c Adopts simplified timestamp approach
drivers/sensor/st/iis3dwb/iis3dwb.c Updates to use new helper function
drivers/sensor/pni/rm3100/rm3100_stream.c Simplifies timestamp acquisition
drivers/sensor/pni/rm3100/rm3100_decoder.c Uses helper for timestamp generation
drivers/sensor/pixart/pat9136/pat9136_stream.c Replaces complex timestamp code with helper
drivers/sensor/pixart/pat9136/pat9136_decoder.c Adopts new helper function
drivers/sensor/pixart/paa3905/paa3905_stream.c Updates timestamp handling
drivers/sensor/pixart/paa3905/paa3905_decoder.c Uses simplified timestamp approach
drivers/sensor/memsic/mmc56x3/mmc56x3_async.c Adopts helper function
drivers/sensor/melexis/mlx90394/mlx90394_async.c Simplifies timestamp code
drivers/sensor/default_rtio_sensor.c Updates fallback sensor implementation
drivers/sensor/broadcom/afbr_s50/afbr_s50.c Uses new helper for timestamp
drivers/sensor/bosch/bmp581/bmp581_decoder.c Adopts simplified approach
drivers/sensor/bosch/bmm350/bmm350_decoder.c Updates timestamp handling
drivers/sensor/bosch/bme280/bme280_async.c Uses helper function
drivers/sensor/bosch/bma4xx/bma4xx_rtio_stream.c Simplifies timestamp acquisition
drivers/sensor/bosch/bma4xx/bma4xx_rtio.c Adopts new helper
drivers/sensor/asahi_kasei/akm09918c/akm09918c_async.c Updates timestamp code
drivers/sensor/adi/adxl372/adxl372_stream.c Uses helper function
drivers/sensor/adi/adxl367/adxl367_stream.c Simplifies timestamp handling
drivers/sensor/adi/adxl362/adxl362_stream.c Adopts new approach
drivers/sensor/adi/adxl345/adxl345_stream.c Updates to use helper function

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 99 to 102
const struct sensor_read_config *cfg = iodev_sqe->sqe.iodev->data;
struct mlx90394_data *data = dev->data;
uint64_t cycles;

rc = mlx90394_trigger_measurement_internal(dev, cfg->channels->chan_type);
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Variable 'rc' is used without declaration. It was removed from the declarations but is still being used.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

good bot

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we could use a build-all test that covers streaming.

Copy link
Member Author

Choose a reason for hiding this comment

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

Filed #97824 to bridge this gap. Thanks for catching it!

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

Labels

DNM This PR should not be merged (Do Not Merge)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants