Skip to content

Conversation

@teburd
Copy link
Contributor

@teburd teburd commented Apr 29, 2022

Provides i2c_transfer_cb as an optional API along with a wrapper that provides i2c_transfer_signal (async with a k_poll_signal notifier).

Notably i2c_transfer_cb should not block so it can be initiated from an ISR if so desired as done in the frdm_k64f example. If it would block, returning -EWOULDBLOCK is expected.

@teburd teburd force-pushed the async_i2c branch 5 times, most recently from 924d001 to 74889b6 Compare May 2, 2022 17:53
@teburd teburd force-pushed the async_i2c branch 3 times, most recently from 2963fb9 to da528fb Compare May 4, 2022 15:08
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label May 4, 2022
@teburd teburd force-pushed the async_i2c branch 5 times, most recently from f3318d8 to beb2168 Compare May 4, 2022 18:12
@teburd teburd marked this pull request as ready for review May 4, 2022 18:13
@teburd teburd requested a review from aunsbjerg May 4, 2022 18:14
@teburd teburd requested a review from sjg20 as a code owner July 7, 2022 15:54
@teburd teburd force-pushed the async_i2c branch 3 times, most recently from 3f28ffb to a4d75df Compare July 13, 2022 23:59
@teburd teburd force-pushed the async_i2c branch 4 times, most recently from 85658cf to d2f24bb Compare July 15, 2022 14:27
@teburd teburd removed the DNM This PR should not be merged (Do Not Merge) label Jul 15, 2022
@teburd
Copy link
Contributor Author

teburd commented Jul 15, 2022

In place of a common set of callback helpers, provide a direct k_poll_signal wrapper alongside the callback version of i2c_transfer, so adds i2c_transfer_cb (callback) and i2c_transfer_signal (k_poll_signal, wraps the callback API).

This matches my discussions @henrikbrixandersen on discord

@teburd teburd force-pushed the async_i2c branch 2 times, most recently from 6d1fba2 to 3050731 Compare July 15, 2022 16:00
teburd added 3 commits July 15, 2022 11:03
i2c_transfer_async is the asynchronous version of i2c_transfer where
the completion of the transfer is notified to the caller using a callback.

This can be used in conjuction with the common callbacks and datatypes
in async.h for directly doing an async transfer with an IPC object
to notify a thread.

Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
Implements i2c_transfer_cb for the mcux driver

Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
Adds a simple test case against the FXOS8700 sensor and its
built in FIFO using i2c. Pulls data from the sensor using both
synchronous and asynchronous transfers.

Signed-off-by: Tom Burdick <thomas.burdick@intel.com>
@teburd
Copy link
Contributor Author

teburd commented Jul 18, 2022

I did spend some time trying to make the k_poll_signal i2c_transfer_signal a user space friendly syscall with some success. The downsides came to needing to alloc/free things when user space was being used to keep the life of a copy of the i2c_msg array around long enough.

It's certainly something that can be addressed in a future PR.

@teburd teburd added the dev-review To be discussed in dev-review meeting label Jul 20, 2022
struct i2c_target_config *cfg);
#ifdef CONFIG_I2C_CALLBACK
typedef int (*i2c_api_transfer_cb_t)(const struct device *dev,
struct i2c_msg *msgs,
Copy link
Contributor

Choose a reason for hiding this comment

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

i still think that it would be better to put all this into a struct. Struct can be later on extended without breaking api (e.g adding list node field if we go into direction of queued transfers).

Current version is suitable only for bus with one device since with more we might hit collision and handling collision is painful. At the end, it should be really fire and forget. Using struct gives path towards that without need of breaking the API in the future.

Copy link
Contributor Author

@teburd teburd Aug 11, 2022

Choose a reason for hiding this comment

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

This can be done by providing an asynchronous lock/unlock pair of functions

i2c_lock_async(dev, cb0, data);
i2c_transfer_cb(dev, i2c_msgs, cb1, data);
i2c_transfer_cb(dev, more_i2c_msgs, cb2, data);
i2c_unlock_async(dev, cb3, data);

Obviously not in a row like that but in a callback chain. Callback chaining like this is what RTIO makes look more like a queue of requests and pollable responses.

That's how I was planning on dealing with this issue rather than trying to embed a request queue directly in the device driver. In effect there's an application allocated request queue over all devices rather than each device having its own. This way you can do request chaining or parallelism with most of the I/O task concurrency that something like a promise pipeline might give you.

It also shares all the code and logic then in something common to all device driver classes.

The blocking api can look like a semaphore given at the end of a fixed callback chain like above.

I could be wrong though, and I thought you tried to embed the queue in the device before but ran into issues.

@nashif nashif merged commit 88de590 into zephyrproject-rtos:main Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Documentation area: I2C area: Tests Issues related to a particular existing or missing test dev-review To be discussed in dev-review meeting platform: NXP NXP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants