Add support for defining the sensor channel in DT

[kornelduleba]

This functionality is needed at least in the case of temperature
sensors. Zephyr specifies three temperature related channels:

• SENSOR_CHAN_DIE_TEMP
• SENSOR_CHAN_AMBIENT_TEMP
• SENSOR_CHAN_GAUGE_TEMP

In addition to that some devices can report temperature from multiple
probes. This normally handled by using a sensor specific channels.

Right now the consumers of such devices need to hardcode the sensor
channel in their logic. Ideally the sensor channel would to use would be
defined in DT. This is necessary to make a "generic" application that
would work with a variety of different sensors. In the future we might
also want to introduce a thermal management framework similar to thermal
zones implemented in Linux. Such logic would need a way to specify a
sensor channel in DT too.

The "sensor-cells" property is marked as optional to retain backward
compatibility with existing sensor DT nodes.
For now only temperature related channels have been redefined in the
dt-bindings header. Note that since dtc can't parse enums, the channel
values had to be redefined as macros.

DT helper macros were provided to parse the new specifier. They're
heavily based on similar existing logic for PWMs.
Some basic tests were implemented to validate their functionality.

Fixes #61163

[yperess]

I think I may not have been 100% clear. What I'd like to see if a new entry here that looks like what's described in #62944

types:
  type: array
  description: |
    List of sensor channel types. Example:
    <SENSOR_CHAN_ACCEL SENSOR_CHAN_GYRO>

Then in sensor.h append to sensor_info

struct sensor_info {
  const struct device *dev;
  const char *vendor;
  const char *model;
  const char *friendly_name;
#ifdef CONFIG_SENSOR_INFO_RUNTIME_CHANNELS
  const uint32_t * const channels;
  size_t num_channels;
#endif
};

Then you would need to add CONFIG_SENSOR_INFO_RUNTIME_CHANNELS to drivers/sensors/Kconfig.sensor_info and modify SENSOR_INFO_DT_DEFINE to pass the right values based on the Kconfig.

Finally, add your sensor_dt_spec which you can verify at compile time like:

/***** Add this to devicetree.h *****/
#define CHECK_FOR_VALUE_IDX(node_id, prop, idx, check_value) \
    (DT_PROP_BY_IDX(node_id, idx) == check_value)

#define CHECK_FOR_VALUE(node_id, field_name, check_value)
    DT_FOREACH_PROP_ELEM_SEP_VARGS(node_id, field_name,
                                   CHECK_FOR_VALUE_IDX, (||), check_value) == true

/***** Add this to sensor.h *****/
struct sensor_dt_spec {
  const struct device *dev;
  uint32_t type;
};

/**
 * example:
 *   struct sensor_dt_spec accel =
 *       SENSOR_DT_GET_TYPE(DT_ALIAS(accel), SENSOR_CHAN_ACCEL_XYZ);
 */
#define SENSOR_DT_GET_TYPE(node_id, type_)                           \
    COND_CODE_1(CHECK_FOR_VALUE(node_id, types, type_),              \
                ({.dev = DEVICE_DT_GET(node_id), .type = (type_),}), \
                (error))

[kornelduleba]

Then you would need to add CONFIG_SENSOR_INFO_RUNTIME_CHANNELS to drivers/sensors/Kconfig.sensor_info and modify SENSOR_INFO_DT_DEFINE to pass the right values based on the Kconfig.

I'm not sure if hiding the channels behind CONFIG_SENSOR_INFO_RUNTIME_CHANNELS is a good idea.

1. We'll need to versions of macros that interact with struct sensor_info, to take the conditionally enabled arguments into account.
2. The user will have to enable this config option, even though the property itself might already be defined in DT. This might be somewhat confusing for folks developing applications and trying to use this feature.

How about dropping the config option and just tanking the increase in the array size?

[teburd]

Why does sensor_dt_spec only support a single channel type, sensors are rarely a single channel type? Or is type meant to in some way describe sensors that support multiple channel types? Perhaps sensor_dt_channel_spec would be a better name here as that's closer to what it actually is. A reader for a single channel of a sensor.

This part is otherwise a bit confusing to me.

[kornelduleba]

My idea here is that sensor_dt_spec defines a pair <sensor, channel> that is consumed by an application.
The concept is based on pwm_dt_spec, which does pretty much the same thing for a PWM.
In particular we can have multiple specs pointing to a single piece of hardware.
I'll rename it to sensor_dt_channel_spec as you suggested.

When it comes to declaring which channels a senor supports, I've implemented that in sensor_info struct - link.

[kornelduleba]

I'll rename it to sensor_dt_channel_spec as you suggested.

done

[teburd]

My idea here is that sensor_dt_spec defines a pair <sensor, channel> that is consumed by an application. The concept is based on pwm_dt_spec, which does pretty much the same thing for a PWM. In particular we can have multiple specs pointing to a single piece of hardware. I'll rename it to sensor_dt_channel_spec as you suggested.

How does this work though in practice without mis-aligning the reads, or how would I define a sensor_dt_spec for multiple channels? What about multiple channels of the same type?

I think there's some missing information in the sensor_dt_spec in multiple ways.

Many sensors are offered that can sample multiple temperature, capacitance, light, voltage, amperage, and likely various other physical measurements.

E.g. a ti tmp468, ad max6581, ams as7341, ti fdc1004, and many many more.

I'd rather see something like

struct sensor_dt_spec {
   const struct device *dev
   uint32_t_t channel_count;
   struct sensor_channel channels[];
};

struct sensor_channel {
      enum sensor_channel_type chan_type;
      uint32_t chan_id;
};

And a way to generate that dt spec from device tree. This way you can construct a reader for say, accel xyz, gyro xyz, mag xyz as a triple. Or perhaps you wish to sample 3 of the 4 capacitance channels of the ti fdc1004 for liquid level measurement. Or have a complex machine with many temperature probes you are measuring with the max6581.

I'm not really sure what connecting a sensor and single channel type (not even what should be a channel identifier pair (type, index) buys us.

[kornelduleba]

How does this work though in practice without mis-aligning the reads, or how would I define a sensor_dt_spec for multiple channels? What about multiple channels of the same type?

What do you mean by misaligning the reads?

For a sensor with multiple channels you'd need to declare <device, channel> for each channel, which I agree is less than ideal. The type of each channel would have to be parsed in the application. I suppose that to address this the sensor_read API could be used. The data would be then processed similarly to what is done in sensor_shell_processing_callback.

(...)
And a way to generate that dt spec from device tree. This way you can construct a reader for say, accel xyz, gyro xyz, mag xyz as a triple. Or perhaps you wish to sample 3 of the 4 capacitance channels of the ti fdc1004 for liquid level measurement. Or have a complex machine with many temperature probes you are measuring with the max6581.
I'm not really sure what connecting a sensor and single channel type (not even what should be a channel identifier pair (type, index) buys us.

The problem with that approach is that we need to define how many items are expected in a "phandle specifier" in the sensor DT node. In my solution a "phandle specifier" for a sensor that supports (device, channel, index) would have #sensor-cells=<2>, one that supports (device, channel) would have #sensor-cells=<1> .
Then, for example a "temperature" sensor(s) consumer could look something like this:

compatible = "zephyr,thermal-zone-foo";
(...)
sensors = <&sensor1 SENSOR_CHAN_DIE_TEMP>, <&sensor2 SENSOR_CHAN_AMBIENT_TEMP>;

I don't think it's possible to use phandle-array to describe a variable number of sensor channels.
Instead we could have dedicated "container nodes" for sensor that would look something like this:

{
compatible = "zephyr,sensor-config"
sensor = <&sensor1>;
channels = <SENSOR_CHAN_FOO SENSOR_CHAN_BAR>;
}

I'm not sure if we want to introduce more "zephyr,foo" "configuration" nodes though.

The problem that I want to solve with this PR is to avoid having to specify the sensor channel in application code.
This is especially problematic for temperature sensors as there are multiple channels that point to a "temperature" type e.g. SENSOR_CHAN_DIE_TEMPERATURE, SENSOR_CHAN_AMBIENT_TEMPERATURE.
I want to write logic that parses temperature readings from multiple sensors. The sensors themselves can use different channels for temperature reading. My solution for that is to have an array of struct sensor_dt_spec type and use each entry to call the sensor_sample_fetch and sensor_channel_get in a loop and a corresponding DT node, as described above.

Also, as you mentioned the "index" is another problem that needs to be fixed for sensors that have multiple can measure stuff using multiple probes of the same type. It's not that easy to solve since it requires some changes to the driver level sensor API. (struct sensor_dt_spec and sensor_channel_get specifically.)

My concern with the sensor_read API is that it allocates quite a lot of data per every device. (SENSOR_DT_READ_IODEV) So if I want to read just one channel from multiple sensors, it's going to be quite heavy.
Having a reader that can iterate over multiple sensor devices, would work great for my use case.

How about something like:

1. The sensor consumer will use a custom zephyr node for describing <sensor_dev, chan_1, chan_2...> as described above.
2. Struct wise we can use the what you proposed, though I'd prefer to keep using enum sensor_channel to limit the scope of the changes.

struct sensor_dt_spec {
   const struct device *dev
   uint32_t channel_count;
   enum sensor_channel channels[];
};

3. Then the struct sensor_read_config would just be a list of struct sensor_dt_spec + some more information(?) The RTIO sensor reader logic would have to be updated to work with that too:

struct sensor_read_config {
   sys_snode_t node;
   struct sensor_dt_spec spec;
}

What do you think?

BTW My end goal is to add support for something similar to Linux thermal zones.

[teburd]

I think I better understand here. See #63830 for what might be better approach for a channel specifier. I'm willing to implement it if people like it.

I don't want us to keep building on the same mistaken enum sensor_channel, it just means more work replacing it.

A device like the max6581, used here for example to measure stratix die temps on this dev board, https://www.intel.com/content/www/us/en/docs/programmable/683561/current/temperature-monitoring.html is a perfect example of what I think you are looking to accomplish or close to it am I wrong?

So pretending I'm creating a thermal zone to control a fan on a stratix lets say I create something like...

compatible = "zephyr,thermal-zone-stratix";
(...)
sensors = <&max6581 ...>, <&max6581 ...>;

I see two problems.

1. If we use the singular existing enum sensor_channel we are already in trouble, it doesn't pan out. We're going to be concocting custom enums and that sucks. Better to start off on the right foot instead using something like what I'm proposing in RFC: Sensor Channel Specifiers #63830
2. The DT data is less than ideally setup for us to do a single read from a single sensor device across a selection of channels. We would need to create, at init or runtime, a grouping of channels per device to then create a read request for that device and set of channels.

This is what I meant by unaligned reads, if we naively read each (sensor, channel) pair we're going to waste a lot of time and energy, and also miss the opportunity (in the stratix case for example) of getting a nice snapshot of the temperature across the die at a single sample clock.

I think 2 can be fixed with some channel aggregation but really the array of tuples makes that harder than it needs to be IMO, 1 is starting off down the wrong path to begin with.

Instead maybe what we need is the idea of a channel set to convert to a sensor_dt_spec like struct.

e.g.

Convertible to a sensor_dt_spec

max6851_stratix_temps: {
compatible = "zephyr,sensor-spec";
sensor = <&max6581>;
channels = <SENSOR_CHAN_TEMP SENSOR_CHAN_TEMP_PROBE 0>, <SENSOR_CHAN_TEMP SENSOR_CHAN_TEMP_PROBE 1>, <SENSOR_CHAN_TEMP SENSOR_CHAN_TEMP_PROBE 2>, ... <SENSOR_CHAN_TEMP SENSOR_CHAN_TEMP_PROBE 6>;
}

Sensors understood to be a phandle array to sensor specs

compatible = "zephyr,thermal-zone-stratix";
(...)
sensors = <&max6581_stratix_temps>;

Now all the channels can be read at once from the max6581 in a single read, sensor_dt_spec makes good sense (device pointer with channel set), and the phandle array is simple.

Better still maybe, each sensor-spec (given a name) could have a defined static global so the iodev isn't repeatedly redefined as it is today.

[kornelduleba]

A device like the max6581, used here for example to measure stratix die temps on this dev board, https://www.intel.com/content/www/us/en/docs/programmable/683561/current/temperature-monitoring.html is a perfect example of what I think you are looking to accomplish or close to it am I wrong?

Yes, that's pretty much it.
One thing is that I'm going to poll 2+ sensors in each consumer.
Some of those sensors might have multiple temperature probes, like #60833.

I briefly looked at #63830 and it sounds good to me.
Let me take a closer look.

[teburd]

For multiple sensor devices with multiple probes you'd have a sensor-spec per sensor device, and you'd need a single read request per each. But at least this way its going to be about as optimal as all of this can be. You could save a bit more memory by creating your own read requests without a memory pool as the memory pool does add a slight memory overhead.

e.g. very roughly

/* These three lines in place of sensor_read() calls */
struct rtio_sqe *sensor0 = rtio_sqe_read_prep(....);
struct rtio_sqe *sensor1 = rtio_sqe_read_prep(....);
rtio_submit(..., 2); /* wait for all sensors to finish reading */

/* process completions and decode the data */
...

[kornelduleba]

FYI the sensor: Expose sensor channels to DT commit is a WIP/RFC.
I'd like to see if the approach is right before committing more.

[mbolivar-ampere]

This file ... feels wrong. @tejlmand

[mbolivar-ampere]

cc @nordicjm

[kornelduleba]

Keep in mind this is not the final version.
My intention with this is to submit an RFC to test the waters, and see if this approach is acceptable.
I've mentioned that in the comment above, but perhaps I should have been more clear about this.
So yes, it's not particularly pretty. :)

[kornelduleba]

At least I need to document it and probably add set it up so that the header is not regenerated every build.

[tejlmand]

not sure why this is made into a CMake file, yet alone a CMake module file.

When introducing new Zephyr CMake modules, then those should be written in a re-usable way, which is surely not the case here.

[yperess]

@MaureenHelm we discussed this a bit over a week ago. @kornelduleba and I couldn't figure out a way to make configure_file work for what we needed. This approach is a bit clunky but it does seem to work as long as the formatting is maintained. Though I wonder if as a temporary solution it might make sense to just not use C as the interim.

For context, we want to avoid having a separate constant for devicetree and enum. The options being considered are:

1. Have duplication (both the #define and enum) but deprecate the enum for 1 release during which we have to make sure any new channels are added to both
2. Create a common way to generate both from one source.

Personally, due to the complexity we're seeing I'm leaning towards lets just have duplication for 1 release and deprecate the enums.

[mbolivar-ampere]

Personally, due to the complexity we're seeing I'm leaning towards lets just have duplication for 1 release and deprecate the enums.

+1 if it keeps this out of the build system

[kornelduleba]

Ok, will do that.

[tejlmand]

not sure why this is made into a CMake file, yet alone a CMake module file.

When introducing new Zephyr CMake modules, then those should be written in a re-usable way, which is surely not the case here.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.