ZIO: Beginnings of a Sampled Input/Output API #14008
Conversation
Codecov Report
@@ Coverage Diff @@
## topic-sensors #14008 +/- ##
================================================
- Coverage 52.87% 52.27% -0.6%
================================================
Files 310 309 -1
Lines 45264 45510 +246
Branches 10456 10536 +80
================================================
- Hits 23933 23792 -141
- Misses 16549 16920 +371
- Partials 4782 4798 +16
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I'm not yet familiar enough with the other IO APIs in Zephyr, but would zio_buf_*, zio_chan_*, and zio_dev_* be better keeping 80 char limitations in mind (versus zio_buffer_*, zio_channel_*, etc., at present)?
I'm adding a proof of concept zio_attr.h header and want to be consistent with the proposal here, but conscious that space is at a premium with names.
|
For attributes, APIs like IIO tend to have a single set of attribute definitions that apply globally to devices, channels, triggers, etc. At present, you have I'm actually tempted to propose having distinct attribute structs, in fact, such as |
|
This code has never seen a compiler, but this is my attempt at thinking out loud about a possible solution for device and channel attributes. The goal is of course to have a flexible system that avoids us painting ourselves into a corner in the future, while enabling a relatively fast, light-weight solution for direct, raw data access. One item worth highlighting it the proposed @BFrog I can push this out to your PR fork/branch, but didn't want to step on your toes since the code may be in flux on your side? zio_attr.h/**
* @file
* @brief ZIO attribute definitions.
*/
/*
* Copyright (c) 2019 Thomas Burdick <thomas.burdick@gmail.com>
* Copyright (c) 2019 Kevin Townsend
*
* SPDX-License-Identifier: Apache-2.0
*/
#ifndef ZEPHYR_INCLUDE_ZIO_ATTR_H_
#define ZEPHYR_INCLUDE_ZIO_ATTR_H_
#include <zephyr/types.h>
#include <device.h>
#ifdef __cplusplus
extern "C" {
#endif
/**
* @brief ZIO attribute enum and struct definitions.
* @defgroup zio_attributes ZIO attribute definitions
* @ingroup zio
* @{
*/
/**
* ZIO devices are composed of device attributes (zio_dev_attr) and channel
* attributes (zio_chan_attr) in the following manner:
*
* ZIO Device----------------------------------------------------------------+
* | |
* | Device Attributes-----------------------------------------------------+ |
* | | +-----------------------------------------------------------------+ | |
* | | | Device Attribute 0 (Display Name) | | |
* | | +-----------------------------------------------------------------+ | |
* | | | Device Attribute 1 (Op. Mode) | | |
* | | +-----------------------------------------------------------------+ | |
* | |
* | +---------------------------------------------------------------------+ |
* | Channels--------------------------------------------------------------+ |
* | | Channel 0---------------------------------------------------------+ | |
* | | | Channel Attributes---------------------------------------------+| | |
* | | | | +----------------------------------------------------------+ || | |
* | | | | | Channel Attribute 0 (Raw Data) | || | |
* | | | | +----------------------------------------------------------+ || | |
* | | | | | Channel Attribute 1 (SI Data) | || | |
* | | | | +----------------------------------------------------------+ || | |
* | | | | | Channel Attribute 2 (Sampling Freq.) | || | |
* | | | | +----------------------------------------------------------+ || | |
* | | | +--------------------------------------------------------------+| | |
* | | +-----------------------------------------------------------------+ | |
* | | |Channel 1 | | |
* | | +-----------------------------------------------------------------+ | |
* | | |Channel 2 | | |
* | | +-----------------------------------------------------------------+ | |
* | +---------------------------------------------------------------------+ |
* +-------------------------------------------------------------------------+
*
* Raw sensor data (always channel 0, and mandatory) could be accessed as
* follows, for example:
*
* s16_t temp_r = mydev.channels[0].attrs[0].data.value.s16_val;
*
* Alternatively, the get and set operations can be wrapped in a helper
* function that returns a standard error code to enable data validation
* when assigning attribute values (sample rate, etc.).
*/
/**
* @brief A type tagged variant for attributes
*/
struct zio_attr_data
{
/**< Value type identifier. */
enum {
float_, /**< Single-precision float tag. */
double_, /**< Double-precision float tag. */
bool_, /**< 1-bit boolean tag. */
u8_, /**< Unsigned 8-bit integer tag. */
u16_, /**< Unsigned 16-bit integer tag. */
u32_, /**< Unsigned 32-bit integer tag. */
u64_, /**< Unsigned 64-bit integer tag. */
s8_, /**< Signed 8-bit integer tag. */
s16_, /**< Signed 16-bit integer tag. */
s32_, /**< Signed 32-bit integer tag. */
s64_, /**< Signed 64-bit integer tag. */
str_, /**< String tag. */
ptr_, /**< Generic pointer tag. */
} tag;
/**< Assigned value. */
union {
float float_val; /**< Single-precision float value. */
double double_val; /**< Double-precision float value. */
u8_t bool_val:1; /**< 1-bit boolean value. */
u8_t u8_val; /**< Unsigned 8-bit integer value. */
u16_t u16_val; /**< Unsigned 16-bit integer value. */
u32_t u32_val; /**< Unsigned 32-bit integer value. */
u64_t u64_val; /**< Unsigned 64-bit integer value. */
s8_t s8_val; /**< Signed 8-bit integer value. */
s16_t s16_val; /**< Signed 16-bit integer value. */
s32_t s32_val; /**< Signed 32-bit integer value. */
s64_t s64_val; /**< Signed 64-bit integer value. */
char *str_val; /**< String (char *) value. */
void *ptr_val; /**< Generic pointer (void *) value. */
} value;
};
/** @brief ZIO device attribute types. */
typedef enum zio_dev_attr_type {
DEV_ATTR_NAME, /**< Short presentation name. */
DEV_ATTR_OP_MODE, /**< Current operating/power mode. */
DEV_ATTR_OP_MODE_LIST, /**< Supported operating/power modes. */
} zio_dev_attr_type_t;
/** @brief ZIO channel attribute types. */
typedef enum zio_chan_attr_type {
CHAN_ATTR_RAW_DATA = 0, /**< Mandatory raw data attribute. */
CHAN_ATTR_SI_DATA, /**< SI data attribute. */
CHAN_ATTR_NAME, /**< Short presentation name. */
CHAN_ATTR_SAMP_FREQ, /**< Current HW sampling frequency. */
CHAN_ATTR_SAMP_FREQ_LIST, /**< Supported HW sample frequencies. */
CHAN_ATTR_OFFSET,
CHAN_ATTR_SCALE, /**< Raw value to SI scale factor. */
CHAN_ATTR_CAL_BIAS, /**< Factory calibration bias. */
CHAN_ATTR_CAL_SCALE, /**< Factory calibration scale factor. */
CHAN_ATTR_EVENT, /**< Current read/event mode. */
CHAN_ATTR_EVENT_LIST, /**< Supported read/event modes. */
CHAN_ATTR_TRIGGER, /**< Current trigger. */
CHAN_ATTR_TRIGGER_LIST, /**< Supported triggers. */
CHAN_ATTR_BUF_ENABLED, /**< Enable or disable the channel buffer. */
CHAN_ATTR_DATA_AVAIL, /**< Indicate if channel data is available. */
} zio_chan_attr_type_t;
/** Device attribute record. */
struct zio_dev_attr {
/** Index for this specific attribute. Assigned when bound to device. */
u8_t idx;
/** Attribute identifier. */
zio_dev_attr_type_t attr_type;
/** Attribute data type and value. */
struct zio_attr_data data;
/** Whether this attribute is enabled (1) or disabled (0). */
u8_t enabled:1;
};
/** Channel attribute record. */
struct zio_chan_attr {
/** Index for this specific attribute. Assigned when bound to channel. */
u8_t idx;
/** Primary attribute type identifier. */
zio_chan_attr_type_t attr_type;
/**
* Secondary attribute type identifier. Used to distinguish multiple
* instances of the same 'attr_type' in the channel.
*/
zio_chan_attr_type_t attr_subtype;
/** Attribute data type and value. */
struct zio_attr_data data;
/** Whether this attribute is enabled (1) or disabled (0). */
u8_t enabled:1;
/** Data tick counter. Increments every time 'data' is updated. */
u32_t tick;
};
/* TODO: Channel attributes should be associated with a buffer, etc.! */
/**
* @}
*/
#ifdef __cplusplus
}
#endif
#endif /* ZEPHYR_INCLUDE_ZIO_ATTR_H_ */ |
teburd
force-pushed
the
zio
branch
4 times, most recently
from
2d415ea
to
574d81c
Compare
|
@jfischer-phytec-iot I think I've fixed most of the formatting/garbage diff issues. I'll of course do a more thorough job if people like this work and accept it. |
teburd
force-pushed
the
zio
branch
2 times, most recently
from
e730e8a
to
fc6fccb
Compare
|
Hi @BFrog .... code seems good so far. I was wondering if you can imagine a use case, though, where you might want to attach a buffer to a specific channel? At present you can only attach a buffer to a device, which is what 90% of people will want I assume (sharing the same buffer across several orientation sensors, for example). I'm just curious if we'd be over-complicating things to allow this at the channel level as well, or if there are valid reasons to make that possible. I don't have a strong opinion or answer off the top of my head myself ... just thinking out loud. Similarly, what's your opinion on being able to assign more than one buffer to a device on complex systems? Over-engineering, valid, problematic??? Also ... would an enum be better for the pow2 approach for buffer size, just to make this explicit within reason? It could still accept (and validate) a straight integer, but would some enums for values to say 14 bits (16384 bytes) make the text clearer or just larger? Seeing 16K might be clearer than seeing the number |
|
I don't know why, but I still find |
|
@BFrog Perhaps the following enums should also be moved into At some point, it might make sense to reduce the number of header files as well, but I'm not sure what the best solution is there yet, and it will likely become more clear as one or two test drivers are assembled. If you agree to move these, let me know and I'll add them to #14245 |
teburd
force-pushed
the
zio
branch
2 times, most recently
from
2637bff
to
60bca93
Compare
A zio_buf is more like a handle to some internal implementation by the device rather than a buffer itself. So attach is attaching the handle to the devices internal buffer. The API doesn't necessarily prevent two attach calls occuring, or attaching to two different devices, though it shouldn't be allowed. Each zio_buf instance should be associated with one device only. You could have several zio_buf's that you poll on for multiple devices. |
teburd
changed the title
|
I don't see any system calls for any of these new driver APIs implemented. There is documentation about system calls here: |
|
@andrewboie great to know, will work on making sure sys calls are accounted for |
| config SYNTH_FIFO_SIZE | ||
| int "FIFO power of 2 size, pow(2,SYNTH_FIFO_SIZE), defaults to pow(2,9)=512" | ||
| default 9 | ||
| depends on SYNTH |
There was a problem hiding this comment.
You can remove depends on SYNTH here and throughout, since you've already wrapped everything with if SYNTH..endif
drivers/zio/synth/Kconfig
Outdated
| default 0 | ||
| depends on SYNTH | ||
| help | ||
| The signal phase in radians of the zeroth channel |
There was a problem hiding this comment.
You say radians here, but degrees above. Which is it? Same with SYNTH_1
There was a problem hiding this comment.
Doh, it should be radians everywhere, thanks!
| #include <misc/__assert.h> | ||
| #include <misc/byteorder.h> | ||
| #include <sensor.h> | ||
| #include <string.h> |
There was a problem hiding this comment.
I think you can remove a few of these includes.
There was a problem hiding this comment.
I think your right! That C file is a placeholder for the time being
include/zio/zio_attr.h
Outdated
| s32_t s32_val; /**< Signed 32-bit integer value. */ | ||
| s64_t s64_val; /**< Signed 64-bit integer value. */ | ||
| char *str_val; /**< String (char *) value. */ | ||
| void *ptr_val; /**< Generic pointer (void *) value. */ |
There was a problem hiding this comment.
I need to keep reading the rest of this PR, but will we create buffers with the zio_attr_data type? This will lead to very inefficient use of RAM for smaller quantities like s8, s16, and s32, since each entry in the buffer will consume 8 bytes.
There was a problem hiding this comment.
No, the format of the data streaming to/from the device is defined by the device driver and described by an array of channel description structs. It is a goal to avoid do any data conversion unless an application calls functions to do so. So if the device has a hardware fifo for example, the device driver would describe the formatting of the data in that fifo rather than converting it in any way.
There was a problem hiding this comment.
So this type is just for attributes, not for data samples?
There was a problem hiding this comment.
That's right, this is only used for attributes
include/zio/zio_attr.h
Outdated
| * Secondary attribute type identifier. Used to distinguish multiple | ||
| * instances of the same 'attr_type' in the channel. | ||
| */ | ||
| enum zio_chan_attr_type attr_subtype; |
There was a problem hiding this comment.
What's an example where you would use this?
There was a problem hiding this comment.
@microbuilder might be able to say more on that matter, but from what I recall with our conversations the idea would be useful for things like a spectroscopy or photodiode array where you might be reading lumens across many channels but at varying wavelengths There might be better or other ways of dealing with describing the physical measurement taking place, but we started with two enums
There was a problem hiding this comment.
The idea here was indeed to make life easier in instances where you have multiple instances of the same attribute. The first type would indicate a common class, and the second can be more precise. I'm not married to the idea, and there are other ways of handling this, but this was my first thought on the matter. Happy to hear alternative approaches!
There was a problem hiding this comment.
Hmm ... thinking about this, maybe it should just be removed and we should see how far we can get with a single attr_type, since I think even in instances of sensors like the TCS34725 with multiple photodiodes, we can make this work with one level of zio_chan_attr_type.
There was a problem hiding this comment.
Will be removed in the next update to this PR then, I like the removal of it for now and seeing how far we get
| /** | ||
| * bit width of channel data, ex: 12 bit samples from a 12 bit adc | ||
| */ | ||
| const u8_t bit_width; |
There was a problem hiding this comment.
Do we need scale factor and offset here too? Or do you have them somewhere else?
|
|
||
|
|
||
| /** | ||
| * @brief Function to trigger a read or write of the device |
There was a problem hiding this comment.
Does it block until the read is finished?
There was a problem hiding this comment.
It would not block. The same buffer polling and pulling function pair would be used to read values back. For ad-hoc reading and writing a function would wrap around the idea of a manual trigger, poll, and pull
There was a problem hiding this comment.
I wouldn't expect it to block, but it's worth documenting.
teburd
force-pushed
the
zio
branch
2 times, most recently
from
48866fd
to
d35ed71
Compare
| * channels (zio_dev_chan) and channel attributes (zio_chan_attr) in the | ||
| * following manner: | ||
| * | ||
| * ZIO Device----------------------------------------------------------------+ |
There was a problem hiding this comment.
If you add five spaces after the * (four spaces relative to the previous text line) this diagram will be rendered in the API HTML docs as a verbatim text block. (Otherwise it will all be rendered as a normal HTML paragraph and won't come out looking as you intended:
* ZIO Device----------------------------------------------------------------+
* | |
|
|
||
| /** | ||
| * @brief ZIO attribute enum and struct definitions. | ||
| * @defgroup zio_attributes ZIO attribute definitions |
There was a problem hiding this comment.
We'll also need to add a .rst file that references this group name to have the zio_attributes documentation appear in our API docs.
|
|
||
| /** | ||
| * @brief ZIO device API definitions. | ||
| * @defgroup zio_device ZIO device definitions |
There was a problem hiding this comment.
We'll also need to add a .rst file that references this group name to have the zio_device documentation appear in our API docs.
There was a problem hiding this comment.
There are a few stylistic and documentation related issues here, but I think this is at a point where we can merge it into topic-sensor to have a common reference point to move this work forward. I'll create a documentation PR that addresses some of the comments around .rst files and formatting.
microbuilder
dismissed
dbkinder’s stale review
I'll add a separate documentation PR to address these and other formatting issues independently.
teburd
force-pushed
the
zio
branch
3 times, most recently
from
c1720f9
to
3cad592
Compare
Add a new sensor device API called zio_dev along with supporting interface and implmentations needed to better encapsulate a large range of functionality typical sampled input/output (sensor and signal generator) type devices commonly provide. Every device has a set of channels and attributes associated with it. Channels may be attached to a fifo like interface (zio_buf) which provides a pollable mechanism for obtaining the raw channel data at a designated watermark level. Signed-off-by: Tom Burdick <thomas.burdick@gmail.com>
Provide a software only example driver and sample application which generates a left and right sine wave at CD quality bit width and rate. Signed-off-by: Tom Burdick <thomas.burdick@gmail.com>
|
@MaureenHelm I believe everything is sorted now |
| ({ \ | ||
| int res = 0; \ | ||
| const struct zio_dev_api *api = dev->driver_api; \ | ||
| if (!api->set_attr) { \ |
There was a problem hiding this comment.
should be get_attr here.
Just not to forget.
A beginning point for a better Sensor API with many ideas taken from various existing APIs in existence with Zephyr in mind.
The new API is centered a generalized device with attributes, channels, and channel attributes. Attributes may be of any known taggable type (zio_attr_data).
Channels represent streams of data either into or out of the device. The beginnings of this API focus on streams of data coming out of the device. These streams may then be subscribed to using a pollable buffer interface (zio_buf). This pollable buffer interface is implemented by the device driver using either a common software implementation (zio_fifo_buf) or a device specific hardware fifo (many Invensense chips, kionix chips, some st and nxp chips).
The pollable buffer notifies application code at a defined watermark.
Data transfers to/from the application can be done in a device specific, and optimal way. Since the buffer layout is device defined no translation of data needs to be done, DMA transfers are entirely possible this way allowing the hardware to do more of what its good at!
There's still a lot to do here, but we (@microbuilder and myself) would like to get this into a common topic branch to begin building more off of.