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

sm_ipt: Provide zephyr porting layer #5191

Closed
wants to merge 1 commit into from

Conversation

dchat-nordic
Copy link
Contributor

@dchat-nordic dchat-nordic commented Jul 27, 2021

Updated zephyr porting layer for
nrf_rpc.
Added zephyr layer for sm_ipt (shared memory
inter-processor transport).

Depends on:
nrfconnect/sdk-nrfxlib#524
nrfconnect/sdk-nrfxlib#525

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

@CLAassistant
Copy link

CLAassistant commented Jul 27, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added CI-matter-test changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jul 27, 2021
@NordicBuilder NordicBuilder added the external External contribution label Jul 27, 2021
@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@nrfconnect nrfconnect deleted a comment from NordicBuilder Aug 10, 2021
@dchat-nordic dchat-nordic removed the external External contribution label Aug 10, 2021
@KAGA164 KAGA164 requested review from KAGA164 and doki-nordic August 11, 2021 09:55
@@ -49,3 +49,4 @@ add_subdirectory(debug)
add_subdirectory(partition_manager)

add_subdirectory_ifdef(CONFIG_NRF_RPC nrf_rpc)
add_subdirectory(sm_ipt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be added always ?


zephyr_library()

zephyr_library_sources_ifdef(CONFIG_SM_IPT sm_ipt_os.c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just:

Suggested change
zephyr_library_sources_ifdef(CONFIG_SM_IPT sm_ipt_os.c)
zephyr_library_sources(sm_ipt_os.c)

Add conditionally add sm_ipt directory CONFIG_SM_IPT

typedef struct k_mutex sm_ipt_os_mutex_t;
#define sm_ipt_os_mutex_init k_mutex_init
#define sm_ipt_os_unlock k_mutex_unlock
static inline void sm_ipt_os_lock(sm_ipt_os_mutex_t *mutex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add blank line above


typedef struct k_sem sm_ipt_os_sem_t;
#define sm_ipt_os_give k_sem_give
static inline void sm_ipt_os_sem_init(sm_ipt_os_sem_t *sem) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add blank line above

static void ipm_callback(const struct device *ipmdev, void *user_data, uint32_t id,
volatile void *data)
{
struct sm_ipt_os_ctx *os_ctx;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
struct sm_ipt_os_ctx *os_ctx;
struct sm_ipt_os_ctx *os_ctx = (struct sm_ipt_os_ctx *) user_data;

Add you can remove next line

struct sm_ipt_os_ctx *os_ctx;
os_ctx = (struct sm_ipt_os_ctx *) user_data;

if (os_ctx->signal_handler != NULL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (os_ctx->signal_handler != NULL)
if (os_ctx->signal_handler)

int sm_ipt_os_init(struct sm_ipt_os_ctx *os_ctx)
{
const struct device *ipm_rx_handle;
uint32_t size = (SHM_SIZE / 8) * 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use solution from Zephyr

@@ -15,7 +15,17 @@
#define _NRF_RPC_LOG_REGISTER1(module) \
_NRF_RPC_LOG_REGISTER2(module)

#define _NRF_RPC_LOG_DECLARE2(module) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need those two levels of macro ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I were to use only level 2, then "CONFIG_ ## module ## _LOG_LEVEL", would result in appending name of module macro, not its value. Level 1 makes sure to pass only value of level 1s define.

@github-actions
Copy link

github-actions bot commented Aug 20, 2021

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
nrfxlib nrfconnect/sdk-nrfxlib@d048f58 nrfconnect/sdk-nrfxlib#525 nrfconnect/sdk-nrfxlib#525/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@github-actions github-actions bot added CI-all-test Run All integration tests and removed CI-all-test Run All integration tests labels Aug 20, 2021
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020 Nordic Semiconductor ASA
* Copyright (c) 2020-2021 Nordic Semiconductor ASA
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't update date, if the file was not changed.

west.yml Outdated
@@ -104,7 +104,7 @@ manifest:
- name: nrfxlib
repo-path: sdk-nrfxlib
path: nrfxlib
revision: 59179ff80cee01c4feb8b747e0ff9caecb55c92d
revision: d74c71a0362baa906031c24275a6bab110ef31fc
Copy link
Contributor

Choose a reason for hiding this comment

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

More convenient is following syntax. If you use it, you don't need to update this line each time dependent PR changes:

revision: pull/524/head

When dependent PR goes into master, you can remove this change.

@github-actions github-actions bot added CI-all-test Run All integration tests DNM and removed CI-all-test Run All integration tests labels Aug 25, 2021
@github-actions github-actions bot removed the CI-all-test Run All integration tests label Aug 26, 2021
Updated zephyr porting layer for
nrf_rpc.
Added zephyr layer for sm_ipt (shared memory
inter-processor transport).

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

Co-authored-by: Dominik Kilian <Dominik.Kilian@nordicsemi.no>
@github-actions github-actions bot added the CI-all-test Run All integration tests label Aug 26, 2021
@DatGizmo
Copy link
Contributor

With the new Integration Test Specification process we don't need the github labels.
I will remove them.

Right now this PR runs all Integration tests because it changes sdk-nrfxlib in west.yml and there is no test-spec.yml in that repo yet.
You can add changes to this PR nrfconnect/sdk-nrfxlib#687 to make the test spec here more targeted

To get the test spec feature working correctly on this PR, you need to rebase it to the latest main to get the .github/test-spec.yml file

Then run CI here to update the test spec.

Information about the test spec feature
CI-Help for issues

@DatGizmo DatGizmo removed CI-all-test Run All integration tests CI-matter-test labels Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. DNM manifest manifest-nrfxlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants