Skip to content

Conversation

@pabigot
Copy link
Contributor

@pabigot pabigot commented Mar 8, 2020

This PR supersedes #22853, providing a manager abstraction for a prioritized sequence of exclusive transactions on a service. It is based on #23229. Only the top few commits are specific to this PR; please comment on the onoff manager and async notification service changes/additions in #23229.

There is likely little point in spending time reviewing this until at least the initial commits of #23229 have enough approval to warrant a real PR.

@pabigot pabigot requested a review from nordic-krch March 8, 2020 14:49
@zephyrbot zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Documentation labels Mar 8, 2020
@zephyrbot
Copy link

zephyrbot commented Mar 8, 2020

All checks are passing now.

checkpatch (informational only, not a failure)

-:705: WARNING:LONG_LINE: line over 80 characters
#705: FILE: include/sys/async_notify.h:324:
+					      async_notify_generic_callback handler)

-:873: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#873: FILE: include/sys/onoff.h:95:
+#define ONOFF_SERVICE_START_SLEEPS __DEPRECATED_MACRO ONOFF_START_SLEEPS

-:874: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#874: FILE: include/sys/onoff.h:96:
+#define ONOFF_SERVICE_STOP_SLEEPS __DEPRECATED_MACRO ONOFF_STOP_SLEEPS

-:875: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#875: FILE: include/sys/onoff.h:97:
+#define ONOFF_SERVICE_RESET_SLEEPS __DEPRECATED_MACRO ONOFF_RESET_SLEEPS

-:876: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#876: FILE: include/sys/onoff.h:98:
+#define ONOFF_SERVICE_HAS_ERROR __DEPRECATED_MACRO ONOFF_HAS_ERROR

-:877: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#877: FILE: include/sys/onoff.h:99:
+#define ONOFF_SERVICE_INTERNAL_BASE __DEPRECATED_MACRO ONOFF_INTERNAL_BASE

-:1013: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#1013: FILE: include/sys/onoff.h:214:
+#define ONOFF_SERVICE_TRANSITIONS_INITIALIZER(_start, _stop, _reset, _flags) \
+	__DEPRECATED_MACRO						     \
+	ONOFF_TRANSISTIONS_INITIALIZER(_start, _stop, _reset, _flags)

-:1029: WARNING:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#1029: FILE: include/sys/onoff.h:225:
+#define ONOFF_SERVICE_INITIALIZER(_transitions)	\
+	__DEPRECATED_MACRO			\
+	ONOFF_MANAGER_INITIALIZER(_transitions)

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

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

-:3838: WARNING:EMBEDDED_FUNCTION_NAME: Prefer using '"%s...", __func__' to using 'callback', this function's name, in a string
#3838: FILE: tests/lib/async_notify/src/main.c:28:
+		      "failed callback fetch");

- total: 0 errors, 11 warnings, 6672 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 PRINTK_WITHOUT_KERN_LEVEL 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.

Extracted transition functions from onoff structure to external one
which allows to keep them in flash.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@pabigot pabigot changed the title add queued operation manager aggregate branch for resource manager updates Mar 18, 2020
@pabigot pabigot changed the title aggregate branch for resource manager updates add queued operation manager Mar 18, 2020
@pabigot pabigot force-pushed the nordic/20200303c branch 2 times, most recently from f3ab85f to 76764a0 Compare March 18, 2020 21:21
@pabigot pabigot force-pushed the nordic/20200303c branch 3 times, most recently from 7bc59ec to df617cc Compare March 20, 2020 11:25
pabigot added 8 commits March 27, 2020 07:56
k_poll() for a signal is often desired for notification of completion
of asynchronous operations, but there are APIs where it may be
necessary to invoke "asynchronous" operations from contexts where
sleep is disallowed, or before the kernel has been initialized.
Extract the general notification solution from the on-off service into
a utility that can be used for other APIs.

Also move documentation out to a resource management section.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
The original API was misnamed, as the intent was to provide a manager
that decoupled state management from the service that needed to be
turned on or off.  Update all the names, shortening them where
appropriate removing unncessary internal components like _service.

Also remove some API that misled developers into believing that onoff
managers are normally expected to be exposed directly to consumers.
While this is a use case, in most situations there are service or
client-specific actions that need to be coupled to transition events.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Upcoming changes will inform the clients of the state of the system at
points where it changes.  Make the state-related bits visible to
enable the clients to determine the current state.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Upcoming revision of notifications for cancelled operations includes
the possibility that a client originally submitted for a request
operation will instead receive a notification for a completed release
operation.  Although this doesn't solve the problem for spinwait or
signal notifications, add the state-at-completion as a parameter to
the callback notification signature.

Since we're changing things, move the user data parameter to the last
argument, which is its position in most other callback APIs.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
An original design goal of the onoff infrastructure was to ensure that
any errors discovered during transitions would be communicated to at
least one client.  To meet this goal cancellation of the last client
associated with an in-progress transition must be rejected.

This ends up significantly complicating the logic associated with
operation cancellation, and impacted clients that were forced to
maintain state to issue a release or request when the in-progress
transition completed.

Rework transition management so that both request and release
transitions can be accomodated at the same time.  Leave a cancelled
request notification uncompleted, as the client can tell by the
cancellation return value that it has received control of the client
object.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
It's possible to have a transition complete with no clients available
to receive the result, which may be a notification of error.
Alternatively a notification that was originally associated with a
release may be converted to a notification associated with a request
due the cancellation of the release.  There is no way to detect this
from spinwait or signal notifications.

Also a module may want to centralize tracking of current state and
error handling for an onoff service that gets used from multiple
contexts.

In support of this provide API to monitor state changes within an
onoff service.  Include notifications when the service transitions out
of stable (ON or OFF) states as well as detection of errors.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
When functionality that requires an onoff service is affected by
external asynchronous actions, such as plugging and unplugging of
connections, the management of the current state of an asynchronous
service is too complex to handle in individual clients.  Provide an
intermediary that can handle an arbitrary sequence of interleaved
requests and releases, tracking the most recent desired state and
notifying the client only when the state has been reached.

DNM until API and behavior has stabilized to the point where the
depth tracking and PRINTK trace facility can be removed.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Full asynchronous support for APIs such as bus transactions generally
require 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>
@pabigot
Copy link
Contributor Author

pabigot commented Mar 29, 2020

onoff solution has been completedly redesigned, this branch is not viable.

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: Tests Issues related to a particular existing or missing test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants