search

iotaledger / access-server

Apache License 2.0
14 stars 3 forks source link

Refactor Access Core API + PxP Plugins #110

Closed bernardoaraujor closed 4 years ago

bernardoaraujor commented 4 years ago

Summary Follow https://github.com/iotaledger/access/pull/107 and finish the work

Details

Requirements

  1. Avoid any protocol (CAN, GPS, GPIO, etc) and wallet references inside Access Core API (access directory). This API must be agnostic and modular, containing only PxP core functionality. All protocol and wallet references must reside in the plugin context. That was the main purpose of https://github.com/iotaledger/access/pull/107
  2. the plugins directory must follow this organization for the PxP plugins: https://rentry.co/ehetq
  3. keep calls to pep_register_callback() and pip_register_callback() inside access.c and not inside main.c. Otherwise the Access Core API is leaking out of the scope of Access Actor.
  4. keep the choice of which PxP plugins are chosen as code inside main.c, pep_plugin.h or pip_plugin.h, but not on access.{c,h}, pep.{c,h} or pip.{c,h}.

Links / references

I'd just add few things in regards to plugins in general. I'll use pip plugin in e940710158d7fd85e7769f580b75c8b4b6ad161c as an example, but it applies to all of them.

We have pip_plugin_t structure, which holds all plugin callbacks. This structure should be passed as an argument to all plugin API calls, but it isn't used anywhere. Instead, we are passing callbacks around, which is very bad practice. For instance, pip_plugin_init and pip_plugin_term should look something like this:

int pip_plugin_init(pip_plugin_t *plugin, pip_plugin_initializer_t initializer);
int pip_plugin_term(pip_plugin_t *plugin);

where initializer is function which initializes plugin structure with callbacks and ddstate. This makes things neat. pip_plugin_term would then just call plugin->term_cb(plugin), which frees ddstate among other plugin specific things. As ddstate would be void* field in plugin, initializer would create new plugin specific ddstate object there, while leaving API agnostic towards its semantics. For each callback in plugin structure, there should be API call, for instance pip_plugin_start, pip_plugin_stop, etc, and they would just call callbacks like term example.

Regarding actual plugin implementation, user would implement callback functions (which can be static), ddstate structure (also doesn't need to be visible outside of plugin's context), and initializer function which initializes pip_plugin_t structure with callbacks and ddstate. So only initializer would need to be visible outside of plugin's c file. All of the plugins functionality would be called through pip_plugin api calls, passing pointer to pip_plugin_t structure.

bernardoaraujor commented 4 years ago

@vlad-ns FYI

bernardoaraujor commented 4 years ago

@djordjeglbvc you said:

Instead, we are passing callbacks around, which is very bad practice.

but then you go on to propose:

int pip_plugin_init(pip_plugin_t *plugin, pip_plugin_initializer_t initializer);

which is still receiving a callback.

I admit I don't know what's the best way to design this solution. My approach on https://github.com/iotaledger/access/commit/b27ec7cb020e78c12cba61fb7087a969163d4ae5 for sure was not the best.

But I want us to be very careful in regards to the place where the developer choice of which plugin implementations are used is made. This choice must not be hardcoded into access.c, because that kills the modular approach. We have been repeating this mistake many times. Access Actor must know nothing about hardware protocols or IOTA wallet.

Meanwhile, pep_register_callback() and pip_register_callback() must be called inside access.c, because doing it anywhere else would characterize Access Core API leak. For reference, check https://github.com/iotaledger/access/commit/f0340546f86440c71571d26b6b82c8b388784bf4, where I move pep_register_callback() into the scope of access_init().

djordjeglbvc commented 4 years ago

@djordjeglbvc you said:

Instead, we are passing callbacks around, which is very bad practice.

but then you go on to propose:

int pip_plugin_init(pip_plugin_t *plugin, pip_plugin_initializer_t initializer);

which is still receiving a callback.

It's not a callback as such, it gets called once during a run-time, and it doesn't get passed around, no api leaking here. pip_plugin_init should probably be named something like pip_plugin_register, as it is it's purpose. Different approach (not so different really) would be to call initializer function on its own, it fills plugin structure with callbacks, and then we pass it to pip_plugin_register. That adds additional step in plugin usage, but doesn't make things cleaner (doesn't make things less clean, either).

I admit I don't know what's the best way to design this solution. My approach on b27ec7c for sure was not the best.

But I want us to be very careful in regards to the place where the developer choice of which plugin implementations are used is made. This choice must not be hardcoded into access.c, because that kills the modular approach. We have been repeating this mistake many times. Access Actor must know nothing about hardware protocols or IOTA wallet.

Meanwhile, pep_register_callback() and pip_register_callback() must be called inside access.c, because doing it anywhere else would characterize Access Core API leak. For reference, check f034054, where I move pep_register_callback() into the scope of access_init().

We could make pep_plugin_t structure here similarly to what I've proposed for pip plugins, then user creates initializer for plugin and passes it through access api without revealing pep api. We would add new function to access api (edit: to register pep plugin), as we would anyway because user needs to at least choose which plugins to use through access api. Callbacks shouldn't use pep calls, or any other api (access or modules within it) calls in general - this is what we solve with callbacks. We can define which data (and how) gets passed between plugin and module by extending pep_plugin_t structure.

djordjeglbvc commented 4 years ago

@bernardoaraujor I'm going through b27ec7cb020e78c12cba61fb7087a969163d4ae5 and there is a comment

// ToDo: how to properly determine the value of actuator index????

Why don't we leave it to the resolver plugin to determine the value of actuator index based on action? We would have one callback which handles one or more actuator actions. We can also add more callbacks, which would then process different groups of actuators. (Having callback per actuator might not be such a good idea in the long run, but multiple callbacks definitely - for different groups of actuators.)

Resolver plugin, sensors and actuators definitions are implemented like a group, or a package, and as I understand only they should be aware of sensors and actuators semantics.

I most likely didn't go through everything thoroughly, so I've probably missed something.

Looping through callbacks

Sorry for jumping from one thing from another, just to share my thoughts on this. There are two good options if using static arrays, really:

I'm in favor of passing array length as a parameter, as we need to keep it anyway while adding callbacks to arrays, but it's just a preference. If we plan to use dynamic registration of callbacks in future (which is not a bad idea), we could move to using linked lists, or just mimic lists logic with arrays (I like to stay away from using dynamic lists on small bare metal embedded systems, as I do with any dynamic allocation). But some callback management would need to be done either way.

djordjeglbvc commented 4 years ago

Continuing from previous comment, maybe turn this:

access_init(&access_context, device_wallet, plugin_initializers);

into this

access_init(&access_context, device_wallet);
access_register_resolver_plugin(demorelayplugin_initializer);
access_register_resolver_plugin(demowalletplugin_initializer);
access_register_pep_plugin(pep_initializer);

or

access_init(&access_context, device_wallet);
access_register_resolver_plugins(plugin_initializers, plugin_initializers_len);
access_register_pep_plugins(pep_initializers, pep_len);

something like that... I've added pep_initializer just for illustration.

bernardoaraujor commented 4 years ago

@djordjeglbvc I haven't read all your comments in detail yet, but first thing I notice is that you didn't understand one point.

resolver has been renamed to pep_plugin, so it doesn't make sense to have access_register_resolver_plugin and access_register_pep_plugin as separate things.

check https://github.com/iotaledger/access/pull/107/commits/92335c2e00bc34cf72e7f4e0684a61369e8d045c

djordjeglbvc commented 4 years ago

@bernardoaraujor you are right, I was quick browsing through code, sorry. I wanted to illustrate use case if we would have more than one plugin system to register plugins to from access API. Also I think it is better to have separate api for plugin registering, instead of passing them to access init function.

bernardoaraujor commented 4 years ago

another remark is that access_init(&access_context, device_wallet); is not ideal. we do not want wallet references inside Access Actor.

that was the purpose of https://github.com/iotaledger/access/pull/107/commits/011c581624c5aa28d8e301fb83d51c64b3f4a4fc. please, let's keep wallet representation only inside pep_plugin_wallet.{c,h} and pip_plugin_wallet.{c,h}

bernardoaraujor commented 4 years ago

@strahinjagolic + @vlad-ns I'm marking as solved the subtasks that are being handled on #113, as well as the one solved by #114

strahinjagolic commented 4 years ago

@bernardoaraujor

rename resolver to pep_plugin: follow 92335c2 rename storage to pap_plugin: follow c35a1f4

These two are not covered by #113 I will handle them.

djordjeglbvc commented 4 years ago

@bernardoaraujor

rename resolver to pep_plugin: follow 92335c2 rename storage to pap_plugin: follow c35a1f4

These two are not covered by #113 I will handle them.

Maybe better to create new issue just for renaming. I think it is better to have more small tasks when possible, makes progress tracking easier.

bernardoaraujor commented 4 years ago

done, check #115

djordjeglbvc commented 4 years ago

@bernardoaraujor Regarding bullet point 2 (register PEP callback on access_init), I've added access apis for registering pap, pep and pip plugins, and they are all registered in app supervisor (check 8f71fcb5ffd24b2f828f2225b5dde97ee4263c44). This made more sense to me, as it is left to SDK user's decision to choose and/or implement plugins for each access subsystem. If this is ok with you, point 2 can be marked as finished.

bernardoaraujor commented 4 years ago

that's ok @djordjeglbvc