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

sensors: Introduce async workflow #57962

Merged
merged 5 commits into from
May 26, 2023

Conversation

yperess
Copy link
Collaborator

@yperess yperess commented May 16, 2023

Introduces a new workflow for sensors involving getting raw sensor data via RTIO and allowing for processing to be done by an application controlled thread via the decoder API.

@yperess
Copy link
Collaborator Author

yperess commented May 16, 2023

@teburd @MaureenHelm @asemjonovs @tristan-google this PR is fully functional, it's just missing documentation AFAICT. Please review, docs will come in a few hours. If context is needed, #56963 contains the full stack of changes including streaming support and Al's icm driver changes.

@yperess yperess requested a review from andyross as a code owner May 16, 2023 19:19
@yperess yperess force-pushed the push-bot/sensor_workflow branch 5 times, most recently from f806ee7 to 037156e Compare May 17, 2023 04:07
@yperess
Copy link
Collaborator Author

yperess commented May 17, 2023

Added the documentation

@nordicjm
Copy link
Collaborator

Since there is this:

.. warning::
This approach to fetching values will soon be deprecated. See the
following section for the updated APIs.

Can you perform a before and after build of a simple sensor sample (not accelerometer based, https://github.com/zephyrproject-rtos/zephyr/tree/main/samples/sensor/bme280 would be good if converted) and post the flash and RAM of both builds?

asemjonovs
asemjonovs previously approved these changes May 17, 2023
Copy link
Collaborator

@teburd teburd left a comment

Choose a reason for hiding this comment

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

Will do some poking around today and play around with the shell sample, really happy to see.

doc/hardware/peripherals/sensor.rst Outdated Show resolved Hide resolved
doc/hardware/peripherals/sensor.rst Outdated Show resolved Hide resolved
drivers/sensor/default_rtio_sensor.c Outdated Show resolved Hide resolved
@yperess yperess force-pushed the push-bot/sensor_workflow branch 2 times, most recently from 286863f to 7c98123 Compare May 17, 2023 22:33
@yperess yperess requested review from asemjonovs and teburd May 17, 2023 22:33
fabiobaltieri
fabiobaltieri previously approved these changes May 25, 2023
doc/hardware/peripherals/sensor.rst Outdated Show resolved Hide resolved
doc/hardware/peripherals/sensor.rst Outdated Show resolved Hide resolved
doc/hardware/peripherals/sensor.rst Outdated Show resolved Hide resolved
include/zephyr/drivers/sensor.h Show resolved Hide resolved
include/zephyr/drivers/sensor.h Show resolved Hide resolved
drivers/sensor/default_rtio_sensor.c Outdated Show resolved Hide resolved
Improve clang-format handling for iterable sections.

Signed-off-by: Yuval Peress <peress@google.com>
Add a new async API based on the RTIO subsystem. This new API allows:
1. Users to create sampling configs (telling the sensor which channels
   they want to sample together).
2. Sample data in an asynchronous manner which provides greater control
   over the data processing priority.
3. Fully backwards compatible API with no driver changes needed for
   functionality (they are needed to improve performance).
4. Helper functions for processing loop.

Signed-off-by: Yuval Peress <peress@google.com>
Update the sensor shell logic to use the new sensor_read() APIs and
make triggers an option of the sensor_shell sample (this avoids the
trigger stealing the interrupt status from one-shot reads).

Signed-off-by: Yuval Peress <peress@google.com>
Always clearing the interrupt status register was causing issues for
the sensor shell when interrupts were enabled but trying to read
one-off samples.

Signed-off-by: Yuval Peress <peress@google.com>
Add documentation about the new async read and decode APIs including
some rough examples.

Signed-off-by: Yuval Peress <peress@google.com>
@yperess
Copy link
Collaborator Author

yperess commented May 26, 2023

Second force push is a rebase because of a merge conflict in drivers/sensor/CMakeLists.txt

@MaureenHelm MaureenHelm merged commit 5ab1473 into zephyrproject-rtos:main May 26, 2023
27 checks passed
@kmeinhar
Copy link
Contributor

kmeinhar commented Jun 1, 2023

@yperess As far as I can tell the sensor shell now requires the async API even for sensors that do not support it.
I think it would make sense to support both types of sensors with the shell.
What would be the best way forward with such a feature? Should i draft a PR for it?

@yperess
Copy link
Collaborator Author

yperess commented Jun 1, 2023

@yperess As far as I can tell the sensor shell now requires the async API even for sensors that do not support it.
I think it would make sense to support both types of sensors with the shell.
What would be the best way forward with such a feature? Should i draft a PR for it?

The async API wraps the existing one so there's no need to do anything. As drivers start to implement it, you'll see a small performance gain. The only catch is if you enable the shell, you need to enable the async API flag but every sensor should work as usual. If you run into an issue with a specific sensor please let me know.

@kmeinhar
Copy link
Contributor

kmeinhar commented Jun 1, 2023

The Problem for us is that the async API pulls in the RTIO system. At least by default this increases our RAM usage by around 4.5KB, which we can not afford on our 32KB RAM device.
After a quick look I was not able to see a good way to drastically reduce this memory requirement.

@teburd
Copy link
Collaborator

teburd commented Jun 1, 2023

Can you please file an issue with west build -t ram_report showing the relevant sections? Very little ram is actually required for rtio, the mempool might the issue at hand here

@kmeinhar
Copy link
Contributor

kmeinhar commented Jun 1, 2023

This here is the only major difference in the RAM report. The Only change in my prj.conf is CONFIG_SENSOR_SHELL=y vs CONFIG_SENSOR_SHELL=n

small.txt
large.txt


    │   ├── sensor                                                                               3063  10.35%
    │   │   ├── default_rtio_sensor.c                                                               4   0.01%
    │   │   │   └── log_dynamic_sensor_compat                                                       4   0.01%
    │   │   └── sensor_shell.c                                                                   3059  10.34%
    │   │       ├── _block_pool_sensor_read_rtio_block_pool                                      2048   6.92%
    │   │       ├── _cqe_pool_sensor_read_rtio_cqe_pool                                           128   0.43%
    │   │       ├── _sqe_pool_sensor_read_rtio_sqe_pool                                           320   1.08%
    │   │       ├── _sys_bitarray_bundles__sys_mem_blocks_bitmap__sys_blocks_sensor_read_rtio_block_pool 4   0.01%
    │   │       ├── _sys_blocks_sensor_read_rtio_block_pool                                        16   0.05%
    │   │       ├── _sys_mem_blocks_bitmap__sys_blocks_sensor_read_rtio_block_pool                 12   0.04%
    │   │       ├── current_cmd_ctx                                                                 1   0.00%
    │   │       ├── iodev_sensor_shell_channels                                                   114   0.39%
    │   │       ├── iodev_sensor_shell_read                                                        20   0.07%
    │   │       ├── iodev_sensor_shell_read_config                                                 16   0.05%
    │   │       ├── log_dynamic_sensor_shell                                                        4   0.01%
    │   │       ├── sensor_attribute_name                                                          60   0.20%
    │   │       ├── sensor_channel_name                                                           228   0.77%
    │   │       ├── sensor_read_rtio                                                               40   0.14%
    │   │       ├── sensor_read_rtio_block_pool                                                     8   0.03%
    │   │       ├── sensor_read_rtio_cqe_pool                                                      20   0.07%
    │   │       └── sensor_read_rtio_sqe_pool     

@teburd
Copy link
Collaborator

teburd commented Jun 1, 2023

The block, sqe, and cqe pools can be made much much smaller here. The rest (channel name related stuff) I believe is needed for the shell auto complete.

I'd accept patches to making the default sizes here more sensible for smaller devices. This could be as small as 1 sq, 1 cq, and a mempool with one block sized to fit your sensor data.

To me this is a bug and a fix in 3.4 would be sensible.

@yperess any thoughts?

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

Successfully merging this pull request may close these issues.

None yet

10 participants