refactor onoff to separate async notification and standardize terminology #23601

pabigot · 2020-03-19T12:59:21Z

This PR separates generic resource management updates that appear to be nearly stable from #23229. The changes are:

refactors the onoff service to reduce space by putting static configuration data into a separate structure;
extracts the async notification infrastructure from the on-off service into a self-contained structure that can be used anywhere asynchronous operations may be used;
recasts the on-off service as a manager that is to be used by services (including API changes to make it more consistent with other managers to come);

zephyrbot · 2020-03-19T13:09:08Z

All checks passed.

checkpatch (informational only, not a failure)

-:480: WARNING:LONG_LINE: line over 80 characters
#480: FILE: include/sys/notify.h:324:
+					      sys_notify_generic_callback handler)

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

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

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

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

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

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

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

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

- total: 0 errors, 9 warnings, 2620 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.

nordic-krch

few comments.

include/sys/async_notify.h

tests/lib/async_notify/src/main.c

lib/os/onoff.c

pabigot · 2020-03-19T14:37:07Z

To proactively address some comments from earlier reviews for which I have not made changes:

Yes, struct async_notify records the result of the operation in a common field rather than in the union where it would be available only for spin-wait notifications. An argument can be made that this is unnecessary, since for signal the result is stored in struct k_poll_signal and for callback it's passed as an argument, thus only spin-wait needs it. Reasons for retaining the existing approach include:
- Consistency: the result of the operation can always be obtained using async_notify_fetch_result() regardless of the notification method used;
- Flexibility: An application may multiplex notifications onto one k_poll_signal object (e.g. fire off a dozen and wake up when the first one completes; tricky but not inconceivable), or may not process the operation completion in the callback (in which case the result needs to be persisted somewhere, most likely on the client data that contains the notification result). If the result is only retained for spinwait notifications the applications have to do extra work.
Yes, the names are long. They're also unambiguous and unlikely to conflict with future API. If there is wide consensus that a specific alternative prefix is preferred I'll change them. (anotify_ instead of async_notify_ has been proposed, but "async" means "not sync(hronous)" so "anotify" means "not notified"? Meh.)

include/sys/async_notify.h

nordic-krch

Could you fix 80liners checkpatch complains? Regarding result field in the async notify. Let's keep it for now as it is. I don't see clearly the use case that needs it so it's hard for me to say if there are other alternatives. If use case is specific to a service then first that comes to my mind is to implement that in the service, in completion callback (before calling async_notify_finalize) result can be stored in dedicated service struct which contains async notification. This way, it would be supported only for services which implements that but on the other hand it will save 4 bytes for each async notify which does not need it.

pabigot · 2020-03-20T11:26:39Z

Could you fix 80liners checkpatch complains?

I think I got all but the ones where there's no obvious place to break a line.

nordic-krch · 2020-03-20T11:58:18Z

@tbursztyka @anangl if any of you could review that? @carlescufi maybe you know someone that would have interest in reviewing that?

carlescufi · 2020-03-23T16:59:52Z

include/sys/async_notify.h

+ * Flag value that overwrites the method field when the operation has
+ * completed.
+ */
+#define ASYNC_NOTIFY_METHOD_COMPLETED 0


Again, I disagree with these long names. I really would like to shorten this to something like ANOTIFY_ and anotify_ or something even shorter, if we can come up with one. I will add this to the API meeting.

The whole namespace would need to change. I don't know if we need to keep async in the name, notify_* only perhaps?

in a way, by default notification is asynchronous in nature so async_ prefix might be redundant.
Some other proposals :

notifier

gnotify - generic notification

k_notify - since (a least currently) it is limited to kernel.

gnotify sounds like glib, your are right k_ is strictly used by the kernel only. z_notify maybe? Though it does not "sound" nice.

yeah, i thought about z_notify but z_ indicates internal function.

I suppose that async_ doesn't add a huge amount of information except that it's intended to be the standard way of notifying of completion of async operations.

Bare notify_ IMO has too large of a conflict with existing and new identifiers that are not part of this API (struct notify_data, notify_remote_info).

Since this is in the sys/ include hierarchy sys_notify_ could mitigate that.

API meeting:
sys_notify_ raised no objections.

how to we call the file? notify.h? since path contains the namespace (sys/notify.h)?

yes, as all the other files there. (besides sys_io.h which is a legacy name, and was not part of sys/ at first if I remember well)

carlescufi · 2020-03-23T17:00:53Z

@tbursztyka @anangl if any of you could review that? @carlescufi maybe you know someone that would have interest in reviewing that?

No, unfortunately beyond @tbursztyka and @anangl I can't think of anyone else at this point. Perhaps @wentongwu or @pdunaj?

tbursztyka · 2020-03-23T17:41:02Z

lib/os/onoff.c

-#define SERVICE_STATE_TRANSITION (ONOFF_SERVICE_INTERNAL_BASE << 1)
-#define SERVICE_STATE_TO_ON (SERVICE_STATE_TRANSITION | SERVICE_STATE_ON)
-#define SERVICE_STATE_TO_OFF (SERVICE_STATE_TRANSITION | SERVICE_STATE_OFF)
+#define ST_OFF 0


about naming, I don't know if going to the shortest possible one is relevant (it becomes cryptic then). You'll need to find something in the middle.

tbursztyka · 2020-03-23T17:42:30Z

besides naming issues, lgtm

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>

doc/reference/misc/notify.rst

nordic-krch · 2020-03-31T06:23:36Z

@carlescufi @tbursztyka naming has been resolved can you look again?

jukkar

Some random notes

jukkar · 2020-04-01T10:37:55Z

include/sys/notify.h

+ * @{
+ */
+
+/* Forward declaration */


This comment could be removed, it just states the obvious.

jukkar · 2020-04-01T10:38:37Z

include/sys/notify.h

+/* Forward declaration */
+struct sys_notify;
+
+/*


This looks like a comment that should be in the documentation, so we should put /** here.
Same comment to other defines below.
Could we replace these defines by enum as they seem to be sequential and related to each other?

It's not public API, but maintainers need to know its intent so it's described. It's also the value of a bitfield in flags so it shouldn't be an enum.

jukkar · 2020-04-01T10:42:08Z

include/sys/notify.h

+ */
+#define SYS_NOTIFY_METHOD_CALLBACK 3
+
+#define SYS_NOTIFY_METHOD_MASK 0x03


If we do not want to document these, they should probably be outside of doxygen group or marked as internal using /** @cond INTERNAL_HIDDEN */

jukkar · 2020-04-01T10:47:18Z

include/sys/notify.h

+ */
+struct sys_notify {
+	union method {
+		/* Pointer to signal used to notify client.


Documentation comment here /**

It's not public API (it's manipulated through accessor functions). And documentation for struct members isn't generated so it wouldn't help.

jukkar · 2020-04-01T10:47:27Z

include/sys/notify.h

+		sys_notify_generic_callback callback;
+	} method;
+
+	/*


jukkar · 2020-04-01T10:47:39Z

include/sys/notify.h

+	 */
+	u32_t volatile flags;
+
+	/*


and here too

jukkar · 2020-04-01T10:48:45Z

include/sys/notify.h

+	int volatile result;
+};
+
+/** @internal */


In other parts of the code, I have used

/** @cond INTERNAL_HIDDEN */ ... /** @endcond */

not sure which is correct

I'm not sure either work, but @internal is common.

jukkar · 2020-04-01T10:50:37Z

include/sys/notify.h

+		rv = 0;
+		*result = notify->result;
+	}
+	return rv;


empty line after }

jukkar · 2020-04-01T10:53:36Z

include/sys/notify.h

+ * @param handler a function pointer to use for notification.
+ */
+static inline void sys_notify_init_callback(struct sys_notify *notify,
+					      sys_notify_generic_callback handler)


This looks like too long line, in this case I would just make sure the line is max 80 chars long even if the indentation would not match. Other option is just to put the 1st parameter to new line.

Generally we don't do that sort of thing just to satisfy the line length test (which is advisory in Zephyr).

jukkar · 2020-04-01T10:56:38Z

tests/lib/notify/testcase.yaml

@@ -0,0 +1,3 @@
+tests:
+  libraries.sys_notify:
+    tags: sys_notify


You could also add here notify tag (easier to remember)

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>

jukkar

LGTM

carlescufi · 2020-04-06T14:41:37Z

besides naming issues, lgtm

Will merge since the naming has been resolved

The following PR's zephyrproject-rtos#23941 zephyrproject-rtos#23601 was merged using old boilerplate inclusion. This commit updates those tests to use find_package(Zephyr) Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>

The following PR's #23941 #23601 was merged using old boilerplate inclusion. This commit updates those tests to use find_package(Zephyr) Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>

The following PR's zephyrproject-rtos#23941 zephyrproject-rtos#23601 was merged using old boilerplate inclusion. This commit updates those tests to use find_package(Zephyr) Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>

pabigot requested review from andrewboie, andyross, carlescufi, galak, MaureenHelm and nashif as code owners March 19, 2020 12:59

pabigot requested review from nordic-krch and tbursztyka March 19, 2020 12:59

pabigot added the area: API Changes to public APIs label Mar 19, 2020

pabigot mentioned this pull request Mar 19, 2020

refactor onoff to separate async notification, standardize terminology, and add functionality #23229

Closed

zephyrbot added area: Tests Issues related to a particular existing or missing test area: Documentation labels Mar 19, 2020

nordic-krch reviewed Mar 19, 2020

View reviewed changes

include/sys/async_notify.h Outdated Show resolved Hide resolved

tests/lib/async_notify/src/main.c Outdated Show resolved Hide resolved

lib/os/onoff.c Outdated Show resolved Hide resolved

pabigot force-pushed the nordic/20200319a branch from 97d2a33 to c883cf5 Compare March 19, 2020 14:54

pabigot commented Mar 19, 2020

View reviewed changes

include/sys/async_notify.h Outdated Show resolved Hide resolved

nordic-krch approved these changes Mar 20, 2020

View reviewed changes

pabigot force-pushed the nordic/20200319a branch from c883cf5 to cf4135d Compare March 20, 2020 11:25

carlescufi requested changes Mar 23, 2020

View reviewed changes

pdunaj self-requested a review March 23, 2020 17:03

tbursztyka reviewed Mar 23, 2020

View reviewed changes

pabigot force-pushed the nordic/20200319a branch 2 times, most recently from 86ee33c to 2474cba Compare March 27, 2020 12:58

sys: onoff: Move transition functions out of service struct

8539666

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 force-pushed the nordic/20200319a branch from 2474cba to c8a3e43 Compare March 29, 2020 16:01

pabigot mentioned this pull request Mar 29, 2020

sys: onoff: redesign to meet changed needs #23898

Merged

nordic-krch reviewed Mar 30, 2020

View reviewed changes

doc/reference/misc/notify.rst Show resolved Hide resolved

carlescufi approved these changes Mar 31, 2020

View reviewed changes

jukkar reviewed Apr 1, 2020

View reviewed changes

pabigot added 2 commits April 1, 2020 07:35

pabigot force-pushed the nordic/20200319a branch from c8a3e43 to 8a96ade Compare April 1, 2020 12:37

jukkar approved these changes Apr 6, 2020

View reviewed changes

carlescufi merged commit 8bd676e into zephyrproject-rtos:master Apr 6, 2020

pabigot deleted the nordic/20200319a branch April 7, 2020 13:08

tejlmand mentioned this pull request Apr 16, 2020

cmake: Updating test samples to use find_package(Zephyr) #24427

Merged

kartben mentioned this pull request Aug 20, 2023

onoff service: Extract transition functions to dedicated structure. #22420

Closed

refactor onoff to separate async notification and standardize terminology #23601

refactor onoff to separate async notification and standardize terminology #23601

Conversation

pabigot commented Mar 19, 2020

zephyrbot commented Mar 19, 2020 • edited Loading

checkpatch (informational only, not a failure)

nordic-krch left a comment • edited Loading

Choose a reason for hiding this comment

pabigot commented Mar 19, 2020 • edited Loading

nordic-krch left a comment

Choose a reason for hiding this comment

pabigot commented Mar 20, 2020

nordic-krch commented Mar 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlescufi commented Mar 23, 2020

Choose a reason for hiding this comment

tbursztyka commented Mar 23, 2020

nordic-krch commented Mar 31, 2020

jukkar 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

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

jukkar left a comment

Choose a reason for hiding this comment

carlescufi commented Apr 6, 2020

zephyrbot commented Mar 19, 2020 •

edited

Loading

nordic-krch left a comment •

edited

Loading

pabigot commented Mar 19, 2020 •

edited

Loading