pkarashchenko commented on PR #6708: URL: https://github.com/apache/incubator-nuttx/pull/6708#issuecomment-1197220491
> > 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`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
