lib: queued_operation: add API for managing a queue of operations #24781

pabigot · 2020-04-28T19:39:56Z

Full asynchronous support for APIs such as bus transactions generally requires managing operations from unrelated clients. This API provides a data structure and functions to manage those operations generically, leaving the service to provide only the service-specific operation description and implementation.

(This replaces #23333 which was based on an earlier version of the on-off API. See that PR for previous review history.)

zephyrbot · 2020-04-28T19:48:56Z

All checks passed.

checkpatch (informational only, not a failure)

-:339: WARNING:LONG_LINE: line over 80 characters
#339: FILE: include/sys/queued_operation.h:254:
+static inline int queued_operation_fetch_result(const struct queued_operation *op,

-:411: WARNING:TYPO_SPELLING: 'succesfully' may be misspelled - perhaps 'successfully'?
#411: FILE: include/sys/queued_operation.h:326:
+ * independent notification when a succesfully initiated reset has

-:550: WARNING:LONG_LINE: line over 80 characters
#550: FILE: lib/os/queued_operation.c:104:
+static inline void trivial_start_and_unlock(struct queued_operation_manager *mgr,

-:612: WARNING:CONSTANT_COMPARISON: Comparisons should place the constant on the right side of the test
#612: FILE: lib/os/queued_operation.c:166:
+			    && (ST_ERROR == from)));

-:653: WARNING:LONG_LINE: line over 80 characters
#653: FILE: lib/os/queued_operation.c:207:
+		= CONTAINER_OF(cli, struct queued_operation_manager, onoff_client);

-:694: WARNING:LONG_LINE: line over 80 characters
#694: FILE: lib/os/queued_operation.c:248:
+				= CONTAINER_OF(node, struct queued_operation, node);

-:919: WARNING:LONG_LINE: line over 80 characters
#919: FILE: lib/os/queued_operation.c:473:
+		= CONTAINER_OF(cli, struct queued_operation_manager, onoff_client);

-:2073: WARNING:LINE_SPACING: Missing a blank line after declarations
#2073: FILE: tests/lib/queued_operation/src/main.c:1072:
+	struct onoff_transitions onoff_transitions = *service.onoff.transitions;
+	onoff_transitions.reset = NULL;

- total: 0 errors, 8 warnings, 2165 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PREFER_KERNEL_TYPES PRINTK_WITHOUT_KERN_LEVEL SPDX_LICENSE_TAG SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

pabigot · 2020-04-30T17:44:53Z

Updated to address the TBD from #22853 (comment) i.e. how to reset the queued operation manager when it depends on an on-off service that has entered an error state.

doc/reference/resource_management/index.rst

nordic-krch · 2020-05-04T11:22:04Z

include/sys/queued_operation.h

what about namespacing it sys_qop,
also queue_operation_manager -> sys_qop_mgr

I'd take queuedop_ ~~sys_queuedop_~~ as the prefix. sys_qop is too opaque. I expect we'll bikeshed this in an API meeting; I'm not changing this now.

(Edited to remove sys_ prefix which is not used for on-off service, and other things in <sys/foo.h>)

nordic-krch · 2020-05-04T11:23:42Z

include/sys/queued_operation.h

it's also quite long. SYS_QOP_APPEND or SYS_QOP_PRI_APPEND would be better, imo.

include/sys/queued_operation.h

nordic-krch · 2020-05-04T11:40:25Z

include/sys/queued_operation.h

what secondary notification means here? how to proceed when cli is null? pend on queue_operation_has_error until return false?

"secondary notification" meant a non-null cli, the path by which the application gets notification of the completion of the reset attempt so it can try the queued operation agin. I'll use independent notification to match the description of cli.

lib/os/queued_operation.c

pabigot

@nordic-krch comments addressed

pabigot · 2020-05-04T14:41:42Z

include/sys/queued_operation.h

I'd take queuedop_ ~~sys_queuedop_~~ as the prefix. sys_qop is too opaque. I expect we'll bikeshed this in an API meeting; I'm not changing this now.

(Edited to remove sys_ prefix which is not used for on-off service, and other things in <sys/foo.h>)

include/sys/queued_operation.h

pabigot · 2020-05-04T14:48:50Z

include/sys/queued_operation.h

"secondary notification" meant a non-null cli, the path by which the application gets notification of the completion of the reset attempt so it can try the queued operation agin. I'll use independent notification to match the description of cli.

lib/os/queued_operation.c

pabigot · 2020-05-04T15:00:08Z

include/sys/queued_operation.h

Given the complexity of getting this correct I'm going to stick with the solution that's known to work.

lib/os/queued_operation.c

nordic-krch · 2020-06-17T11:31:47Z

include/sys/queued_operation.h

i was wondering about possibility to detect that given operation is already enqueued and return error (-EBUSY?). Maybe node field could be used for that (set to null when op is idle).
I'm having following scenario: sensor which has one queued operation instance which is used to schedule reading and i won't to detect situation when someone schedules next read (reusing same queued operation) before previous one is completed.

That changes the contract used consistently for the resource management APIs: the client (in this case the sensor acting on behalf of a caller) submits a request, which is then owned by the manager until something notifies the client that the manager has released the request.

For your use case the sensor must be aware that there's an in-progress operation and refuse the attempt to schedule another.

nordic-krch · 2020-06-17T11:35:39Z

include/sys/queued_operation.h

what about adding helper function like

int queued_operation_sync_submit(struct queued_operation_manager *mgr, struct queued_operation *op, int priority);

which from thread context would schedule operation and pend for completion. That would safe complexity of setting up sys_notify. As i have some work based on this branch I've added there something like this which is relying on POLL being enabled (so semaphore version would need to be added as well):

int queued_operation_sync_submit(struct queued_operation_manager *qop_mgr, struct queued_operation *qop, int priority) { int err; int ret; struct k_poll_signal signal = K_POLL_SIGNAL_INITIALIZER(signal); struct k_poll_event event = K_POLL_EVENT_INITIALIZER(K_POLL_TYPE_SIGNAL, K_POLL_MODE_NOTIFY_ONLY, &signal); sys_notify_init_signal(&qop->notify, &signal); err = queued_operation_submit(qop_mgr, qop, priority); if (err < 0) { return err; } err = k_poll(&event, 1, K_FOREVER); if (err < 0) { return err; } err = sys_notify_fetch_result(&qop->notify, &ret); if (err < 0) { return err; } return ret; }

Like the other resource management APIs this really isn't intended for direct use by application code. At a minimum there should be a service-specific API that can do type checking on the operation before it's submitted. In the reference code in the test that is service_submit(). I would expect there to be a companion service_submit_sync() to provide this feature.

I don't think lifting a generic submit_sync API into the manager is worthwhile at this time. It's possible that may change in the future, but locking it down when there are no in-tree users of this functionality is premature.

pabigot · 2020-06-23T12:44:55Z

Marked DNM until we have a PR that provides a clear reason for adding this to the infrastructure.

nordic-krch · 2020-06-23T13:50:05Z

Marked DNM until we have a PR that provides a clear reason for adding this to the infrastructure.

what about #23371 ?

Text failed to distinguish between success of initiating a reset and success of the reset. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

Full asynchronous support for APIs such as bus transactions generally requires managing operations from unrelated clients. This API provides a data structure and functions to manage those operations generically, leaving the service to provide only the service-specific operation description and implementation. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

github-actions · 2020-11-28T01:03:33Z

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.

pabigot requested review from MaureenHelm, andrewboie, andyross, carlescufi, galak and nashif as code owners April 28, 2020 19:39

pabigot requested a review from nordic-krch April 28, 2020 19:40

zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Documentation labels Apr 28, 2020

pabigot force-pushed the nordic/20200421a branch from 914c462 to 759458b Compare April 30, 2020 17:43

nordic-krch mentioned this pull request May 4, 2020

[RFC] Device: generalizing and improving control features (sync, lock and timeout) #24511

Closed

nordic-krch reviewed May 4, 2020

View reviewed changes

pabigot force-pushed the nordic/20200421a branch from 759458b to cf3d57e Compare May 4, 2020 15:24

pabigot commented May 4, 2020

View reviewed changes

pabigot mentioned this pull request May 12, 2020

sys: onoff: redesign to meet changed needs #23898

Merged

pabigot mentioned this pull request May 27, 2020

device_pm: clarify and document usage #24653

Closed

pabigot force-pushed the nordic/20200421a branch from cf3d57e to 71c2fa8 Compare June 8, 2020 13:45

nordic-krch reviewed Jun 17, 2020

View reviewed changes

carlescufi requested review from henrikbrixandersen, mnkp and tbursztyka June 17, 2020 12:17

pabigot added the DNM This PR should not be merged (Do Not Merge) label Jun 23, 2020

pabigot mentioned this pull request Jun 30, 2020

[DNM] [RFC] drivers: i2c: Add i2c_async for asynchronous transfers #23371

Closed

pabigot force-pushed the nordic/20200421a branch from 71c2fa8 to 1e28a78 Compare July 20, 2020 13:11

pabigot force-pushed the nordic/20200421a branch from 1e28a78 to 6ba1809 Compare September 2, 2020 12:10

pabigot force-pushed the nordic/20200421a branch from 6ba1809 to 5d28f36 Compare September 3, 2020 10:22

pabigot added 2 commits September 28, 2020 05:26

sys: onoff: clarify return value interpretation

714f4ea

Text failed to distinguish between success of initiating a reset and success of the reset. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

pabigot force-pushed the nordic/20200421a branch from 5d28f36 to 8bfd256 Compare September 28, 2020 10:26

github-actions bot added the Stale label Nov 28, 2020

github-actions bot closed this Dec 12, 2020

lib: queued_operation: add API for managing a queue of operations #24781

lib: queued_operation: add API for managing a queue of operations #24781

Uh oh!

Conversation

pabigot commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zephyrbot commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

checkpatch (informational only, not a failure)

Uh oh!

pabigot commented Apr 30, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabigot May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

pabigot May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabigot commented Jun 23, 2020

Uh oh!

nordic-krch commented Jun 23, 2020

Uh oh!

github-actions bot commented Nov 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pabigot commented Apr 28, 2020 •

edited

Loading

zephyrbot commented Apr 28, 2020 •

edited

Loading

pabigot May 4, 2020 •

edited

Loading

pabigot May 4, 2020 •

edited

Loading