You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/01/17 22:22:17 UTC
[GitHub] [incubator-nuttx] btashton opened a new pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
btashton opened a new pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705
## Summary
This is a continuation of #2674 which allows for the driver to use a static buffer for small transfers instead of having to malloc.
## Impact
Should improve the performance of the i2c driver for smaller transfers.
## Testing
nrf52 Feather with i2ctool talking to MPU-6050
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
v01d commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559248318
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -308,11 +313,20 @@ static int nrf52_i2c_transfer(FAR struct i2c_master_s *dev,
/* Combine buffers */
- pack_buf = kmm_malloc(priv->msgv[0].length +
- priv->msgv[1].length);
- if (pack_buf == NULL)
+ if ((priv->msgv[0].length +
+ priv->msgv[1].length) <=
+ CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE)
+ {
+ pack_buf = (uint8_t *)priv->copy_buf;
+ }
+ else
{
- return -1;
+ pack_buf = kmm_malloc(priv->msgv[0].length +
+ priv->msgv[1].length);
+ if (pack_buf == NULL)
+ {
+ return -1;
Review comment:
maybe better -ENOMEM?
##########
File path: arch/arm/src/nrf52/Kconfig
##########
@@ -633,4 +633,16 @@ config NRF52_I2C_MASTER_DISABLE_NOSTART
bool "Disable the I2C Master NOSTART flag support"
default n
+config NRF52_I2C_MASTER_COPY_BUF_SIZE
+ int "Static buffer size for NOSTART flag support"
+ depends on !NRF52_I2C_MASTER_DISABLE_NOSTART
+ default 4
+ ---help---
+ To combine two i2c messages that are part of a
+ single transaction (NO_STOP-NO_START) the nrf52
+ hardware requires these be joined into a single
+ transfer. This static buffer will be used if the
+ transaction will fit otherwise it will fall back
+ on malloc.
Review comment:
Could you move part of this explanation to `NRF52_I2C_MASTER_DISABLE_NOSTART` (to explain why this option exists)?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559337019
##########
File path: arch/arm/src/nrf52/Kconfig
##########
@@ -632,5 +632,24 @@ menu "I2C Master Configuration"
config NRF52_I2C_MASTER_DISABLE_NOSTART
bool "Disable the I2C Master NOSTART flag support"
default n
+ ---help---
+ To combine two i2c messages that are part of a
+ single transaction (NO_STOP-NO_START) the nrf52
+ hardware requires these be joined into a single
+ transfer. This can be expensive and some devices
+ can get away with multi-part transfers as separate
+ transfers. Enable this at your own risk!
+
+config NRF52_I2C_MASTER_COPY_BUF_SIZE
+ int "Static buffer size for NOSTART flag support"
+ depends on !NRF52_I2C_MASTER_DISABLE_NOSTART
+ default 4
Review comment:
Sorry, my calculation is wrong, 4 bytes is enough.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559255110
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -308,11 +313,20 @@ static int nrf52_i2c_transfer(FAR struct i2c_master_s *dev,
/* Combine buffers */
- pack_buf = kmm_malloc(priv->msgv[0].length +
- priv->msgv[1].length);
- if (pack_buf == NULL)
+ if ((priv->msgv[0].length +
+ priv->msgv[1].length) <=
+ CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE)
+ {
+ pack_buf = (uint8_t *)priv->copy_buf;
+ }
+ else
{
- return -1;
+ pack_buf = kmm_malloc(priv->msgv[0].length +
+ priv->msgv[1].length);
+ if (pack_buf == NULL)
+ {
+ return -1;
Review comment:
Done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559335914
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -308,11 +313,20 @@ static int nrf52_i2c_transfer(FAR struct i2c_master_s *dev,
/* Combine buffers */
- pack_buf = kmm_malloc(priv->msgv[0].length +
- priv->msgv[1].length);
- if (pack_buf == NULL)
+ if ((priv->msgv[0].length +
+ priv->msgv[1].length) <=
+ CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE)
+ {
+ pack_buf = (uint8_t *)priv->copy_buf;
Review comment:
You are right, for some reason I thought the compiler would give a warning for implicit cast.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559255314
##########
File path: arch/arm/src/nrf52/Kconfig
##########
@@ -633,4 +633,16 @@ config NRF52_I2C_MASTER_DISABLE_NOSTART
bool "Disable the I2C Master NOSTART flag support"
default n
+config NRF52_I2C_MASTER_COPY_BUF_SIZE
+ int "Static buffer size for NOSTART flag support"
+ depends on !NRF52_I2C_MASTER_DISABLE_NOSTART
+ default 4
+ ---help---
+ To combine two i2c messages that are part of a
+ single transaction (NO_STOP-NO_START) the nrf52
+ hardware requires these be joined into a single
+ transfer. This static buffer will be used if the
+ transaction will fit otherwise it will fall back
+ on malloc.
Review comment:
I added help text to that option as well including the risk. I left this help text on this option so people can still have the context when configuring.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559294965
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -68,6 +68,11 @@ struct nrf52_i2c_priv_s
uint8_t msgc; /* Message count */
struct i2c_msg_s *msgv; /* Message list */
uint8_t *ptr; /* Current message buffer */
+#ifdef CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE
+ /* Static buffer used for continued messages */
+
+ uint8_t copy_buf[CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE];
Review comment:
The whole point is to not have to do the allocate and free for small transfers. This provides a large performance increase.
Small register transfers use the static allocation and large transfers use the dynamic heap.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559307045
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -68,6 +68,11 @@ struct nrf52_i2c_priv_s
uint8_t msgc; /* Message count */
struct i2c_msg_s *msgv; /* Message list */
uint8_t *ptr; /* Current message buffer */
+#ifdef CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE
+ /* Static buffer used for continued messages */
+
+ uint8_t copy_buf[CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE];
Review comment:
> But , why not avoid the allocaiton every time for the large transfer?
I thought about doing realloc but I'm worried about leaving these possible large allocations that cannot be freed which may have only been used to write a large buffer to an eeprom.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559336105
##########
File path: arch/arm/src/nrf52/Kconfig
##########
@@ -632,5 +632,24 @@ menu "I2C Master Configuration"
config NRF52_I2C_MASTER_DISABLE_NOSTART
bool "Disable the I2C Master NOSTART flag support"
default n
+ ---help---
+ To combine two i2c messages that are part of a
+ single transaction (NO_STOP-NO_START) the nrf52
+ hardware requires these be joined into a single
+ transfer. This can be expensive and some devices
+ can get away with multi-part transfers as separate
+ transfers. Enable this at your own risk!
+
+config NRF52_I2C_MASTER_COPY_BUF_SIZE
+ int "Static buffer size for NOSTART flag support"
+ depends on !NRF52_I2C_MASTER_DISABLE_NOSTART
+ default 4
Review comment:
Why would that be 8 and not 4, or did you mean 32bit address/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559303362
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -68,6 +68,11 @@ struct nrf52_i2c_priv_s
uint8_t msgc; /* Message count */
struct i2c_msg_s *msgv; /* Message list */
uint8_t *ptr; /* Current message buffer */
+#ifdef CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE
+ /* Static buffer used for continued messages */
+
+ uint8_t copy_buf[CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE];
Review comment:
But , why not avoid the allocaiton every time for the large transfer?
```
struct nrf52_i2c_priv_s
{
...
uint8_t *copy_buf;
size_t copy_len;
...
};
static int nrf52_i2c_transfer(FAR struct i2c_master_s *dev,
FAR struct i2c_msg_s *msgs,
int count)
{
if ((priv->msgv[0].length + priv->msgv[1].length) <= priv->copy_len)
{
pack_buf = priv->copy_buf;
}
else
{
pack_buf = kmm_realloc(priv->copy_buf, priv->msgv[0].length + priv->msgv[1].length);
if (pack_buf == NULL)
{
return -ENOMEM;
}
priv->copy_buf = pack_buf;
priv->copy_len = priv->msgv[0].length + priv->msgv[1].length;
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559294965
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -68,6 +68,11 @@ struct nrf52_i2c_priv_s
uint8_t msgc; /* Message count */
struct i2c_msg_s *msgv; /* Message list */
uint8_t *ptr; /* Current message buffer */
+#ifdef CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE
+ /* Static buffer used for continued messages */
+
+ uint8_t copy_buf[CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE];
Review comment:
The whole point is to not have to do the allocate and free for small transfers. This provides a large performance increase.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559289444
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -68,6 +68,11 @@ struct nrf52_i2c_priv_s
uint8_t msgc; /* Message count */
struct i2c_msg_s *msgv; /* Message list */
uint8_t *ptr; /* Current message buffer */
+#ifdef CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE
+ /* Static buffer used for continued messages */
+
+ uint8_t copy_buf[CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE];
Review comment:
why not change copy_buf to pointer and extend the buffer on the fly?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2705: nrf52: Add a static copy buffer for i2c NOSTART transfers
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2705:
URL: https://github.com/apache/incubator-nuttx/pull/2705#discussion_r559329075
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -496,7 +510,7 @@ static int nrf52_i2c_transfer(FAR struct i2c_master_s *dev,
errout:
#ifndef CONFIG_NRF52_I2C_MASTER_DISABLE_NOSTART
- if (pack_buf != NULL)
+ if ((pack_buf != NULL) && (pack_buf != (uint8_t *)priv->copy_buf))
Review comment:
don't need cast and ()
##########
File path: arch/arm/src/nrf52/nrf52_i2c.c
##########
@@ -308,11 +313,20 @@ static int nrf52_i2c_transfer(FAR struct i2c_master_s *dev,
/* Combine buffers */
- pack_buf = kmm_malloc(priv->msgv[0].length +
- priv->msgv[1].length);
- if (pack_buf == NULL)
+ if ((priv->msgv[0].length +
+ priv->msgv[1].length) <=
+ CONFIG_NRF52_I2C_MASTER_COPY_BUF_SIZE)
+ {
+ pack_buf = (uint8_t *)priv->copy_buf;
Review comment:
don't need cast?
##########
File path: arch/arm/src/nrf52/Kconfig
##########
@@ -632,5 +632,24 @@ menu "I2C Master Configuration"
config NRF52_I2C_MASTER_DISABLE_NOSTART
bool "Disable the I2C Master NOSTART flag support"
default n
+ ---help---
+ To combine two i2c messages that are part of a
+ single transaction (NO_STOP-NO_START) the nrf52
+ hardware requires these be joined into a single
+ transfer. This can be expensive and some devices
+ can get away with multi-part transfers as separate
+ transfers. Enable this at your own risk!
+
+config NRF52_I2C_MASTER_COPY_BUF_SIZE
+ int "Static buffer size for NOSTART flag support"
+ depends on !NRF52_I2C_MASTER_DISABLE_NOSTART
+ default 4
Review comment:
should we extend to 8 bytes to handle 2 bytes address and 2 bytes register?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org