sesnor:optimize bmi160 sensor adapt to uorb.

[Otpvondoiats]

Summary

Add bmi160 to support uorb frame,retain the original character device capability.

Impact

no effect.

Testing

ubuntu 20.04/SIM
bus: I2C
pass

[raiden00pl]

This is a breaking change for applications that use bmi160 (the same for #10080) as a character device. We should not force users to use uorb framework, at least not without wider community approval. At this point the best option would be to support uorb and char dev interfaces and control them with Kconfig.

[Otpvondoiats]

This is a breaking change for applications that use bmi160 (the same for #10080) as a character device. We should not force users to use uorb framework, at least not without wider community approval. At this point the best option would be to support uorb and char dev interfaces and control them with Kconfig.

I add configuration items and submit

[acassis]

@Otpvondoiats and @raiden00pl I think it is a good discussion, if we could support the sensors as char device and then add an option to them be compatible with uorb, it should be ideal.

I know that @patacongo created a way to let a driver to incorporate another one, but I could find it. I think nobody used it until now, but it is some "hidden feature" of NuttX. I think he did it after creating the driver that convert char device to block devices.

[patacongo]

I know that @patacongo created a way to let a driver to incorporate another one, but I could find it. I think nobody used it until now, but it is some "hidden feature" of NuttX. I think he did it after creating the driver that convert char device to block devices.

Within a driver, you can open and contain another driver using file_open() and access the other driver with file_read(), file_write()(), file_ioctl(), etc. This avoids using file descriptors.

Search for file_open under drivers/. There are a few examples.

[raiden00pl]

I agree that simultaneous support for sensors as char device and with uorb would be ideal.

[acassis]

@Otpvondoiats @xiaoxiang781216 may I change this PR and the other bmp180 to Draft until we find a way to support sensor driver and char device and include a layer over it to support uorb ?

[xiaoxiang781216]

sure

[Otpvondoiats]

@raiden00pl @acassis @patacongo I've submitted a new version with support for both character devices and Uorb communication. Try not to modify the original character device code, and keep the original code appearance. BMI160 itself supports SPI and I2C, and there are macro distinctions in it. With the addition of character devices and UORB distinctions, the code will be messy. #10080

[raiden00pl]

@Otpvondoiats Have you tried implementing the uorb sensor as a wrapper on character device sensor as it was mentioned before ? (based on file_read(), file_write()(), file_ioctl(), etc.). I'm curious if this approach is likely to work. I initially did something like this for lsm9ds1 but I haven't had a chance to test it on HW yet.
With this approach, we could avoid duplication a lot of code (ideally all sensors should support two sensor interfaces).

[xiaoxiang781216]

@Otpvondoiats Have you tried implementing the uorb sensor as a wrapper on character device sensor as it was mentioned before ? (based on file_read(), file_write()(), file_ioctl(), etc.). I'm curious if this approach is likely to work.

it will bloat the code size since file operation is implemented twice, and waste some RAM.

I initially did something like this for lsm9ds1 but I haven't had a chance to test it on HW yet. With this approach, we could avoid duplication a lot of code (ideally all sensors should support two sensor interfaces).

The old interface couple with the hardware directly, which make the userspace write a general sensor program become an impossible task. For example, lis2dh, lis3dh, lis3dsh and mpu60x0 report the different measurement result.

So, it isn't good to write the new senor driver with the old driver model(actually, there is no common upper half at all).

[acassis]

Nice work @Otpvondoiats ! But I think the IOCTLs and data rate control should be in the main driver instead of only existing only in the _uorb.

So the user application even when not using uorb will be able to use IOCTLs to control many features, like it is done in some char driver sensors already.

[xiaoxiang781216]

Nice work @Otpvondoiats ! But I think the IOCTLs and data rate control should be in the main driver instead of only existing only in the _uorb.

So the user application even when not using uorb will be able to use IOCTLs to control many features, like it is done in some char driver sensors already.

@acassis as I mention before, the old sensor driver doesn't follow nuttx driver model, e.g.:

1. Separate upperhalf and lowerhalf
2. Define the common IOCTL and struct

So, we don't plan to do any enhancement to the old drivers which demonstrate the bad practice, but continuing to improve the new driver model to encourage people switch to it.

[raiden00pl]

The old interface couple with the hardware directly, which make the userspace write a general sensor program become an impossible task. For example, lis2dh, lis3dh, lis3dsh and mpu60x0 report the different measurement result.

You might as well standardize the character driver interface for sensors without forcing the user to use uorb. So that's not an argument.

So, it isn't good to write the new senor driver with the old driver model(actually, there is no common upper half at all).

Why not ? Will uorb be good for all possible use cases?
What are the differences in resources used between uorb and the old approach?
Will it work for applications where we need a high frequency stream of sensor data (let's say IMU for gimbal controller)?
What about small systems? For example, the implementation of an intelligent sensor for industrial applications, where we only need to read sensor data and send it over industrial protocol.
I don't think we have enough information at the moment.

Separate upperhalf and lowerhalf

Since when separate upperhalf and lowerhalf is nuttx driver model ? It seems to me that most drivers don't follow this approach and use bus interface (I2C/SPI) directly.

Define the common IOCTL and struct

Totally agree here. A unified interface is something we must strive for. But uorb interface is only one way to achieve this.

The old approach seems to use less resources (memory, CPU and hence power consumption) so there are situations where uorb just doesn't make sense.
There are a lot of applications where uorb seems redundant and there is no reason to force the user to use it.

[xiaoxiang781216]

The old interface couple with the hardware directly, which make the userspace write a general sensor program become an impossible task. For example, lis2dh, lis3dh, lis3dsh and mpu60x0 report the different measurement result.

You might as well standardize the character driver interface for sensors without forcing the user to use uorb. So that's not an argument.

The fact is that all sensors expose the different interface util uorb try to unify the interface across all possible sensors. It welcome other people provide a new solution to unify the interface, but it's

So, it isn't good to write the new senor driver with the old driver model(actually, there is no common upper half at all).

Why not ? Will uorb be good for all possible use cases? What are the differences in resources used between uorb and the old approach?

I am not sure whether uorb is good for all possible cases, but it's always bad that each driver exposes the different IOCTL to userspace just like lis2dh, lis3dh, lis3dsh and mpu60x0.

Will it work for applications where we need a high frequency stream of sensor data (let's say IMU for gimbal controller)? What about small systems?

It's fine with sample rate around 1KHz.

For example, the implementation of an intelligent sensor for industrial applications, where we only need to read sensor data and send it over industrial protocol. I don't think we have enough information at the moment.

Separate upperhalf and lowerhalf

Since when separate upperhalf and lowerhalf is nuttx driver model ?

Most driver which expose file ioctl to userspace follow upperhalf/lowerhalf model.

It seems to me that most drivers don't follow this approach and use bus interface (I2C/SPI) directly.

could you give more examples except sensor? at least, analog, audio, crypto, efuse, input, ioexpander, lcd, leds, math, mmcsd, motor, mtd, net, battery, rc, rptun, serial, timer, video follow the

Define the common IOCTL and struct

Totally agree here. A unified interface is something we must strive for. But uorb interface is only one way to achieve this.

Yes, but before another interface similar interface exist,

The old approach seems to use less resources (memory, CPU and hence power consumption) so there are situations where uorb just doesn't make sense. There are a lot of applications where uorb seems redundant and there is no reason to force the user to use it.

Nobody forces user to use uorb, both interfaces keep in this patch. But we don't have enough resource to improve the raw driver interface which we don't use, so it's better to decouple both implementations.

[acassis]

@xiaoxiang781216 I completely agree that uorb helped to force sensors to follow the common standard for IOCTLs, but this could have applied to character drivers as well, if someone had defined the standard to follow.

If we create a thin layer that uorb could use over char device sensor drivers then users could use the sensors with or without uorb. Maybe a single sensor_uorb.c could be used to couple to any sensor driver.

[raiden00pl]

It's fine with sample rate around 1KHz.

Good to know. Could you reveal the CPU freq? I guess uorb is not a problem for powerful processors, but with limited resources character device seems more reasonable.

could you give more examples except sensor? at least, analog, audio, crypto, efuse, input, ioexpander, lcd, leds, math, mmcsd, motor, mtd, net, battery, rc, rptun, serial, timer, video follow the

You can just grep -R file_operations drivers/ and filter out all upper-half logic implementation. Most of it is sensors, but quite a few drivers from what you listed are there as well. To be honest, I didn't count which approach is more popular, but it doesn't matter much. I want to show that the division into upper/lower is not the norm for I2C/SPI based drivers.

Nobody forces user to use uorb, both interfaces keep in this patch. But we don't have enough resource to improve the raw driver interface which we don't use, so it's better to decouple both implementations.

I completely understand that you aren't interested in developing the old approach and that's ok. But from the initial look of this PR and discussion here, I got the impression that you'd want to get rid of the old sensors approach. So I'm hoping that's not the case, at least without much open discussion.

Two separate sensor implementations will duplicate a lot of code. If you don't want to separate some common logic between implementations for the sensor, you can at least move the register definitions to a common private header for bmi160.c and bmi160_uorb.c

[patacongo]

@xiaoxiang781216 I completely agree that uorb helped to force sensors to follow the common standard for IOCTLs, but this could have applied to character drivers as well, if someone had defined the standard to follow.

Bob Feretich did develop something he called the Sensor Cluster Driver which was a frameworkd for sensor character drivers. It did not see much usage bit it is fully in place and implemented for a few sensor drivers. There is a general description in drivers/sensors/README.txt and include/nuttx/sensors/cluster_driver.h. Perhaps some or all of that could be reused?

Ref: https://github.com/apache/nuttx/blob/master/drivers/sensors/README.txt#L217

[xiaoxiang781216]

@xiaoxiang781216 I completely agree that uorb helped to force sensors to follow the common standard for IOCTLs, but this could have applied to character drivers as well, if someone had defined the standard to follow.

Bob Feretich did develop something he called the Sensor Cluster Driver which was a frameworkd for sensor character drivers. It did not see much usage bit it is fully in place and implemented for a few sensor drivers. There is a general description in drivers/sensors/README.txt and include/nuttx/sensors/cluster_driver.h. Perhaps some or all of that could be reused?

Ref: https://github.com/apache/nuttx/blob/master/drivers/sensors/README.txt#L217

we evaluate this interface before, but it also doesn't define any standard IOCTL and struct(e.g. accel, gryo...), so the different accel vendor provide the different IOCTL to expose the accel data to userspace.

[patacongo]

we evaluate this interface before, but it also doesn't define any standard IOCTL and struct(e.g. accel, gryo...), so the different accel vendor provide the different IOCTL to expose the accel data to userspace.

Then it probably should be removed at some point. It is not supported by many drivers and would not be on the sensor architecture roadmap.

[xiaoxiang781216]

@raiden00pl could you review the new change?