Skip to content

Deprecate blocking read sensor APIs #70651

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
1 of 8 tasks
yperess opened this issue Mar 25, 2024 · 14 comments
Open
1 of 8 tasks

Deprecate blocking read sensor APIs #70651

yperess opened this issue Mar 25, 2024 · 14 comments
Assignees
Labels
Architecture Review Discussion in the Architecture WG required area: Sensors Sensors RFC Request For Comments: want input from the community

Comments

@yperess
Copy link
Collaborator

yperess commented Mar 25, 2024

Introduction

The introduction of the asynchronous sensor read and stream APIs have resolved a lot of the previous issues involved with sensors. Though at the current state of things, there are 2 different ways for applications to read sensor data. The redundancy becomes the burden of the driver authors. In order to alleviate this, we propose that after the upcoming 3.7 LTS release, we deprecate the following functions:

  • sensor_sample_fetch()
  • sensor_sample_fetch_chan()
  • sensor_channel_get()

The first two functions can be replaced by the asynchronous sensor_read() functionality, while the latter is replaced by the decoder API. In order to deprecate these, the following conditions must be met:

Hard requirements:

  • Update the samples (at the time of writing there are 19 sensor samples)
  • Update sensor documentation
  • Provide a blocking alternative to sensor_read(). This function (sensor_read_blocking() will allow developers to migrate more easily by effectively fetching, waiting, and decoding). We should also have a backwards compatibility function to convert the new q31_t values to the legacy sensor_value struct such that apps have the easiest time migrating to the new APIs.
  • Fix the sensor clock (RFC# TBD). I haven't formalized this yet, but effectively, we need a chosen node for a clock which would default to the system one if none were selected. Then all sensor time will be measured off this one clock (which may be an external counter).

Soft requirements:

The above are considered "soft" requirements, because they're only hard requirements for streaming implementations. We could deprecate the existing fetch/get paths without them.

Enhancements:

  • Allow specifying channels and attributes in devicetree
@yperess yperess added the RFC Request For Comments: want input from the community label Mar 25, 2024
@yperess yperess self-assigned this Mar 25, 2024
@nordicjm
Copy link
Collaborator

If you have one application that has blocking read, and convert that to this new async. method, can you list the RAM and flash usages for both of these so the 2 can be compared?

@teburd
Copy link
Collaborator

teburd commented Mar 25, 2024

Generally what you will find, past the really simple temp sensor polled once in a great long while type setups... is a cost add in rom and cost reduction in ram from what I've seen.

There's some things we haven't done to minimize that rom cost add and potentially reduce ram usage even further.

@teburd
Copy link
Collaborator

teburd commented Mar 25, 2024

Sensor WG:

  • Luis/Yuval: Note the memory ownership making it difficult to provide an implementation of fetch/get with async + decoder in the RFC
  • Yuval: BMA4xx driver only implements new API... shim-less application with a single sensor vs existing API for ram/rom usage
  • Maureen: There will be an interim period with shimmed drivers with increased ram/rom usage from those
  • Yuval: No fetch/get removal until shim is gone (marked deprecated until then).
  • Tom: There's going to be some ROM increase, likely RAM decrease, and more correct behavior. Not much effort has been put to ROM optimization.
  • Yuval/Maureen: We need to quantify, to some ballpark estimate, what is the cost (~1k rom/X ram, etc)
  • Tom: Quantify which use cases, one temp sensor? Many sensors?
  • Maureen: We need to quantify the simplest case
  • Yuval: Single accelerometer sample, take it, convert it, and measure it.
  • Luis: What are the benefits of this new API, why do we want it?
  • Tom: link issues about why this API is being deprecated
  • Yuval: when do we want to start the deprecation path? We can meet with arch wg before 3.7, April do architecture review (prior to 3.7) to get feedback.
  • Maureen: Bring it to the arch wg soon, ZDS in April. How and when will the driver be migrated?
  • Tom: I think breaking down the maintainership of sensors by vendor helps break down the problem.
  • Yuval: Find/ask authors to update drivers, if not then maintainers would need to work towards doing it.
  • Tom: Exactly, why I want to break down sensors by vendor (tdk/bosch/st/analog)
  • Maureen: Thinking of backporting changes is important. Thinking of deprecating the existing API, when do we stop accepting new drivers using the old API?
  • Yuval: Any non-inflight PRs after the deprecation using the old API shouldn't be accepted

@stubb0rnCoder
Copy link

Is there a reasoning behind the exclusive usage of fixed-point floats instead of native float?

A lot of modern cores, especially the popular "consumer" ones, have a FPU and using FP-floats might even be slower on these systems than using native f32.

@yperess
Copy link
Collaborator Author

yperess commented Apr 8, 2024

Is there a reasoning behind the exclusive usage of fixed-point floats instead of native float?

A lot of modern cores, especially the popular "consumer" ones, have a FPU and using FP-floats might even be slower on these systems than using native f32.

The reasoning is mostly overhead for the driver implementors. You're free to use float in your data processing and I'm happy to provide a translation for that. The normal data flow is as follows:

  • Driver reads raw data
  • User decodes data into q31 format

In your case, we could easily cast the q31 to floats by creating a mirror struct (it would look roughly like this):

struct sensor_three_axis_data {
  struct sensor_header;
  struct sensor_three_axis_sample_data {
    uint32_t timestamp_offset;
    int8_t shift;
    union {
      q31_t values[3];
      struct {
        q31_t x;
        q31_t y;
        q31_t z;
      };
    };
};

#ifdef CONFIG_FPU
struct sensor_three_axis_float_data {
  struct sensor_header;
  struct sensor_three_axis_float_sample_data {
    uint32_t timestamp_offset;
    union {
      float values[3];
      struct {
        float x;
        float y;
        float z;
      };
    };
};

static inline void sensor_three_axis_data_to_float(
    const struct sensor_three_axis_data *src,
    struct sensor_three_axis_float_data *dst
) {
  ...
}
#endif

@teburd
Copy link
Collaborator

teburd commented May 6, 2024

WG 05/06/2024

Top level doc lays out pieces, and terminology, what each thing does at a high level.

Followed by a story driven documentation around solving sensor related problems...

  • Tell a story around use cases
    • Low power low rate sensing
      • environmental
      • asset tracking
    • Event driven (streaming!) sensing
      • robotics
      • industrial
      • wearables
      • portable computing

These docs can tie in both the driver API (how it helps build a foundation to solving these problems) leading into the subsystem and how it allows for additional features in our vision.

Subsystem leads to...

  • filtering
  • sensor fusion
  • data transforms (fft, feature detections like taps/steps/etc)
  • data logging and replay
  • visualization
  • allowing for data bridging to other devices

Doxygen provides the nitty gritty of what is there and how it works.

@KeHIntel
Copy link
Collaborator

KeHIntel commented May 8, 2024

@yperess I have a few questions on the legacy sensor API deprecation proposal:

  • What does legacy API deprecation mean? Does it mean that these APIs will be eventually removed in Zephyr?
  • How about legacy Zephyr sensor device drivers? Are we going to upgrade them, or we implement shim for them, or we simply deprecate them, or we review them one by one and decide case by case?
  • If we are going to upgrade them, who is going to do it, and is there a timeline?
  • If we are going to add shim to them for now, then later are we going to remove shim?
  • I agree that mixing the new API with legacy API is confusing, instead of deprecate the legacy API, will it be better to simply separate the new API out (like put it to sensor_async.h)? Meaning that we keep the legacy API and sensor device drivers as is, and introduce the new async API (in separate header) and new async sensor device drivers (in separate folder).
  • For the new API naming like sensor_stream_xxx, will it be better to be renamed? Streaming sensor and event sensor are two different terminologies in sensor world, conceptual wise, using sensor_stream_xxx for event based sensor looks a bit weird, and can easily create confusion.
  • I am not sure the overhead of new API on the simplest sensor device driver, like HALL sensor, basically it does not use any new feature introduced by the new API. Do we have any comparison data?

@ubieda
Copy link
Member

ubieda commented Mar 13, 2025

Putting my updated thoughts on this issue for continuity:

In the context of #86835 I think we should agree on what the ultimate plan is:

  • user-facing fetch/get APIs
  • fetch/get implementations under sensor_driver_api

My impression based on what's been discussed is that we want developers to use and adopt the read/decode API, however I think (from a code-size/execution aspect) it may not make sense to deprecate one and not the other. Let me explain:

  • Deprecating the User-Facing API will have users utilizing the new semantics, however if their sensors do not implement read/decode APIs it will be painful/costly to have them go through the Shim to read/decode.
  • OTOH the challenge to migrate the sensor-facing API to v2 is significant: as of today, there are just around 15 native implementations of .submit on sensors, while there are ~200-ish sensor drivers implementing .sample_fetch and not .submit.

To get aligned, I suggest the following plan of action:

  1. Get benchmarks on Memory/Code-size for a low data-rate sensor (e.g: TPH, like BME280?) using Fetch/Get APIs vs Read/Decode (no Shim included vs Shim included). Based on this we could decide whether it's required to deprecate one or both.
  2. If applicable, agreement on who will champion migrating sensor drivers to support Read/Decode natively.
  3. Define timeline for deprecation, both:
    • User-facing
    • Sensor-facing (if applicable).

@yperess
Copy link
Collaborator Author

yperess commented Mar 13, 2025

I'm not sure I agree. I see your concern about the bloat of these intermediate steps (1) fallback submit implementation and (2) deprecated API shim. But I think that with the size of this migration there's no way around it. I guess my point is that whatever the benchmarking says, do we really think it's best to keep both APIs around? I would think not, the maintenance and overhead of writing drivers that support both is very big. I would say instead, we should be focusing on how can we get through the migration as quickly as possible so that overhead is short lived.

I 100% agree that we should have someone own the migration. I'm happy to volunteer. And we need to be able to demonstrate that we're making good progress on that migration during every release.

The roadmap I'd like to see is:

  1. Present this to the architecture group and make sure everyone that's using sensors knows about the migration
  2. Agree on a chair and a velocity (N drivers per release)
  3. Deprecate fetch/get
  4. Have the chair coordinate with sensor users to update the drivers. We'll have to depend on the community because even if the WG was to take this on, there's no way we can buy every one of these devices to test.
  5. Have regular updates on the migration in both the TSC and Architecture WG so it's visible.

@yperess
Copy link
Collaborator Author

yperess commented Apr 10, 2025

I'd like to present the following to the Architecture WG as a first step Sensor async API migration.pdf

(this might not be the final slide deck, just the current draft. I'll post here any updates.)

@yperess yperess added the Architecture Review Discussion in the Architecture WG required label Apr 10, 2025
@carlescufi
Copy link
Member

@yperess when would be a good date for you?

@yperess
Copy link
Collaborator Author

yperess commented Apr 14, 2025

@yperess when would be a good date for you?

Any day of the Arch WG works for me. Whenever you have time to squeeze me in.

@bearsh
Copy link
Contributor

bearsh commented Apr 15, 2025

What about adding #77100 as another soft requirement?

@yperess
Copy link
Collaborator Author

yperess commented Apr 15, 2025

What about adding #77100 as another soft requirement?

I think that's appropriate for a phase 2. At first we just want to deprecate the static functions exposed in sensor.h so we get users off the API. The migration off the sensor driver API struct will likely include #77100

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture Review Discussion in the Architecture WG required area: Sensors Sensors RFC Request For Comments: want input from the community
Projects
Status: Todo
Status: No status
Development

No branches or pull requests

9 participants