Skip to content

Conversation

@pgu-swir
Copy link
Contributor

I have done a port on another platform and during that, I've made a few changes that you might be interested in.

  • fix the deadlock in arbitrated reference here Arbitrated client stucks -> dead lock #93 by simply using the PHY layer timeout
  • some proposal on arbitrated for TCP as well where I think the client should not respond by success when the server is not connected. It created another deadlock

I'll do another PR for the platform specifics

@pgu-swir
Copy link
Contributor Author

I've added a TCP_NODELAY in the TCP transport layer as I think we want every packet to be send immediately. I've also set it in the server after the accept() and not on the main serverSocket b/c some stack do not inherit parameters (I'm not even sure this is a standard behavior)

@pgu-swir
Copy link
Contributor Author

I saw the test failed on gcc/amd64/Linux but it's strange as all what is added is TCP_NODELAY and the build works, just the test fails (other OS/compilers seem to work). Could it be that now with TCP_NODELAY the response is coming to fast and the issue is in the test itself? Previously, the Nagle's algorithm was causing a fair bit of delay in TCP transport

@pgu-swir
Copy link
Contributor Author

Let me know if more comments/explanations are needed

@Hadatko
Copy link
Member

Hadatko commented Sep 28, 2020

Need be choose which changes we want to pull into official repo and close we don't want (or ask for changes). Related pullrequest:
#90
#33

@pgu-swir
Copy link
Contributor Author

pgu-swir commented Oct 5, 2020

It's really up to you. My changes include client & server TCP_NODELAY which was missing in the #33 patch. For #90, it's addressing the issue a bit differently. I've added a "full init" function that includes the "isServer" bool function as well as separated "configure" and "init" function. It does, I think, what #90 was aiming at.

Copy link
Member

@MichalPrincNXP MichalPrincNXP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @pgu-swir , @Hadatko ,
let we share my view about this PR. In general, I find this PR more "advanced" when comparing with #90 (no response from the author, focus on tcp setup only) and #33 (TCP_NODELAY socket flag focus only). To be honest I am not able to judge if this #121 also solves the MAC OS issue being discussed in #33, hope it does, @mentaal would you be able to test it on your side, please?
Additional minor notes:

  • I have triggered Travis build on this PR again and it passed
  • Internal testing passed too
  • I am not sure about the amount of setup functions, to keep it clean for users I would provide just erpc_transport_tcp_init_full(), are the other func. really needed, is there a case when the user would need to perform each individual steps separately instead of calling just one erpc_transport_tcp_init_full() API?
  • minor merge conflict in erpc_tcp_transport.cpp needs to be solved
  • erpc_manually_constructed.h: I do not think this change is needed as there is no construct with 6 parameters, have I missed sth.?

@pgu-swir
Copy link
Contributor Author

pgu-swir commented Oct 6, 2020

Hello @pgu-swir , @Hadatko ,

  • I am not sure about the amount of setup functions, to keep it clean for users I would provide just erpc_transport_tcp_init_full(), are the other func. really needed, is there a case when the user would need to perform each individual steps separately instead of calling just one erpc_transport_tcp_init_full() API?

I would tend to agree, I provided these initially thinking that one might want the open/close of the server (socket) without rebuilding a server or initializing without opening the socket immediately. But in my own example, I always do the init/configure/open = init_full alltogether, so I could rename the 'init_full' version to 'init' and just keep this one

  • minor merge conflict in erpc_tcp_transport.cpp needs to be solved

I can do that, it's just that afaik, C++ allows initialization of struct with empty braces (C does not) so I usually do that to avoid a memset call. But I can revert that if you prefer

  • erpc_manually_constructed.h: I do not think this change is needed as there is no construct with 6 parameters, have I missed sth.?

You're correct, it was it is for a different patch that I have ready now where eRPC is ported to another processor where I need these 6 parameters. I just put them as a provision

@MichalPrincNXP
Copy link
Member

Hello @pgu-swir , @Hadatko ,

  • I am not sure about the amount of setup functions, to keep it clean for users I would provide just erpc_transport_tcp_init_full(), are the other func. really needed, is there a case when the user would need to perform each individual steps separately instead of calling just one erpc_transport_tcp_init_full() API?

I would tend to agree, I provided these initially thinking that one might want the open/close of the server (socket) without rebuilding a server or initializing without opening the socket immediately. But in my own example, I always do the init/configure/open = init_full alltogether, so I could rename the 'init_full' version to 'init' and just keep this one

  • minor merge conflict in erpc_tcp_transport.cpp needs to be solved

I can do that, it's just that afaik, C++ allows initialization of struct with empty braces (C does not) so I usually do that to avoid a memset call. But I can revert that if you prefer

  • erpc_manually_constructed.h: I do not think this change is needed as there is no construct with 6 parameters, have I missed sth.?

You're correct, it was it is for a different patch that I have ready now where eRPC is ported to another processor where I need these 6 parameters. I just put them as a provision

Thank you, would you be able to rebase, solve the conflict and update the code as mentioned above till this Thursday? There is internal code freeze on Friday and I would like to have your PR being part of the next 1.8.0 release. Thanks a lot.
Michal

@pgu-swir
Copy link
Contributor Author

pgu-swir commented Oct 7, 2020

Thank you, would you be able to rebase, solve the conflict and update the code as mentioned above till this Thursday? There is internal code freeze on Friday and I would like to have your PR being part of the next 1.8.0 release. Thanks a lot.
Michal

Done - I had a 'moment' with some of the rebasing but the changes are the ones expected

@pgu-swir pgu-swir force-pushed the basic-changes branch 2 times, most recently from 6198692 to 5431461 Compare October 7, 2020 22:42
@MichalPrincNXP MichalPrincNXP merged commit 0cf20ee into EmbeddedRPC:develop Oct 8, 2020
@MichalPrincNXP
Copy link
Member

@pgu-swir , thanks a lot for your effort!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants