Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/static api #14

Merged

Conversation

Hadatko
Copy link
Contributor

@Hadatko Hadatko commented Mar 10, 2021

Hi @MichalPrincNXP ,
this is WIP PR. It is not best and it is touching other platforms/env. I was working in nxp directory structure so some cases, which are now different (some files are generic here, but in nxp repo they looked env/platform specific) need be fixed. Could you propose me suggestions for this PR? Thank you.

Related to #13

@MichalPrincNXP
Copy link
Contributor

Hi Dusan, thank you, the proposal looks ok to me. I have couple of comments that will be added into the code review.

@MichalPrincNXP
Copy link
Contributor

Also, I am afraid we could get into troubles when having multiinstance. I am writing it here as this needs to be analyzed.

@Hadatko
Copy link
Contributor Author

Hadatko commented Mar 19, 2021

Also, I am afraid we could get into troubles when having multiinstance. I am writing it here as this needs to be analyzed.

Sure, from my point of you, it shouldn't be worse or better now in multiinstance case. I was trying to keep static context in same scope as semaphore, mutex,...

@MichalPrincNXP
Copy link
Contributor

Dusan, as per this instruction, could you please allow me to edit this PR. I would like to pass the update from my side to be reviewed and tested by you? Do you agree? Thank you.

@Hadatko
Copy link
Contributor Author

Hadatko commented Sep 15, 2021

Sure, i agree, just i don't see simple way how can you push directly to this branch.

@Hadatko
Copy link
Contributor Author

Hadatko commented Sep 15, 2021

I cannot see the checkbox for allowing that. Maybe due to: isaacs/github#1681

Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
Copy link
Contributor

@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.

  1. rpmsg_queue_rx_cb_data_t typedef placement needs to be finalized
  2. #include "rpmsg_default_config.h" needs to be added into all rpmsg_env_specific.h

lib/include/rpmsg_env.h Outdated Show resolved Hide resolved
lib/include/rpmsg_lite.h Show resolved Hide resolved
lib/include/rpmsg_env.h Show resolved Hide resolved
Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
Signed-off-by: Cervenka Dusan <cervenka@acrios.com>
@Hadatko
Copy link
Contributor Author

Hadatko commented Sep 16, 2021

Is it ok now?

@MichalPrincNXP
Copy link
Contributor

Is it ok now?

I think so, let me retest on my side once again, ok?

@Hadatko
Copy link
Contributor Author

Hadatko commented Sep 16, 2021

Sure.

@MichalPrincNXP MichalPrincNXP merged commit 5fe561d into nxp-mcuxpresso:master Sep 17, 2021
@MichalPrincNXP
Copy link
Contributor

Thank you for the effort, Dusan!

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

Successfully merging this pull request may close these issues.

2 participants