Skip to content

sm_ipt: Add Shared Memory Inter-Processor Transport#524

Closed
dchat-nordic wants to merge 1 commit intonrfconnect:mainfrom
dchat-nordic:add_sm_ipt
Closed

sm_ipt: Add Shared Memory Inter-Processor Transport#524
dchat-nordic wants to merge 1 commit intonrfconnect:mainfrom
dchat-nordic:add_sm_ipt

Conversation

@dchat-nordic
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor

doki-nordic commented Aug 3, 2021

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 KAGA164, carlescufi, doki-nordic, grochu and hubertmis and removed request for hubertmis August 5, 2021 08:39
@doki-nordic
Copy link
Copy Markdown
Contributor

doki-nordic commented Aug 6, 2021

@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
#include <stdbool.h>

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

@doki-nordic doki-nordic Aug 6, 2021

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.

@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
Copy Markdown
Contributor

doki-nordic commented Aug 6, 2021

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
Copy Markdown
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
@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
doki-nordic
doki-nordic requested changes Aug 23, 2021
View reviewed changes
@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
@doki-nordic doki-nordic requested a review from peknis August 23, 2021 13:45
@doki-nordic
Copy link
Copy Markdown
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
@dchat-nordic dchat-nordic requested a review from peknis August 24, 2021 07:57
hubertmis
hubertmis suggested changes Aug 26, 2021
View reviewed changes
Copy link
Copy Markdown

@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
Copy Markdown
Contributor

@doki-nordic doki-nordic Aug 27, 2021

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
Copy Markdown
Contributor Author

@dchat-nordic dchat-nordic Aug 30, 2021

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
Copy Markdown
Contributor

carlescufi commented Nov 3, 2021

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

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

+2 more reviewers

@hubertmis hubertmis hubertmis requested changes

@KAGA164 KAGA164 KAGA164 approved these changes

Reviewers whose approvals may not affect merge requirements

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