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

Add support for IIO events #1081

Merged
merged 16 commits into from
Nov 22, 2023
Merged

Add support for IIO events #1081

merged 16 commits into from
Nov 22, 2023

Conversation

pcercuei
Copy link
Contributor

@pcercuei pcercuei commented Nov 6, 2023

Add new API functions to allow receiving IIO events.

  • iio_device_create_event_stream returns a iio_event_stream object, which can then be used to read IIO events;
  • iio_event_stream_destroy to destroy a previously created stream;
  • iio_event_stream_read to read an iio_event, either non-blocking (in which case -EAGAIN is returned if there is no event) or blocking (if you want to run it in a thread with a callback mechanism).

The iio_event is the exact same as the iio_event_data object from Linux' <linux/iio/events.h> so the data can be extracted similarly. A few functions were added to make it easier to decode events: iio_event_get_type, iio_event_get_direction, iio_event_get_channel.

Events can be configured (to set thresholds etc.) and enabled/disabled using the standard IIO attribute mechanism, as their settings now appear as standard iio_channel attributes.

@pcercuei pcercuei force-pushed the pcercuei/iio-events branch 3 times, most recently from 71881a2 to d75e8c8 Compare November 6, 2023 14:59
@pcercuei pcercuei force-pushed the pcercuei/iio-events branch 6 times, most recently from 65a15e3 to 12a66cd Compare November 8, 2023 10:18
@rgetz
Copy link
Contributor

rgetz commented Nov 10, 2023

don't we want to list that events are possible in iio_info?

@pcercuei
Copy link
Contributor Author

I think I will update iio_info in a separate PR. There are a few things I want to do there anyway, notably handle attribute reads errors better, and add some color.

}

struct iio_event_stream *
iio_device_create_event_stream(const struct iio_device *dev)
Copy link

Choose a reason for hiding this comment

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

I have always found it helpful to have the same namespace prefix for all functions in a file. So perhaps this could be iio_event_stream_create_for_device() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't be consistent with what we already have, e.g. iio_device_create_buffer, iio_buffer_create_block, iio_buffer_create_stream.

Copy link

Choose a reason for hiding this comment

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

fair enough

/**
* @brief Read an event from the event stream.
* @param stream A pointer to an iio_event_stream structure
* @out_event An pointer to an iio_event structure, that will be filled by
Copy link

Choose a reason for hiding this comment

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

Suggested change
* @out_event An pointer to an iio_event structure, that will be filled by
* @param out_event An pointer to an iio_event structure, that will be filled by

static inline enum iio_event_type
iio_event_get_type(const struct iio_event *event)
{
return (enum iio_event_type)((event->id >> 56) & 0xff);
Copy link

Choose a reason for hiding this comment

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

It could be helpful to mention that this is the equivalent of the IIO_EVENT_CODE_EXTRACT_TYPE macro from linux/iio/events.h.

(same applies to other similar functions here and in the .c file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do.

local.c Outdated
uint64_t event = 1;
int ret;

ret = write(pdata->cancel_fd, &event, sizeof(event));
Copy link

Choose a reason for hiding this comment

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

Should this retry the write on EINTR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think EINTR can happen at all - the write won't block.

Copy link

Choose a reason for hiding this comment

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

ah yes, of course

ret = poll(pollfd, 2, timeout_rel);
} while (ret == -1 && errno == EINTR);

if ((pollfd[1].revents & (POLLIN | POLLNVAL)))
Copy link

Choose a reason for hiding this comment

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

Should this be moved after the ret checks below?

Copy link

Choose a reason for hiding this comment

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

Also, don't we need to read from the eventfd to reset it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't actually want to reset it; otherwise the local_read_event function would only fail the first time.

As for the order - the pollfd[1].revents check is done before the check for ret < 0 just to be consistent with what's already in buffer_check_ready. It shouldn't matter much anyway.

Copy link

@dlech dlech Nov 14, 2023

Choose a reason for hiding this comment

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

Looking back, I see where the docs say that you have to call iio_event_stream_destroy() to cancel the read, so makes sense now that there is no reset to read again.

@@ -34,7 +34,7 @@
#define is_little_endian() (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
#endif

#define BIT(x) (1 << (x))
#define BIT(x) (1ull << (x))
Copy link

Choose a reason for hiding this comment

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

Seems odd to make this 64-bit but not the other BIT macros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only make the BIT macro able to create 64-bit masks, the BIT_MASK and BIT_WORD still need to work on 32-bit words. I could make them work with 64-bit words, but that means changing the iio_channels_mask struct, which is a bit outside this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reviewing btw.

@pcercuei pcercuei force-pushed the pcercuei/iio-events branch from 12a66cd to a94e552 Compare November 15, 2023 11:47
chn1 = Channel(dev, _ev_get_channel(_byref(event), dev._device, False))
chn2 = Channel(dev, _ev_get_channel(_byref(event), dev._device, True))
except Exception:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be more explicit for the exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to OSError.

@pcercuei pcercuei force-pushed the pcercuei/iio-events branch from a94e552 to dcb882e Compare November 16, 2023 10:01
@rgetz
Copy link
Contributor

rgetz commented Nov 18, 2023

@tfcollins should look at the Codacy issues, and point out any that should be fixed before a merge - since they are all Python.

@pcercuei pcercuei force-pushed the pcercuei/iio-events branch 3 times, most recently from a72f1e9 to 6c78814 Compare November 20, 2023 16:25
@pcercuei
Copy link
Contributor Author

@tfcollins should look at the Codacy issues, and point out any that should be fixed before a merge - since they are all Python.

I fixed some of them, some other I will leave as-is. The "parent __init__ not called" one because the children class' __init__ just returns an exception (as we don't want to permit direct instantiation), the super() one to be consistent with the rest of the script file.

Add the new iio_chan_type enum entries that were added in kernel v6.7.

Signed-off-by: Paul Cercueil <[email protected]>
Add a new API and the corresponding high-level code to support reading
and decoding IIO events.

Signed-off-by: Paul Cercueil <[email protected]>
Keep a file descriptor to the /dev/iio:deviceX node opened. When
creating a buffer, this file descriptor will be duplicated.

This ensures that we can create a IIO events file descriptor without
having to create a IIO buffer first, and without the problem of trying
to open the main FD twice.

Signed-off-by: Paul Cercueil <[email protected]>
This allows applications to control the conditions under which IIO
events will trigger.

Signed-off-by: Paul Cercueil <[email protected]>
Add the necesary callbacks to create an events stream, and read events
with the local backend.

Signed-off-by: Paul Cercueil <[email protected]>
This can be used to specify the timeout value for a specific
communication pipe.

Signed-off-by: Paul Cercueil <[email protected]>
Add three new functions tailored for IIO events support. These functions
allow to create an IIO events stream, read from it, and close it.

The open / close functions use the regular I/O pipe of the responder,
while the event read function will use an I/O pipe dedicated to the
event stream. This allows the call to be blocking, only returning when
an event has been read, without disturbing the command flow.

Note that there is no listener thread that would continuously poll the
IIO events; those are only read when iiod_client_read_event() is called.
This is fine, as IIO events are buffered anyway on the Linux kernel
side, so events are not lost.

Signed-off-by: Paul Cercueil <[email protected]>
There is no need to have a mutable pointer there, use a const pointer
instead.

Signed-off-by: Paul Cercueil <[email protected]>
With IIOD's current responder design, we can only answer to a command if
we know which I/O pipe is used for answering. This means that if we
cannot obtain that information, we cannot answer.

In that case,

Signed-off-by: Paul Cercueil <[email protected]>
Add support for the IIOD_OP_CREATE_EVSTREAM, IIOD_OP_FREE_EVSTREAM, AND
IIOD_OP_READ_EVENT commands.

These can be used to open and close event streams, and read events from
an event stream.

Signed-off-by: Paul Cercueil <[email protected]>
Provide the necesary callbacks to support IIO events with the network
backend.

Signed-off-by: Paul Cercueil <[email protected]>
Provide the necesary callbacks to support IIO events with the USB
backend.

Signed-off-by: Paul Cercueil <[email protected]>
Provide the necesary callbacks to support IIO events with the serial
backend.

Signed-off-by: Paul Cercueil <[email protected]>
The previous commit(s) reworking IIO attributes changed the type of the
'attr' variable from a const string to a 'struct iio_attr *'.

It appears that the code was not properly updated at two different
spots; update it so that it works properly.

Signed-off-by: Paul Cercueil <[email protected]>
This utility program can be used to monitor events sent by a given IIO
device.

Signed-off-by: Paul Cercueil <[email protected]>
Add support for reading IIO events in the Python bindings.

This is done by calling iio.Device.event_stream() using a context
manager, which will create an EventStream object, and then use the
read() method:

dev = ctx.devices["iio:device4"]
with dev.event_stream() as evstream:
    ev = evstream.read()
    if ev is not None:
        print("Got event!")
    else:
        time.sleep(0.1)

The read() method takes a 'nonblock' argument, which defaults to True.
Python code can then poll for events regularly.

Polling is better, because doing a blocking read does not give any
guarantee about when the call will return.

Signed-off-by: Paul Cercueil <[email protected]>
@pcercuei pcercuei force-pushed the pcercuei/iio-events branch from 6c78814 to b574558 Compare November 21, 2023 15:18
Copy link
Contributor

@mhennerich mhennerich left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@pcercuei pcercuei merged commit 7d97827 into main Nov 22, 2023
23 of 24 checks passed
@pcercuei pcercuei deleted the pcercuei/iio-events branch November 22, 2023 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants