EmbeddedRPC / erpc

Embedded RPC
https://github.com/EmbeddedRPC/erpc/wiki
BSD 3-Clause "New" or "Revised" License
704 stars 202 forks source link

erpc_arbitrated_client_init double call when ERPC_ALLOCATION_POLICY == ERPC_ALLOCATION_POLICY_STATIC #345

Open Diagano opened 1 year ago

Diagano commented 1 year ago

Hi, erpc_arbitrated_client_init double call when ERPC_ALLOCATION_POLICY == ERPC_ALLOCATION_POLICY_STATIC will erase the global pointer to the client even while the client and all its static resources have not been altered

The code seemingly attempts to identify\handle this case (No assert):

#if ERPC_ALLOCATION_POLICY == ERPC_ALLOCATION_POLICY_STATIC
    if (s_codecFactory.isUsed() || s_codec.isUsed() || s_arbitrator.isUsed() || s_crc16.isUsed() || s_client.isUsed())
    {
        client = NULL;
    }

But then goes on and overwrites the g_client regardless: g_client = dynamic_cast<ClientManager *>(client);

(Actually the dynamic flow is rather similar, there is no equivalent check and g_client will simply point to the latest one) So two questions here: If this is considered an acceptable flow (I assume, no assert here) is this "clear" of g_client an intended side effect? Is it possibly preferable to also clear g_client at the end of erpc_arbitrated_client_deinit?

(actually while at it... in erpc_arbitrated_client_deinit()

    erpc_assert(client == client.get());
    s_codecFactory.destroy();
    crc16.destroy();
    s_codec.destroy();
    s_arbitrator.destroy();
    s_client.destroy();

crc16.destroy() - wasnt this supposed to be s_crc16.destroy()? was is supposed to be erpc_assert(client == s_client.get());

Hadatko commented 1 year ago

I missed you created two issues. So i will link this one to the PR

Hadatko commented 1 year ago

This is prepreparation for multiple client support. Currently only one client can be used.