-
Notifications
You must be signed in to change notification settings - Fork 7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sensor: sensing: unified api #64478
sensor: sensing: unified api #64478
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that we're not adding a new API, that it appears to mostly avoid copying the data around, and that there is a simple callback list involved.
I like that even algorithms are effectively using a pull then push model. As I read the code a read submission comes in from the sensor subsystem and upon completion data is broadcast to the callback list. The read for algorithms is completed once enough callbacks (on data event) occur and the algorithm runs. This feels and looks pretty nice to me.
I think many things here could use with some more code comments and documentation explaining how things are working, but to me this looks like its on the right track.
I think more broadly though that each sensing_sensor needs to connect a sensor to multiple lists of callbacks rather than a single list of callbacks. The assumption that I believe should be made here is that a sensor or algorithm may be a source of multiple data streams (e.g. the muxed FIFO issue noted in #60063) and that for each channel (accel xyz could be a channel) a set of callbacks are associated.
I could very well be wrong on that but I guess that's how I'd expect things to work in some way or another.
while (true) { | ||
struct rtio_cqe cqe; | ||
|
||
rc = rtio_cqe_copy_out(&sensing_rtio_ctx, &cqe, 1, K_FOREVER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice usage of the user space abilities here
|
||
static inline uint64_t get_us(void) | ||
{ | ||
return k_ticks_to_us_floor64(k_uptime_ticks()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ticks are likely going to represent in the most common case millisecond levels of precision. So I'm skeptical that this get_us(void) function will be able to do what we need it to.
Likely what we want is k_cycle_get_64() and converting the cycle count to microseconds. Or perhaps doing some math to get the cycles since the last tick (maybe we need this functionality) so that sub-tick timing precision without rollovers can be obtained. This would be useful for sensor drivers as well I believe as they need a way to precisely timestamp the data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* true: it's time for client consuming the data | ||
* false: client time not arrived yet, not consume the data | ||
*/ | ||
if (!sensor_test_consume_time(sensor, conn, get_us())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it would drop data if the consumer is not yet expecting it based on get_us(). I'd note that ticks are imprecise and this could fail to pass the branch in cases that we might otherwise expect. I'd also note that this is going to decimate data without filtering and this was a concern given in the architecture wg. I don't have a strong opinion on the filtering aspect, but we should definitely ensure the timing precision is good so this sort of branching occurs when we want it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we discussed this in the AWG before we created the sensor WG. We shouldn't do any filtering in the subsystem. It's better to present the client with ALL the data that's available and let them choose a decimation strategy.
subsys/sensing/dispatch.c
Outdated
conn->source->dev->name); | ||
continue; | ||
} | ||
conn->callback_list.on_data_event(conn, data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that we do not need to copy data as the callbacks are happening
{ | ||
struct hinge_angle_context *data = dev->data; | ||
|
||
if (data->sqe) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an interesting idea though I'm not sure I follow entirely the thinking. In this case submit() is expecting to set the current submission the hinge angle algorithm works on. It can do this because there's the knowledge that only the sensor dispatch thread is going to be submitting presumably. So only one read request is ever going to be around I suppose.
I'd rather see this work like everything else and push/pop from a queue. In moving away from submit starting work as it does today I have #62605. In this case submit would push to the queue, the current sqe to fulfill would be setup in a poll call in that case, and only done if in submit it is needed.
e.g.
this would look more like..
/* All request producers call submit */
static void hinge_submit(const struct device *dev, struct rtio_iodev_sqe *sqe)
{
...
rtio_mpsc_push(iodev->sq, sqe);
if (!atomic_ptr_get(data->sqe)) {
rtio_add_pollable(sqe->r, iodev);
}
}
/* Request consumer (the iodev) does its work in poll */
static void hinge_poll(struct const device *dev)
{
...
data->sqe = rtio_mpsc_pop(iodev->sq);
/* Do work here if possible */
}
return ret; | ||
} | ||
|
||
static int hinge_submit(const struct device *dev, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an infallible call and should be void hinge_submit(const struct device *dev, struct rtio_iodev_sqe *sqe);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hinge_submit
is an instance of typedef int (*sensor_submit_t)(const struct device *sensor, struct rtio_iodev_sqe *sqe)
, so it should be int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some cleanup needs to be done around this API it seems, thanks for pointing this out
Thanks for the review, on above requirement, we have a little different concept. In our view, the sensors in sensing subsystem should be a standard part instead of multi-function tools. A standard part means that it has standard input/output defined, no matter it is built by whom. A standard part can help people easily assembly them together into a sensor tree and create a functional sensing subsystem, like playing LEGO blocks. So for the case that one sensor may be a source of multiple data streams, the preference is to split it into multiple standard sensors, each to serve one type of usages. |
Thanks for your comments. I'll update it later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase, streaming APIs landed.
int64_t scaled_value; | ||
int64_t shifted_value; | ||
|
||
shifted_value = (int64_t)q << shift; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to be careful with this it caught me off guard a few times, if shift is negative this will not work as expected. you need to left shift on positive and right shift by the absolute value on negative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your reminder. This function can be removed after using sensor_read
and decoder
instead of fetch/get
ret = sensor_sample_fetch_chan(cfg->hw_dev, data->custom->chan_all); | ||
if (ret) { | ||
LOG_ERR("%s: sample fetch failed: %d", dev->name, ret); | ||
rtio_iodev_sqe_err(sqe, ret); | ||
return ret; | ||
} | ||
|
||
ret = sensor_channel_get(cfg->hw_dev, data->custom->chan_all, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be using fetch/get anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Would update later.
static struct phy_3d_sensor_data _CONCAT(data, _inst); \ | ||
static const struct phy_3d_sensor_config _CONCAT(cfg, _inst) = {\ | ||
.hw_dev = DEVICE_DT_GET( \ | ||
DT_PHANDLE(DT_DRV_INST(_inst), \ | ||
underlying_device)), \ | ||
.sensor_type = DT_PROP(DT_DRV_INST(_inst), sensor_type),\ | ||
}; \ | ||
SENSING_SENSOR_DT_DEFINE(DT_DRV_INST(_inst), \ | ||
&phy_3d_sensor_reg, &_CONCAT(ctx, _inst), \ | ||
SENSING_SENSOR_DT_INST_DEFINE(_inst, &phy_3d_sensor_reg, NULL, \ | ||
&phy_3d_sensor_init, NULL, \ | ||
&_CONCAT(data, _inst), &_CONCAT(cfg, _inst), \ | ||
POST_KERNEL, CONFIG_SENSOR_INIT_PRIORITY, \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a critical nuance, the physical sensors should for example when using the ICM42688 create:
- sensor device (
const struct device
) - 3 separate
const struct sensing_sensor_info
objects forACCEL
,GYRO
, andDIE_TEMPERATURE
.
The current model creates a separate device
for each data type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll fix it.
I would like to do the following change:
- Change the type of
sensor-type
fromint
toarray
# Copyright (c) 2023, Intel Corporation
# SPDX-License-Identifier: Apache-2.0
#
description: Sensing subsystem sensor common properties bindings.
include: sensor-device.yaml
properties:
--- sensor-type:
+++ sensor-types:
--- type: int
+++ type: array
required: true
description: sensor type id (follow HID spec definition)
- In
SENSING_SENSOR_DEFINE
, it will create multiple instances ofsensing_sensor
by enumeratingsensor-types
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a key distinction here, you need 1 sensor but multiple sensor_info nodes. Please see my sample PR at #56963
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR, and add the commit sensing: support multiple sensor types in one device
In this commit, the single bmi160 device would create two sensing_sensor instances for Accel
and Gyro
.
1ddcdfc
to
020dab8
Compare
there is an issue with llvm:
|
5dc27cd
to
b4aeea1
Compare
I'll check it, thanks. |
107926b
to
2c83510
Compare
* true: it's time for client consuming the data | ||
* false: client time not arrived yet, not consume the data | ||
*/ | ||
if (!sensor_test_consume_time(sensor, conn, get_us())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we discussed this in the AWG before we created the sensor WG. We shouldn't do any filtering in the subsystem. It's better to present the client with ALL the data that's available and let them choose a decimation strategy.
static const struct phy_3d_sensor_custom custom_accel = { | ||
.chan_all = SENSOR_CHAN_ACCEL_XYZ, | ||
.shift = SENSING_ACCEL_Q31_SHIFT, | ||
.q31_to_sensor_value = accel_q31_to_sensor_value, | ||
.sensor_value_to_q31 = accel_sensor_value_to_q31, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like we're still making the phy_3d_sensor an either/or. In the case of the BMI160 you'll need to support both accel and gyro here.
switch (cfg->sensor_type) { | ||
case SENSING_SENSOR_TYPE_MOTION_ACCELEROMETER_3D: | ||
data->custom = &custom_accel; | ||
break; | ||
case SENSING_SENSOR_TYPE_MOTION_GYROMETER_3D: | ||
data->custom = &custom_gyro; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell this is still the same as my previous comments. You're creating 1 struct device
per type. You need to have 1 struct device
per physical sensor and N struct sensor_info
objects based on the number of types that are supported.
.hw_dev = DEVICE_DT_GET( \ | ||
DT_PHANDLE(DT_DRV_INST(_inst), \ | ||
underlying_device)), \ | ||
.sensor_type = DT_PROP(DT_DRV_INST(_inst), sensor_type),\ | ||
.sensor_type = DT_PROP_BY_IDX(DT_DRV_INST(_inst), sensor_types, 0),\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just using the first type in the list.
Looks like some minor lint issues on the commits and some code lines are preventing CI from being green, please fix this as usually people won't review without a green CI pass |
922949a
to
b3dfabc
Compare
1) move variable shift behind struct sensing_sensor_value_header in struct sensing_sensor_value_q31 2) add pointer checking for sensing API 3) add context sensing_callback_list and sensing_data_event_t 4) add macro SENSING_SENSITIVITY_INDEX_ALL 5) rename zephyr,sensing-phy-3d-sensor.yaml Signed-off-by: Guangfu Hu <[email protected]>
1. use the sensor_driver_api instead of the sensing_sensor_api. 2. clean up the sensing_sensor.h after using the unified api. 3. select RTIO while CONFIG_SENSING is enabled. 4. remove the sensing_sensor_post_data and sensing_sensor_xxx_data_ready by introducing the RTIO Signed-off-by: Zhang Lixu <[email protected]>
1. set/get interval sensitivity is implmented in sensor management module. 2. add the runtime thread to handle the sensor configuration. Signed-off-by: Zhang Lixu <[email protected]>
Create the dispatch thread to send sensor data to its client by checking time. Signed-off-by: Zhang Lixu <[email protected]>
Add two helper functions as the couple functions of sensor_value_to_milli and sensor_value_to_micro. Signed-off-by: Zhang Lixu <[email protected]>
This patch is to implement the phy_3d_sensor in sensing subsystem. It's a wrapper layer of AGM sensor driver for sensing subsystem. It supports the accelerometer and gyrometer now. This has been tested with bmi160 on native_posix simulator. Signed-off-by: Zhang Lixu <[email protected]>
Add stream-mode property to indicate sensing sensor working stream mode or poll mode. Signed-off-by: Zhang Lixu <[email protected]>
Add hinge angle virtual sensor for sensing subsystem, hinge angle sensor takes both base accel and lid accel as reporter. Signed-off-by: Zhang Lixu <[email protected]>
1. add sensing_set_config/sensing_get_config in sample code 2. add hinge_angle sensor in sample code. Build and test: west build -p -b native_sim samples/subsys/sensing/simple/ -t run Signed-off-by: Zhang Lixu <[email protected]>
Many sensors have multiple functions, for example, icm42688 supports accel, gyro and temperature, and the sensor streaming api always mixes the multiple functions in one function call. So we need add a layer in sensing subsystem to dispatch the result returned from sensor streaming api for each function. I changed the sensor-type(int) to sensor-types(array) in sensing sensor device bindings, so that one device can map to multiple instances of sensing sensor. Signed-off-by: Zhang Lixu <[email protected]>
Some doc comments were making doxygen unhappy and the doc build fail. The description of the sensor connection was also updated slightly to simplify the description a bit. Signed-off-by: Tom Burdick <[email protected]>
Fixes a few small code formatting issues in sensor_mgmt.c adding brackets where needed for an if and droping a line continuation slash that failed one of the CI lints. Signed-off-by: Tom Burdick <[email protected]>
Initializers must be compile time constants (not expressions!) for clang to be happy. Clang rightfully pointed out that the callback_list member of sensing_connection was being initialized using an expression that was not compile-time constant. The macros passed along a pointer created by taking the address of a global constant and then at initialization time attempting to dereference it using the * operator. So in the end the compiler has identifier its trying to first take the address of and then later dereference in an initializer. Clang didn't appreciate this, though gcc seemed to be just fine. Actually clang 17 might work just fine with this as well, but clang 16 (the clang I tried) didn't like it at all. Instead store the pointer to the callback list rather than copying the struct. This could be done the other way and not passing a pointer through the macros perhaps. Signed-off-by: Tom Burdick <[email protected]>
b3dfabc
to
437501e
Compare
The sensing_connections can be shared between sensing_sensors under the same device. Signed-off-by: Zhang Lixu <[email protected]>
1. add function sensing_iodev_submit and defined struct sensing_submit_config 2. fix the issue of phy_3d_sensor Signed-off-by: Zhang Lixu <[email protected]>
Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to the sensing API look good to me
@@ -110,7 +115,8 @@ typedef void *sensing_sensor_handle_t; | |||
*/ | |||
typedef void (*sensing_data_event_t)( | |||
sensing_sensor_handle_t handle, | |||
const void *buf); | |||
const void *buf, | |||
void *context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs doxygen comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Submitted #68317 to fix it.
sensing_data_event_t on_data_event; | ||
void *context; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need doxygen comment
Implement the sensing subsystem.
This includes the following features:
sensor_driver_api
instead ofsensing_sensor_api
to fixDuplicates APIs
issue raised in RFC: Architecting a sensor subsystem #62223phy_3d_sensor
driver as the reference code of physical sensor.hinge_angle
driver as the reference code of virtual sensor.The sensing subsystem working process is as below.
Depends: #60063
Links