PR to issue #143: Assert costumizing #148

willmann9527 · 2020-11-03T16:54:51Z

Modifications to set up a proprietary assert function or to disable asserts (empty define).
The code has not been build cause I have no opportunitie to do it, but it should run as usual.

willmann9527 · 2020-11-16T09:19:04Z

Is there any action from my side needed?

Hadatko · 2020-11-16T09:34:13Z

Is there any action from my side needed?

Sorry currently i don't have time for review and i gues @MichalPrincNXP also. @MichalPrincNXP could you put some estimation here?

MichalPrincNXP · 2020-11-18T09:31:24Z

Hello @willmann9527 , I am sorry I am not able to focus on your PR now. Please, give me some time, I will get back to you, OK?

Hadatko · 2022-01-02T15:42:09Z

Hello, sorry that we didn't focus on your PR for such a long time. I like your solution but currently there are conflicts and redundant imports. Can you update your PR? Sorry once more.

willmann9527 · 2022-01-26T14:23:19Z

I fixed it locally but do not have the permission to push. Could you allow me?

Hadatko · 2022-01-26T14:25:11Z

Hi @willmann9527 we don't have any permission to your branch. You need update your branch and push into your branch same as current one or create new one

Hadatko · 2022-01-26T14:27:06Z

This is your fork and you are owner of it https://github.com/willmann9527/erpc/tree/Assert_costumizing_%23143

…_#143 # Conflicts: # erpc_c/infra/erpc_arbitrated_client_manager.cpp # erpc_c/infra/erpc_basic_codec.cpp # erpc_c/infra/erpc_client_manager.cpp # erpc_c/infra/erpc_framed_transport.cpp # erpc_c/infra/erpc_message_buffer.cpp # erpc_c/infra/erpc_transport_arbitrator.cpp # erpc_c/port/erpc_setup_extensions_freertos.cpp # erpc_c/setup/erpc_setup_mbf_dynamic.cpp # erpc_c/setup/erpc_setup_mbf_static.cpp # erpc_c/transports/erpc_rpmsg_lite_rtos_transport.cpp # erpc_c/transports/erpc_rpmsg_tty_rtos_transport.cpp

Hadatko · 2022-01-26T15:15:48Z

Build didn't pass. Could you try make clean && make all beffore push?

Hadatko · 2022-01-26T15:44:39Z

Build didn't pass. Could you try make clean && make all beffore push?

willmann9527 · 2022-01-26T15:48:29Z

Where is the project you are running to build the code?

Hadatko · 2022-01-26T15:53:43Z

@willmann9527 i am not sure what do you mean. For building PR request we are using circleci. Build is triggered automatically and you should be able to see result and details

Hadatko · 2022-01-26T16:13:54Z

i would improve this commit. 639d72a

Better way to implement is add note in configs that this option exists:
all erpc_config.h:
//! @name Assert function definition
//@{
//! User custom asser defition. Include header file if needed before bellow line. If assert is not enabled, default will be used.
// #define erpc_assert(condition)
//@}

And default:
erpc_config_internal.h:
#if !defined(erpc_assert)
#include <cassert>
#define erpc_assert(condition) assert(condition) //!< Assert function.
#endif

Hadatko · 2022-01-26T16:39:22Z

This is not good change: 973987e

Please read once again my proposal. If you have any question ask me. Thank you ;)

willmann9527 · 2022-01-26T16:50:02Z

The thing is that I have already replaced all asserts by erpc_assert. In your proposal we are going to get a lot of build errors. In addition, it is not possible to customize the assert. Or do I overlook something?

willmann9527 · 2022-01-26T16:58:01Z

erpc_config_internal.h is not included in all these files.

Hadatko · 2022-01-26T23:02:42Z

@willmann9527 So the thing is that erpc_config.h contains user definition. erpc_config_internal.h is including erpc_config.h and creates default definition for user for variables which were not defined by user in erpc_config.h. So that is the reason why i wrote how it should looks in erpc_config_internal.h (default definition) and input commented code in erpc_config.h. Our proposal will work for most of peaople without noticing change. But your current solution will bring conflicts for all users as they will not have declared erpc_assert variable.

erpc_config_internal.h is included in many files and it can be included trough other includes.... So most of files where you did change doesn't need new include.

erpc_config.h => erpc_config_internal.h => others files

Hadatko · 2022-01-27T09:37:18Z

@willmann9527 You are almost at the end of this PR. Thank you for your patiente. Now based on my suggestions please remove erpc_config.h includes (they are now even more redundant as before) and include "erpc_config_internal.h" where it has to be (i think only one place which will solve current build fail).

Hadatko · 2022-01-27T11:21:58Z

If you switched to section File changes, you will still see a lot of includes you didn't remove. Also there is one more suggestion to replace old assert with new one which you missed:
https://github.com/EmbeddedRPC/erpc/pull/148/files

Hadatko · 2022-01-28T09:45:37Z

Still around 2 requests to finish this task: https://github.com/EmbeddedRPC/erpc/pull/148/files

Hadatko · 2022-01-28T10:11:10Z

I hope it is ok i pushed some code into your branch ;) Now i think it is ready to merge. Thank you for your contribution.

Hadatko

@MichalPrincNXP I am approving this PR based on build result and my review.

erpc_c/infra/erpc_arbitrated_client_manager.cpp

erpc_c/infra/erpc_basic_codec.cpp

erpc_c/infra/erpc_client_manager.cpp

erpc_c/infra/erpc_framed_transport.cpp

erpc_c/infra/erpc_message_buffer.cpp

erpc_c/transports/erpc_inter_thread_buffer_transport.cpp

erpc_c/transports/erpc_rpmsg_linux_transport.cpp

erpc_c/transports/erpc_rpmsg_lite_rtos_transport.cpp

erpc_c/transports/erpc_rpmsg_tty_rtos_transport.cpp

erpc_c/port/erpc_config_internal.h

willmann9527 · 2022-01-28T10:34:01Z

No worries, that is fine.

erpc_c/port/erpc_config_internal.h

MichalPrincNXP

How about the update of unit test files (test/common folder)? I would replaced assert by erpc_assert here as well.

Hadatko · 2022-01-31T08:32:22Z

How about the update of unit test files (test/common folder)? I would replaced assert by erpc_assert here as well.

I think i covered them all now.

MichalPrincNXP

All tests passed. Thank you for all the effort and the patience!

Assert costumizing EmbeddedRPC#143

0d758ed

willmann9527 changed the title ~~Assert costumizing #143~~ PR to issue #143: Assert costumizing Nov 3, 2020

MichalPrincNXP self-assigned this Nov 18, 2020

MichalPrincNXP added the enhancement label Nov 18, 2020

willmann9527 added 2 commits January 26, 2022 15:54

test

5201990

Fixed failed build.

8565311

willmann9527 added 2 commits January 26, 2022 16:58

Added missing include

dfc4045

Copied customized assert to all other config files.

639d72a

Improved erpc_assert config

973987e

Separate user and internal settings.

c984d28

Hadatko assigned Hadatko and unassigned MichalPrincNXP Jan 27, 2022

willmann9527 and others added 3 commits January 27, 2022 16:55

Update erpc_mu_transport.h

5615682

Removed erpc_config.h includes.

71aa1a8

Update erpc_mu_transport.h

31980ac

Hadatko added 4 commits January 28, 2022 10:57

Move erpc_assert definition to bottom

a862597

Added assert support for mbed os.

d6d1940

Use generic erpc_assert implementation

806814b

Added missing endif into macro condition

9b667eb

Hadatko approved these changes Jan 28, 2022

View reviewed changes

Hadatko requested a review from MichalPrincNXP January 28, 2022 10:14

Added support for Freertos configASSERT

c515837

MichalPrincNXP reviewed Jan 31, 2022

View reviewed changes

erpc_c/port/erpc_config_internal.h Outdated Show resolved Hide resolved

MichalPrincNXP reviewed Jan 31, 2022

View reviewed changes

Hadatko added 7 commits January 31, 2022 09:19

Fixed c vs c++ include

309d547

Update assert

17c30e8

Update assert

0e62039

Update assert

2af43b1

Update assert

bb0e967

Update assert

c194193

Update assert

d13143c

Update support for Freertos configASSERT

3f52e89

MichalPrincNXP approved these changes Jan 31, 2022

View reviewed changes

MichalPrincNXP merged commit d42a139 into EmbeddedRPC:develop Jan 31, 2022

Hadatko linked an issue Feb 4, 2022 that may be closed by this pull request

Assert costumizing #143

Closed

Hadatko removed a link to an issue Feb 4, 2022

Assert costumizing #143

Closed

Hadatko linked an issue Feb 4, 2022 that may be closed by this pull request

Assert costumizing #143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR to issue #143: Assert costumizing #148

PR to issue #143: Assert costumizing #148

willmann9527 commented Nov 3, 2020

willmann9527 commented Nov 16, 2020

Hadatko commented Nov 16, 2020

MichalPrincNXP commented Nov 18, 2020

Hadatko commented Jan 2, 2022

willmann9527 commented Jan 26, 2022

Hadatko commented Jan 26, 2022

Hadatko commented Jan 26, 2022

Hadatko commented Jan 26, 2022 •

edited

Loading

Hadatko commented Jan 26, 2022

willmann9527 commented Jan 26, 2022

Hadatko commented Jan 26, 2022

Hadatko commented Jan 26, 2022 •

edited

Loading

Hadatko commented Jan 26, 2022 •

edited

Loading

willmann9527 commented Jan 26, 2022

willmann9527 commented Jan 26, 2022

Hadatko commented Jan 26, 2022 •

edited

Loading

Hadatko commented Jan 27, 2022

Hadatko commented Jan 27, 2022

Hadatko commented Jan 28, 2022 •

edited

Loading

Hadatko commented Jan 28, 2022

Hadatko left a comment

willmann9527 commented Jan 28, 2022

MichalPrincNXP left a comment

Hadatko commented Jan 31, 2022

MichalPrincNXP left a comment

PR to issue #143: Assert costumizing #148

PR to issue #143: Assert costumizing #148

Conversation

willmann9527 commented Nov 3, 2020

willmann9527 commented Nov 16, 2020

Hadatko commented Nov 16, 2020

MichalPrincNXP commented Nov 18, 2020

Hadatko commented Jan 2, 2022

willmann9527 commented Jan 26, 2022

Hadatko commented Jan 26, 2022

Hadatko commented Jan 26, 2022

Hadatko commented Jan 26, 2022 • edited Loading

Hadatko commented Jan 26, 2022

willmann9527 commented Jan 26, 2022

Hadatko commented Jan 26, 2022

Hadatko commented Jan 26, 2022 • edited Loading

Hadatko commented Jan 26, 2022 • edited Loading

willmann9527 commented Jan 26, 2022

willmann9527 commented Jan 26, 2022

Hadatko commented Jan 26, 2022 • edited Loading

Hadatko commented Jan 27, 2022

Hadatko commented Jan 27, 2022

Hadatko commented Jan 28, 2022 • edited Loading

Hadatko commented Jan 28, 2022

Hadatko left a comment

Choose a reason for hiding this comment

willmann9527 commented Jan 28, 2022

MichalPrincNXP left a comment

Choose a reason for hiding this comment

Hadatko commented Jan 31, 2022

MichalPrincNXP left a comment

Choose a reason for hiding this comment

Hadatko commented Jan 26, 2022 •

edited

Loading

Hadatko commented Jan 26, 2022 •

edited

Loading

Hadatko commented Jan 26, 2022 •

edited

Loading

Hadatko commented Jan 26, 2022 •

edited

Loading

Hadatko commented Jan 28, 2022 •

edited

Loading