server plugins should not be limited to a single system

[dlech]

As I have been working on the camera server plugin, I've noticed a problem with the way the current plugin architecture creates a plugin instance per system (i.e. CameraServerImpl::CameraServerImpl(System& system) : PluginImplBase(system)). The problem is that the plugin state is per system when really it needs to be global. This means that consumers of MAVSDK have to do lots of extra work to ensure that each plugin instance for a single camera remains in a consistent state with the others. Example: 1528f0c.

I suppose other server plugins haven't noticed this problem before since they probably only communicate with one system. However, cameras need to communicate with both an autopilot and a ground station at the same time.

I think a solution to this would be to create a new "server" plugin type that is initialized with CameraServerImpl::CameraServerImpl(Mavsdk& mavsdk) : ServerPluginImplBase(mavsdk) instead. Then a single plugin instance could receive and send messages to any number of systems. (And broadcast to multiple systems as required by CAMERA_IMAGE_CAPTURED.)

Otherwise, we should reconsider mavlink/MAVSDK-Proto#274 (comment), particularly the Set* methods. Basically, the camera server plugin should have no state and everything is handled by callbacks to user code. I'm not sure this is really possible though since the callbacks themselves are considered part of the state.

[dayjaby]

What about just adding a constructor which takes a list of systems?

E.g.
https://github.com/dayjaby/MAVSDK/blob/e9bb9ecc87bc7c6b8abdd9c5d80e5d07ecf0e63a/src/core/plugin_impl_base.h
https://github.com/dayjaby/MAVSDK/blob/e9bb9ecc87bc7c6b8abdd9c5d80e5d07ecf0e63a/src/core/plugin_impl_base.cpp

We can still preserve the _parent field as most plugins use only a single system anyway.

[dlech]

I considered something like this as well. There would also have to be an additional method to add systems that were discovered after the plugin was created (or only this method and no new constructor). But that seems like it makes things a bit nebulous as to which plugins support multiple systems and which ones do not. And it also requires the user to implement boilerplate code to attach the systems to the plugin when they are discovered.

[julianoes]

Interesting! I think I understand the problem. So with normal plugins like action and telemetry, they are the representation of the system discovered, so a model of the thing that they are talking to. The server plugins however, are the system itself, or rather the mavsdk instance itself.

So, to me, it's almost like the server plugins are a level higher and instantiated directly from mavsdk, something like this:

I think it would be possible to build this but it will make the auto-generation story a little more complex.

Also, a lot of the internal APIs and functionality are currently in System and would probably have to move one level up to mavsdk. I think that's fine but it will be some churn.

To re-use my diagram above, use this link.

[dlech]

Yes that is the idea.

If we want one MAVSDK instance to do everything (multiple components on the MAVSDK system), it might look more like this.

link

[julianoes]

Hm, that's interesting. I think it makes sense your way because it allows to use one connection for all of them together. If we were to split up mavsdk per component it would mean that you could have two mavsdk instances with the same system ID but different component.

[julianoes]

@dlech FYI I just started some refactoring towards this, and I have a some roughly compiling WIP state: #1733, I'll probably continue in the next few days.

[julianoes]

That's fixed with the server components.