sys: onoff: redesign to meet changed needs #23898

pabigot · 2020-03-29T17:00:28Z

The previous architecture proved unable to support user expectations, so the API has been rebuilt from first principles. Backward compatibility cannot be maintained for this change.

Key changes include:

Formerly the service-provided transition functions were allowed to sleep, and the manager took care to not invoke them from ISR context, instead returning an error if unable to initiate a transition. In the new architecture transition functions are required to work regardless of calling context: it is the service's responsibility to guarantee the transition will proceed even if it needs to be transferred to a thread. This eliminates state machine complexities related to calling context.
Constants identifying the visible state of the manager are exposed to clients through both notification callbacks and a new monitor API that allows clients to be notified of all state changes. Formerly the release operation was async, and would be delayed for the last release to ensure a client would exist to be notified of any failures. It is now synchronous.
Formerly the cancel operation would fail on the last client associated with a transition. The cancel operation is now synchronous. A helper function is provided to safely synchronously release a request regardless of whether it has completed or is in progress, satisfying the use case underlying Add cancel function to onoff service #22974.
The user-data parameter to asynchronous notification callbacks has been removed as user data can be retrieved from the CONTAINER_OF the client data.

This branch is based on #23601 and replaces #23229.

~~Draft until #23601 is merged and the patch that removes diagnostic code has been squashed into this commit.~~

zephyrbot · 2020-03-29T17:03:02Z

All checks are passing now.

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

nordic-krch · 2020-03-30T09:01:39Z

doc/reference/resource_management/index.rst

-  be started the function will result in an error.
+  error state to off.  Transitions must be invokable from both thread
+  and interrupt context.
+* The request and reset operations are asynchronous using sys_notify.


using sys_notify. Could you make this a link to notify documentation?

nordic-krch

I have just few minor comments. Implementation looks good.

nordic-krch · 2020-03-30T09:26:17Z

include/sys/onoff.h

+ * the request was processed, if successful.
+ * @retval -EIO if service has recorded an an error.
+ * @retval -EINVAL if the parameters are invalid.
+ * @retval -EAGAIN if the reference count would overflow.
 * @retval -EWOULDBLOCK if the function was invoked from non-thread


it's no longer valid.

nordic-krch · 2020-03-30T09:27:04Z

include/sys/onoff.h

@@ -456,77 +335,47 @@ static inline void onoff_client_init_callback(struct onoff_client *cli,
 * transition to on can sleep, the transition cannot be started and


this paragraph is no longer valid, right?

nordic-krch · 2020-03-30T09:31:56Z

include/sys/onoff.h

 * @retval -EWOULDBLOCK if the function was invoked from non-thread
 * context and successful initiation could result in an attempt to
 * make the calling thread sleep.
 */
-int onoff_request(struct onoff_service *srv,
+int onoff_request(struct onoff_manager *mgr,
 		  struct onoff_client *cli);

 /**
 * @brief Release a reserved use of an on-off service.
 *


maybe it's worth adding a comment here that releasing may trigger stop operation on service and that result of that operation will be passed to the monitor.

nordic-krch · 2020-03-30T09:35:38Z

include/sys/onoff.h

- * transition there will always be at least one client that will be
- * notified when the operation completes.
+ * Cancelling is supported only for onoff_request() and onoff_reset()
+ * operations, and is a synchronous operation.  Be aware that any


It would be good to add explicit information that ownership of the client structure returns to the client when function returns.

nordic-krch · 2020-03-30T13:03:57Z

tests/lib/onoff/src/main.c

+	u32_t state;
+	int res;
+};
+static struct transition_record xions[32];


can you somewhere comment on what xion stands for? transitions, monitors?

nordic-krch · 2020-03-30T13:08:00Z

include/sys/onoff.h

+ * @retval non-negative the observed state (ONOFF_STATE_ON) of the
+ * machine at the time of the release, if the release succeeds.
+ * @retval -EIO if service has recorded an an error.
+ * @retval -ENOTSUP if the machine is not in a state that permits release.


imo -EALREADY would be better. Similar to cancelation: -EALREADY if cli was not a record of an uncompleted notification

I think that would be misleading. It's not that this specific client has already released (where -EALREADY would be appropriate), it's that the machine is in a state where no client can be released: off, transitioning to off, transitioning to on, resetting. In all those cases invoking this is a true error, not a redundant operation.

All other comments have been addressed.

The previous architecture proved unable to support user expectations, so the API has been rebuilt from first principles. Backward compatibility cannot be maintained for this change. Key changes include: * Formerly the service-provided transition functions were allowed to sleep, and the manager took care to not invoke them from ISR context, instead returning an error if unable to initiate a transition. In the new architecture transition functions are required to work regardless of calling context: it is the service's responsibility to guarantee the transition will proceed even if it needs to be transferred to a thread. This eliminates state machine complexities related to calling context. * Constants identifying the visible state of the manager are exposed to clients through both notification callbacks and a new monitor API that allows clients to be notified of all state changes. * Formerly the release operation was async, and would be delayed for the last release to ensure a client would exist to be notified of any failures. It is now synchronous. * Formerly the cancel operation would fail on the last client associated with a transition. The cancel operation is now synchronous. * A helper function is provided to safely synchronously release a request regardless of whether it has completed or is in progress, satisfying the use case underlying zephyrproject-rtos#22974. * The user-data parameter to asynchronous notification callbacks has been removed as user data can be retrieved from the CONTAINER_OF the client data. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>

carlescufi · 2020-04-08T12:05:39Z

@tbursztyka and @jukkar if you have time to review this it would be appreciated.

nordic-krch · 2020-04-14T12:42:10Z

lib/os/onoff.c

+		__ASSERT_NO_MSG((state == ONOFF_STATE_TO_ON)
+				|| (state == ONOFF_STATE_TO_OFF)
+				|| (state == ONOFF_STATE_RESETTING));
+		rv = state;


what about cleaning client state here. Otherwise sys_notify_fetch_result will return -EAGAIN.

Yes, it will. Why is that wrong? In an earlier review you asked that cancel not record a completion because the natural value for it -ECANCELED might conflict with a legitimate result. So the client data is left in an imcomplete state, for which no result is available.

ok, lets leave it. I was thinking about a way to determine who is currently owning the client and fetch result could be used if state was cleaned here and in cancel_or_release.

nordic-krch · 2020-04-14T12:44:11Z

include/sys/onoff.h

+	int rv = onoff_cancel(mgr, cli);
+
+	if (rv == -EALREADY) {
+		rv = onoff_release(mgr);


client state shall be cleaned here. I see that we are missing a call in sys_notify for that. Something like sys_notify_reset.

See comment above: there is no notion of cleaning client state in this infrastructure. The client is responsible for initializing it before hand-off to the manager, and for interpreting whatever the manager set when it was returned. The only thing the manager does is set completion state, if the operation completes before the client data is returned.

jukkar · 2020-04-14T14:02:19Z

lib/os/onoff.c

@@ -40,7 +129,8 @@ static int validate_args(const struct onoff_manager *mgr,
 	int rv = sys_notify_validate(&cli->notify);


While browsing the onoff.c, I noticed that the code does not follow the coding guideline but introduces variables in the middle of the blocks. Would it be possible to fix these?

Sorry to be obtuse; can you point me to the relevant guideline?

Mid-block declarations have been legal in C for twenty years, in various discussions I remember consensus that C99 was assumed to be the minimum supported version. Though I don't think anybody documented that.

Of course compiler supports this just fine. We are following linux coding style in zephyr.

I don't see anything about this in https://www.kernel.org/doc/html/latest/process/coding-style.html

There's good reason to define the variable at the point where you can assign it an initial value, which is what I've done. So while it would be possible to "fix" these, I don't plan to do so. You may block the PR if you feel strongly enough.

Personally I wish we could get some traction on #21392 so we could stop having these arguments and instead have an objective standard for acceptability.

We already rely on too many C99 (and GNU99) features to deny its existence and effectiveness, like some cave-residing individuals do.

I also believe mid-function variable declaration, when it makes sense, is helpful and should even be preferred if it results in a tidier/easier-to-follow code.

In fact, mid-function beginning-of-scope variable declarations are widely preferred in the Linux kernel, although that is arguably different to what is being done here.

wentongwu · 2020-05-12T13:52:10Z

@pabigot how to use this infrastructure? for i2c example, there are app thread, slave, master, do we need a manager for slave because it provides service for app and need another manager for master because it provides service for slave?

pabigot · 2020-05-12T14:40:14Z

The expectation is that there is a single manager for the device, and anything that wants to use the device puts in a reservation that's keeps the device until the dependency has been released.

It's unclear whether this structure would be externally visible; it may be that reservations are handled with a power management API, or by a queued operation manager #24781 that ensures the device doesn't shut down as long as there's pending work, even if other pending work is currently using the device.

wentongwu · 2020-05-13T02:22:34Z

@pabigot so the dependency will be built when anyone request service with onoff_request, not from DT. And the PR #24781 is a good idea and implementation, so that means every device driver whose traffic is asynchronous way will follow this model?

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

pabigot force-pushed the nordic/20200319b branch from de86202 to a735ac5 Compare March 29, 2020 17:08

pabigot requested a review from nordic-krch March 29, 2020 17:11

pabigot force-pushed the nordic/20200319b branch from a735ac5 to dcf14a2 Compare March 30, 2020 00:26

nordic-krch reviewed Mar 30, 2020

View reviewed changes

pabigot force-pushed the nordic/20200319b branch from dcf14a2 to 751225e Compare March 30, 2020 15:18

pabigot force-pushed the nordic/20200319b branch from 751225e to fae646a Compare April 6, 2020 14:50

pabigot marked this pull request as ready for review April 6, 2020 16:27

pabigot requested review from andrewboie, andyross, carlescufi, galak, MaureenHelm and nashif as code owners April 6, 2020 16:27

carlescufi requested review from tbursztyka and jukkar April 6, 2020 16:33

pabigot mentioned this pull request Apr 10, 2020

device power management irq lock #24230

Open

wentongwu self-requested a review April 13, 2020 06:00

nordic-krch mentioned this pull request Apr 14, 2020

drivers: clock_control: nrf: Use onoff service #24334

Merged

nordic-krch reviewed Apr 14, 2020

View reviewed changes

jukkar reviewed Apr 14, 2020

View reviewed changes

nordic-krch approved these changes Apr 15, 2020

View reviewed changes

carlescufi approved these changes Apr 22, 2020

View reviewed changes

carlescufi merged commit 14e2ca4 into zephyrproject-rtos:master Apr 22, 2020

pabigot deleted the nordic/20200319b branch April 27, 2020 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys: onoff: redesign to meet changed needs #23898

sys: onoff: redesign to meet changed needs #23898

pabigot commented Mar 29, 2020 •

edited

Loading

zephyrbot commented Mar 29, 2020 •

edited

Loading

nordic-krch Mar 30, 2020

nordic-krch left a comment

nordic-krch Mar 30, 2020

nordic-krch Mar 30, 2020

nordic-krch Mar 30, 2020

nordic-krch Mar 30, 2020

nordic-krch Mar 30, 2020

nordic-krch Mar 30, 2020

pabigot Mar 30, 2020

pabigot Mar 30, 2020

carlescufi commented Apr 8, 2020

nordic-krch Apr 14, 2020

pabigot Apr 14, 2020

nordic-krch Apr 15, 2020

nordic-krch Apr 14, 2020

pabigot Apr 14, 2020

jukkar Apr 14, 2020

pabigot Apr 14, 2020 •

edited

Loading

jukkar Apr 14, 2020

pabigot Apr 14, 2020

stephanosio Apr 14, 2020

wentongwu commented May 12, 2020

pabigot commented May 12, 2020

wentongwu commented May 13, 2020

		@@ -456,77 +335,47 @@ static inline void onoff_client_init_callback(struct onoff_client *cli,
		* transition to on can sleep, the transition cannot be started and

		@@ -40,7 +129,8 @@ static int validate_args(const struct onoff_manager *mgr,
		int rv = sys_notify_validate(&cli->notify);

sys: onoff: redesign to meet changed needs #23898

sys: onoff: redesign to meet changed needs #23898

Conversation

pabigot commented Mar 29, 2020 • edited Loading

zephyrbot commented Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

nordic-krch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Apr 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pabigot Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wentongwu commented May 12, 2020

pabigot commented May 12, 2020

wentongwu commented May 13, 2020

pabigot commented Mar 29, 2020 •

edited

Loading

zephyrbot commented Mar 29, 2020 •

edited

Loading

pabigot Apr 14, 2020 •

edited

Loading