You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "jlaitine (via GitHub)" <gi...@apache.org> on 2023/01/27 14:08:09 UTC
[GitHub] [nuttx] jlaitine opened a new pull request, #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
jlaitine opened a new pull request, #8303:
URL: https://github.com/apache/nuttx/pull/8303
…alue
When sending small number of bytes with larger CONFIG_USEC_PER_TICK this function should return at least 1. Solve this by rounding up the result.
Signed-off-by: Jukka Laitinen <ju...@ssrc.tii.ae>
## Summary
This function should always return at least 1. For example with values
bytecount == 1
CONFIG_USEC_PER_TICK == 10000 (default)
CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE = 500
Since USEC2TICK does rounding to nearest, the calculation in ticks would result in (((500 * 1) + (10000 / 2)) / 10000) == ( 5500 / 10000 ) == 0
Solve this by rounding it up (adding CONFIG_USEC_PER_TICK / 2 - CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE), which always results 1 tick at 1 byte regardless of the selected USEC_PER_TICK or STM32F7_I2C_DYNTIMEO_USECPERBYTE values.
## Impact
Fixes the i2c on stm32f7 with default USEC_PER_TICK setting
## Testing
Tested on stm32f7 based board
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on code in PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#discussion_r1089117000
##########
arch/arm/src/stm32f7/stm32_i2c.c:
##########
@@ -784,11 +784,13 @@ static uint32_t stm32_i2c_toticks(int msgc, struct i2c_msg_s *msgs)
bytecount += msgs[i].length;
}
- /* Then return a number of microseconds based on a user provided scaling
- * factor.
+ /* Then return a number of ticks based on a user provided scaling
+ * factor, rounded up.
*/
- return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount);
+ return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount +
Review Comment:
That makes quite large error, imagine calculation result being e.g. 1.9; would be rounded down to 1. As this is a timeout value it is better to always round up. Of course, a simple + 1 for ticks would work, not sure if I was just making it too exact in rounding up ?
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on code in PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#discussion_r1089062098
##########
arch/arm/src/stm32f7/stm32_i2c.c:
##########
@@ -784,11 +784,13 @@ static uint32_t stm32_i2c_toticks(int msgc, struct i2c_msg_s *msgs)
bytecount += msgs[i].length;
}
- /* Then return a number of microseconds based on a user provided scaling
- * factor.
+ /* Then return a number of ticks based on a user provided scaling
+ * factor, rounded up.
*/
- return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount);
+ return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount +
Review Comment:
Yes, this is a wider problem. Only thing is, that I cannot test those other platforms and I don't want to touch low level driver without testing them.
I don't know how this broke in the first place, the i2c used to work fine previously, and now with the latest nuttx it didn't work pretty much at all (all smaller i2c transfers have broken). So what happened here? Has someone pushed in some untested change to i2c drivers?
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on code in PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#discussion_r1089117000
##########
arch/arm/src/stm32f7/stm32_i2c.c:
##########
@@ -784,11 +784,13 @@ static uint32_t stm32_i2c_toticks(int msgc, struct i2c_msg_s *msgs)
bytecount += msgs[i].length;
}
- /* Then return a number of microseconds based on a user provided scaling
- * factor.
+ /* Then return a number of ticks based on a user provided scaling
+ * factor, rounded up.
*/
- return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount);
+ return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount +
Review Comment:
That makes quite large error, imagine calculation result being e.g. 1.9; it would be rounded down to 1. As this is a timeout value it is better to always round up. Of course, a simple + 1 for ticks would work, I am not sure if I was just making it too exact in rounding up and made it look complex ?
For runtime the apparent complexity doesn't matter, it is calculated by preprocessor. And I think the equation is correct.
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] davids5 commented on a diff in pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#discussion_r1089716141
##########
arch/arm/src/stm32f7/stm32_i2c.c:
##########
@@ -784,11 +784,13 @@ static uint32_t stm32_i2c_toticks(int msgc, struct i2c_msg_s *msgs)
bytecount += msgs[i].length;
}
- /* Then return a number of microseconds based on a user provided scaling
- * factor.
+ /* Then return a number of ticks based on a user provided scaling
+ * factor, rounded up.
*/
- return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount);
+ return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount +
Review Comment:
Yes
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on a diff in pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#discussion_r1089056667
##########
arch/arm/src/stm32f7/stm32_i2c.c:
##########
@@ -784,11 +784,13 @@ static uint32_t stm32_i2c_toticks(int msgc, struct i2c_msg_s *msgs)
bytecount += msgs[i].length;
}
- /* Then return a number of microseconds based on a user provided scaling
- * factor.
+ /* Then return a number of ticks based on a user provided scaling
+ * factor, rounded up.
*/
- return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount);
+ return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount +
Review Comment:
Many places use the similar approach, should we update them too? (Please grep DYNTIMEO_USECPERBYTE).
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on a diff in pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on code in PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#discussion_r1089342271
##########
arch/arm/src/stm32f7/stm32_i2c.c:
##########
@@ -784,11 +784,13 @@ static uint32_t stm32_i2c_toticks(int msgc, struct i2c_msg_s *msgs)
bytecount += msgs[i].length;
}
- /* Then return a number of microseconds based on a user provided scaling
- * factor.
+ /* Then return a number of ticks based on a user provided scaling
+ * factor, rounded up.
*/
- return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount);
+ return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount +
Review Comment:
If it looks complex, we can simplify it a bit and keep it still exact; instead of " -CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE" in the end, just -1 works the same. I'll make this fix to prettify it. Would that do, @davids5 ?
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] acassis merged pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis merged PR #8303:
URL: https://github.com/apache/nuttx/pull/8303
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] davids5 commented on a diff in pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#discussion_r1089078075
##########
arch/arm/src/stm32f7/stm32_i2c.c:
##########
@@ -784,11 +784,13 @@ static uint32_t stm32_i2c_toticks(int msgc, struct i2c_msg_s *msgs)
bytecount += msgs[i].length;
}
- /* Then return a number of microseconds based on a user provided scaling
- * factor.
+ /* Then return a number of ticks based on a user provided scaling
+ * factor, rounded up.
*/
- return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount);
+ return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount +
Review Comment:
Why not be more deliberate. v != 0 ? v : 1 and not tie it to the other values.
##########
arch/arm/src/stm32f7/stm32_i2c.c:
##########
@@ -784,11 +784,13 @@ static uint32_t stm32_i2c_toticks(int msgc, struct i2c_msg_s *msgs)
bytecount += msgs[i].length;
}
- /* Then return a number of microseconds based on a user provided scaling
- * factor.
+ /* Then return a number of ticks based on a user provided scaling
+ * factor, rounded up.
*/
- return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount);
+ return USEC2TICK(CONFIG_STM32F7_I2C_DYNTIMEO_USECPERBYTE * bytecount +
Review Comment:
Why not be more deliberate. v != 0 ? v : 1 and not tie it to the other values?
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] jlaitine commented on pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "jlaitine (via GitHub)" <gi...@apache.org>.
jlaitine commented on PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#issuecomment-1408097540
@xiaoxiang781216 I realized what caused the issue in the first place. I am not sure what is the best way forward so I created an issue about it. see https://github.com/apache/nuttx/issues/8346
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] xiaoxiang781216 commented on pull request #8303: arch/arm/src/stm32f7/stm32_i2c.c: Round up stm32_i2c_toticks return v…
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #8303:
URL: https://github.com/apache/nuttx/pull/8303#issuecomment-1408110482
Sure, let's discuss there.
--
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@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org