zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.87k stars 6.62k forks source link

Sensor Subsystem: data types #48150

Closed yperess closed 2 years ago

yperess commented 2 years ago

The subsystem should not distinguish between calibrated and uncalibrated types in the data structure layer.

The currently proposed API includes:

struct senss_sensor_value_3d_int32 {
    struct senss_sensor_value_header header;
    struct {
        uint32_t timestamp_delta;
        union {
            int32_t v[3];
            struct {
                int32_t x;
                int32_t y;
                int32_t z;
            };
        };
    } readings[1];
};

struct senss_sensor_value_uncalib_3d_int32 {
    struct senss_sensor_value_header header;
    struct {
        uint32_t timestamp_delta;
        union {
            int32_t v[3];
            struct {
                int32_t x;
                int32_t y;
                int32_t z;
            };
        };
        union {
            int32_t bias[3];
            struct {
                int32_t bias_x;
                int32_t bias_y;
                int32_t bias_z;
            };
        };
    } readings[1];
};

This means that any client requesting uncalibrated data will consume twice as much memory. Like previous comments, I don't believe that we should be deviating from existing public APIs in this domain and we should be replicating the CHRE model. In said model, the bias events are handled in the callbacks passed into the open function (see https://github.com/zephyrproject-rtos/zephyr/issues/48149). The bias event will then use the same data structure as a data event.

For example:

  1. A sensor is running at 6kHz
  2. Data will be passed to the client's data_event_callback as it is aggregated
  3. When a new bias is available, the subsystem will call the client's bias_event_callback
  4. Both calls to data_event_callback and bias_event_callback will use the same struct sensor_subsystem_sensor_value_3d

The thinking here is that bias events are rare. For most applications they're done once in the factory and never updated again. There's no need to send the same data to the client on every single sample. As an example, AR/VR applications always care about uncalibrated data. In those cases, we're likely running the sensor at full tilt and will be using twice as much memory (as well as copy operations) to provide data that will always be ignored (AR/VR applications don't care about the bias in most cases).

PROPOSAL

Follow the model set by CHRE in https://cs.android.com/android/platform/superproject/+/master:system/chre/chre_api/include/chre_api/chre/sensor_types.h;l=360;drc=66cdc563f0e2be5c0253d38756125b801239a698 and use a single data structure for both bias and data events. Make the differentiation at the callback level instead. Since the data structures are timestamped, applications will be able to detect when in relation to the data, the bias changed. This will provide a near 50% memory saving when clients use uncalibrated data.

microbuilder commented 2 years ago

I wouldn't say this is most often limited to the factory -- for example, an IMU will be calibrated before use, and the mag cal data, if done on the fly, might only be available after a certain amount of motion -- but I agree the current approach is wasteful and the calibration data shouldn't be associated with every sample in any use case I can think of myself, and the callback here is a sensible way to handle that.

huhebo commented 2 years ago

Concerns about the separated bias data event callback approach:

  1. How to sync the time-stamp of the sensor data and the bias data?

    The CHRE sensor API recomends the PAL should deliver the bias data at the same interval that sensor data is delivered:

       Additionally, as specified for configureSensor(), it is recommended that
       the PAL should deliver the bias data at the same interval that sensor data
       is delivered, in order to optimize for power, with the bias data being
       delivered first so that nanoapps are easily able to translate sensor data
       if necessary. If bias events are not delivered at the same interval as the
       sensor data, they should be delivered as close to the corresponding sensor
       data as possible to reduce the amount of time nanoapps need to remember
       multiple bias updates.

    For some sensors, the bias data may rarely changs, but for some sensors such as mag, it's may changs frequently, the worse case is the change rate is same as the sensor data depends on the iron enviroments.

    And for seprated specfic bias data structure, we need a seprated time-stamp data filed, such as chre defined a header and time-stamp data fields, in this case, the seprated approach not saved memory but may need more memory since it's need extra data header and time-stamp data fields.

  2. How to handle batch mode for an un-calibrated sensor including it's sensor data and bias data ?

    Separated approach will introduce complexity for App to sync with buffered sensor data and buffered bias data.

Benefits for the un-calibrated sensor type with combined sensor data and bias data approach (approach proposed in senss #45370)

  1. No sync problem for sensor data and bias data since they already synced in a same sensor data structure.

    For the worse case that the bias updates same as the sensor data, it's saved memory since the data header and time-stamp fields are shared with sensor data and bias data.

  2. We do have some use case that the bias data is more useful not just for calibration.

    For example, we can use bias data for an un-calibrated mag sensor to help detect current environment, such as in a car, a metro, or a room, in this case, the bias data not used for calibration, but itself is a key inputs for the algorithm to distinguish different environments with different iron impacts. In this case, the bias data is more like a sensor data and need update frequently.

  3. Unified and simplified sensor concepts and APIs both for App and internal implements.

    For example, sensor subsystem use totally the same common data/config path for all sensors, no need special path for bias (separated callback etc) both in normal streaming mode and batch buffer mode, this will make App more simple and easy to use bias data, no need sync sensor data with bias data etc.

huhebo commented 2 years ago

This topic is a continue discussion from https://github.com/zephyrproject-rtos/zephyr/pull/45370#discussion_r901209004

yperess commented 2 years ago

Concerns about the separated bias data event callback approach:

  1. How to sync the time-stamp of the sensor data and the bias data? The CHRE sensor API recomends the PAL should deliver the bias data at the same interval that sensor data is delivered:

      Additionally, as specified for configureSensor(), it is recommended that
      the PAL should deliver the bias data at the same interval that sensor data
      is delivered, in order to optimize for power, with the bias data being
      delivered first so that nanoapps are easily able to translate sensor data
      if necessary. If bias events are not delivered at the same interval as the
      sensor data, they should be delivered as close to the corresponding sensor
      data as possible to reduce the amount of time nanoapps need to remember
      multiple bias updates.

    For some sensors, the bias data may rarely changs, but for some sensors such as mag, it's may changs frequently, the worse case is the change rate is same as the sensor data depends on the iron enviroments. And for seprated specfic bias data structure, we need a seprated time-stamp data filed, such as chre defined a header and time-stamp data fields, in this case, the seprated approach not saved memory but may need more memory since it's need extra data header and time-stamp data fields.

Right, so in the one example you've given you're right, but for 99.9% of applications that's not the case. For those cases, this separate approach would save a lot of memory. Can you point at the bias algorithm? I'm mostly familiar with using a casa filter to collect a sample window and generate a bias from that, this algorithm cannot generate a new bias per sample. @teburd are you familiar with a bias algorithm that generates a new bias per sample?

  1. How to handle batch mode for an un-calibrated sensor including it's sensor data and bias data ? Separated approach will introduce complexity for App to sync with buffered sensor data and buffered bias data.

Benefits for the un-calibrated sensor type with combined sensor data and bias data approach (approach proposed in senss #45370)

  1. No sync problem for sensor data and bias data since they already synced in a same sensor data structure. For the worse case that the bias updates same as the sensor data, it's saved memory since the data header and time-stamp fields are shared with sensor data and bias data.

Right, but the worst case is so rare that it should not be guiding the design. Batching can simply be flushed when a new bias is present. I would suggest that if you have a bias algorithm that will generate a new bias per sample, you do it in the application layer and not in the sensor subsystem.

  1. We do have some use case that the bias data is more useful not just for calibration. For example, we can use bias data for an un-calibrated mag sensor to help detect current environment, such as in a car, a metro, or a room, in this case, the bias data not used for calibration, but itself is a key inputs for the algorithm to distinguish different environments with different iron impacts. In this case, the bias data is more like a sensor data and need update frequently.

You could do the same with the callback for a bias event.

  1. Unified and simplified sensor concepts and APIs both for App and internal implements. For example, sensor subsystem use totally the same common data/config path for all sensors, no need special path for bias (separated callback etc) both in normal streaming mode and batch buffer mode, this will make App more simple and easy to use bias data, no need sync sensor data with bias data etc.

Maybe, but I would argue that if you want the data/bias separate already then you're probably doing something custom. If you wanted to just add them you'd register for calibrated data instead of uncalibrated.

teburd commented 2 years ago

@teburd are you familiar with a bias algorithm that generates a new bias per sample?

I've been thinking about this all morning. I think the answer is yes. Temperature compensating bias may require a continuous bias change. In my own product experience I had one product (chromatography related) where the sensors bias would drift with temperature. This is true of IMU sensors as well. Temperature variations mean bias drifts.

Also, there are some clear ideas that automatic and continuous magnetometer calibration is a thing (I haven't used it myself) when doing some quick searching, which I have to say is super cool.

I'm far from an expert though, and I'm deferring to @huhebo's expertise on this. If there's a use case that needs bias updates frequently then I'd expect @huhebo to be correct and know that.

If memory density is a real concern, then perhaps you could pack the bias and data in the same size struct today by using the 16 bit raw values of both bias and sensor data from the sensor and providing accessor functions to decode things into meaningful values. This of course does require knowledge of the sensor configuration at the time of the reading.

That all said I do understand the concern. However if the concern is memory usage, then there are other ways to address that I believe.

yperess commented 2 years ago

@teburd are you familiar with a bias algorithm that generates a new bias per sample?

I've been thinking about this all morning. I think the answer is yes. Temperature compensating bias may require a continuous bias change. In my own product experience I had one product (chromatography related) where the sensors bias would drift with temperature. This is true of IMU sensors as well. Temperature variations mean bias drifts.

Also, there are some clear ideas that automatic and continuous magnetometer calibration is a thing (I haven't used it myself) when doing some quick searching, which I have to say is super cool.

I'm far from an expert though, and I'm deferring to @huhebo's expertise on this. If there's a use case that needs bias updates frequently then I'd expect @huhebo to be correct and know that.

If memory density is a real concern, then perhaps you could pack the bias and data in the same size struct today by using the 16 bit raw values of both bias and sensor data from the sensor and providing accessor functions to decode things into meaningful values. This of course does require knowledge of the sensor configuration at the time of the reading.

That all said I do understand the concern. However if the concern is memory usage, then there are other ways to address that I believe.

I believe my concern is that 99% of applications will not need or expect bias to update frequently or at all. Also, since this is the data provided by the sensor subsystem it's possible to add any custom or more advanced calibration algorithms ontop of the sensor subsystem (instead of inside it). My ideal architecture to this would look like:

The alternative proposed here forces every application to consume larger memory and additional copies of said memory. I would much prefer that in the rare case where bias needs to be computed per sample, this will be the application's job (I don't foresee us implementing every bias calculation out there into the subsystem and having a Kconfig for each). The subsystem is suppose to optimize for the common case.

huhebo commented 2 years ago

We do have the usages in Windows, such as many 2in1 products, and I don't believe they are 1% of applications :)

For the case need more advanced calibration algorithms on top of the sensor subsystem, we can define new sensor type, e.g,. a raw sensor type, then no any bias data defined in the sensor value data structure, but just some raw sensor data fields. This is the advantage and flexibility of the sensor subsystem.

yperess commented 2 years ago

We do have the usages in Windows, such as many 2in1 products, and I don't believe they are 1% of applications :)

Are you saying that if I write a windows application I'll be able to see sensor samples with a different bias on every sample? Can you point me at the proper set-up for this? I'd be very interested to try it and see what that data looks like.

For the case need more advanced calibration algorithms on top of the sensor subsystem, we can define new sensor type, e.g,. a raw sensor type, then no any bias data defined in the sensor value data structure, but just some raw sensor data fields. This is the advantage and flexibility of the sensor subsystem.

Right, but in windows you don't need that to be the data structure coming out of the subsystem do you? You can easily combine the data + bias in the application processor, this would:

  1. Save you on bandwidth sending data from the ISH to the AP
  2. Reduce the memory requirements of the sensor subsystem

I'm not arguing that the data isn't easier to handle when together (even Android's callback includes both in the same Java class, see documentation and sensor type), but that's exactly it, it's done in the application layer, not in the EC which is more resource constrained. So my proposal for this is to provide a minimal implementation with different callbacks. This avoids having to create a new sensor type for different calibration algorithms.

yperess commented 2 years ago

Here's a rough diagram for the flow of bias calculators I've envisioned bias flow

yperess commented 2 years ago

Intel/Google sync next steps:

  1. Come up with a way for applications to provide their own bias calculations (this would allow for proprietary bias calculations).
  2. Provide a way for the final data format to be customizable per application. Example being explored is that the default SENSOR_TYPE_ACCELEROMETER_UNCALIBRATED is mapped to struct sensor_data_3d_float can be overridden by the application to another data struct (one that provides both the data + bias in the case of the ISH).
yperess commented 2 years ago

@huhebo @MaureenHelm @keith-zephyr here's an idea that I think might give everyone what they need. I believe that if we treat bias calculators no different than a virtual sensor it solves the issue. So lets use the issue discussed yesterday as an exmaple.

Combined data example:

  1. The ISH needs both bias + data in the same sample and it doesn't make sense to add multiplexing outside of the subsystem.
  2. ISH defines a new virtual sensor and assigns it a SENSOR_TYPE_ISH_ACCELEROMETER_UNCALIBRATED and SENSOR_EVENT_UNCALIBRATED_ACCELEROMETER_DATA.
  3. The virtual sensor requests data from SENSOR_TYPE_ACCELEROMETER_UNCALIBRATED and provides a new data event for its own sensor type.
  4. Clients in the ISH can now register for SENSOR_TYPE_ISH_ACCELEROMETER_UNCALIBRATED and get their data

Bias example:

  1. A client wants to generate a custom bias event (separate from the sensor data).
  2. The client registers a virtual sensor and assigns it a SENSOR_TYPE_ACCELEROMETER and SENSOR_EVENT_ACCELEROMETER_BIAS_INFO.
  3. The virtual sensors requests data from SENSOR_TYPE_ACCELEROMETER_UNCALIBRATED and generates bias info events
  4. This virtual sensor will only be enabled by the subsystem when there exists a client that configures SENSOR_TYPE_ACCELEROMETER to also provide bias events

The key distinction here is that we split the sensor type from the event. Events can be either data or bias. In the case of ISH they're combined into a single data event which will still be multiplexed through the subsystem. This will introduce no overhead to the smaller systems (I'm still in the process of getting some statistics on this) while still allowing the ISH to do what it needs to.

References:

yperess commented 2 years ago

Here's a quick graph surveying the existing boards in Zephyr. I excluded some of the high end because they make the graph unreadable, but here's the raw data...

Ram Size
<=  32  kB        25.65%
<= 128  kB        59.69%
<= 512  kB        85.34%
<=   2  MB        92.15%
<=   8  MB        95.03%
<=  32  MB        98.17%
<= 128  MB        98.95%
<= 512  MB        99.74%
   512+ MB        100.00%

% of boards vs  RAM Size

keith-zephyr commented 2 years ago

@huhebo Do you any feedback for Yuval's proposal? https://github.com/zephyrproject-rtos/zephyr/issues/48150#issuecomment-1249623867

huhebo commented 2 years ago

@yperess and @keith-zephyr

The proposal seems use a virtual sensor concept for the separated bias data, then it can have separated update frequency according usage needs.

So, for ISH windows, which need same frequency with sensor data, can still use the combined sensor type definition, but with other cases which bias data updates rarely, can use the separated virtual sensor for just bias data.

If my understanding is correct, I think that's a very good idea, all of these just used existing designed virtual sensor concepts!

yperess commented 2 years ago

Sounds great, I'm going to close this issue with the final conclusion that:

  1. We'll provide 2 event types for the MVP
    1. data events
    2. bias events
  2. The default implementation will have 1 data structure and the callback will be the key differentiation.
  3. Bias calculations will be done via virtual sensors framework
  4. 3P consumers of the sensor subsystem (such as the ISH for Windows) can define these custom bias calculators that generate custom sensor data types that combine both the bias and raw data into one and will use the data event callback code path.