You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2022/05/17 06:24:11 UTC
[GitHub] [mynewt-core] bvdberg opened a new pull request, #2839: nrf5340: avoid have 2 interrupts for each IPC message
bvdberg opened a new pull request, #2839:
URL: https://github.com/apache/mynewt-core/pull/2839
The Nimble IPC messages are written in 2 parts: the cmd byte and the rest.
It is unneccesary to already interrupt the other core for the first byte,
since it will be interrupted by the 2nd write already.
(a 2nd patch to Nimble will use this new function)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] kasjer commented on a diff in pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
kasjer commented on code in PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#discussion_r874663231
##########
hw/drivers/ipc_nrf5340/include/ipc_nrf5340/ipc_nrf5340.h:
##########
@@ -68,7 +69,22 @@ void ipc_nrf5340_recv(int channel, ipc_nrf5340_recv_cb cb, void *user_data);
*
* @return 0 on success and negative error on failure
*/
-int ipc_nrf5340_send(int channel, const void *data, uint16_t len);
+int ipc_nrf5340_send(int channel, const void *data, uint16_t len, bool last);
Review Comment:
oops, still modified :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] utzig commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
utzig commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1128463667
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### hw/drivers/ipc_nrf5340/src/ipc_nrf5340.c
<details>
```diff
@@ -405,12 +405,14 @@
/* assert since that will always fail for non-blocking write
* indicating use error
*/
- if (space == 0) return SYS_ENOMEM;
+ if (space == 0) {
+ return SYS_ENOMEM;
+ }
#endif
} while (space == 0);
ipc_nrf5340_shm_write(shm, &data, 1);
- // dont notify other side! (will be done by later calls to ipc_nrf5340_send)
+ /* dont notify other side! (will be done by later calls to ipc_nrf5340_send) */
return 0;
}
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] bvdberg commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
bvdberg commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1128718986
doh! I did build though. Ok fixed :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] kasjer commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
kasjer commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1131574915
> This will break blocking send if data is bigger than internal shm size... I think notification should stil be done from withing loop (note that for non-blocking config this loop iterates only once anyway)
Maybe changing from:
```c
NRF_IPC->TASKS_SEND[channel] = 1;
```
to:
```c
if (last || len > space) {
NRF_IPC->TASKS_SEND[channel] = 1;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] bvdberg commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
bvdberg commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1132484895
Well spotted. Modified as @kasjer proposed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] utzig commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
utzig commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1132486299
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] kasjer commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
kasjer commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1128665002
@bvdberg I think that ipc_nrf5340_send() is already used by some users (not only by nimble and what is in mynewt core) that's why I propose to have another function and send ipc_nrf5340_send() would just used it.
Changing prototype is not a way to go I think.
I'm sorry if it was not clear in my previous comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] bvdberg commented on a diff in pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
bvdberg commented on code in PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#discussion_r874596080
##########
hw/drivers/ipc_nrf5340/include/ipc_nrf5340/ipc_nrf5340.h:
##########
@@ -56,6 +56,18 @@ void ipc_nrf5340_reset(void);
*/
void ipc_nrf5340_recv(int channel, ipc_nrf5340_recv_cb cb, void *user_data);
+/**
+ * Sends single byte data over specified IPC channel. IPC uses ring buffer for data passing.
+ * If IPC_NRF5340_BLOCKING_WRITE is not enable and there is not enough space to
+ * store data SYS_ENOMEM error is returned. This function does not notify the other side
+ *
+ * @param channel IPC channel number to send on
+ * @param data Data to be sent over IPC.
+ *
+ * @return 0 on success and negative error on failure
+ */
+int ipc_nrf5340_send_no_notify(int channel, uint8_t data);
Review Comment:
I think this is indeed better, less functions on the API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] utzig commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
utzig commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1132483893
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] bvdberg commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
bvdberg commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1135436174
I made all the changes, new it should be perfect ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] sjanc merged pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
sjanc merged PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] utzig commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
utzig commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1128946905
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] bvdberg commented on a diff in pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
bvdberg commented on code in PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#discussion_r874446293
##########
hw/drivers/ipc_nrf5340/include/ipc_nrf5340/ipc_nrf5340.h:
##########
@@ -56,6 +56,18 @@ void ipc_nrf5340_reset(void);
*/
void ipc_nrf5340_recv(int channel, ipc_nrf5340_recv_cb cb, void *user_data);
+/**
+ * Sends single byte data over specified IPC channel. IPC uses ring buffer for data passing.
+ * If IPC_NRF5340_BLOCKING_WRITE is not enable and there is not enough space to
+ * store data SYS_ENOMEM error is returned. This function does not notify the other side
+ *
+ * @param channel IPC channel number to send on
+ * @param data Data to be sent over IPC.
+ *
+ * @return 0 on success and negative error on failure
+ */
+int ipc_nrf5340_send_no_notify(int channel, uint8_t data);
Review Comment:
Yes that would also be fine, I'll change to that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] kasjer commented on a diff in pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
kasjer commented on code in PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#discussion_r874446894
##########
hw/drivers/ipc_nrf5340/include/ipc_nrf5340/ipc_nrf5340.h:
##########
@@ -56,6 +56,18 @@ void ipc_nrf5340_reset(void);
*/
void ipc_nrf5340_recv(int channel, ipc_nrf5340_recv_cb cb, void *user_data);
+/**
+ * Sends single byte data over specified IPC channel. IPC uses ring buffer for data passing.
+ * If IPC_NRF5340_BLOCKING_WRITE is not enable and there is not enough space to
+ * store data SYS_ENOMEM error is returned. This function does not notify the other side
+ *
+ * @param channel IPC channel number to send on
+ * @param data Data to be sent over IPC.
+ *
+ * @return 0 on success and negative error on failure
+ */
+int ipc_nrf5340_send_no_notify(int channel, uint8_t data);
Review Comment:
Maybe `int ipc_nrf5340_write(int channel, const void *data, uint16_t len, bool last)`
then current `ipc_nrf5340_send()` could just call it with **true**;
and new function could grab current code of ipc_nrf5340_send() and have something like that
```c
if (last) {
NRF_IPC->TASKS_SEND[channel] = 1;
}
```
just an idea to think about
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] bvdberg commented on a diff in pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
bvdberg commented on code in PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#discussion_r874597788
##########
hw/drivers/ipc_nrf5340/include/ipc_nrf5340/ipc_nrf5340.h:
##########
@@ -56,6 +56,18 @@ void ipc_nrf5340_reset(void);
*/
void ipc_nrf5340_recv(int channel, ipc_nrf5340_recv_cb cb, void *user_data);
+/**
+ * Sends single byte data over specified IPC channel. IPC uses ring buffer for data passing.
+ * If IPC_NRF5340_BLOCKING_WRITE is not enable and there is not enough space to
+ * store data SYS_ENOMEM error is returned. This function does not notify the other side
+ *
+ * @param channel IPC channel number to send on
+ * @param data Data to be sent over IPC.
+ *
+ * @return 0 on success and negative error on failure
+ */
+int ipc_nrf5340_send_no_notify(int channel, uint8_t data);
Review Comment:
I pushed the new version
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] kasjer commented on a diff in pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
kasjer commented on code in PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#discussion_r874446894
##########
hw/drivers/ipc_nrf5340/include/ipc_nrf5340/ipc_nrf5340.h:
##########
@@ -56,6 +56,18 @@ void ipc_nrf5340_reset(void);
*/
void ipc_nrf5340_recv(int channel, ipc_nrf5340_recv_cb cb, void *user_data);
+/**
+ * Sends single byte data over specified IPC channel. IPC uses ring buffer for data passing.
+ * If IPC_NRF5340_BLOCKING_WRITE is not enable and there is not enough space to
+ * store data SYS_ENOMEM error is returned. This function does not notify the other side
+ *
+ * @param channel IPC channel number to send on
+ * @param data Data to be sent over IPC.
+ *
+ * @return 0 on success and negative error on failure
+ */
+int ipc_nrf5340_send_no_notify(int channel, uint8_t data);
Review Comment:
Maybe `int ipc_nrf5340_write(int channel, const void *data, uint16_t len, bool last)`
then current `ipc_nrf5340_send()` could just call it with **true**;
and new function could grab current code of ipc_nrf5340_send() and have something like that
```c
if (last) {
NRF_IPC->TASKS_SEND[channel] = 1;
}
```
just an idea to thing about
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] bvdberg commented on pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
bvdberg commented on PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#issuecomment-1128690762
sounds fair, i have added ipc_nrf5340_write() with the bool last argument. The ipc_nrf5340_send() now uses write(.., true)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-core] sjanc commented on a diff in pull request #2839: nrf5340: avoid have 2 interrupts for each IPC message
Posted by GitBox <gi...@apache.org>.
sjanc commented on code in PR #2839:
URL: https://github.com/apache/mynewt-core/pull/2839#discussion_r874434812
##########
hw/drivers/ipc_nrf5340/include/ipc_nrf5340/ipc_nrf5340.h:
##########
@@ -56,6 +56,18 @@ void ipc_nrf5340_reset(void);
*/
void ipc_nrf5340_recv(int channel, ipc_nrf5340_recv_cb cb, void *user_data);
+/**
+ * Sends single byte data over specified IPC channel. IPC uses ring buffer for data passing.
+ * If IPC_NRF5340_BLOCKING_WRITE is not enable and there is not enough space to
+ * store data SYS_ENOMEM error is returned. This function does not notify the other side
+ *
+ * @param channel IPC channel number to send on
+ * @param data Data to be sent over IPC.
+ *
+ * @return 0 on success and negative error on failure
+ */
+int ipc_nrf5340_send_no_notify(int channel, uint8_t data);
Review Comment:
this looks like very specific case (1 byte of data), maybe add more generic "int ipc_nrf5340_write(int channel, const void *data, uint16_t len)" instead? and then for completeness also "void ipc_nrf5340_notify(int channel)" that would only notify with no data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org