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.

Sign up for GitHub

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

Jump to bottom

sm_ipt: Add Shared Memory Inter-Processor Transport #524

Closed
wants to merge 1 commit into from
Closed

sm_ipt: Add Shared Memory Inter-Processor Transport #524

wants to merge 1 commit into from

Conversation

dchat-nordic
Copy link
Contributor

@dchat-nordic dchat-nordic commented Jul 26, 2021 •
edited
Loading

SM IPT is an OS independent shared memory
communication library, which works on shared memory
and provides os-porting templates.

Signed-off-by: Dominik Chat dominik.chat@nordicsemi.no

Co-authored-by: Dominik Kilian Dominik.Kilian@nordicsemi.no

@CLAassistant
Copy link

CLAassistant commented Jul 26, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

This was referenced Jul 27, 2021
Closed
Closed
carlescufi
carlescufi previously requested changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

NACKing for now until I learn more about the purpose of this library

@doki-nordic
Copy link
Contributor

Main purpose of this library is a backend for nRF RPC. Additionally, it can be later used as a backend for IPC Service.

Main benefits of this library (compared to current RPMsg solution):

  • Can be easily ported to platforms other than Zephyr. One of the nRF RPC goals is to support multiple OSes or even baremetal. Currently there is only Zephyr/RPMsg backed which is not simply portable.
  • Better performance. @dchat-nordic did preliminary measurements and RPC calls are ~30% faster. It is achieved by:
    • Simpler implementation (it has less CPU overhead).
    • It uses zero-copy approach to reduce memory transfers.
  • Less memory consumption:
    • zero-copy approach requires no additional buffers,
    • No need for additional receiving thread and its stack. Specific nRF RPC thread is triggered directly from the interrupt.
  • Arriving data can be processed in any order. This makes nRF RPC less blocking.

@dchat-nordic dchat-nordic force-pushed the add_sm_ipt branch 6 times, most recently from 68d59bd to 989537d Compare August 5, 2021 08:35
@dchat-nordic dchat-nordic marked this pull request as ready for review August 5, 2021 08:38
@dchat-nordic dchat-nordic requested a review from tejlmand as a code owner August 5, 2021 08:38
@dchat-nordic dchat-nordic requested review from doki-nordic, carlescufi, KAGA164, hubertmis and grochu and removed request for hubertmis August 5, 2021 08:39
@doki-nordic
Copy link
Contributor

@dchat-nordic, please update commit message, to follow commit guidelines:

  • Change commit author name to match "Signed-off-by" in the commit message
  • Put single empty line after commit title and before commit details.

doki-nordic
doki-nordic requested changes Aug 6, 2021
View reviewed changes
sm_ipt/sm_ipt.c Outdated Show resolved Hide resolved
sm_ipt/sm_ipt.c Outdated Show resolved Hide resolved
sm_ipt/sm_ipt.c
#include <stdbool.h>

#include "sm_ipt.h"
#include "sm_ipt_log.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

(just a side comment)
We are repeating the same pattern for logs here as in nRF RPC. We can consider some common solution for them, but not necessary now in this PR.

sm_ipt/sm_ipt.c Show resolved Hide resolved
sm_ipt/template/sm_ipt_os_tmpl.h Outdated Show resolved Hide resolved
sm_ipt/template/sm_ipt_log_tmpl.h Outdated Show resolved Hide resolved
sm_ipt/template/sm_ipt_log_tmpl.h Show resolved Hide resolved
@doki-nordic doki-nordic requested a review from peknis August 6, 2021 12:06
@doki-nordic doki-nordic added the DNM Do not merge label Aug 6, 2021
@doki-nordic
Copy link
Contributor

This PR requires high level documentation.

@dchat-nordic dchat-nordic requested a review from b-gent as a code owner August 9, 2021 11:51
@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Aug 9, 2021
@dchat-nordic dchat-nordic force-pushed the add_sm_ipt branch 2 times, most recently from 8e04175 to 0f86928 Compare August 9, 2021 12:02
@dchat-nordic dchat-nordic requested a review from peknis August 19, 2021 07:20
@peknis
Copy link
Contributor

peknis commented Aug 19, 2021

Please, include also the original image (Visio file?). Someone may want to adjust it at some point. Also, pay attention to the colors and shapes as instructed in https://projecttools.nordicsemi.no/confluence/display/TECHDOC/Figure+guide.

When I acquire Visio I will attach Visio file.

It's a pity you cannot get the Visio file. However, the image looks a lot better now and is aligned with the figure guidelines. I think we can live without the native format so far.

peknis
peknis requested changes Aug 19, 2021
View reviewed changes
sm_ipt/doc/usage.rst Outdated Show resolved Hide resolved
@dchat-nordic dchat-nordic requested a review from peknis August 19, 2021 09:33
KAGA164
KAGA164 approved these changes Aug 20, 2021
View reviewed changes
sm_ipt/sm_ipt.c Show resolved Hide resolved
doki-nordic
doki-nordic requested changes Aug 23, 2021
View reviewed changes
sm_ipt/doc/img/architecture.svg Outdated Show resolved Hide resolved
sm_ipt/doc/usage.rst Show resolved Hide resolved
sm_ipt/doc/usage.rst Outdated Show resolved Hide resolved
sm_ipt/doc/usage.rst Outdated Show resolved Hide resolved
sm_ipt/doc/usage.rst Outdated Show resolved Hide resolved
sm_ipt/sm_ipt.c Show resolved Hide resolved
sm_ipt/sm_ipt.c Show resolved Hide resolved
sm_ipt/sm_ipt.c Outdated Show resolved Hide resolved
sm_ipt/sm_ipt.c Outdated Show resolved Hide resolved
sm_ipt/sm_ipt.c Outdated Show resolved Hide resolved
@doki-nordic doki-nordic removed the DNM Do not merge label Aug 23, 2021
@dchat-nordic dchat-nordic force-pushed the add_sm_ipt branch 3 times, most recently from b989777 to 390a82a Compare August 23, 2021 13:24
doki-nordic
doki-nordic requested changes Aug 23, 2021
View reviewed changes
sm_ipt/doc/usage.rst Outdated Show resolved Hide resolved
@doki-nordic doki-nordic requested a review from peknis August 23, 2021 13:45
@doki-nordic
Copy link
Contributor

doki-nordic commented Aug 23, 2021 •
edited
Loading

@peknis, There are small changes in usage.rst since you last review (https://github.com/nrfconnect/sdk-nrfxlib/compare/129c3ec..0f500c5)

Can you look at it?

peknis
peknis requested changes Aug 24, 2021
View reviewed changes
sm_ipt/doc/usage.rst Outdated Show resolved Hide resolved
sm_ipt/doc/usage.rst Outdated Show resolved Hide resolved
@dchat-nordic dchat-nordic requested a review from peknis August 24, 2021 07:57
hubertmis
hubertmis requested changes Aug 26, 2021
View reviewed changes
Copy link
Contributor

@hubertmis hubertmis left a comment

Choose a reason for hiding this comment

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

I have some concerns about strategy for IPC transport availability in nRF Connect SDK. I believe we need to align on long-term plans before we merge a new transport to the repository.

doki-nordic
doki-nordic reviewed Aug 27, 2021
View reviewed changes
sm_ipt/sm_ipt.c
tx = ctx->out.shm_queue->queue_tx;
dst = tx;

if (dst >= QUEUE_ITEMS_CNT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should unlock the mutex just before memory_corrupted_error();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dchat-nordic @doki-nordic
sm_ipt: Add Shared Memory Inter-Processor Transport
c55b8b8 
SM_IPT is OS independent shared memory
communication protocol, which works on shared memory
and provides os-porting header templates.

Signed-off-by: Dominik Chat <dominik.chat@nordicsemi.no>

Co-authored-by: Dominik Kilian <Dominik.Kilian@nordicsemi.no>
@carlescufi
Copy link
Contributor

Should we close this for now? It can always be reopened

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

@doki-nordic doki-nordic doki-nordic approved these changes

@hubertmis hubertmis hubertmis requested changes

@peknis peknis peknis approved these changes

@KAGA164 KAGA164 KAGA164 approved these changes

@tejlmand tejlmand Awaiting requested review from tejlmand tejlmand is a code owner

@carlescufi carlescufi Awaiting requested review from carlescufi carlescufi is a code owner

@grochu grochu Awaiting requested review from grochu

@b-gent b-gent Awaiting requested review from b-gent b-gent is a code owner

Assignees
No one assigned
Labels
doc-required PR must not be merged without tech writer approval.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dchat-nordic @CLAassistant @doki-nordic @hubertmis @peknis @carlescufi @KAGA164