[android] replace VendorServer with OtDaemonServer

[wgtdkp]

To make it easier to update the OtDaemonServer implementation
since we have to save the OtDaemonServer as a field of Application
anyway.

[zhanglongxia]

LGTM

[DarthBo]

I liked the generic VendorServer though, we used it to hook our own api in... though the header did need patching to be usable.

Instead of replacing the generic VendorServer with something Android specific, could we not make the header more flexible so it's useable for everyone? Don't implement the constructor, and add some private storage.

The PIMPL pattern seems fitting for this?

class VendorServer
{
public:
    /**
     * The constructor of vendor server.
     *
     * @param[in] aNcp  A reference to the NCP controller.
     *
     */
    VendorServer(otbr::Ncp::ControllerOpenThread &aNcp);

    /**
     * This method initializes the vendor server.
     *
     */
    void Init(void);

private:
    class Impl;
    std::unique_ptr<Impl> mImpl;
};

[wgtdkp]

@DarthBo Can you edit your application.cpp directly without being proxied by VendorServer? As you mentioned, you already need to patch the header, so just forget about it. You can add your server just we do for the OtDaemonServer, but on your own branch (you can make whatever changes on your own branch anyway). Previous VendorServer code doesn't provide any abstraction so there isn't too much value other than keeping the github and aosp branches from diverge, but it shouldn't be an issue of your branch :)

[DarthBo]

@wgtdkp I could fork it, sure, but I'd prefer to avoid that.

We get around the issues of the header by replacing it with some CMake magic, no forks needed. I probably should have tried to upstream my suggested header changes earlier, sorry about that.

Before the Vendor Server, we used our own fork, and it was a pain to keep the stack updated because there was always something that changed. I'm sure you had the same problem in Android as well.

I thought the point of the VendorServer was to avoid this pain for all vendors. Replacing something generic (but flawed) with something android specific (instead of just fixing it) feels backwards, and not a little like cheating. I doubt every vendor can upstream their own ifdef...

[wgtdkp]

Hi @DarthBo, If you can't live without VendorServer, then it's proven to be useful and I am happy to revert this change :)

We get around the issues of the header by replacing it with some CMake magic, no forks needed. I probably should have tried to upstream my suggested header changes earlier, sorry about that.

I an curious how can you managed to work, are you using otbr-agent as a library of your vendor server? I don't see how it can work without modifying the upstream code.

[wgtdkp]

The PIMPL pattern seems fitting for this?

class VendorServer
{
public:
    /**
     * The constructor of vendor server.
     *
     * @param[in] aNcp  A reference to the NCP controller.
     *
     */
    VendorServer(otbr::Ncp::ControllerOpenThread &aNcp);

    /**
     * This method initializes the vendor server.
     *
     */
    void Init(void);

private:
    class Impl;
    std::unique_ptr<Impl> mImpl;
};

It looks like we need at least three classes to support a custom FooServer as I don't want to call my specific VendorServer as "Impl".

What about making VendorServer an abstract class instead?

class VendorServer {
public:
    static shared_ptr<VendorServer> newInstance();
    virtual void Init(void) = 0;
}

class FooServer: public VendorServer {
// Adds whatever implementation you need
}

class Application {
public:
    Application()
        : mVendorServer(VendorServer::newInstance()) {}

private:
    shared_ptr<VendorServer> mVendorServer;
}

So we needs only two classes.

BTW, I need shared_ptr for Android, does it work for you?

[wgtdkp]

Opening #1995 to make generic VendorServer. @DarthBo Can you help check if this resolves your issue?

[DarthBo]

I an curious how can you managed to work, are you using otbr-agent as a library of your vendor server? I don't see how it can work without modifying the upstream code.

We include ot-br-posix as a git submodule, the root project has the VendorServer implementation and links against otbr-agent. We override the header by including ours first with target_include_directories(otbr-agent BEFORE PRIVATE include_overrides). I'll be glad to no longer need that hack though.

What about making VendorServer an abstract class instead?

That also works for me

BTW, I need shared_ptr for Android, does it work for you?

Sure!