driver/sensor: add unified management for sensor

[Donny9]

Summary

Add unified management for sensor.
The driver model supports 24 kinds of sensor and provide a simple sensor driver model.
It's devided into two layers: upper half and lower half.
Upper half layer provides sensor character device registration, circualr buffer managerment, batch mode setting, etc.
Lower half layer mainly deals with sensor register and do sensor operation sets: activate, set_interval, batch.

Impact

It can make sensor driver more simple, management more convenient and more standard.

Testing

daily test

[protobits]

Interesting. How do you envision this being used?
Would a sensor using this interface be supported upstream or do you intend to have this as an option to internally support sensors via this interface?
Would this require porting currently supported sensors to this interface? I wonder if using a common interface would not become restrictive in order to provide the best support for a sensor.

[btashton]

@Donny9 are you thinking of this kind of being like the Linux IIO subsystem?
https://www.kernel.org/doc/html/latest/driver-api/iio/intro.html

What about triggering and devices that have multiple channels?

[Donny9]

Interesting. How do you envision this being used?
Would a sensor using this interface be supported upstream or do you intend to have this as an option to internally support sensors via this interface?
Would this require porting currently supported sensors to this interface? I wonder if using a common interface would not become restrictive in order to provide the best support for a sensor.

1. I will provide a driver of wtgahrs2 as a example to description this driver model using.
   2.There are 58 types of drivers in current nuttx version. It's impossible for me to transplant all the sensor into this model, because i don't have so many sensor devices and i can't verify them. Currently, all the sensor driver of nuttx are chaotic, They are not unified ioctl cmd, type, event data structure. So, This will cause the upper layer of the application to rely heavily on the sensor driver implementation.
   3.This model of i submitted will not limit the use of the sensor driver before. I hope everyone will use the model in the future to uniformly manage all sensors, such as linux, rtt and so on.

[Donny9]

@Donny9 are you thinking of this kind of being like the Linux IIO subsystem?
https://www.kernel.org/doc/html/latest/driver-api/iio/intro.html

What about triggering and devices that have multiple channels?

Yes, this model is particularly similar to the linux iio system, whose upper and linux iio subsystems are functionally similar. Upper half layer of this model implements sensor_register api, circular buffer management, batch mode setting and expose push_event to lower half layer.
Lower half layer of this model implements sensor operations, initialize and communication with i2c,spi,...interface.

[btashton]

@Donny9 I would be very supportive of an approach like this. I had the same feeling trying to use the sensor subsystem and had asked about building something like this out a couple years ago. I probably have 10 or 15 of the sensors that are currently supported, especially the i2c ones, so I could probably contribute some of the lower half drivers over time and certainly do some testing.

[Donny9]

we can simulate the motion algorithm in sim platform with wtgahrs2 by serial interface "/dev/ttyUSBX" on ubuntu host.

This driver of wtgahrs2 follow this model of i submitted.

This is spec. it includes accelermeter, gyroscope, magnetic, baromenter, gps.
WTGAHRS2_V1.0.pdf

[Donny9]

@Donny9 I would be very supportive of an approach like this. I had the same feeling trying to use the sensor subsystem and had asked about building something like this out a couple years ago. I probably have 10 or 15 of the sensors that are currently supported, especially the i2c ones, so I could probably contribute some of the lower half drivers over time and certainly do some testing.

I have written sensorhub software on linux and ceva/qcom dsp before, and i found that they are particularly good compared with the nuttx sensor driver. so i looked at rtt, linux, android hal, AliOS,zephy,mynewt_core. You can try porting a device on this model.

https://github.com/alibaba/AliOS-Things/blob/master/include/service/udata/udata.h
https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/sensors/1.0/types.hal
https://android.googlesource.com/platform/hardware/interfaces/+/refs/heads/master/contexthub/1.0/types.hal
https://github.com/zephyrproject-rtos/zephyr/blob/master/include/drivers/sensor.h
https://github.com/RT-Thread/rt-thread/blob/master/components/drivers/sensors/sensor.h
https://github.com/apache/mynewt-core/blob/master/hw/sensor/include/sensor/sensor.h

[protobits]

I also like the idea of unifying interfaces and reducing the need to repeat it across each sensor. But I worry if this is really a general sensor solution. Mainly since this introduces an intermediate buffering which may not be desired (due to space and added processing, in contrast to simply have the lower half write into the buffer passed by the caller). Maybe this buffer could optionally be disabled somehow.

[Donny9]

This is sensortest for this model. The url is apache/nuttx-apps#434

[btashton]

I also like the idea of unifying interfaces and reducing the need to repeat it across each sensor. But I worry if this is really a general sensor solution. Mainly since this introduces an intermediate buffering which may not be desired (due to space and added processing, in contrast to simply have the lower half write into the buffer passed by the caller). Maybe this buffer could optionally be disabled somehow.

Seems like we could add an interface to register a buffer with the driver? IIO has this internal interface:
https://www.kernel.org/doc/html/latest/driver-api/iio/buffers.html#c.iio_device_attach_buffer

[Donny9]

I also like the idea of unifying interfaces and reducing the need to repeat it across each sensor. But I worry if this is really a general sensor solution. Mainly since this introduces an intermediate buffering which may not be desired (due to space and added processing, in contrast to simply have the lower half write into the buffer passed by the caller). Maybe this buffer could optionally be disabled somehow.

The buffer is originally designed to solve the problem that the upper application fails to read sensor data timely due to jamming, and it will bring some protection. We suggest that the length of this buffer should be 2-3 sensor events for a sensor with a high sampling rate. If the buffer is low sampling rate, it can be set to 1 sensor event.

The events generated in this sensor model are all active and not read by the read function, because the read function is sometimes slow, which can cause performance problems for application .

[btashton]

@v01d what if the buffer itself could be reinitialized via ioctl? Then it would still be controlled by the application, but the buffer would reside in kernel space where is probably should be anyway?

We would basically just call sensor_buffer_create again.

One of the other advantages is applications can query the kernel for a sensor value like instantaneous temperature without having to set up the sensor.

[Donny9]

I also like the idea of unifying interfaces and reducing the need to repeat it across each sensor. But I worry if this is really a general sensor solution. Mainly since this introduces an intermediate buffering which may not be desired (due to space and added processing, in contrast to simply have the lower half write into the buffer passed by the caller). Maybe this buffer could optionally be disabled somehow.

Seems like we could add an interface to register a buffer with the driver? IIO has this internal interface:
https://www.kernel.org/doc/html/latest/driver-api/iio/buffers.html#c.iio_device_attach_buffer

Yes, we've thought about this way before. But I think it would be better to give the circular buffer pointer to the application via the read function?

[btashton]

I also like the idea of unifying interfaces and reducing the need to repeat it across each sensor. But I worry if this is really a general sensor solution. Mainly since this introduces an intermediate buffering which may not be desired (due to space and added processing, in contrast to simply have the lower half write into the buffer passed by the caller). Maybe this buffer could optionally be disabled somehow.

Seems like we could add an interface to register a buffer with the driver? IIO has this internal interface:
https://www.kernel.org/doc/html/latest/driver-api/iio/buffers.html#c.iio_device_attach_buffer

Yes, we've thought about this way before. But I think it would be better to give the circular buffer pointer to the application via the read function?

I generally think that makes sense I guess the question is if we make the buffer dynamic or not. I am inclined to allow the application to resize the buffer if the ref count on it is 0.

[xiaoxiang781216]

@v01d and @btashton, there is one problem to let user control the buffer size: to support the hardware FIFO which is very popular on the new accel/gyro sensor model, sensor upper layer will extend the buffer temporarily to accumulate the bulk data and shrink the size once the userspace read out the data.

Compare with IIO, the major difference is that:

1. Don't support to access/control sensor through sysfs
2. Don't support to issue the i2c/spi transaction directly inside the read callback
3. Driver writer control how much buffer need through sensor_lowerhalf_s::buffer_bytes

The limitation for item 2 is to reduce the driver implementation complexity.

[btashton]

there is one problem to let user control the buffer size: to support the hardware FIFO which is very popular on the new accel/gyro sensor model, sensor upper layer will extend the buffer temporarily to accumulate the bulk data and shrink the size once the userspace read out the data.
y.

I understand why the FIFO is important, but I think this still can work:

• Track the ref count via the number of open fd on the driver
• If count is 1 allow ioctl to resize the fifo, this would clear the fifo and resize the underlying malloc
• if count is > 1 then the driver would fail to resize since it is already in use

[xiaoxiang781216]

Yes, it could be added very easily with the current implementation. To simplify the design, we have two concept about the buffer size:

1. Driver can control the buffer size in no batch mode by sensor_lowerhalf_s::buffer_bytes
2. Application can control through ioctl e.g. SIOC_SET_BUFFER_SIZE(of course, another patch)
3. sensor upper will extend the buffer as need in batch mode to accumulate the bulk data

The first two items let user decide how much the jitter or latency should be used, the last item avoid the driver writer involve the complex buffer management.

[Donny9]

I also like the idea of unifying interfaces and reducing the need to repeat it across each sensor. But I worry if this is really a general sensor solution. Mainly since this introduces an intermediate buffering which may not be desired (due to space and added processing, in contrast to simply have the lower half write into the buffer passed by the caller). Maybe this buffer could optionally be disabled somehow.

Seems like we could add an interface to register a buffer with the driver? IIO has this internal interface:
https://www.kernel.org/doc/html/latest/driver-api/iio/buffers.html#c.iio_device_attach_buffer

Yes, we've thought about this way before. But I think it would be better to give the circular buffer pointer to the application via the read function?

I generally think that makes sense I guess the question is if we make the buffer dynamic or not. I am inclined to allow the application to resize the buffer if the ref count on it is 0.

[protobits]

Regarding either driver or application buffer control, I think it is more important to let the application control it since the lower driver does not necessarily know how the user intends to access it.
I understand you see the need for a buffer for fast sensors, but you could ideally setup the sensor to a slow rate and have the application simply poll() the device in a separate thread. For "slower" sensors this makes sense and the whole intermediate buffer handling would be just added complexity.

Also, how can you deal with ICs having different sensors inside? Sometimes these are different I2C devices in the same chip and sometimes they are the same device but with separate register sets (for example, temperature + pressure). While you could separate it into two drivers, sometimes this is not a good idea since it is better to be aware that they share a bus or that the sensor cannot actually give data from both independently.

[Donny9]

Regarding either driver or application buffer control, I think it is more important to let the application control it since the lower driver does not necessarily know how the user intends to access it.
I understand you see the need for a buffer for fast sensors, but you could ideally setup the sensor to a slow rate and have the application simply poll() the device in a separate thread. For "slower" sensors this makes sense and the whole intermediate buffer handling would be just added complexity.

Also, how can you deal with ICs having different sensors inside? Sometimes these are different I2C devices in the same chip and sometimes they are the same device but with separate register sets (for example, temperature + pressure). While you could separate it into two drivers, sometimes this is not a good idea since it is better to be aware that they share a bus or that the sensor cannot actually give data from both independently.

This issue about multi type of sensor in IC has been solved. This lower half driver will multi call sensor_register for ICs having different sensors inside(the red box at image). ex: wtgahrs2 driver. we can use different sensor ops for different type of sensor.(the green box at image)

We will add ioctl cmd about setting buffer size and lowerhalf::buffer_bytes as a default value, This will implement in other patch.

As for the suggestion of directly using I2C or SPI to access registers in read function, we will consider adding a interfaces which is parallel to the present, and it will use buffer of userspace to read sensor data by i2c or spi. This will other patch.

[xiaoxiang781216]

@v01d @btashton does @Donny9 's reply address your concern? If yes, please merge this PR, so @Donny9 can work on the suggested feature.

[protobits]

Where is this wtgahrs.c file? It does not seem to be part of this PR. Maybe I'm missing something.

Regarding the sensor model itself, it is difficult for me to evaluate how well it maps to existing sensors. I think there's a risk of this being too restrictive and it is something that you will only find out when you try to look at various drivers and see if they all can work the same way or their behaviour would change.

That said, we could merge this and consider it a feature that needs more evaluation before fully embracing it (and porting all drivers to it). I wouldn't want to have two sets of drivers: those that work with this interface and a lot of others that do not. Maybe as you say, it is a matter of slowly evolving this into something that considers all scenarios.

Regarding the buffer, there are two points here. First, I think that for some drivers it is better to have the option of having no extra buffer at all. The posix read() will supply a buffer where the driver can directly write into. I don't think it makes sense for all cases to have an intermediate buffer. In that case, I would expect the intermediate buffer be skipped. Second, consider adding support for dropping old values when the queue is full. This came up recently with the ADC driver: most times you care more about last N values than first N values until buffer is full.

[Donny9]

Where is this wtgahrs.c file? It does not seem to be part of this PR. Maybe I'm missing something.

Regarding the sensor model itself, it is difficult for me to evaluate how well it maps to existing sensors. I think there's a risk of this being too restrictive and it is something that you will only find out when you try to look at various drivers and see if they all can work the same way or their behaviour would change.

That said, we could merge this and consider it a feature that needs more evaluation before fully embracing it (and porting all drivers to it). I wouldn't want to have two sets of drivers: those that work with this interface and a lot of others that do not. Maybe as you say, it is a matter of slowly evolving this into something that considers all scenarios.

Regarding the buffer, there are two points here. First, I think that for some drivers it is better to have the option of having no extra buffer at all. The posix read() will supply a buffer where the driver can directly write into. I don't think it makes sense for all cases to have an intermediate buffer. In that case, I would expect the intermediate buffer be skipped. Second, consider adding support for dropping old values when the queue is full. This came up recently with the ADC driver: most times you care more about last N values than first N values until buffer is full.

Yes, Your concerns are understandable, but it is almost impossible for me to transplant all the existing sensors to this model at present. We can only recommend us to use the sensor model, and then gradually add new functions to improve it and replace the previous disordered model.

On the two points thar you mentioned, the first one needs to add by read function directly read register content to save intermediate buffer, i will finish it as soon as possible; the seconds one for intermediate buffer, it's been taken into account in the design, we will overwire old sensor event when intermediate buffer is fulls.

Finally, you can see wthahrs2.c in this commits, url:15c5ab7.

Thanks for your advice.

[protobits]

Thanks @Donny9
Yes, of course I don't mean that you should port every driver. I meant that without looking into various drivers it is difficult to know whether this sensor model works in all cases. If we are flexible on this implementation indeed this could be merged and then improve it depending on each case.

Regarding direct read, I'm not sure if we're talking about the same thing. I wasn't suggesting that you should be able to read I2C registers directly. I was suggesting that the lower part should be able to write into the pointer that read() receives directly. The push_event() interface means that this would not be possible. The only way would be that the upper part be able to request data from the lower part (something like read_sensor(), which would receive a pointer where to read into). This could be a blocking operation, compatible with blocking read. I think it would be good to consider such simple case as an option. It could also be done via signaling (the lower part could invoke a upper half method to signal that new data is available).

[btashton]

I'm tempted to merge this with the understanding that this driver is experimental and we are not locking in the interfaces until the next release. If we did that I would expect that we work to make sure we resolve some of these questions.
If this is merged to a branch or master, I would give a go at adding support for an IMU or two over the weekend to see how that works.

I think it is important that we consider the cost of turning this in terms of ram and flash for smaller platforms (which also may be lower power) which I think is part of what @v01d is getting at as well.

Also there is the "Common Sensor Register Interface" that I think was intended to solve this same problem.
https://github.com/apache/incubator-nuttx/blob/2956b8516baf30c4099bd35c00919ae5c2cc973c/drivers/sensors/README.txt#L49

I dont think it makes a lot of sense to be supporting three different classes of sensor drivers, so hopefully we can get down to one or two.

[xiaoxiang781216]

Where is this wtgahrs.c file? It does not seem to be part of this PR. Maybe I'm missing something.

Regarding the sensor model itself, it is difficult for me to evaluate how well it maps to existing sensors. I think there's a risk of this being too restrictive and it is something that you will only find out when you try to look at various drivers and see if they all can work the same way or their behaviour would change.

That said, we could merge this and consider it a feature that needs more evaluation before fully embracing it (and porting all drivers to it). I wouldn't want to have two sets of drivers: those that work with this interface and a lot of others that do not. Maybe as you say, it is a matter of slowly evolving this into something that considers all scenarios.

Regarding the buffer, there are two points here. First, I think that for some drivers it is better to have the option of having no extra buffer at all. The posix read() will supply a buffer where the driver can directly write into. I don't think it makes sense for all cases to have an intermediate buffer. In that case, I would expect the intermediate buffer be skipped. Second, consider adding support for dropping old values when the queue is full. This came up recently with the ADC driver: most times you care more about last N values than first N values until buffer is full.

Here, it's the plan for the new patch:

1. Add read_sensor callback to sensor_lowerhalf_s
2. If lowerhalf implement this callback, upperhalf skip all intermediate buffer stuff
3. upperhalf's read will call the new callback directly in this case

On the other hand, the current implelmentation(buffer + push_event) will activate If read_sensor is NULL. So the driver writer can select one approach which is more suitable for his case, but the userspace always use the same method to access the sensor regardless wether the buffer exist or not.

[btashton]

Here, it's the plan for the new patch:

1. Add read_sensor callback to sensor_lowerhalf_s

2. If lowerhalf implement this callback, upperhalf skip all intermediate buffer stuff

3. upperhalf's read will call the new callback directly in this case

On the other hand, the current implelmentation(buffer + push_event) will activate If read_sensor is NULL. So the driver writer can select one approach which is more suitable for his case, but the userspace always use the same method to access the sensor regardless wether the buffer exist or not.

I don't think that addresses the issue. Its not if the the lower half driver implements a buffer it is if the user can provide the buffer to be used.

Perhaps we could allow the user application to register an actual FIFO? Right now I can have a temperature sensor and simply say I want to read 10 samples starting now. I do that by issuing a read of 10 bytes to the driver and providing it the buffer. This is nice and thin. Now for an IMU this is likely terrible. I want to be sampling data and processing it at the same time, this is where this interface here really shines. The question is how do we also support that give me 10 temperature samples to this buffer starting now case.

[xiaoxiang781216]

So @btashton you want the userspace provide a buffer/FIFO to driver and avoid the memory copy?

1. Sensor normally connect through the external bus(I2C, SPI, UART) and never tightly couple with SoC, so I expect the special memory buffer management like GPU/Display/Video isn't required and memory can be always allocated inside kernel.
2. We can expose the internal buffer and queue offset to userspace like what fb or alsa driver done.

[btashton]

So @btashton you want the userspace provide a buffer/FIFO to driver and avoid the memory copy?

1. Sensor normally connect through the external bus(I2C, SPI, UART) and never tightly couple with SoC, so I expect the special memory buffer management like GPU/Display/Video isn't required and memory can be always allocated inside kernel.

I think we should be careful with that assumption, I could see wanting to bring ADC under this at some point in the future. The interfaces we currently have there are not ideal either.

2. We can expose the internal buffer and queue offset to userspace like what fb or alsa driver done.

I would rather let @v01d respond, I'm probably speaking too much for him on this. But I think the point is we are moving the buffer from the application to the kernel and by doing that causing this extra copy to happen as well as complicating things like flushing the sensor buffer. IIO has the one-shot via sysfs so I suppose that kind of read would be similar to what exists today, I guess the question is can the driver buffer be optional in that case?

ADI and ST both have been involved in the IIO efforts in Linux so there certainly is buy-in for this kind of abstraction from companies who really care. I have only used it in the context of some of the ADI RF frontends.

[xiaoxiang781216]

Base on the converation, I would suggest that we first merge this PR and then:

1. @Donny9 provide the enhancement @v01d and @btashton suggest
2. @btashton and @v01d try to implement some interesting sensor driver on top of new driver model
3. And fix the desgin flaw found in the implemenation

And then announce the new sensor driver model in the next official release.

[btashton]

Base on the converation, I would suggest that we first merge this PR and then:

1. @Donny9 provide the enhancement @v01d and @btashton suggest
2. @btashton and @v01d try to implement some interesting sensor driver on top of new driver model
3. And fix the desgin flaw found in the implemenation

And then announce the new sensor driver model in the next official release.

That sounds reasonable to me. I'll let@v01d sign off.

I can do some sensor porting this weekend.

[protobits]

Ok, I agree with your approach. I just merged.

[btashton]

@Donny9 I am going to kick my sensor work until later in the week. I want to add support for the Linux i2c char device (https://www.kernel.org/doc/Documentation/i2c/dev-interface) to the simulator. That way I can plug in a FTDI 232H USB adaptor and hook up i2c/spi sensors directly to it and do all the work inside the simulator.

Adafruit has a nice little adaptor (https://www.adafruit.com/product/2264) that has the Stemma QT connector on the end which is what most of the sensor dev boards I have use.