Skip to content

Conversation

@hale2bopp
Copy link

No description provided.

@MichalPrincNXP
Copy link
Member

Hello @hale2bopp , it's great that you have been able to solve the #89 issue (caused by the so-far-not integrated code), you have got the erpc example running on MAC with TCP transport and thanks for providing this pull request.
I would accept this pull request when:

  • rebased to develop branch
  • removing the c_matrix_example folder (hard to maintain, add.erpc duplicated with examples\matrix_multiply_tcp_python\service\erpc_matrix_multiply.erpc, no need to store generated shim code, etc.)
  • #include and print-outs removed from erpc_setup_tcp.cpp (to be consistent with other sources)
  • erpc_transport_setup.h - update Doxygen description for erpc_transport_tcp_init functions, describe all parameters as done for other functions
  • erpc_setup_tcp.cpp - license text aligned with other source files (replacing the long license text by SPDX-License-Identifier: BSD-3-Clause)

Thank you
Michal

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.

As described in the conversation, I would accept this pull request when:

  • rebased to develop branch
  • removing the c_matrix_example folder (hard to maintain, add.erpc duplicated with examples\matrix_multiply_tcp_python\service\erpc_matrix_multiply.erpc, no need to store generated shim code, etc.)
  • #include and print-outs removed from erpc_setup_tcp.cpp (to be consistent with other sources)
  • erpc_transport_setup.h - update Doxygen description for erpc_transport_tcp_init functions, describe all parameters as done for other functions
  • erpc_setup_tcp.cpp - license text aligned with other source files (replacing the long license text by SPDX-License-Identifier: BSD-3-Clause)

Thank you
Michal

@hale2bopp
Copy link
Author

Hi Michal,

Thanks so much for reviewing my pull-request!

Could you clarify what you mean by the last point - erpc_setup_tcp.cpp - license text aligned with other source files (replacing the long license text by SPDX-License-Identifier: BSD-3-Clause) ?

@MichalPrincNXP
Copy link
Member

Hi Michal,

Thanks so much for reviewing my pull-request!

Could you clarify what you mean by the last point - erpc_setup_tcp.cpp - license text aligned with other source files (replacing the long license text by SPDX-License-Identifier: BSD-3-Clause) ?

Hi @hale2bopp , I meant if you could use the same license description (BSD-3-Clause) as done in other setup files, like erpc_setup_rpmsg_lite_master.cpp (for instance). Thanks.

@hale2bopp
Copy link
Author

Hi Michal,

Thanks for reviewing my pull request. I have made the following changes -

  • rebased to develop branch ✅
  • removing the c_matrix_example folder (hard to maintain, add.erpc duplicated with examples\matrix_multiply_tcp_python\service\erpc_matrix_multiply.erpc, no need to store generated shim code, etc.) ✅
  • #include and print-outs removed from erpc_setup_tcp.cpp (to be consistent with other sources) ✅
  • erpc_transport_setup.h - update Doxygen description for erpc_transport_tcp_init functions, describe all parameters as done for other functions ✅
  • erpc_setup_tcp.cpp - license text aligned with other source files (replacing the long license text by SPDX-License-Identifier: BSD-3-Clause) ✅

I then pushed the changes.

@MichalPrincNXP
Copy link
Member

Hello @hale2bopp ,
unfortunately, I am still not able to accept you pull request. It has not been rebased on the head of develop branch (the latest commit sha on develop branch is 9a82e6, see https://github.com/EmbeddedRPC/erpc/commits/develop), and also you have added one more example folder (freertos_cpp_example) instead of removing the c_matrix_example folder.

In fact, the pull request should contain just erpc_transport_setup.h modifications and new erpc_setup_tcp.cpp file adding.

Thank you

@hale2bopp
Copy link
Author

Hi Michal,

Appreciate your time in reviewing my pull request.

I've removed the extra folders - I've been trying to 'rebase to the develop branch' but it shows :

git status
rebase in progress; onto bd8bf57
You are currently rebasing branch 'c_matrix_example' on 'bd8bf57'.
(all conflicts fixed: run "git rebase --continue")

bd8bf57 is the sha for a previous commit on the develop branch - could you guide me in rebasing it to the latest commit on the develop branch?

@Hadatko
Copy link
Member

Hadatko commented Apr 13, 2020

Hi @hale2bopp , you likely didn't update develop branch on your forked repository.
https://stackoverflow.com/questions/48349103/update-my-repository-on-github
You need to add upstream repository reference (to origin repository) and then push commits from develop branch of upstream repository to your forked repository.

@MichalPrincNXP MichalPrincNXP changed the base branch from master to develop April 21, 2020 11:20
@MichalPrincNXP
Copy link
Member

Hello @hale2bopp ,
please note that I have changed the base branch of this pull request to "develop".
github_base_develop

Please sync changes from the develop branch (SHA-1: af02b3e is the develop head now) and rebase your work on this branch. Then get rid of all changes except of erpc_transport_setup.h modifications and new erpc_setup_tcp.cpp file. Then push your c_matrix_example branch again (git push --force will be required most probably). Thank you for the effort.

@MichalPrincNXP
Copy link
Member

Hello @hale2bopp , your erpc_transport_setup.h modifications and new erpc_setup_tcp.cpp file introduction could be useful for some erpc users and I am still ready to accept your pull request once correctly prepared (as described in previous conversation). Are you going to make these changes and finalize this pull request? Please let me know. Thank you.

@MichalPrincNXP
Copy link
Member

Fixed differently in #121

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.

4 participants