Skip to content
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

sensing: implement the phy_3d_sensor #60331

Closed
wants to merge 11 commits into from

Conversation

lixuzha
Copy link
Collaborator

@lixuzha lixuzha commented Jul 13, 2023

sensing: implement the phy_3d_sensor
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 lsm6dso and lis2dw12 on hardware and bmi160 on
native_posix simulator. It can work fine while sensor is either in polling
mode or in data ready trigger mode. And set_sensitivity function is just
implemented following api definition and not tested yet.

Tested with:
$ west build -p -b native_posix samples/subsys/sensing/simple/ -t run

drivers: sensor: add helper functions to convert milli/micro to sensor_value

Depends on: #59469

1) modify reporter type from int to const sensing_sensor_handle_t in
sensing_sensor_process_t callback
2) move variable shift behind struct sensing_sensor_value_header in
struct sensing_sensor_value_q31
3) add pointer checking for sensing API

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
Add hinge angle virtual sensor for sensing subsystem, hinge angle sensor
takes both base accel and lid accel as reporter

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
set/get interval sensitivity is implmented in sensor management module,
and also add related test case

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
create runtime thread and wait event semphore, meanwhile, trigger event
semphore in set interval/sensitivity API to notify runtime thread to
arbitrate interval/sensitivity

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
implement hinge angle sensor init and set interval function

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
add runtime module, implement loop sensors in runtime module

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
1) enhance sensor need execute function, and data ready and sensor has
new data checking
2) implement sensor process data function, included both physical sensor
and virtual sensor data processing
3) implement sensor send dato to client, including time and sensitivity
checking and update sensor consume time

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
implement sleep time calculation in loop sensors

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
1) enhance sensor data process, check time, sensitivity condition
2) send data to its client
3) create dispatch thread to process sensor data from runtime module

Signed-off-by: Guangfu Hu <guangfu.hu@intel.com>
Add two helper functions as the couple functions of sensor_value_to_milli
and sensor_value_to_micro.

Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
ctx = sensing_sensor_get_ctx_data(dev);

if (!ctx->data_ready_enabled) {
ret = sensor_sample_fetch(ctx->hw_dev);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can/will likely cause a lot of false errors. Since the phy_3d_sensor is instantiated for each type and not once for the actual physical sensor. Imagine you have a standard IMU which provides accel/gyro/die_temp. This means you'll have 2 instances for the phy_3d_sensor and I'm assuming a phy_1d_sensor. If you try to read all 3 separately (in succession), the fetch call will fail on the 2nd and 3rd calls since new data isn't available yet. This is why my original design (referencing #58925) calls for only 1 device node and 3 info nodes to be bound in the example above. The problem is still complicated, but at least all the read requests will funnel into the same device instance so it would be easier (since they share the data/config structs).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments. I use sensor_sample_fetch_chan instead of sensor_sample_fetch now. It can handle this case well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, It'll have 2 instances for the phy_3d_sensor and a phy_1d_sensor. For accel, it will use SENSOR_CHAN_ACCEL_XYZ, and it use SENSOR_CHAN_GYRO_XYZ for gyro and SENSOR_CHAN_DIE_TEMP for die_temp.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still won't work because the fetch checks the status register, if you don't have a new sample between fetch calls, this will fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dose between fetch calls mean between two fetch accel calls or between fetch accel call and fetch gyro call?

  • Case 1: between two fetch accel calls
    In this case, the sensing subsystem(polling mode) and data ready trigger function(data ready trigger mode) will make sure that there is a new sample between two fetch accel calls.
  • Polling mode:
    The freq of fetch called by Sensing subsystem is always less than sensor working ODR.
  • Data ready trigger mode:
    The fetch is triggerd by the data ready trigger function which set by sensor_trigger_set.
    And data ready trigger function only be called by sensor driver while there is a new sample to fetch.
  • Case 2: between fetch accel call and fetch gyro call
    In this case, the sensor driver should ensure the fetch accel call should only check and clear drdy_acc bit, and fetch gyro call should also only check and clear drdy_gyro bit. With this guarantee, it can work normally like the case 1.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dose between fetch calls mean between two fetch accel calls or between fetch accel call and fetch gyro call?

  • Case 1: between two fetch accel calls
    In this case, the sensing subsystem(polling mode) and data ready trigger function(data ready trigger mode) will make sure that there is a new sample between two fetch accel calls.
  • Polling mode:
    The freq of fetch called by Sensing subsystem is always less than sensor working ODR.
  • Data ready trigger mode:
    The fetch is triggerd by the data ready trigger function which set by sensor_trigger_set.
    And data ready trigger function only be called by sensor driver while there is a new sample to fetch.
  • Case 2: between fetch accel call and fetch gyro call
    In this case, the sensor driver should ensure the fetch accel call should only check and clear drdy_acc bit, and fetch gyro call should also only check and clear drdy_gyro bit. With this guarantee, it can work normally like the case 1.

I'm talking about case 2, where you need to read accel and gyro. Most devices don't have separate data ready bits let alone registers. These status registers clear when read so when you fetch for the accel request you'll clear the status register and won't have data for the gyro. If you wait for new data for the gyro you'll either:

  1. Get data from another frame (which may make the fusion of the 2 not very useful)
  2. Have to wait a long time if the sensor requires warming up (some sensors take a long time to collect a sample for one-shot readings).

I didn't want to dive into it, but you can expand this further to the FIFO. Say you have an accel/gyro/die_temp driver that leverages a hardware FIFO. A single reading will contain samples from all 3 sensor types. If you have separate drivers like the current subsystem I'm not sure how you plan on fanning out the data. The accel phy_3d_sensor will get the full FIFO and will need to know about the other drivers. My model uses a single driver and 3 info nodes that are bound. This way the driver is aware of all the capabilities that are enabled and is able to service all 3 sensor types from one reading or the FIFO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For case 2, a lot of sensors (like BMI160, LSM6DSO) support separated data_rdy bits, so their existing sensor device driver can be used directly; for sensors which do not support separated data_rdy bits, the solution is simply add soft data_rdy bit in its sensor device driver implementation.
On FIFO, given today there is no existing Zephyr sensor device driver that supports FIFO, we did not plan to cover FIFO based combo sensor case in this PR. We can provide an example PR based on #60063 to show how phy_3d_sensor wrapper works with FIFO enabled sensor, formal PR will not cover FIFO until #60063 got merged.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create and RFC for the proposal to support soft data ready bits per channel. I'm having a hard time seeing how this will scale and will be testable.

While I understand that you did not plan to cover the FIFO based sensor here, could you please explain how this will work. Currently there's a struct device created per sensor type. How will they be linked together?

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 lsm6dso and lis2dw12 on hardware and bmi160 on
native_posix simulator. It can work fine while sensor is either in polling
mode or in data ready trigger mode. And set_sensitivity function is just
implemented following api definition and not tested yet.

Tested with:
$ west build -p -b native_posix samples/subsys/sensing/simple/ -t run

Signed-off-by: Zhang Lixu <lixu.zhang@intel.com>
@lixuzha lixuzha requested a review from yperess July 18, 2023 06:19
tejlmand added a commit to tejlmand/zephyr that referenced this pull request Sep 13, 2023
Fixes: zephyrproject-rtos#62589
Follow-up: zephyrproject-rtos#60331

The PR zephyrproject-rtos#60331 moved CMake code to new folder location causing generated
library names to change.
This change impacted the use of those libraries in the Zephyr armlink
CMake code, causing CMake failures at configure time.

This PR fixes this failure by updating the armlink CMake code to use
the new library names.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
tejlmand added a commit to tejlmand/zephyr that referenced this pull request Sep 13, 2023
Fixes: zephyrproject-rtos#62589
Follow-up: zephyrproject-rtos#60331

The PR zephyrproject-rtos#60331 moved CMake code to new folder location causing generated
library names to change.
This change impacted the use of those libraries in the Zephyr armlink
CMake code, causing CMake failures at configure time.

This PR fixes this failure by updating the armlink CMake code to use
the new library names.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Sep 24, 2023
@github-actions github-actions bot closed this Oct 8, 2023
@lixuzha
Copy link
Collaborator Author

lixuzha commented Nov 27, 2023

Basing on design changed, submit #64478 to instead of this one. Closed this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants