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 2020/11/04 13:48:13 UTC
[GitHub] [incubator-nuttx] Donny9 opened a new pull request #2215: add new feature about sensor driver model
Donny9 opened a new pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215
## Summary
driver/sensor: add new feature about sensor driver
1.support for multi-user access
2.support special cmd to control sensor
3.support userspace to set size of intermediate buffer by ioctl: SNOIC_SET_BUFFER_SIZE
4.add fetch api to use userspace buffer rather than intermediate buffer of upperhalf driver
5.fetch api support block and non-block ways.
It will directly access sensor register by i2c/spi when open mode is non-block,
It will wait until sensor data ready and call fetch api.
## Impact
disable intermediate buffer by lowerhalf driver use fetch api.
## Testing
daily test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725429026
I'm looking at the changes. I wanted to ask, any reason why you are defining your own queue structure instead of using definitions in queue.h?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518232810
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
Review comment:
```suggestion
* Fetch sensor register data by this function. It will use buffer
```
We should avoid writing the documentation in the first person.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725507199
I grepped the sources and found various mentions to circular buffers being used, but they all appear to be ad-hoc implementations, so a separate ring buffer structure could be useful to have.
I agree to making the memory related changes later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r519264908
##########
File path: include/nuttx/sensors/ioctl.h
##########
@@ -273,4 +273,14 @@
#define SNIOC_GET_NEVENTBUF _SNIOC(0x0070)
+/* Command: SNIOC_SET_BUFFER_SIZE
+ * Description: Use to set size of intermediate circual buffer in upper half driver.
Review comment:
done
##########
File path: include/nuttx/sensors/ioctl.h
##########
@@ -273,4 +273,14 @@
#define SNIOC_GET_NEVENTBUF _SNIOC(0x0070)
+/* Command: SNIOC_SET_BUFFER_SIZE
+ * Description: Use to set size of intermediate circual buffer in upper half driver.
+ * Argument: This is the size of buffer pointer.
+ * Note: The application layer can set size of intermediate circual buffer
Review comment:
done
##########
File path: include/nuttx/sensors/ioctl.h
##########
@@ -273,4 +273,14 @@
#define SNIOC_GET_NEVENTBUF _SNIOC(0x0070)
+/* Command: SNIOC_SET_BUFFER_SIZE
+ * Description: Use to set size of intermediate circual buffer in upper half driver.
+ * Argument: This is the size of buffer pointer.
+ * Note: The application layer can set size of intermediate circual buffer
+ * by this ioctl cmd. The size is in bytes, it should be a multiple of
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
Donny9 commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-722769756
> Just pointing out some typos and improvement opportunities in the documentation.
Thank you very much, I'll study this documentation guide and modify them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725442229
And if I understand correctly, the internal buffer is always allocated to the sensor structure size, even when only fetch() will be used, right? It would be better to only do so if fetch() will not be used, otherwise it is just wasted memory.
Also, would it be possible (and would it make sense) to let the driver define its own structure? I'm thinking about custom sensors types which may not map exactly to any of the structs defined. Maybe this could be done by letting the lower part define the struct size and dev prefix string.
That is all the comments I have. I feel I would have to try to use this interface to really understand it and see if there's any limitation, but it looks good so far.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r519264948
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
Review comment:
done
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
+ * We recommend lowerhalf driver writer to use this function for slower
+ * ODR(output data rate) of sensor because this way saves space and simple.
+ *
+ * If fectch isn't NULL, upper half driver will disable intermediate
Review comment:
done
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
Review comment:
done
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
+ * We recommend lowerhalf driver writer to use this function for slower
+ * ODR(output data rate) of sensor because this way saves space and simple.
Review comment:
done
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
+ * We recommend lowerhalf driver writer to use this function for slower
+ * ODR(output data rate) of sensor because this way saves space and simple.
+ *
+ * If fectch isn't NULL, upper half driver will disable intermediate
+ * buffer and userspace can't set buffer size by ioctl.
+ *
+ * You can call this function to read sensor register data by i2c/spi bus
Review comment:
done
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -601,12 +673,13 @@ int sensor_register(FAR struct sensor_lowerhalf_s *dev);
* upper half driver.
*
* Input Parameters:
- * dev - A pointer to an instance of lower half sensor driver. This
- * instance is bound to the sensor driver and must persists as long
- * as the driver persists.
+ * dev - A pointer to an instance of lower half sensor driver. This
+ * instance is bound to the sensor driver and must persists as long
Review comment:
done
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -581,17 +651,19 @@ extern "C"
* numbers. eg: accel0, accel1
*
* Input Parameters:
- * dev - A pointer to an instance of lower half sensor driver. This
- * instance is bound to the sensor driver and must persists as long
- * as the driver persists.
+ * dev - A pointer to an instance of lower half sensor driver. This
+ * instance is bound to the sensor driver and must persists as long
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725461036
> Also, shouldn't the buffer be released on close() on the last reference? Right now it requires the unregistration of the sensor
What about this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518243481
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -581,17 +651,19 @@ extern "C"
* numbers. eg: accel0, accel1
*
* Input Parameters:
- * dev - A pointer to an instance of lower half sensor driver. This
- * instance is bound to the sensor driver and must persists as long
- * as the driver persists.
+ * dev - A pointer to an instance of lower half sensor driver. This
+ * instance is bound to the sensor driver and must persists as long
Review comment:
```suggestion
* instance is bound to the sensor driver and must persist as long
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
Donny9 commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725451863
>
>
> > I'm looking at the changes. I wanted to ask, any reason why you are defining your own queue structure instead of using definitions in queue.h?
>
> The circualr buffer and queue are different for their behavior. We need to overwrite old data when buffer is full, and it's better to operate buffer directly rather than queue node.
I will add mm/circ_buf for circular buffer management on other patch later, and driver/sensor and driver/rc will call these api about circ_buf_xxx to use circular buffer.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518227732
##########
File path: include/nuttx/sensors/ioctl.h
##########
@@ -273,4 +273,14 @@
#define SNIOC_GET_NEVENTBUF _SNIOC(0x0070)
+/* Command: SNIOC_SET_BUFFER_SIZE
+ * Description: Use to set size of intermediate circual buffer in upper half driver.
Review comment:
```suggestion
* Description: Use to set size of intermediate circular buffer in upper half driver.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518239315
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
Review comment:
What does sensor_read refers to?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518242180
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
+ * We recommend lowerhalf driver writer to use this function for slower
+ * ODR(output data rate) of sensor because this way saves space and simple.
+ *
+ * If fectch isn't NULL, upper half driver will disable intermediate
+ * buffer and userspace can't set buffer size by ioctl.
+ *
+ * You can call this function to read sensor register data by i2c/spi bus
Review comment:
```suggestion
* You can call this function to read sensor register data by I2C/SPI bus
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518228599
##########
File path: include/nuttx/sensors/ioctl.h
##########
@@ -273,4 +273,14 @@
#define SNIOC_GET_NEVENTBUF _SNIOC(0x0070)
+/* Command: SNIOC_SET_BUFFER_SIZE
+ * Description: Use to set size of intermediate circual buffer in upper half driver.
+ * Argument: This is the size of buffer pointer.
+ * Note: The application layer can set size of intermediate circual buffer
+ * by this ioctl cmd. The size is in bytes, it should be a multiple of
Review comment:
```suggestion
* by this ioctl command. The size is in bytes, it should be a multiple of
```
I think it is best to avoid abbreviations in the documentation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
Donny9 commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725456704
>
>
> And if I understand correctly, the internal buffer is always allocated to the sensor structure size, even when only fetch() will be used, right? It would be better to only do so if fetch() will not be used, otherwise it is just wasted memory.
>
> Also, would it be possible (and would it make sense) to let the driver define its own structure? I'm thinking about custom sensors types which may not map exactly to any of the structs defined. Maybe this could be done by letting the lower part define the struct size and dev prefix string.
>
> That is all the comments I have. I feel I would have to try to use this interface to really understand it and see if there's any limitation, but it looks good so far.
1. When fetch isn't NULL, the intermediate buffer is disabled on sensor_register to save ram. Done on this patch.
2. I think we should move towards unification. If there is no conforming type or structure, we should add them and make them as versatile as possible. It's especially good for application developers.
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725434848
Also, shouldn't the buffer be released on close() on the last reference? Right now it requires the unregistration of the sensor
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518232810
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
Review comment:
```suggestion
* Fetch sensor register data by this function. It will use buffer
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
Donny9 commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-721745429
@v01d @btashton hello, I submitted some patches about sensor model features and optimization. Please review, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518240681
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
+ * We recommend lowerhalf driver writer to use this function for slower
+ * ODR(output data rate) of sensor because this way saves space and simple.
Review comment:
```suggestion
* It is recommended that the lowerhalf driver writer use this function for slower
* sensor ODR (output data rate) because this way saves space and it's simpler.
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-721744158
@v01d and @btashton this patch series address your suggestion here: https://github.com/apache/incubator-nuttx/pull/2039.
Please take a look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518243373
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -601,12 +673,13 @@ int sensor_register(FAR struct sensor_lowerhalf_s *dev);
* upper half driver.
*
* Input Parameters:
- * dev - A pointer to an instance of lower half sensor driver. This
- * instance is bound to the sensor driver and must persists as long
- * as the driver persists.
+ * dev - A pointer to an instance of lower half sensor driver. This
+ * instance is bound to the sensor driver and must persists as long
Review comment:
```suggestion
* instance is bound to the sensor driver and must persist as long
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518228067
##########
File path: include/nuttx/sensors/ioctl.h
##########
@@ -273,4 +273,14 @@
#define SNIOC_GET_NEVENTBUF _SNIOC(0x0070)
+/* Command: SNIOC_SET_BUFFER_SIZE
+ * Description: Use to set size of intermediate circual buffer in upper half driver.
+ * Argument: This is the size of buffer pointer.
+ * Note: The application layer can set size of intermediate circual buffer
Review comment:
```suggestion
* Note: The application layer can set size of intermediate circular buffer
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518241136
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
+ * We recommend lowerhalf driver writer to use this function for slower
+ * ODR(output data rate) of sensor because this way saves space and simple.
+ *
+ * If fectch isn't NULL, upper half driver will disable intermediate
+ * buffer and userspace can't set buffer size by ioctl.
+ *
+ * You can call this function to read sensor register data by i2c/spi bus
Review comment:
```suggestion
* Call this function to read sensor register data by I2C/SPI bus
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Donny9 commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
Donny9 commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725451007
>
>
> I'm looking at the changes. I wanted to ask, any reason why you are defining your own queue structure instead of using definitions in queue.h?
The circualr buffer and queue are different for their behavior. We need to overwrite old data when buffer is full, and it's better to operate buffer directly rather than queue node.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725497073
> > ```
> > 1. When fetch isn't NULL, the intermediate buffer is disabled on sensor_register to save ram. Done on this patch.
> > ```
>
> I see. But the call to the buffer creation function is still done and if I understand correctly, due to the rounding up, one struct will be allocated in the buffer still.
It isn't an issue more with new mm/circ_buf, so we can temporarily ignore it here.
>
> > ```
> > 2. I think we should move towards unification. If there is no conforming type or structure, we should add them and make them as versatile as possible. It's especially good for application developers.
> > ```
>
> I agree, but I'm thinking in the lines of something that wouldn't necessarily be ported upstream, leaving downstream user the possibility of defining a particular driver with specific needs. But we can go back to this later on.
>
Ok, we can add a special register api later.
> > Thank you.
> > > > I'm looking at the changes. I wanted to ask, any reason why you are defining your own queue structure instead of using definitions in queue.h?
> > >
> > >
> > > The circualr buffer and queue are different for their behavior. We need to overwrite old data when buffer is full, and it's better to operate buffer directly rather than queue node.
> >
> >
> > I will add mm/circ_buf for circular buffer management on other patch later, and driver/sensor and driver/rc will call these api about circ_buf_xxx to use circular buffer.
>
> Are you sure there's no circular buffer structure defined anywhere else in NuttX?
We can't find one, could you point to a general and standalone implementation?
> > Also, shouldn't the buffer be released on close() on the last reference? Right now it requires the unregistration of the sensor
>
> What about this?
Since the upcomming patch will replace the buffer implemenation to a common one, how about move this change to that PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518232810
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
Review comment:
```suggestion
* Fetch sensor register data by this function. It will use buffer
```
I think it is best if the documentation is not written in the first person.
Some references:
https://developers.google.com/style/person
https://docs.microsoft.com/en-us/style-guide/grammar/person
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ghn-certi commented on a change in pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
ghn-certi commented on a change in pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#discussion_r518238635
##########
File path: include/nuttx/sensors/sensor.h
##########
@@ -495,6 +495,58 @@ struct sensor_ops_s
CODE int (*batch)(FAR struct sensor_lowerhalf_s *lower,
FAR unsigned int *latency_us);
+
+ /**************************************************************************
+ * Name: fetch
+ *
+ * We can fetch sensor register data by this function. It will use buffer
+ * of sensor_read provided and disables intermediate buffer of upper half.
+ * We recommend lowerhalf driver writer to use this function for slower
+ * ODR(output data rate) of sensor because this way saves space and simple.
+ *
+ * If fectch isn't NULL, upper half driver will disable intermediate
Review comment:
```suggestion
* If fetch isn't NULL, upper half driver will disable intermediate
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725460869
> > > I'm looking at the changes. I wanted to ask, any reason why you are defining your own queue structure instead of using definitions in queue.h?
> >
> >
> > The circualr buffer and queue are different for their behavior. We need to overwrite old data when buffer is full, and it's better to operate buffer directly rather than queue node.
>
> I will add mm/circ_buf for circular buffer management on other patch later, and driver/sensor and driver/rc will call these api about circ_buf_xxx to use circular buffer.
Are you sure there's no circular buffer structure defined anywhere else in NuttX?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
v01d commented on pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215#issuecomment-725460270
> 1. When fetch isn't NULL, the intermediate buffer is disabled on sensor_register to save ram. Done on this patch.
I see. But the call to the buffer creation function is still done and if I understand correctly, due to the rounding up, one struct will be allocated in the buffer still.
>
> 2. I think we should move towards unification. If there is no conforming type or structure, we should add them and make them as versatile as possible. It's especially good for application developers.
>
I agree, but I'm thinking in the lines of something that wouldn't necessarily be ported upstream, leaving downstream user the possibility of defining a particular driver with specific needs. But we can go back to this later on.
>
> 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #2215: add new feature about sensor driver model
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2215:
URL: https://github.com/apache/incubator-nuttx/pull/2215
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org