driver/sensors[2]: enhance and update sensor driver

[Donny9]

Summary

1. Add prefix "sensor_" for sensor device, let's sensor name is same as sensor event structure.
2. Add the file input parameter to make the sensor driver have multi-user features.
3. define sensor ioctl structure to support ioctl by rpmsg

Impact

Testing

Vela Ci

[Donny9]

This is PR is a part of #6653

[pkarashchenko]

In general this design solution is a bit odd to me. We couple the vfs layer with driver implementation layer. In general all other drivers (correct me if I'm wrong) are not built in that way and in theory driver can be used even without vfs node.
Maybe introducing some generic approach for multi-user driver is better. Like creating a list of users at lower half and passing user id of any similar to driver instead of filep.
I mean that current approach allow us to achieve the goal, but IMO destroys the driver model modularity.

[Donny9]

In general this design solution is a bit odd to me. We couple the vfs layer with driver implementation layer. In general all other drivers (correct me if I'm wrong) are not built in that way and in theory driver can be used even without vfs node.
Maybe introducing some generic approach for multi-user driver is better. Like creating a list of users at lower half and passing user id of any similar to driver instead of filep.
I mean that current approach allow us to achieve the goal, but IMO destroys the driver model modularity.

Yes, this design will be a bit different from other drivers and designed for rpmsg sensor in PR:#6653

In the rpmsg sensor, we need to use file_xxx to directly access the device node, and use o_flags with O_REMOTE to indicate that it is a remote access to prevent recursion when calling the sensor_ops method.

• "we need to use file_xxx to directly access the device node"
  Because remote and local user need to support down-sample logic，so they must obtain sensor event by interface sensor_read, otherwise, this code will be duplicate.
  Example:
  When there is data locally, if there is a remote subscription, the local remote stub will call the file_read function to read and send the sampled data to the remote sensor_rpmsg_ept_cb, it will call file_write to write data to queue buffer, let remote users get it.
• “prevent recursion”
  When the local user sets the sampling rate of a remote sensor, the request will be sent to the remote first. The remote needs to determine whether the request comes from another core. If it is from another core and supports setting the sampling rate, it will set and return the result, otherwise it will return unsupported Instead of continuing to send to the remote for set.

[pkarashchenko]

I will take a closer look today

[Donny9]

I will take a closer look today

Thank you.

[pkarashchenko]

In general this design solution is a bit odd to me. We couple the vfs layer with driver implementation layer. In general all other drivers (correct me if I'm wrong) are not built in that way and in theory driver can be used even without vfs node.
Maybe introducing some generic approach for multi-user driver is better. Like creating a list of users at lower half and passing user id of any similar to driver instead of filep.
I mean that current approach allow us to achieve the goal, but IMO destroys the driver model modularity.

Yes, this design will be a bit different from other drivers and designed for rpmsg sensor in PR:#6653

In the rpmsg sensor, we need to use file_xxx to directly access the device node, and use o_flags with O_REMOTE to indicate that it is a remote access to prevent recursion when calling the sensor_ops method.

• "we need to use file_xxx to directly access the device node"
  Because remote and local user need to support down-sample logic，so they must obtain sensor event by interface sensor_read, otherwise, this code will be duplicate.
  Example:
  When there is data locally, if there is a remote subscription, the local remote stub will call the file_read function to read and send the sampled data to the remote sensor_rpmsg_ept_cb, it will call file_write to write data to queue buffer, let remote users get it.
• “prevent recursion”
  When the local user sets the sampling rate of a remote sensor, the request will be sent to the remote first. The remote needs to determine whether the request comes from another core. If it is from another core and supports setting the sampling rate, it will set and return the result, otherwise it will return unsupported Instead of continuing to send to the remote for set.

• "we need to use file_xxx to directly access the device node" -- I just took a look into the sensor interface and we do not have many alternatives to that. The one thing that I do not fully understand is why do we not replace struct sensor_lowerhalf_s *lower with FAR struct file *filep, but add additional parameter? I mean that passing struct sensor_lowerhalf_s *lower together with FAR struct file *filep seems to be a bit redundant as lower is filep->f_inode->i_private->lower. Is this just to minimise code diff?
  Maybe the alternative may be equipping struct sensor_ops_s with int flags parameter. I mean that struct sensor_ops_s does not fully match "file" paradigm. Even newly added methods open/close are more like attach/detach or ref/unref (please correct me if I'm wrong in my understanding).
  My main concern is that we are losing encapsulation with this change. I mean that till now the struct sensor_ops_s is describing interface to struct sensor_lowerhalf_s, but with this changes it is not true anymore. If we decide it to be file like interface, then let's wipe out struct sensor_lowerhalf_s from the interface.
• “prevent recursion” -- I think since nxmutex interface is used so nxrmutex_is_hold can be alternative for recursion prevention like in syslog_dev_takesem from drivers/syslog/syslog_device.c.

[Donny9]

• "we need to use file_xxx to directly access the device node" -- I just took a look into the sensor interface and we do not have many alternatives to that. The one thing that I do not fully understand is why do we not replace struct sensor_lowerhalf_s *lower with FAR struct file *filep, but add additional parameter? I mean that passing struct sensor_lowerhalf_s *lower together with FAR struct file *filep seems to be a bit redundant as lower is filep->f_inode->i_private->lower. Is this just to minimise code diff?
  Maybe the alternative may be equipping struct sensor_ops_s with int flags parameter. I mean that struct sensor_ops_s does not fully match "file" paradigm. Even newly added methods open/close are more like attach/detach or ref/unref (please correct me if I'm wrong in my understanding).
  My main concern is that we are losing encapsulation with this change. I mean that till now the struct sensor_ops_s is describing interface to struct sensor_lowerhalf_s, but with this changes it is not true anymore. If we decide it to be file like interface, then let's wipe out struct sensor_lowerhalf_s from the interface.
• “prevent recursion” -- I think since nxmutex interface is used so nxrmutex_is_hold can be alternative for recursion prevention like in syslog_dev_takesem from drivers/syslog/syslog_device.c.

1. "I mean that passing struct sensor_lowerhalf_s *lower together with FAR struct file *filep seems to be a bit redundant as lower is filep->f_inode->i_private->lower. "

• Struct file represents the context of each user accessing the file. As with the sensor driver, if a driver supports simultaneous access by multiple users, it must have its own context to distinguish different user behaviors.

• Each sensor driver corresponds to an upper half and a lower half, which are shared by all users.

• Before this PR, the user's identification was distinguished by file in the upper half, but if we also need to distinguish different users in the lower half, what should we do? The rpmsg sensor is added to the sensor driver as a general lower half to transmit local sensor data to other cores or respond to data requests from other cores. It needs to distinguish between different users, such as whether the user is local or remote.

• The process of sending local data to remote and remote requesting data is as follows

2. “prevent recursion” -- I think since nxmutex interface is used so nxrmutex_is_hold can be alternative for recursion prevention like in syslog_dev_takesem from drivers/syslog/syslog_device.c.

• Yes, I used recursive locks and poured the locks in the upper half into the lower half to prevent recursion, but this is not enough.
• For example there are two cores, A and B. B subscribes to the topic on A. When A advertises the topic, it will broadcast the message to all cores subscribed to the topic, including B. A's broadcast action is usually to call orb_advertise, internally call sensor_open, and then create the context of sensor_user_s. When B's sensor_rpmsg_ept_cb receives the broadcast message, it needs to create an A's agent on B to receive A's topic data, which is also created through file_open. At this time, it needs to be opened using O_REMOTE to indicate that this is a remote If the user does not do this, the advertiser request will be sent to other cores again, resulting in confusion.
  

[pkarashchenko]

Thank you for clarification. Let's pass filep as a second parameter at least. I mean that sensor lower seems to be "this" for ops and user is a supplementary parameter.

[Donny9]

Thank you for clarification. Let's pass filep as a second parameter at least. I mean that sensor lower seems to be "this" for ops and user is a supplementary parameter.

Thank you for clarification. Let's pass filep as a second parameter at least. I mean that sensor lower seems to be "this" for ops and user is a supplementary parameter.

Done!

[exteriorform]

why did the additional parameter struct file *filep was added but it was not used in function at all?

[xiaoxiang781216]

why did the additional parameter struct file *filep was added but it was not used in function at all?

it's used in sensor_rpmsg_open