Skip to content

Conversation

@cmark85
Copy link
Contributor

@cmark85 cmark85 commented Oct 22, 2024

This commit introduces a new Sensor Clock API, enabling the retrieval of cycle counts and conversion to nanoseconds based on the system or external clock. The API includes:

  • sensor_clock_get_cycles() to get the current cycle count from the sensor clock.
  • sensor_clock_cycles_to_ns() to convert cycles to nanoseconds using the clock's frequency.

The implementation supports both system clocks and external clocks defined in the device tree, making the sensor clock integration more flexible for various sensor use cases.

@github-actions
Copy link

Hello @CienetmarkChen, 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. 😊

@cmark85 cmark85 force-pushed the CienetmarkChen/add-sensor-clock-api branch 2 times, most recently from 04a9e31 to 930b227 Compare October 22, 2024 04:04
@kartben
Copy link
Contributor

kartben commented Oct 22, 2024

@CienetmarkChen Can you elaborate on the problem you're trying to solve with this PR? I am not sure I understand. Thanks!

@cmark85
Copy link
Contributor Author

cmark85 commented Oct 23, 2024

Hi kartben,
The Zephyr asynchronous sensor API supports precise timestamping for high-accuracy applications. Currently, the API provides nanosecond timestamps through the sensor_data_header in sensor_data_types.h. However, the method used to obtain these timestamps often lacks precision due to inaccurate tick-based timing, making driver-provided timestamps unreliable. Additionally, the API doesn’t easily support external clocks.
To address this, we want to provide a new file under include/zephyr/drivers/sensor_clock.h which will declare:

int sensor_clock_get_cycles(uint64_t *cycles)

uint64_t sensor_clock_cycles_to_ns(uint64_t cycles)

RFC: #63651

@cmark85 cmark85 force-pushed the CienetmarkChen/add-sensor-clock-api branch from 930b227 to 9784107 Compare October 30, 2024 06:27
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.

Can you update existing drivers which use the async API to leverage this sensor_clock?

@cmark85 cmark85 force-pushed the CienetmarkChen/add-sensor-clock-api branch 2 times, most recently from 1ae0322 to 39b9f0a Compare November 8, 2024 02:06
yperess
yperess previously approved these changes Nov 8, 2024
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.

@teburd @MaureenHelm this seems good to me now, when you can, could you take a look?

teburd
teburd previously approved these changes Nov 14, 2024
@teburd
Copy link
Contributor

teburd commented Nov 14, 2024

I think this is ok, though because we're doing cycles solely, its quite possible to roll-over quickly in some cases. E.g. a 32bit cycle counter going at 1ghz (e.g. imxrt1176 cm7 core) will roll over every... ~4 seconds.

I don't know if this is something we need to care about yet, but certainly some issue should be noted that this might be an issue that needs addressing.

@yperess
Copy link
Contributor

yperess commented Nov 14, 2024

I think this is ok, though because we're doing cycles solely, its quite possible to roll-over quickly in some cases. E.g. a 32bit cycle counter going at 1ghz (e.g. imxrt1176 cm7 core) will roll over every... ~4 seconds.

I don't know if this is something we need to care about yet, but certainly some issue should be noted that this might be an issue that needs addressing.

Yeah, I think we need to have a how-to guide for designing your sensor clocks. Like if your system clock is running 1gHz with a 32 bit counter but your sensors only run at 400Hz, having an external clock that's closer to 400Hz makes a lot of sense to avoid that roll-over.

@jeremybettis
Copy link
Contributor

@MaureenHelm Any blocker here?

@yperess
Copy link
Contributor

yperess commented Dec 3, 2024

Can this merge? It's getting stale and there's more work building on top of it.

@kartben
Copy link
Contributor

kartben commented Dec 3, 2024

Can this merge? It's getting stale and there's more work building on top of it.

let's page @MaureenHelm once more then :)

Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Can you update existing drivers which use the async API to leverage this sensor_clock?

There are a few existing drivers missing this update (ADI and ST)

fabiobaltieri
fabiobaltieri previously approved these changes Dec 23, 2024
yperess
yperess previously approved these changes Dec 24, 2024
@yperess
Copy link
Contributor

yperess commented Jan 8, 2025

@MaureenHelm would you be able to take another pass at this one? I believe they've addressed your concerns.

Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Sorry I didn't catch this before, but the sample needs an README.rst. Everything else looks good.

ubieda
ubieda previously approved these changes Jan 14, 2025
@ubieda
Copy link
Member

ubieda commented Jan 14, 2025

We should link this PR to #63651

This commit introduces a new Sensor Clock API, enabling the retrieval
of cycle counts and conversion to nanoseconds based on the system or
external clock. The API includes:

- `sensor_clock_get_cycles()` to get the current cycle count from the
  sensor clock.
- `sensor_clock_cycles_to_ns()` to convert cycles to nanoseconds using
  the clock's frequency.

The implementation supports both system clocks and external clocks
defined in the device tree, making the sensor clock integration more
flexible for various sensor use cases.

Signed-off-by: Mark Chen <[email protected]>
@cmark85 cmark85 dismissed stale reviews from ubieda, yperess, and fabiobaltieri via fb6f4ae January 15, 2025 04:08
@cmark85 cmark85 force-pushed the CienetmarkChen/add-sensor-clock-api branch from 5e0e68f to fb6f4ae Compare January 15, 2025 04:08
@cmark85
Copy link
Contributor Author

cmark85 commented Jan 15, 2025

Sorry I didn't catch this before, but the sample needs an README.rst. Everything else looks good.

Hi @MaureenHelm ,
We have added a README.rst file to the sensor clock sample folder. Please take a look. Thank you.

Copy link
Contributor

@kartben kartben left a comment

Choose a reason for hiding this comment

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

thanks @MaureenHelm for spotting the missing README :)

@kartben kartben merged commit f4da9b9 into zephyrproject-rtos:main Jan 15, 2025
24 checks passed
@github-actions
Copy link

Hi @CienetmarkChen!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

rc = sensor_clock_get_cycles(&cycles);
if (rc != 0) {
LOG_ERR("Failed to get sensor clock cycles");
rtio_iodev_sqe_err(drv_data->streaming_sqe, err);
Copy link
Member

Choose a reason for hiding this comment

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

This driver no longer builds due to use of err instead of what I assume was meant to be rc. Will open PR to fix.

Copy link
Member

Choose a reason for hiding this comment

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

zephyr/drivers/sensor/tdk/icm42688/icm42688_rtio_stream.c:305:61: error: 'err' undeclared (first use in this function)
  305 |                 rtio_iodev_sqe_err(drv_data->streaming_sqe, err);
      |                                                             ^~~           

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

Labels

area: Samples Samples area: Sensors Sensors platform: ADI Analog Devices, Inc. Release Notes To be mentioned in the release notes

Projects

Development

Successfully merging this pull request may close these issues.

10 participants