search

apache / nuttx

Apache NuttX is a mature, real-time embedded operating system (RTOS)
https://nuttx.apache.org/
Apache License 2.0
2.82k stars 1.17k forks source link

[BUG] imxrt:lpi2c kconfig and source code discrepancy #13114

Open danielappiagyei-bc opened 2 months ago

danielappiagyei-bc commented 2 months ago

Description / Steps to reproduce the issue

The imxrt's kconfig provides configs for I2C transaction timeouts:

The compile-time configuration logic is performed in these lines of imcrt_lpi2c.c. At the end of this code block, assuming you're not using dynamic timeouts, CONFIG_IMXRT_LPI2C_TIMEOTICKS will be the value the driver uses for timeouts (whether it uses the value set by the user in a defconfig/kconfig or it's manually computed from the supplied seconds and ms)

Issue 1

Assuming the top-level I2C config is enabled and IMXRT_LPI2C_DYNTIMEO is not set, these are always defined and given default values in the kconfig. If you want to define timeouts in terms of s and ms instead of ticks, then the lines on 86 - 90 should handle that, but in reality they don't and are dead code because CONFIG_IMXRT_LPI2C_TIMEOTICKS is always defined (since it has a default value). Solution: Remove the default value of IMXRT_LPI2C_TIMEOTICKS from the kconfig Solution Impact on Users: When users upgrade to the latest nuttx, if users never explicitly set this value in a defconfig, then the defaults values of IMXRT_LPI2C_TIMEOSEC and IMXRT_LPI2C_TIMEOMS will be used instead, which may not result in the same number of timeout ticks as before

Issue 2

IMXRT_LPI2C_DYNTIMEO_STARTSTOP isn't really used in the file. It's value is just set to a value in the config block on lines 92 - 95 and never used again. Solution: Remove this from the kconfig and from the source file Solution Impact on Users: none, except a compile-time failure because of a missing config option if they were explicitly setting it in a defconfig

On which OS does this issue occur?

[Linux]

What is the version of your OS?

Ubuntu 22.04

NuttX Version

master

Issue Architecture

[arm]

Issue Area

[Kconfig]

Verification

danielappiagyei-bc commented 2 months ago

@davids5 hello good sir, thank you again sir for creating another excellent driver so I didn't have to! Do you concur with my findings above?

davids5 commented 2 months ago

@danielappiagyei-bc

You are welcome.

Good job unwinding it! I think I only tested IMXRT_LPI2C_DYNTIMEO (I only use the setting). Also this code is in other archs as well - so the fix may be far reaching.

I do concur. Kconfig should supply the values and the enables. All the default guard in code (setting defaults) should go away.

I also think the the value can be just timeout in us and a 32 bit value. That will give use us resolution and 4.29 S max.

ticks is also not a good unit. It needs to insure the value is 1 for the min us - but then it can be 10ms for 1 tick. (we use 1 ms tick)

So that should be addressed also.

What do you think?

BTW, I do see CONFIG_IMXRT_LPI2C_DYNTIMEO_STARTSTOP used https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_lpi2c.c#L838

danielappiagyei-bc commented 2 months ago

Hi @davids5, I apologize for the late response, I got busy with work and meant to reply!

You're right, the config is used. My corp uses an older version of nuttx and it wasn't used but I thought I checked master and didn't see it, my bad.

Yeah I also agree with getting rid of ticks and just sticking with microseconds. Let me scope out the work and see if I can get it done, it may be awhile unfortunately.

RE: other arches I see, let me check out these other issues and get back to you