Plugin API redesign v2.1 #316

algernon · 2018-05-04T13:43:33Z

This is a work in progress, building on top of #276.

noseglasses · 2018-05-04T13:57:55Z

doc/plugin-api-internals.md

+Which in turn becomes:
+
+```c++
+static auto apply(Key &mappedKey, byte row, byte col, uint8_t keyState) {


static auto apply(Key &mappedKey, byte row, byte col, uint8_t keyState) {

should be

static bool apply(Key &mappedKey, byte row, byte col, uint8_t keyState) {

to be valid C++11. About the actual return value, see the comment below.

noseglasses · 2018-05-04T13:58:31Z

doc/plugin-api-internals.md

+
+```c++
+static auto apply(Key &mappedKey, byte row, byte col, uint8_t keyState) {
+  ContinueIfHookReturnsTrue hook_return_val;


ContinueIfHookReturnsTrue is actually a class, better replace it with bool here.

Most precise would be

decltype(ContinueIfHookReturnsTrue::eval(true)) hook_return_val;

but that's of no use here.

noseglasses · 2018-05-04T14:03:06Z

doc/plugin-api-internals.md

+we end up with:
+
+```c++
+static auto apply(Key &mappedKey, byte row, byte col, uint8_t keyState) {


Same here

static auto apply(Key &mappedKey, byte row, byte col, uint8_t keyState) {

algernon · 2018-05-05T05:02:40Z

We talked with @obra at length last night, about naming things, and came up with the following:

OrderedPlugins would become HookPoint, because it offers a way to call a collection of plugin methods for a particular hook point.
HookTask_* would become pluginMethod_*, because it wraps that.
We'd invert ContinuationPredicate (because continue is the default), and rename it to ShouldAbort, adjusting the code as necessary. Inverting, because continue should be the default, rename because "predicate" is a great word, but ShouldAbort describes the same intent in easier to understand words.
We'd also rename HookTask_*::invoke to pluginMethod_*::call, because we felt it sounded better.
Hooks would also get new names, to clean up the mess we had before. They'll follow the same pattern, and better describe when they're called. We're also getting rid of "Hook" in their name.
- eventHandlerHook becomes onEvent.
- preReportHook becomes beforeReportingState
- postReportHook becomes afterEachCycle
- init becomes onSetup
- we'd introduce beforeEachCycle
- A separate PR will introduce beforeGatheringEvents, afterGatheringEvents, afterReportingState

noseglasses · 2018-05-05T07:23:13Z

Nice renaming.

Wouldn't it be good to stick with the convention that class names are uppercase and go with PluginMethod_* instead?

algernon · 2018-05-05T13:32:53Z

It would be, and we went with PluginMethod_ indeed. That was the original plan too, I just typoed previously O:)

algernon · 2018-05-05T13:33:46Z

I think the code is now ready for review. I still have to clean up the history a little bit, but that shouldn't change the end result.

obra · 2018-05-05T22:13:48Z

14:57 <@obra> Looking at the amount of code we have for dealing with returntypes makes me really want to enforce the same return type on everything. I'm not saying that we Must do that, but it feels like we
should.
14:58 <@algernon> I wouldn't be opposed to that, if we make that return type an enum
14:58 <@algernon> that's ~extensible
14:58 <@algernon> also makes plugin code easier to understand
14:59 <@algernon> right now, you need to know that onEvent should return true to continue, false to abort
14:59 <@obra> The current values would be SUCCESS, DID_NOT_RUN, FAILURE?
14:59 <@algernon> for someone reading onEvent, without prior knowledge of how onEvents are chained, will be rightly confused
15:00 <@obra> though those don't actually say whether we would want to stop processing
15:00 <@algernon> right now, only onEvent uses the return value, and there the values would be CONTINUE and ABORT
15:00 <@algernon> or perhaps NEXT/EXIT
15:01 <@algernon> return NEXT => let the next handler have a go at it; return EXIT => bye-bye
15:01 <@obra> since this isn't 1970, we can use more verbose names :)
15:01 <@algernon> oh, right. we had a power outage and the clock reset to the epoch!
15:01 * algernon adjusts
15:04 <@algernon> return NEXT_HANDLER (or NEXT_IN_CHAIN, or CONTINUE_WITH_NEXT_HANDLER) + return WITHOUT_FURTHER_PROCESSING
15:05 <@obra> we don't really call them handlers in this context.
15:06 <@obra> well, not after that renaming, anyway
15:08 <@obra> return ABORT_HANDLING ABORT_PROCESSING EVENT_CONSUMED
15:08 <@obra> return DONE_PROCESSING
15:08 <@obra> return CANCEL_PROCESSING_EVENT
15:09 <@algernon> I like EVENT_CONSUMED best so far
15:09 <@obra> sure. we could also spec that that enum value only makes sense in the onEvent handler
15:10 <@algernon> it says what the plugin considers, not what the mechanism outside of it will do
15:10 <@obra> right
15:10 <@obra> return OK for the other default case
15:10 <@algernon> that sounds good.
15:11 <@obra> it probably makes sense to have a return ERROR there too, even if we don't typically act on it.
15:11 <@obra> I could see a debugging build where we do something with that
15:11 <@algernon> btw, can you dump this in #316 too? (along with anything else you'd l

obra · 2018-05-05T22:14:43Z

15:13 <@algernon> comment cleanup, inlining macros (call*, HOOK_STATIC_METHODS), "why is this in _internal?" (hook_rooting), moving stuff around (MAP out of internal, etc)
15:14 <@algernon> tbh, if you paste my line above, that should be enough, I'll find the logs from there =)

noseglasses · 2018-05-06T11:42:33Z

src/macro_helpers.h

-__NL__   )
+   (                                                                      __NL__ \
+   []{ /* Here we are in the body of a dummy lambda function.             __NL__ \
+          []{} is, BTW, the shortest way to write a lambda.               __NL__ \


The __NL__ macro is meant to improve readability of pre-processed code. Adding it to lines that contain only comments will result in a bunch of empty lines in the resulting pre-processed code. I suppose that's unwanted. Because of this, the __NN__ macro was added. Its sole purpose is not to break the flow of newline macros in the original code and to improve its readability without generating undesired newlines for these lines after pre-processing.

Yeah. My thought process was around the tradeoff between the generated code being more compact and the human-editable version of the code being more maintainable.
Ultimately, I expect that folks will be spending a lot more time with the 'real' implementation than the generated code. And some extra newlines in the generated code feel to me like they're not a significant hardship relative to making editing the code more complicated.

I've actually been wondering if astyle would do a good enough job with the generated code that we could instead just get rid of the NL macros entirely.

And some extra newlines in the generated code feel to me like they're not a significant hardship relative to making editing the code more complicated.

Agreed.

I've actually been wondering if astyle would do a good enough job with the generated code that we could instead just get rid of the NL macros entirely.

As far as I get it, astyle is currently processing the original code. This won't fix the problems that arise with multi-line pre-processor macros. Every \ followed by a \n is completely removed, and the whole macro ends up in a single, possibly very long line. That's why the __NL__ is useful.

Of course you could run astyle on the pre-processed code, it will probably add some line breaks. But I prefer the pre-processed code to have the same layout as the original code which aids orientation and code comparison.

noseglasses · 2018-05-06T12:16:35Z

Wow, I am impressed how fast you are progressing with this. May I add some remarks about the recent changes.

it probably makes sense to have a return ERROR there too, even if we don't typically act on it.

I like the idea of reducing the complexity by using a single return value type. However, your discussion suggests that you intent to add values that are possibly ignored. That sounds like a pretty bad idea, unless it is perfectly documented. As a programmer I always expect the calling instance to react reasonably on my return codes and I rely on that. When return codes are ignored, in general that means a design flaw or at least an obvious trap. That's why I would suggest to only define those values that are really required and acted upon.
Passing error codes around in a firmware doesn't seem like a good idea either. You don't want to have release builds with error handling. Error handling code for debug versions is not a good idea either. I'd encourage to use assertions for that purpose. They tend to improve code readability as they describe what is expected.

The Kaleidoscope coding style guide prefers enums to be named as constants. I would strongly recommend to use all-caps names for macros only. This makes it possible to rely on what is a symbol (no-strange side effects from replacement & type safety) and what is a macro (be prepared for any surprises). This differentiation, if taken serious, helps a lot when debugging.

Is there any reason why you intent to use plain old enums instead of enum classes for the handlers' return values? Unless there are really good reasons against, the latter should be preferred. See here about the difference between both enum types.

I read that you don't intent to call the plugins' dedicated callback methods 'handlers'. How do you intent to call them instead? In other contexts like GUI frameworks this is exactly what would be called event/signal handlers. The only difference here is that they are not dynamically registered. Nevertheless, the name seems appropriate to me.

gedankenexperimenter · 2018-05-06T12:51:58Z

I would strongly recommend to use all-caps names for macros only.

Seconded.

I read that you don't intent to call the plugins' dedicated callback methods 'handlers'. How do you intent to call them instead? In other contexts like GUI frameworks this is exactly what would be called event/signal handlers. The only difference here is that they are not dynamically registered.

In the case of the event handlers, the name "handler" is certainly appropriate, but many of the hook functions don't actually "handle" anything. And in many cases, even the "event handler" hook functions might do something to respond to an event, but don't actually handle that event completely, so one could argue that "handler" is a bit of a misnomer even there.

noseglasses · 2018-05-06T14:08:50Z

You are right, handlers aren't forced to handle. There are at least three names of this concept I am aware of: events&handlers, signals&slots and hooks. Maybe there are others. In any case, I think we should stick with conventions and use one of the established terms.

noseglasses · 2018-05-06T14:14:01Z

src/kaleidoscope_internal/hook_rooting.h

+                                                                          __NL__ \
+   struct PluginMethod_##PLUGIN_METHOD {                                  __NL__ \
+                                                                          __NL__ \
+      typedef SHOULD_ABORT_TYPE shouldAbort;                              __NL__ \


shouldAbort might better be named uppercase as it is a type not an instance.

algernon · 2018-05-06T20:49:15Z

TODO list for myself:

Figure out a better name for hook_rooting.h
Explain at the top of said file why it is in src/kaleidoscope_internal
Move preprocessor_macro_map.h elsewhere, or present a good case why it should be internal (even though it is just preprocessor macros, so not namespaced!)
Figure out what to do with return types. Having them all be the same certainly has advantages.

obra · 2018-05-07T04:53:40Z

@noseglasses:

events&handlers, signals&slots and hooks.

In Kaleidoscope, we don't use the 'signals and slots' nomenclature for anything.
'hooks' are the places where plugins can provide implementations of functionality.
'events' are physical changes to key state (or things that pretend to be physical changes)
'handlers' are the functions in plugins that satisfy hooks.

noseglasses · 2018-05-07T08:14:14Z

@obra:

In Kaleidoscope, we don't use the 'signals and slots' nomenclature for anything.
'hooks' are the places where plugins can provide implementations of functionality.
'events' are physical changes to key state (or things that pretend to be physical changes)
'handlers' are the functions in plugins that satisfy hooks.

I was refering to this part of your communication:

@algernon> return NEXT_HANDLER (or NEXT_IN_CHAIN, or CONTINUE_WITH_NEXT_HANDLER) + return WITHOUT_FURTHER_PROCESSING
15:05 <@obra> we don't really call them handlers in this context.
15:06 <@obra> well, not after that renaming, anyway

Which appears to be contradicted by your more recent post.

'handlers' are the functions in plugins that satisfy hooks.

FWIW, I spend enough time with this piece of code that I am well aware that there are no signals&slots 😉

obra · 2018-05-07T08:20:54Z

@noseglasses -

Yup, we're very much still feeling out the names. I was trying to figure out how the words you used mapped to what we have in Kaleidoscope and slightly misunderstood your comment as implying that we were using all those names.

Right now, 'Handler' only actually gets used for key events.

noseglasses · 2018-05-07T08:47:41Z

Yup, we're very much still feeling out the names.

Yes, and I am very happy with most of the names you found. I really appreciate you taking so much time to name everything appropriately. Correct and telling names will help future developers a lot.

Right now, 'Handler' only actually gets used for key events.

I see. So what do we call the other callbacks then. From my perspective, any of these methods have a signal character. Forget about the slots here (I never liked that name anyway). In many other projects those methods would probably be called signal handlers. To me it sounds a bit strange 'to handle a signal', but I am not a native speaker. Anyway, it is quite commonly used that way.

The way you call the methods on..., after..., before... are also quite typical for the signal/handler concept. Those methods signal what just happened or what is going to happen next. Gtk+/Gtkmm e.g. use a very similar naming.

algernon · 2018-05-08T08:31:19Z

Just pushed a new update that changes the return type of all plugin methods to kaleidoscope::Plugin::Result (an enum class). We see a little bit of PROGMEM increase, but it does not appear to grow with the number of plugins enabled. I haven't checked if the compiler is smart enough to optimize out the shouldAbort checks at compile-time with this setup, but even if it doesn't, the minor code size increase, and the possible (but also very tiny) performance impact is well worth it for the simplified code.

Next up: cleaning up the history, and updating all the plugins.

noseglasses · 2018-05-10T10:21:54Z

src/kaleidoscope_internal/event_dispatch.h

+     EventHandlerResult Hooks::HOOK_NAME SIGNATURE {                      __NL__ \
+        return kaleidoscope_internal::EventDispatcher::template           __NL__ \
+        apply<kaleidoscope_internal::EventHandler_ ## HOOK_NAME>          __NL__ \
+             (__VA_ARGS__);                                               __NL__ \


Replace

(__VA_ARGS__)

with

ARGS_LIST

noseglasses · 2018-05-10T17:00:38Z

src/kaleidoscope/eventhandlerresult.h might better be renamed src/kaleidoscope/event_handler_result.h to be better conforming (and more readable).

algernon · 2018-05-11T06:14:12Z

This is no longer WIP, and I force-pushed a squashed together branch.

noseglasses · 2018-05-11T07:09:28Z

I know that you are eager to finish this and so am I.

There is one tiny detail about the _FOR_EACH_EVENT_HANDLER macro function that I overlooked yesterday.

I added a leading _ to all the names of macro functions that I considered part of Kaleidoscope's internals (not for use in plugins, etc.). Originally, this included _FOR_EACH_EVENT_HANDLER which was initially residing in of one of the headers in kaleidoscope_internal.

After it has been moved to its own header src/event_handler.h, it might better be named FOR_EACH_EVENT_HANDLER. Thus, marking it as a macro function that's accepted to be used by users, too, for debugging, etc.

algernon · 2018-05-11T07:22:05Z

I'd keep the leading underscore, if for nothing else, to signal that this is close to the internals, and should not be used lightly.

obra · 2018-05-11T07:33:09Z

Yeah. At least for now, this feels like "not part of the public API"

…

On Fri, May 11, 2018 at 12:22 AM Gergely Nagy ***@***.***> wrote: I'd keep the leading underscore, if for nothing else, to signal that this is close to the internals, and should not be used lightly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#316 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACxaPkG9re6dZXI7BQeLTCXksDOyQ1Lks5txTwhgaJpZM4TyrGu> .

noseglasses · 2018-05-11T07:41:44Z

src/plugin.h

+#endif
+
+ public:
+  // The following callbacks handle the synchronized communication


This text needs to be updated as it is left over from when the event handlers were directly defined in Plugin. I think we should refer the reader to src/event_handlers.h where all the event handlers are defined and documented.

The the terminology also needs to be updated according to the new glossary.
Also, I wouldn't write about callbacks here, as the term callback is mostly used for stand alone functions and not for class methods, even though they serve the same purpose.

Nice catch, thanks! Pushed an update that changes this text to a pointer to event_handlers.h.

noseglasses · 2018-05-11T08:06:22Z

There's still the issue with the redundant Kaleidoscope instance which is, from my point of view, not urgent but at least worth an issue ticket after V2 has been merged.

As far as I can see class Kaleidoscope_ does not have any non-static data members right now. So currently the costs for the redundance are zero. Nevertheless, the instance redundancy will hurt in terms of PROGMEM/RAM and also in other ways as soon as there are non static data members added to Plugin in future revisions.

algernon · 2018-05-11T08:11:55Z

Whoops! Forgot about that. Fixed. Thanks again!

noseglasses · 2018-05-11T09:12:51Z

src/Kaleidoscope.h

+// For compatibility reasons we enable the global variable Kaleidoscope
+// in global namespace.
+//
+extern kaleidoscope::Kaleidoscope_ &Kaleidoscope;


Not urgent:

Damn! It just occured to me that we could have fixed this with a using declaration to generate a perfect alias and thereby saving 2 bytes of RAM.

using kaleidoscope::Kaleidoscope;

If someone changes this, don't forget to remove the instanciation of &Kaleidoscope in kaleidoscope.cpp.

Hah! Nice. It also appears to have saved us a few (20) bytes of PROGMEM too.

20 bytes! For a simple reference. I didn't expect that much.

algernon · 2018-05-11T22:54:09Z

Back to being WIP:

We need to deprecate Kaleidoscope.use(), the text needs a review.
We will have plugins that support the V2 API only, and should not be used with Kaleidoscope.use. These should error out. The ENFORCE_KALEIDOSCOPE_V2_PLUGIN_API macro is for this purpose.

We can support V1 & V2, but that means I need to update every plugin with wrappers. Mind you, that may end up being the better option... but a v2-only enforcement might still be useful nevertheless.

algernon · 2018-05-11T22:59:16Z

Note to self, with regards to the .use() deprecation, but applies to ENFORCE too:

00:53 obra | algernon: something closer to the other use deprecation message, telling them how to do it.
00:53 obra | Also possibly add: If this error doesn't make sense to you or you have any trouble, please send a copy of your .ino sketch file to help@keyboard.io and we can help fix it.

algernon · 2018-05-11T23:01:35Z

Also, @obra had a sweet idea: have an UPGRADING.md file, which details what is deprecated, what is their replacement, and how to migrate. Contains both past and present deprecations, with date of deprecation and date of expected removal.

algernon · 2018-05-12T04:45:05Z

Updated the Kaleidoscope.use() deprecation text, reviewed by @obra.
Made it so that it only fires once per use, and internal uses do not produce a warning.
Removed ENFORCE_..., as I'm going to update all plugins to support v1 too, and that should be the norm during the transition period.
UPGRADE.md still needs to be written.

@noseglasses

With this redesign, we introduce a new way to create plugins, which is easier to extend with new hook points, provides a better interface, uses less memory, less program space, and is a tiny bit faster too. It all begins with `kaleidoscope::Plugin` being the base class, which provides the hook methods plugins can implement. Plugins should be declared with `KALEIDOSCOPE_INIT_PLUGINS` instead of `Kaleidoscope.use()`. Behind this macro is a bit of magic (see the in-code documentation) that allows us to unroll the hook method calls, avoid vtables, and so on. It creates an override for `kaleidoscope::Hooks::*` methods, each of which will call the respective methods of each initialized plugin. With the new API come new names: all of the methods plugins can implement received new, more descriptive names that all follow a similar pattern. The old (dubbed V1) API still remains in place, although deprecated. One can turn it off by setting the `KALEIDOSCOPE_ENABLE_V1_PLUGIN_API` define to zero, while compiling the firmware. This work is based on #276, written by @noseglasses. @obra and @algernon did some cleaning up and applied a little naming treatment. Signed-off-by: noseglasses <shinynoseglasses@gmail.com> Signed-off-by: Jesse Vincent <jesse@keyboard.io> Signed-off-by: Gergely Nagy <algernon@keyboard.io>

algernon · 2018-05-12T06:12:07Z

Phew. Should be ready, again.

noseglasses · 2018-05-12T09:06:47Z

+1 for the nice DEPRECATED_USE macro and its application.

algernon · 2018-05-12T09:12:46Z

The text of that is largely @obra's work, all credit to him. And yes, it's <3 <3!

noseglasses · 2018-05-12T10:11:49Z

doc/glossary.md

+
+Event handler
+
+: A function, usually provided by a `Plugin` that is run by a `Hook`


Plugin is not well defined because it is used for the class Plugin and also for Arduino libraries, that sometimes don't contain Plugin classes. Might be a good candidate for the glossary.

noseglasses · 2018-05-12T10:25:00Z

+1 for the glossary and several +1s for all the effort to provide a smooth transition between APIs.

algernon mentioned this pull request May 4, 2018

Redesign of the plugin and hooking interface II #276

Closed

noseglasses reviewed May 4, 2018

View reviewed changes

algernon self-assigned this May 5, 2018

algernon added the enhancement New feature or request label May 5, 2018

algernon force-pushed the f/plugin-redesign-2.1 branch from 2f2f106 to ed62dc0 Compare May 5, 2018 08:48

algernon mentioned this pull request May 5, 2018

makefiles/rules.mk (cpplint): Allow non-const references keyboardio/Kaleidoscope-Build-Tools#1

Merged

noseglasses reviewed May 6, 2018

View reviewed changes

algernon mentioned this pull request May 6, 2018

Coding Style Guidelines #134

Closed

noseglasses reviewed May 10, 2018

View reviewed changes

algernon force-pushed the f/plugin-redesign-2.1 branch from a088338 to 08d91c2 Compare May 11, 2018 06:07

algernon changed the title ~~WIP: Plugin API redesign v2.1~~ Plugin API redesign v2.1 May 11, 2018

noseglasses reviewed May 11, 2018

View reviewed changes

algernon force-pushed the f/plugin-redesign-2.1 branch from 08d91c2 to 1f536f0 Compare May 11, 2018 07:48

algernon force-pushed the f/plugin-redesign-2.1 branch from 1f536f0 to 36cd0aa Compare May 11, 2018 08:11

noseglasses reviewed May 11, 2018

View reviewed changes

algernon force-pushed the f/plugin-redesign-2.1 branch from 36cd0aa to 2bbcdb5 Compare May 11, 2018 11:24

algernon force-pushed the f/plugin-redesign-2.1 branch from 8b78505 to 020dceb Compare May 12, 2018 04:42

algernon force-pushed the f/plugin-redesign-2.1 branch from 023673d to 8130dfd Compare May 12, 2018 06:11

noseglasses reviewed May 12, 2018

View reviewed changes

algernon merged commit bddfc7b into master May 12, 2018

algernon deleted the f/plugin-redesign-2.1 branch May 12, 2018 15:24

algernon mentioned this pull request Jan 16, 2019

Simplify definition of Key type #294

Closed


		Event handler

		: A function, usually provided by a `Plugin` that is run by a `Hook`

Plugin API redesign v2.1 #316

Plugin API redesign v2.1 #316

Conversation

algernon commented May 4, 2018

noseglasses May 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noseglasses May 4, 2018 • edited

Choose a reason for hiding this comment

algernon commented May 5, 2018 • edited

noseglasses commented May 5, 2018

algernon commented May 5, 2018

algernon commented May 5, 2018

obra commented May 5, 2018

obra commented May 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noseglasses commented May 6, 2018

gedankenexperimenter commented May 6, 2018

noseglasses commented May 6, 2018

Choose a reason for hiding this comment

algernon commented May 6, 2018 • edited

obra commented May 7, 2018

noseglasses commented May 7, 2018

obra commented May 7, 2018

noseglasses commented May 7, 2018 • edited

algernon commented May 8, 2018

Choose a reason for hiding this comment

noseglasses commented May 10, 2018

algernon commented May 11, 2018

noseglasses commented May 11, 2018

algernon commented May 11, 2018

obra commented May 11, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noseglasses commented May 11, 2018

algernon commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algernon commented May 11, 2018

algernon commented May 11, 2018

algernon commented May 11, 2018

algernon commented May 12, 2018 • edited

algernon commented May 12, 2018

noseglasses commented May 12, 2018

algernon commented May 12, 2018

Choose a reason for hiding this comment

noseglasses commented May 12, 2018

noseglasses May 4, 2018 •

edited

noseglasses May 4, 2018 •

edited

algernon commented May 5, 2018 •

edited

algernon commented May 6, 2018 •

edited

noseglasses commented May 7, 2018 •

edited

algernon commented May 12, 2018 •

edited