Feature/support multiple clients

[Hadatko]

Pull request

Choose Correct

• [ ] bug
• [X] feature

Describe the pull request

Better support of C++ usecases -> mainly multiple clients usage.

To Reproduce

Expected behavior

test_passing :D

Screenshots

Desktop (please complete the following information):

• OS:
• eRPC Version:

Steps you didn't forgot to do

• [X I checked if other PR isn't solving this issue.
• [X] I read Contribution details and did appropriate actions.
• [X] PR code is tested.
• [X] PR code is formatted.

Additional context

[Hadatko]

Currently WIP (alpha)

[Hadatko]

@MichalPrincNXP test_annotation can be compiled. I moved C related generated code into its own files. This way C++ code will be cleaner. Also more transparent for C develepors how the C++ code is wrapped.

[mnathieu]

Hi! I am really interested in the feature (especially the C++ version). Great addition. Are you still active on it? Any plan to add it to a future release?

[Hadatko]

HI @mnathieu , it is still my top priority. I had solution twice, but twice it was not complete due to issues with callbacks and c++. I am expecting that i will finish integration to the end of this year. Question is if documentation update will be part of it, or i will add documentation update next year. I guess release depend on that.

[Hadatko]

HI @MichalPrincNXP, FYI: tests are passing. Errors which appear are due to pytest which need to be modified and some clang compiler incompatibility. I need refactor code a bit , but that shouldn't be an huge change.

[tmordeko]

Hi @Hadatko we are looking forward for this feature , when do you plan to release it ?

[Hadatko]

Hi @tmordeko , i think i will be able to release it here (to develop branch) before March 2023. It looks working well now, but code need be edited a bit (formatted/refactored). Maybe release it in small steps (i already created few smaller PRs). Oficial release to master should be around June/July.

[Hadatko]

Hi @MichalPrincNXP could you take o look on this code? It should be ready more or less. If you would like it i would improve pytests and update documentation.

Hi @amgross i am curious about tour opinion too. Do you think you could review it as well?

One thing which i know is not good is generating common datatypes. I wanted to have two header files. One for c and one for c++. For c++ i wanted to hide them under namescope erpcShim. I had compilation issues which i think i would be able to solve (related const definition). But then i started to think if it is good idea and if i should rather use global namespace.

[amgross]

hi @Hadatko , I don't sure if I will have time for it, so maybe

[Hadatko]

@amgross No problem, i know it is quite big and can eat a lot of free time. I wanted just let you know about changes as you are really helping us with this project ;)

[amgross]

@Hadatko I suggest (for next time) not to mix typo and such in logical PR, at least for me it make code review harder, but it is up to your coding style of course

[PhilippHaefele]

Hi @Hadatko, I did some tests on this branch for using it as a inter task communication on an MCU with C++ and want to give some feedback.

The implementation is in general working in a small example i made and does the expected things 😃👍. The C++ worked out really nice so far and i was able to create multiple clients (and servers. Was the easiest for the beginning. I most likely will create an issue for how to do this in a better way). One thing that's maybe feasible in the generated C++ code is that I would like enums to be created as a enum class (maybe C++ enum is also a good choice, but i'm used to use enum class) instead of the c-style typedef one. I would really like that as it preserves from enum value name conflicts, as i try to remove the c-style enum value prefixes in C++. This should also help when using @external together with the new C++ interfaces.

I'm not sure if this is doable and still being able to have the corresponding C interface/type, but i think it's worth trying.

Thanks for your work and hoping that this MR get's out of draft state and merged soon 🙂

[Hadatko]

Hi @PhilippHaefele , i am really happy that i got your feedback. Enum class is something i would like to implement too (i was just thinking how to do it properly when C is used. maybe using static_cast<> should do trick). I wanted some review also from @MichalPrincNXP but he hadn't have time so far, but it should have next days.

[MichalPrincNXP]

Hi @PhilippHaefele , i am really happy that i got your feedback. Enum class is something i would like to implement too (i was just thinking how to do it properly when C is used. maybe using static_cast<> should do trick). I wanted some review also from @MichalPrincNXP but he hadn't have time so far, but it should have next days.

Hi Dusan, I started internal testing and except of some minor reported bugs all seems to be ok. It would be also good to have a new test case covering multiple clients scenario, but this could be added later. Please address the reported issues (or I can do the update if you want) and we can merge. Thank you.

[Hadatko]

Do you mean use enum classes? I will try to do it asap

[PhilippHaefele]

@Hadatko I did a quick compile and code check (lack of my hardware setup right now) and it should be enough to replace typedef enum with enum class in the generated code inside the _common.hpp (C++) file and leave the _common.h (C) as it is. The needed static_casts are already in place and should work out like before. Only the code generator is currently to smart for this as it detects enum values (from two different enums) with the same name: error: line 2: duplicate symbol with name 'OK' (original on line 5). That's totally right for the C implementation, but not for C++. So that should maybe only a warning that this will not work with C. Generated python code also seems to have scoped enums and should work as well.

[PhilippHaefele]

@Hadatko I did a quick compile and code check (lack of my hardware setup right now) and it should be enough to replace typedef enum with enum class in the generated code inside the _common.hpp (C++) file and leave the _common.h (C) as it is. The needed static_casts are already in place and should work out like before. Only the code generator is currently to smart for this as it detects enum values (from two different enums) with the same name: error: line 2: duplicate symbol with name 'OK' (original on line 5). That's totally right for the C implementation, but not for C++. So that should maybe only a warning that this will not work with C. Generated python code also seems to have scoped enums and should work as well.

I was able to work around the duplicate symbol of enum values using @external with just one dummy with a scoped name that does not exist in reality (see example below). As eRPC does convert enums to an int32_t there's not to much harm in doing this. So usable in the special case of external types with enum class as intended.

@external
enum DeviceServiceStatus {
    DeviceServiceStatus_Dummy = 0
}

[Hadatko]

@Hadatko I did a quick compile and code check (lack of my hardware setup right now) and it should be enough to replace typedef enum with enum class in the generated code inside the _common.hpp (C++) file and leave the _common.h (C) as it is. The needed static_casts are already in place and should work out like before. Only the code generator is currently to smart for this as it detects enum values (from two different enums) with the same name: error: line 2: duplicate symbol with name 'OK' (original on line 5). That's totally right for the C implementation, but not for C++. So that should maybe only a warning that this will not work with C. Generated python code also seems to have scoped enums and should work as well.

Hi @PhilippHaefele I would keep it as error (as we have for functions declarations too). Maybe we will change it in future but now our priority is C compilation. Maybe we can add some macro like @allowCPP, or we will add option in generator to decide if we want C or C++ implementation

[Hadatko]

@MichalPrincNXP added c++ enum classes. Not sure if there is anything left. Allow erpc to have enum members with same name in different enum types, function names in interfaces,... we can do in next release...

[Hadatko]

It looks like clang has an issue with this implementation. I know why, and i was close to solve it but i didn't finished it. I will try to do it tomorrow. (c++ scope versus c scope of variables)

[MichalPrincNXP]

@Hadatko , also please have a look at my review notes I have put into this PR thread (found during the on-board testing). Thanks

[Hadatko]

@MichalPrincNXP i don't see any. Did you finished review?

[MichalPrincNXP]

I can see my comments in this conversation. Anyway, here are links: https://github.com/EmbeddedRPC/erpc/pull/271/files/fed7be6c94fa0f280b9789136e0047a21772ccdd#diff-5234420183101463278ce44c006aa1a7387eb41045b3d002463e5c274e35c07d https://github.com/EmbeddedRPC/erpc/pull/271/files/fed7be6c94fa0f280b9789136e0047a21772ccdd#diff-e8da56bc258098c11512228e2ff58ef22e7cee0a6b872923c994591fe876c7d2 https://github.com/EmbeddedRPC/erpc/pull/271/files/fed7be6c94fa0f280b9789136e0047a21772ccdd#diff-e8da56bc258098c11512228e2ff58ef22e7cee0a6b872923c994591fe876c7d2

[Hadatko]

@MichalPrincNXP I cannot see any comment you provided. I guess you need finish reviewing to enabling them.

[MichalPrincNXP]

@MichalPrincNXP I cannot see any comment you provided. I guess you need finish reviewing to enabling them.

yes, I am sorry

[PhilippHaefele]

@Hadatko I did a quick compile and code check (lack of my hardware setup right now) and it should be enough to replace typedef enum with enum class in the generated code inside the _common.hpp (C++) file and leave the _common.h (C) as it is. The needed static_casts are already in place and should work out like before. Only the code generator is currently to smart for this as it detects enum values (from two different enums) with the same name: error: line 2: duplicate symbol with name 'OK' (original on line 5). That's totally right for the C implementation, but not for C++. So that should maybe only a warning that this will not work with C. Generated python code also seems to have scoped enums and should work as well.

Hi @PhilippHaefele I would keep it as error (as we have for functions declarations too). Maybe we will change it in future but now our priority is C compilation. Maybe we can add some macro like @allowCPP, or we will add option in generator to decide if we want C or C++ implementation

Sound reasonable and as I have the solution with @external for my usecase I'm fine with it. A generator option or IDL annotation you mentioned would still be nice for other use cases and users in the future.

Anything i can help you with?

[Hadatko]

@MichalPrincNXP i made changes based on your PR. Let me know if they met your requests. Also i enabled in clang config to keep empty lines between includes, which will simplify keeping includes on some specific position...

[Hadatko]

@MichalPrincNXP @PhilippHaefele for know i want finish this PR and we don't have much time so i reverted commit with enum classes support in shim code. And we can create new PR where we will solve that.

[MichalPrincNXP]

@MichalPrincNXP i made changes based on your PR. Let me know if they met your requests. Also i enabled in clang config to keep empty lines between includes, which will simplify keeping includes on some specific position...

Thank you, let me check it again on hw

[MichalPrincNXP]

Hi Dusan, I have successfully converted the NXP erpc_matrix_multipy example to use the latest generator and the erpc code. However, I am facing issues with the erpc_two_way_rpc example. First, incorrect includes are generated in several shim code files: #include "erpc_two_way_rpc_Core0Interface.h" -> #include "erpc_two_way_rpc_Core0Interface**_common**.h" Second, incorrect generated code in erpc_two_way_rpc_Core0Interface_server.cpp / Core0Interface_interface_getNumberCallback_t_shim function:

        if ((serviceID == ) && (functionID == 4))
        {
            m_handler->getNumberFromCore1(param1);
        }

Error[Pe029]: expected an expression	C:\D\work\sdk_w\mcu-sdk-2.0\middleware\multicore\example\multicore_examples\erpc_common\erpc_two_way_rpc\service\erpc_two_way_rpc_Core0Interface_server.cpp	53	
Error[Pe167]: argument of type "uint32_t" is incompatible with parameter of type "uint32_t *"	C:\D\work\sdk_w\mcu-sdk-2.0\middleware\multicore\example\multicore_examples\erpc_common\erpc_two_way_rpc\service\erpc_two_way_rpc_Core0Interface_server.cpp	55	

Could you please help to address these issues?