API Change: IPM - callback improvement #26780

tbursztyka · 2020-07-09T18:50:51Z

Problem Description:

ipm_callback_t do not provide the IPM device's callback caller as parameter, forcing users to use the context parameter instead for this purpose, which in turn limits its usage (they cannot provide some private data, or have to mangle thing together storing the IPM dev pointer into the private data).

Proposed Change (detailed):

All other API provide the callback's caller device pointer, and thus adding it as first parameter to ipm_callback_t

Dependencies:

All users of ipm_register_callback() function need to adapt their callbacks accordingly.

tbursztyka · 2020-07-10T11:55:22Z

@carlescufi let's bring that one up at the next API meeting as well

carlescufi · 2020-07-20T14:52:55Z

@nordic-krch and @anangl could you please take a look?

anangl · 2020-07-21T08:09:16Z

drivers/ipm/ipm_nrfx_ipc.c

@@ -36,7 +37,8 @@ static void nrfx_ipc_handler(uint32_t event_mask, void *p_context)
 			__ASSERT(event_idx < NRFX_IPC_ID_MAX_VALUE,
 				 "Illegal event_idx: %d", event_idx);
 			event_mask &= ~BIT(event_idx);
-			nrfx_ipm_data.callback(nrfx_ipm_data.callback_ctx,
+			nrfx_ipm_data.callback(nrfx_ipm_data.dev,


This is a single instance part of the driver, so you could use DEVICE_GET(ipm_nrf) here and avoid adding the dev field.

anangl · 2020-07-21T08:36:20Z

drivers/ipm/ipm_nrfx_ipc.c

@@ -133,7 +135,8 @@ static void vipm_dispatcher(uint32_t event_mask, void *p_context)
 		event_mask &= ~BIT(event_idx);
 		if (nrfx_vipm_data.callback[event_idx] != NULL) {
 			nrfx_vipm_data.callback[event_idx]
-				(nrfx_vipm_data.callback_ctx[event_idx],
+				(nrfx_vipm_data.ipm_device,


The ipm_device field is currently not initialized. It is actually not used at all (looks like it's some remnant of an earlier version of the driver). You'll need to initialize it in vipm_nrf_init() to use it this way (and perhaps rename it to dev, to avoid confusion).

indeed, done

I'm sorry, now I see I was wrong in my previous comment. This field should be changed to an array of struct device * and its entries should be initialized in vipm_nrf_##_idx##_register_callback() (like callback[_idx] and callback_ctx[_idx] are), not in vipm_nrf_init().

anangl · 2020-07-27T13:04:30Z

drivers/ipm/ipm_nrfx_ipc.c

@@ -36,7 +36,8 @@ static void nrfx_ipc_handler(uint32_t event_mask, void *p_context)
 			__ASSERT(event_idx < NRFX_IPC_ID_MAX_VALUE,
 				 "Illegal event_idx: %d", event_idx);
 			event_mask &= ~BIT(event_idx);
-			nrfx_ipm_data.callback(nrfx_ipm_data.callback_ctx,
+			nrfx_ipm_data.callback(DEVICE_GET(ipm_nrf),


This requires also DEVICE_DECLARE(ipm_nrf); to be added above.

anangl · 2020-07-27T13:11:05Z

drivers/ipm/ipm_nrfx_ipc.c

@@ -133,7 +135,8 @@ static void vipm_dispatcher(uint32_t event_mask, void *p_context)
 		event_mask &= ~BIT(event_idx);
 		if (nrfx_vipm_data.callback[event_idx] != NULL) {
 			nrfx_vipm_data.callback[event_idx]
-				(nrfx_vipm_data.callback_ctx[event_idx],
+				(nrfx_vipm_data.ipm_device,


I'm sorry, now I see I was wrong in my previous comment. This field should be changed to an array of struct device * and its entries should be initialized in vipm_nrf_##_idx##_register_callback() (like callback[_idx] and callback_ctx[_idx] are), not in vipm_nrf_init().

tbursztyka · 2020-07-27T13:38:30Z

@anangl For vipm: which device should it be? tDEVICE_GET(ipm_nrf) or the actual vipm? If it's the later, current change is fine. If it should be the first, then let's just use it as is, without .dev attribute.

anangl · 2020-07-27T13:53:35Z

@anangl For vipm: which device should it be? tDEVICE_GET(ipm_nrf) or the actual vipm? If it's the later, current change is fine. If it should be the first, then let's just use it as is, without .dev attribute.

@tbursztyka When CONFIG_IPM_NRF_SINGLE_INSTANCE is not enabled, there is no ipm_nrf device, but instead there are multiple devices (in count of NRFX_IPC_ID_MAX_VALUE) created here by UTIL_LISTIFY. Thus, dev pointer needs to be stored for each of them, and that needs to be done in vipm_nrf_##_idx##_register_callback(), as in vipm_nrf_init() you don't have _idx to access the corresponding array entry.

tbursztyka · 2020-07-27T14:10:26Z

Wait, that driver seems to be doing things upside down... why having one static struct vipm_nrf_data nrfx_vipm_data; when each and every vipm device could have its own static struct vipm_nrf_data nrfx_vipm_##idx##_data; (and the attributes there would not be arrays)?

That seems to add complexity for no reason.

tbursztyka · 2020-07-28T07:20:54Z

@anangl I have followed the change you advised. However, this vipm driver would definitely gain in simplicity by using device's private data instead of those arrays (it would require only an external static sturct device *ipc_device[NRFX_IPC_ID_MAX_VALUE]). That would save rom a bit, since the code would be shared for all instances, and ram usage would be the same.

anangl · 2020-07-28T07:45:31Z

However, this vipm driver would definitely gain in simplicity by using device's private data instead of those arrays.

I agree. I have no idea why the driver has been done this way. It's quite confusing, too. This unusual approach used here was the cause I was wrong in my initial comment. I just misunderstood it.

Let's normalize the callback like it is done in other APIs. This will avoid the need to do not so nice code (storing the ipm device pointer as global, or else) and more importantly will help to move all device instance to constant. Fixes zephyrproject-rtos#26923 Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

Of course IPM drivers now provide their device instance. There are 2 drivers using IPM callbacks as well, so they get the imp device instance pointer now through the callback. Fixes zephyrproject-rtos#26923 Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

Applications using IPM callbacks get the imp device instance pointer now through the callback. Which help to clarify a bit the code here and there as well. Fixes zephyrproject-rtos#26923 Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

Provide the IPM device pointer to the callback. Fixes zephyrproject-rtos#26923 Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

s/context/user_data Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

Documenting the change. Fixes zephyrproject-rtos#26923 Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

github-actions bot added area: API Changes to public APIs area: Samples Samples platform: NXP NXP platform: STM32 ST Micro STM32 labels Jul 9, 2020

tbursztyka force-pushed the ipm_callback_fix branch from c02e929 to bbea413 Compare July 10, 2020 05:47

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Jul 10, 2020

tbursztyka removed area: Samples Samples area: Tests Issues related to a particular existing or missing test platform: NXP NXP platform: STM32 ST Micro STM32 labels Jul 10, 2020

github-actions bot added area: Samples Samples area: Tests Issues related to a particular existing or missing test platform: NXP NXP platform: STM32 ST Micro STM32 labels Jul 10, 2020

tbursztyka requested a review from carlescufi July 10, 2020 11:54

tbursztyka force-pushed the ipm_callback_fix branch from bbea413 to 709adbb Compare July 14, 2020 06:00

tbursztyka changed the title ~~[RFC] IPM callback fix~~ [RFC] API Change: IPM - callback improvement Jul 14, 2020

github-actions bot added the area: Documentation label Jul 14, 2020

tbursztyka added area: IPC Inter-Process Communication and removed area: Documentation area: Samples Samples area: Tests Issues related to a particular existing or missing test platform: NXP NXP platform: STM32 ST Micro STM32 labels Jul 14, 2020

github-actions bot added area: Documentation area: Samples Samples area: Tests Issues related to a particular existing or missing test platform: NXP NXP platform: STM32 ST Micro STM32 labels Jul 14, 2020

tbursztyka mentioned this pull request Jul 19, 2020

[RFC] API change - Normalize DMA, IPM and UART callbacks signatures including the caller's device pointer. #26923

Closed

carlescufi approved these changes Jul 20, 2020

View reviewed changes

anangl requested changes Jul 21, 2020

View reviewed changes

MaureenHelm approved these changes Jul 21, 2020

View reviewed changes

tbursztyka force-pushed the ipm_callback_fix branch 2 times, most recently from 77b729a to 7e09b81 Compare July 27, 2020 11:12

anangl reviewed Jul 27, 2020

View reviewed changes

tbursztyka changed the title ~~[RFC] API Change: IPM - callback improvement~~ API Change: IPM - callback improvement Jul 28, 2020

tbursztyka force-pushed the ipm_callback_fix branch from 7e09b81 to 43cbe50 Compare July 28, 2020 07:18

anangl approved these changes Jul 28, 2020

View reviewed changes

tbursztyka force-pushed the ipm_callback_fix branch 2 times, most recently from 4fc927b to 813cf7a Compare July 29, 2020 07:21

carlescufi removed the DNM This PR should not be merged (Do Not Merge) label Jul 30, 2020

Tomasz Bursztyka added 7 commits July 30, 2020 09:49

tests: ipm: Apply IPM callback signature change

244befe

Provide the IPM device pointer to the callback. Fixes zephyrproject-rtos#26923 Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

ipm: Callback's user data has to be called user_data

c97448d

s/context/user_data Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

drivers: ipm: Align all drivers using user_data for the callback

89c1f6a

s/context/user_data Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

doc: Add a IPM API change entry to release-notes for 2.4 version

53378b3

Documenting the change. Fixes zephyrproject-rtos#26923 Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>

tbursztyka force-pushed the ipm_callback_fix branch from 813cf7a to 53378b3 Compare July 30, 2020 07:50

carlescufi merged commit f0743df into zephyrproject-rtos:master Jul 30, 2020

Architecture Project automation moved this from In Progress to Done Jul 30, 2020

tbursztyka deleted the ipm_callback_fix branch July 31, 2020 07:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Change: IPM - callback improvement #26780

API Change: IPM - callback improvement #26780

tbursztyka commented Jul 9, 2020 •

edited

tbursztyka commented Jul 10, 2020

carlescufi commented Jul 20, 2020

anangl Jul 21, 2020

tbursztyka Jul 27, 2020

anangl Jul 21, 2020

tbursztyka Jul 27, 2020

anangl Jul 27, 2020 •

edited

anangl Jul 27, 2020

tbursztyka Jul 27, 2020

anangl Jul 27, 2020 •

edited

tbursztyka commented Jul 27, 2020

anangl commented Jul 27, 2020 •

edited

tbursztyka commented Jul 27, 2020

tbursztyka commented Jul 28, 2020

anangl commented Jul 28, 2020

API Change: IPM - callback improvement #26780

API Change: IPM - callback improvement #26780

Conversation

tbursztyka commented Jul 9, 2020 • edited

tbursztyka commented Jul 10, 2020

carlescufi commented Jul 20, 2020

anangl Jul 21, 2020

Choose a reason for hiding this comment

tbursztyka Jul 27, 2020

Choose a reason for hiding this comment

anangl Jul 21, 2020

Choose a reason for hiding this comment

tbursztyka Jul 27, 2020

Choose a reason for hiding this comment

anangl Jul 27, 2020 • edited

Choose a reason for hiding this comment

anangl Jul 27, 2020

Choose a reason for hiding this comment

tbursztyka Jul 27, 2020

Choose a reason for hiding this comment

anangl Jul 27, 2020 • edited

Choose a reason for hiding this comment

tbursztyka commented Jul 27, 2020

anangl commented Jul 27, 2020 • edited

tbursztyka commented Jul 27, 2020

tbursztyka commented Jul 28, 2020

anangl commented Jul 28, 2020

tbursztyka commented Jul 9, 2020 •

edited

anangl Jul 27, 2020 •

edited

anangl Jul 27, 2020 •

edited

anangl commented Jul 27, 2020 •

edited