Redesign of the plugin and hooking interface II #276

noseglasses · 2017-12-25T14:23:47Z

Finally, I found some time to think a bit more about the redesign of the plugin hooking interface started as #240 .

Parts of the changes proposed in #240 were not yet optimal, namely

Class Kaleidoscope_ is preferred against other classes and free functions of the core that need to call hook methods.
A pointer to an instance of a polymorphic hook adaptor class is used within Kaleidoscope. This type of access is overkill as the adaptor class could actually be a singleton.
The hook adaptor class uses virtual functions to root hook methods. This is non-optimal, especially for those hooks that are called frequently, such as eventHandlerHook.

The solution to all this is very simple. Let's have a Hooks class that has non-virtual but static functions that define the entry points to hook method routing. This allows for proper encapsulation to allow calls to hook methods only from specific classes and functions of the firmware core.

This new approach solves issue 1). By using global functions as hook entry points, we automatically also get rid of issues 2) and 3).

This change improves performance (removal of virtual functions) and PROGMEM and RAM storage (removal of vtables and vptrs as we do not have any polymorphic classes involved).

The reason why this did not come to my mind in the first place is, that I was distracted by the term plugin. In other contexts, plugin refers to something runtime pluggable and unpluggable.
In the firmware, however, plugins ase hardwired. This provides great potential for optimizations as we do not have to care for polymorphic interfaces.

This PR is a squashed version of #240 that eventually consisted of too many individual commits. Also #240 started to become unreadable. That's why I started a new PR.

Changes against #240

Hook routing system

The auxiliary polymorphic classes PluginHookAdapter and PluginHookAdapter were removed and replaced by a new class Hooks that encapsules the entry points of the hook routing system.

Plugin registration

The macro KALEIDOSCOPE_CONNECT_PLUGINS is no more necessary, as a registration of the hooking system with class Kaleidoscope_ in no more necessary. Now plugins can be registered via a single invocation of function macro KALEIDOSCOPE_INIT_PLUGINS(...).

Addition of new hooks

~~To add a new hook, changes are now required in three places instead of four.~~
EDIT 1: This is no more true after the introduction of weak symbols in details/hooks.cpp. There remain four places where code for new hooks has to be added. This is documented at the top of details/hook_routing.h.

Encapsulation

The restriction of who is allowed to invoke a hook is now centralized by defining
friends of class Hooks. This is more transparent than adding friend declarations to
Kaleidoscope_ and much more safe than allowing hooks to be called by everybody.

Naming

Symbol names

Appart from some symbol name changes in the internals of the hook routing system.
The plugin base class is now kaleidoscope::Plugin (with a compatibility typedef
KaleidoscopePlugin in global namespace. I allowed myself the freedom to rename existing symbols that appear in newly written code, to reduce the maintainance overhead that is necessary when these
symbols are eventually changed. Any symbols whose names start with Kaleidoscope should be renamed when moved to namespace kaleidoscope to keep names short.

Macro names

Internal macros (those defined and used in code parts that are considered as details)
have been prefixed with a single underscore.

Namespaces

As I implemented quite a lot of new code, I added everything to namespace kaleidoscope. We could have a nested namespace kaleidoscope::hooks but I am not sure that would help much.
To make it fully compatible, I moved some of the existing classes Kaleidoscope_, Key and KaleidoscopePlugin to namespace kaleidoscope. This changes is not supposed to break anything as all existing classes were typedefed in global namespace and declared deprecated.

Implementation details

The hooking system comes with a great deal of implementation details that are quite complex for C++ newbies. All those parts that are definitely not meant to be read and used by non core-maintainters have been moved to a subdirectory details. I recommend to do the same with all other stuff that is not meant for the eyes of plugin developers. It is a good custom to flag certain parts of the code as "poisonous" to tell new users that it is not necessary to deal with this part of the code in order to implement new functionality. Also adding code to detail folders informs that user that those parts are subject to changes and do by no means define a programming interface.

Let KaleidoscopePlugin::init() call KaleidoscopePlugin::begin() to support legacy plugins Added missing startLoopHook() and endLoopHook() calls. Removed HookAdapterAccessor. Fixed astyle formatting errors Replaced loop hook method names Fixed a small hook method naming error caused by regex replacement Added instuctions about how to deal with pre-processed code Fixed code style Fixed a typo Added hook signature checks and fixed macro continuation line alignment Added a check that ensures at compile time that derived plugins' hook methods have the correct signature. Fixed the alignment of macro continuation line markers. Added reporting of the culprit plugin to hook method signature checks Refactored Kaleidoscope.h that has grown too large Some modification to increases hook signature check error output Added a missing newline Fixed case of hook task helper classes Implemented major simplifications of the new plugin hooking system Applied make astyle Mod Redesign of the plugin and hooking interface (II) astyle

noseglasses · 2017-12-25T15:13:24Z

Travis tests for this PR fail because of an attempt to build the firmware with an unsuitable sketch file.

If you want to try the latest changes, keep track of my branch
https://github.com/noseglasses/Arduino-Boards/tree/plugin_interface_overhaul_2

noseglasses · 2017-12-25T15:15:30Z

src/Kaleidoscope.cpp

@@ -9,6 +11,9 @@ Kaleidoscope_::Kaleidoscope_(void) {

 void
 Kaleidoscope_::setup(void) {
+
+  kaleidoscope::Hooks::init();


Hooks are now access via static functions of class Hook. The call to init() initializes all plugins. This is what formerly has been done through Kaleidoscope.run().

noseglasses · 2017-12-25T15:18:07Z

src/Kaleidoscope.h


 #include KALEIDOSCOPE_HARDWARE_H
 #include "key_events.h"
 #include "kaleidoscope/hid.h"
 #include "layers.h"
+#include "details/preprocessor_macro_map.h"


Implementation details now live in files below directory details.

noseglasses · 2017-12-25T15:19:32Z

src/Kaleidoscope.h

@@ -36,29 +40,7 @@ extern HARDWARE_IMPLEMENTATION KeyboardHardware;
 const uint8_t KEYMAP_SIZE
 __attribute__((deprecated("Kaleidoscope.setup() does not require KEYMAP_SIZE anymore."))) = 0;

-class Kaleidoscope_;
-
-class KaleidoscopePlugin {


Class KaleidoscopePlugin now lives in header plugin.h it was renamed to kaleidoscope::Plugin, thereby keeping the original name as deprecated type.

noseglasses · 2017-12-25T15:19:56Z

src/Kaleidoscope.h

-  static void replaceEventHandlerHook(eventHandlerHook oldHook, eventHandlerHook newHook);
-  static void appendEventHandlerHook(eventHandlerHook hook);
-  static void useEventHandlerHook(eventHandlerHook hook);
+  static void replaceEventHandlerHook(eventHandlerHook oldHook, eventHandlerHook newHook)


Old event handlers were declared deprecated.

noseglasses · 2017-12-25T15:21:52Z

src/Kaleidoscope.h


  static bool focusHook(const char *command);
 };

-extern Kaleidoscope_ Kaleidoscope;
+extern kaleidoscope::Kaleidoscope_ Kaleidoscope;


The singleton instance Kaleidoscope has for compatibility reasons moved to namespace kaleidoscope. The old name is preserved as a deprecated symbol.

noseglasses · 2017-12-25T15:22:38Z

src/Kaleidoscope.h

 __attribute__((deprecated("Use Kaleidoscope.useEventHandlerHook instead")));
-void loop_hook_use(Kaleidoscope_::loopHook hook)
+void loop_hook_use(kaleidoscope::Kaleidoscope_::loopHook hook)


Added appropriate namespace.

noseglasses · 2017-12-25T15:24:09Z

src/Kaleidoscope.h

+// hooking system. The macro accepts a list of plugin instances that
+// must have been instanciated at global scope.
+//
+#define KALEIDOSCOPE_INIT_PLUGINS(...) _KALEIDOSCOPE_INIT_PLUGINS(__VA_ARGS__)


This macro is used in the sketch at global scope in global namespace to register plugins.
Could be renamed to KALEIDOSCOPE_REGISTER_PLUGINS(...). Maybe that's a better name.

noseglasses · 2017-12-25T15:26:43Z

src/details/hook_rooting.h

+// return type of a hook member function.
+//
+template<typename T>
+struct ReturnTypeTraits {};


Traits classes are needed to determine specific properties of C++ types at compile time. They are an essential part of TMP. Here we determine the return type of a specific hook method.

noseglasses · 2017-12-25T15:30:42Z

src/details/hook_rooting.h

+// It must be invoked in global namespace as it adds namespace kaleidoscope
+// to the Hooks' function implementations.
+//
+#define _HOOKS_STATIC_METHODS_IMPLEMENTATION                                   \


This is the implementation of the static functions of class Hook. The implementation depends on the registered plugins that are considered by the HookLoop class. That is the reason why it must be implemented within a macro. The macro is called at the end of _KALEIDOSCOPE_INIT_PLUGINS.

noseglasses · 2017-12-25T15:33:40Z

src/key_defs.h


-typedef union Key_ {


In C we have to add a typedef to be able to use a struct or union name without prepending it with keywords struct or union. This typedef is useless in C++.

The original name Key_ is preserved by an appropriate typedef in global namespace (see below).

noseglasses · 2017-12-25T15:34:46Z

src/key_defs.h

@@ -28,14 +29,14 @@ typedef union Key_ {
  inline bool operator==(uint16_t rhs) {
    return this->raw == rhs;
  }
-  inline bool operator==(const Key_ rhs) {


After removing the typdedef of Key_ to Key, we have to operate on Key.

noseglasses · 2017-12-25T15:36:14Z

src/key_defs.h

+// For compatibility reasons make the Key class also available
+// in global namespace.
+//
+typedef kaleidoscope::Key Key __attribute__((deprecated("class Key in global namespace is deprecated. Please use kaleidoscope::Key instead.")));


Compatibility typedefs.

noseglasses · 2017-12-25T15:43:43Z

src/plugin.h

+
+class Kaleidoscope_;
+
+struct EventKey {


The parameters of the old eventHandlerHook have been grouped in struct EventKey. This enables adding further parameters in the future without breaking the interface.

noseglasses · 2017-12-25T15:47:43Z

src/details/hooks.h

+// be called from other places than the friend classes and functions listed
+// below, just add a suitable friend declaration.
+
+class Hooks {


Any (core) compilation units that are supposed to call hook functions only include details/hooks.h without a need to know anything about hook routing. Header hooks.h has been moved to details to make sure that it is not part of the plugin interface.

algernon · 2017-12-25T21:13:27Z

Thanks for the new PR, and the explanations. I started to work my way through the previous, but wasn't able to finish due to Christmas. I'll work through this one between the holidays.

… backwards compatibility

noseglasses · 2017-12-27T18:08:54Z

src/details/hooks.cpp

+
+namespace kaleidoscope {
+
+__attribute__((weak))


Added weak implementation of Hooks's static methods to enable backwards compatibility. Without this, any sketch that does not register new plugins with KALEIDOSCOPE_INIT_PLUGINS(...) would cause undefined symbol errors because of the missing methods of class Hooks.

Added a comment to explain the use of weak symbols in hooks.cpp

Updated the comment about how to add new hooks

…e inside of hook tasks.

algernon · 2018-03-09T14:54:39Z

In the past few weeks, while trying to figure out how we can get this in, we arrived at a possible way to do so, that we hope will satisfy everyone involved. But before I get there, a short explanation why this has not been merged yet.

The new API looks good, and is a direction we want to go in. The PROGMEM & RAM savings are also very lucrative. But therein lies the problem: to get there, one needs to do some non-trivial amounts of "magic". While most - if not all - of that would be invisible for plugin developers and end-users, it is something that is hard to understand for those with less C++ knowledge (myself included). As such, we did not feel comfortable merging code we have trouble understanding - I went through it, and with the help of your comments and explanation, I understand it. But it wasn't easy to get there, to follow the code. We want to end up with something maintainable in the long run, something we understand too, something we can explain to future contributors too.

Yet, we do want to have everything in, eventually. So I'd propose a two-stage merge: first we get the new API in, but with virtual functions (and thus less magic), and the rooting part in a separate step. If my understanding is correct, doing this second step separately would not break the API, so plugins would not need to change. This also has the advantage of having a clearer picture about what's the new API, and what is the optimization that allows us to skip using virtuals.

What do you think about this approach? (I'm happy to do the grunt work this would involve, fwiw).

noseglasses · 2018-03-09T15:50:46Z

Yeah, I know, the implementation is not a beauty. But that's just because C++ is a beast ;-)
I understand your points about maintainability very well.

I am curious to see, how you are planning to implement the part that gets rid of statically sized arrays.

I am not completely convinced of the two-stage merge. Once you introduce virtual hook methods in the Plugin class, users will (potentially) add the virtual keyword and the override keyword to their plugins. When you decide to change the API later on, by turning virtual methods into non-virtual ones, you run the risk of breaking user code. But maybe I just didn't get what you are actually up to.

algernon · 2018-04-20T09:27:32Z

I think I made a breakthrough, that will allow us to do a one-stage merge, with less magic. The key is the auto keyword, and using something similar to Kaleidoscope.use() to unroll the hook functions. The downside of this is a bit of code repetition, but I can hide that behind one macro.

Something along these lines:

#define KALEIDOSCOPE_MAKE_PLUGIN_HOOK(hookName, method) \
  inline void hookName () {}                            \
  inline void hookName(auto *p) {                       \
    p->method();                                        \
  }                                                     \
  template <typename... Plugins>                        \
  void hookName(auto *first, Plugins&&... plugins) {    \
    hookName(first);                                    \
    hookName(plugins...);                               \
  }

KALEIDOSCOPE_MAKE_PLUGIN_HOOK(runPreScan, preScan);
KALEIDOSCOPE_MAKE_PLUGIN_HOOK(runPostScan, postScan);

However, this does come with downsides too: we can pass anything to these functions, as long as they have the appropriately named method. They do not have to be children of the base kaleidoscope::Plugin class. Passing something that does not have a begin method results in a compile-time error.

I think this is something we can live with, because the code is - in my opinion at least - easier to understand, and the compiled thing should be pretty much the same, too.

noseglasses · 2018-04-20T10:09:35Z

auto *p as function argument is AFAIK illegal in C++11.

You could use a template to achieve the same, like

template<typename Plugin__>
inline void hookName(Plugin__ *p) {
   p->method();
}

That would be proper C++, but would also allow the duck typing you mentioned.

algernon · 2018-04-20T10:25:55Z

Hm, indeed, it doesn't work with c++11, needs c++14. Will try your suggestion, and see if it works with the gcc shipped with Arduino.

(So far, I've been trying with a much more recent gcc)

Update:

#define KALEIDOSCOPE_MAKE_PLUGIN_HOOK(hookName, method)                 \
  inline void hookName () {}                                            \
  template<typename Plugin__>                                           \
  inline void hookName (Plugin__ *p) { p->method(); }                   \
  template <typename Plugin__, typename... Plugins>                     \
  void hookName(Plugin__ *first, Plugins&&... plugins) {                \
    hookName(first);                                                    \
    hookName(plugins...);                                               \
  }

This compiles with -std=c++11, and does the right thing. Thanks @noseglasses!

noseglasses · 2018-04-20T15:20:11Z

But what about those hooks whose return values decide about further processing? And what about the call parameters of some of the hooks? Those are the reason why the original magic was there in the first place.

noseglasses · 2018-04-20T21:13:05Z

FWIW, auto arguments to general functions aren't legal c++14, either. They are only allowed with lambdas there.

algernon · 2018-04-29T16:04:52Z

Quick update: there will be movement forwards on this soon. I already summed up my work for @obra, will do so in public too very soon.

Just wanted to note that there is progress being made, and the redesign is not forgotten. :)

algernon · 2018-05-04T11:45:31Z

src/details/hook_rooting.h

+};
+
+_HOOK_TASK(init, ContinueIfHookReturnsTrue) // Defines HookTask_init
+_HOOK_TASK(eventHandlerHook, AlwaysContinue) // Defines HookTask_eventHandlerHook


Shouldn't init be AlwaysContinue (since it is void anyway), and eventHandlerHook ContinueIfHookReturns?

Yes, you are right that's a mistake. Unless ... you want to let plugin initializations block each other, which does not make much sense.

noseglasses · 2018-05-04T12:12:25Z

I would strongly recommend to add

#if !KALEIDOSCOPE_DISABLE_V1_API
virtual void begin(void) {}
#else 
void begin(void) {}
#endif

in src/plugin.h.

Otherwise, all plugins will have a useless virtual function pointer&table even when the old API is disabled.

FWIW, the second void in void begin(void) ... is a C reminiscense (obsolete in C++).

algernon

I have made a few changes to this PR, on the f/plugin-redesign-2.1 branch of Kaleidoscope. The major differences are:

A few things this PR deprecates are undeprecated (either because they resulted in warnings even in cases where the old APIs weren't even used, or because they introduced too much change)
I got rid of EventKey for now, because it is an orthogonal change, which we'd like to handle separately (within the same major version, but separate PR, with a separate discussion, possibly together with merging row+col into an index)
The old API can be disabled with a compiler flag.
I added a WIP document explaining how the template & preprocessor magic works, along with an example or two to follow how the code is transformed.
OrderedPlugins was moved to kaleidoscope_internal, due to a naming pattern that caused issues (the compiler tried to use a class as an object, and failed in spectacular ways with cryptic messages).

Other than this, I have one question, which I asked in-line.

I still need to finish up a few loose ends, go over naming with @obra, but finally there is progress I can talk about!

I'd love to have feedback on the changes, and on the WIP document I prepared in the aforementioned branch.

algernon · 2018-05-04T12:27:39Z

I would strongly recommend to add ...

Thanks, added. Also fixed the init and eventHandlerHook predicates, thanks for the confirmation!

noseglasses · 2018-05-04T12:46:05Z

I'd love to have feedback on the changes, and on the WIP document I prepared in the aforementioned branch.

Just read your WIP document. Sweet! 😍 Never has any code that I've written been explained so nicely.
On, the other hand, never has any code that I've written been demonized more 😈
I've you're one day done with work on Kaleidoscope, can I hire you? I have ~500 000 lines of similar C++ code in a simulation program that I distribute. It could need some explanations, too 😉

Joking apart. Really great job. I know how hard it can be to understand and deal with other peoples' code.

All your changes make sense to me.

I am still fond of the idea of plugin grouping. If you are interested I can provide a PR as soon as the redesign and your changes have been merged to master.

algernon · 2018-05-04T12:59:11Z

On, the other hand, never has any code that I've written been demonized more smiling_imp

I'm sorry if it came/comes across as demonizing. Most of my issues I may have voiced are more against C++ than your code in particular. (Did I mention I'm not a big fan of C++? But Clojure isn't well suited for keyboard firmware, so C++ it is for now :p)

Once I managed to figure it all out, connect the pieces, when it dawned on me how it makes things work, I started to see it in a very different light. It is a beautiful piece of code. Twisted too, and tangled, with a hint of madness sprinkled on top - but beautiful nevertheless.

I am still fond of the idea of plugin grouping. If you are interested I can provide a PR as soon as the redesign and your changes have been merged to master.

I think I like the idea, but I'd suggest opening an issue first, so we can discuss naming and perhaps a few other things. This would allow you to only start working once things like that are agreed upon. Noone would need to update it again, would we find the need to change a name or two here and there.

noseglasses · 2018-05-04T13:09:07Z

Most of my issues I may have voiced are more against C++ than your code in particular. (Did I mention I'm not a big fan of C++? But Clojure isn't well suited for keyboard firmware, so C++ it is for now :p)

There are times when I feel the same. But I am afraid, I am too old to switch to another nicer low level language. Hopefully, C++ will stay around for a while.

with a hint of madness sprinkled on top

You are a true poet.

I think I like the idea, but I'd suggest opening an issue first, so we can discuss naming and perhaps a few other things. This would allow you to only start working once things like that are agreed upon. Noone would need to update it again, would we find the need to change a name or two here and there.

Good idea.

noseglasses · 2018-05-04T13:37:51Z

@algernon: Could you open a temporary PR with your changes in f/plugin-redesign-2.1? I found some small issues in the WIP document and it would be easiest if I could comment the respective lines directly.

algernon · 2018-05-04T13:48:31Z

Done, #316.

algernon · 2018-05-05T14:45:34Z

Closing this in favour of #316.

@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 new 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: Gergely Nagy <algernon@keyboard.io>

@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 new 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>

@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 new 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>

@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>

@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>

@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>

@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>

@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>

@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>

noseglasses mentioned this pull request Dec 25, 2017

Redesign of the plugin and hooking interface #240

Closed

noseglasses commented Dec 25, 2017

View reviewed changes

algernon self-assigned this Dec 25, 2017

algernon self-requested a review December 25, 2017 21:13

algernon added the enhancement New feature or request label Dec 25, 2017

Added weak implementations of static members of class Hooks to enable…

2c6a0e9

… backwards compatibility

noseglasses commented Dec 27, 2017

View reviewed changes

noseglasses and others added 7 commits December 27, 2017 20:04

Update hooks.cpp

0e4e54a

Added a comment to explain the use of weak symbols in hooks.cpp

Update hook_rooting.h

681217b

Updated the comment about how to add new hooks

Renamed HookLoop to more expressive OrderedPlugins

329c1d3

Allow custom defined ordered plugins

f7c8c97

Moved hook task definitions out of macro. Moved continuation predicat…

56287c4

…e inside of hook tasks.

Added a function macro to simplify the definition of wrapper plugins.

5fe8543

Fixed style

3e35ebd

algernon reviewed May 4, 2018

View reviewed changes

algernon mentioned this pull request May 4, 2018

Plugin API redesign v2.1 #316

Merged

algernon closed this May 5, 2018

noseglasses deleted the pr_plugin_interface_overhaul_2 branch April 4, 2019 14:42

Redesign of the plugin and hooking interface II #276

Redesign of the plugin and hooking interface II #276

Conversation

noseglasses commented Dec 25, 2017 • edited Loading

Changes against #240

Hook routing system

Plugin registration

Addition of new hooks

Encapsulation

Naming

Symbol names

Macro names

Namespaces

Implementation details

noseglasses commented Dec 25, 2017

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

noseglasses Dec 25, 2017 • 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

Choose a reason for hiding this comment

noseglasses Dec 25, 2017 • edited Loading

Choose a reason for hiding this comment

algernon commented Dec 25, 2017

Choose a reason for hiding this comment

algernon commented Mar 9, 2018

noseglasses commented Mar 9, 2018

algernon commented Apr 20, 2018 • edited Loading

noseglasses commented Apr 20, 2018 • edited Loading

algernon commented Apr 20, 2018 • edited Loading

noseglasses commented Apr 20, 2018 • edited Loading

noseglasses commented Apr 20, 2018 • edited Loading

algernon commented Apr 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noseglasses commented May 4, 2018 • edited Loading

algernon left a comment

Choose a reason for hiding this comment

algernon commented May 4, 2018

noseglasses commented May 4, 2018

algernon commented May 4, 2018

noseglasses commented May 4, 2018

noseglasses commented May 4, 2018 • edited Loading

algernon commented May 4, 2018

algernon commented May 5, 2018

noseglasses commented Dec 25, 2017 •

edited

Loading

noseglasses Dec 25, 2017 •

edited

Loading

noseglasses Dec 25, 2017 •

edited

Loading

algernon commented Apr 20, 2018 •

edited

Loading

noseglasses commented Apr 20, 2018 •

edited

Loading

algernon commented Apr 20, 2018 •

edited

Loading

noseglasses commented Apr 20, 2018 •

edited

Loading

noseglasses commented Apr 20, 2018 •

edited

Loading

noseglasses commented May 4, 2018 •

edited

Loading

noseglasses commented May 4, 2018 •

edited

Loading