You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/07/26 02:52:05 UTC
[GitHub] [incubator-nuttx] Donny9 opened a new pull request, #6708: driver/sensors[2]: enhance and update sensor driver
Donny9 opened a new pull request, #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708
## 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
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#issuecomment-1196418035
> I will take a closer look today
Thank you.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#issuecomment-1199042919
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.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#issuecomment-1195299110
> 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:https://github.com/apache/incubator-nuttx/pull/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.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #6708:
URL: https://github.com/apache/nuttx/pull/6708#issuecomment-1455082479
> 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
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#issuecomment-1197765549
> * "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.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#issuecomment-1199187014
> 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!
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#discussion_r929705036
##########
include/nuttx/sensors/sensor.h:
##########
@@ -899,6 +958,14 @@ struct sensor_reginfo_s
};
#endif
+/* This structure describes the context custom ioctl for device */
+
+struct sensor_ioctl_s
+{
+ size_t len; /* The length of argument of ioctl */
+ char data[0]; /* The argument buf of ioctl */
Review Comment:
This is C89 incompatible.
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#issuecomment-1196400998
I will take a closer look today
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
Donny9 commented on PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#issuecomment-1194933138
This is PR is a part of https://github.com/apache/incubator-nuttx/pull/6653
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on a diff in pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by GitBox <gi...@apache.org>.
Donny9 commented on code in PR #6708:
URL: https://github.com/apache/incubator-nuttx/pull/6708#discussion_r931045612
##########
include/nuttx/sensors/sensor.h:
##########
@@ -899,6 +958,14 @@ struct sensor_reginfo_s
};
#endif
+/* This structure describes the context custom ioctl for device */
+
+struct sensor_ioctl_s
+{
+ size_t len; /* The length of argument of ioctl */
+ char data[0]; /* The argument buf of ioctl */
Review Comment:
Done!
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] exteriorform commented on pull request #6708: driver/sensors[2]: enhance and update sensor driver
Posted by "exteriorform (via GitHub)" <gi...@apache.org>.
exteriorform commented on PR #6708:
URL: https://github.com/apache/nuttx/pull/6708#issuecomment-1455056107
why did the additional parameter struct file *filep was added but it was not used in function at all?
--
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org