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 2020/02/13 15:20:45 UTC
[GitHub] [incubator-nuttx] NicholasChin opened a new pull request #270:
Small I2C fixups
NicholasChin opened a new pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270
Commit a44fb6e simply switches the place of a symbol so I2C options are nested under "I2C Driver Support" properly.
Commit 0ec84b8 is for an issue I was seeing with my IMXRT platform where it seemed to poll the AT24 so fast that it burned through the 100 retries before the AT24 was ready to accept commands. 500 retries is the absolute max that you should need as 500 NACK'd transaction at 1MHz takes the same time as the AT24's max write 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] NicholasChin commented on issue #270: Small I2C
fixups
Posted by GitBox <gi...@apache.org>.
NicholasChin commented on issue #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#issuecomment-585863095
@patacongo ARCH_HAVE_I2CRESET is a hidden option, yes. The options that I were referring to are visible options.
Currently these options show up nested under "Device Drivers" when they should nest under "I2C Driver Support":
1) I2C_SLAVE
2) I2C_POLLED
3) I2C_RESET
4) I2C_TRACE
5) I2C_DRIVER.
The Kconfig must look like this:
```
menuconfig I2C
...
if I2C
// nested options
endif
config ARCH_HAVE_I2CRESET
```
For Kconfig to nest these options properly. This is what my commit changes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#270: Small I2C fixups
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#discussion_r378996148
##########
File path: drivers/i2c/Kconfig
##########
@@ -54,3 +50,7 @@ config I2CMULTIPLEXER_PCA9540BDP
endmenu # I2C Multiplexer Support
endif
+
+config ARCH_HAVE_I2CRESET
Review comment:
You could move the ARCH_HAVE_I2CRESET item into the drivers/Kconfig file before CONFIG_I2C is selected. That would resolve the problem.
I personally believe that all of the ARCH_HAVE_ definitions belong in the arch/Kconfig menu. That is were they were originally and there is where they best belong. The ARCH naming conventions is only correct if they reside in the arch/Kconfig file. But there were moved all over in a recent PR and now the naming is wrong and a lot of dependencies relationships have been broken and kludged around like this one.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo closed pull request #270: Small I2C
fixups
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #270: Small I2C
fixups
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#issuecomment-585859504
That does not matter because that option is not visual in the menus. It is a hidden option that can only be selected by architecture code. Certainly that is not an excuse to introduce errors into the configuration.
Configuration options that have no prompt string do not appear in the menus. This is internal implementation of configuration system and not part of the UI menus.
Everything is correct as it is. Please do not change anything.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] NicholasChin commented on a change in pull
request #270: Small I2C fixups
Posted by GitBox <gi...@apache.org>.
NicholasChin commented on a change in pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#discussion_r378962319
##########
File path: drivers/i2c/Kconfig
##########
@@ -53,4 +49,8 @@ config I2CMULTIPLEXER_PCA9540BDP
endmenu # I2C Multiplexer Support
+config ARCH_HAVE_I2CRESET
Review comment:
Hi Xiang,
I've pulled ARCH_HAVE_I2CRESET out of the if I2C conditional. The conditional must be first in this file as menuconfigs will only nest configs if the conditional is the next block after declaring the menuconfig.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#270: Small I2C fixups
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#discussion_r378973957
##########
File path: drivers/i2c/Kconfig
##########
@@ -54,3 +50,7 @@ config I2CMULTIPLEXER_PCA9540BDP
endmenu # I2C Multiplexer Support
endif
+
+config ARCH_HAVE_I2CRESET
Review comment:
We have been through this one before. ARCH_HAVE_I2CRESET must be outside of if I2C, otherwise Kconfig generates errors. This cannot be accepted into the repository.
That same mistake was made in a previous PR and that had to be reverted because of all of the configuration errors it causes. This cannot be accepted into the repository.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#270: Small I2C fixups
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#discussion_r379005150
##########
File path: drivers/i2c/Kconfig
##########
@@ -54,3 +50,7 @@ config I2CMULTIPLEXER_PCA9540BDP
endmenu # I2C Multiplexer Support
endif
+
+config ARCH_HAVE_I2CRESET
Review comment:
i suppose it could even go at the end of drivers/i2c/Kconfig
That is not very comfortable. But I am not comfortable with the breaking of the naming convention either. The first word of the configuration name is suppose to be a clue where you can find the configuration definition. So everything in drivers/Kconfig should be named DRIVERS_, everything under drivers/i2c/Kconfig should be named I2C.
This belong in arch/Kconfig. Evertything in arch/Kconfig should be named ARCH_
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #270: Small I2C
fixups
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#issuecomment-585854407
I will close this PR now. If you remove the offending commit, it may be reopened.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #270: Small I2C
fixups
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#issuecomment-586591898
@NicholasChin Please not that I have reviewed the driver configurations and found several other occurrences of the error that you reported here. The driver configuration got pretty well screwed up.
I have submitted #286 that should should fix the case you note in drivers/i2c/Kconfig and the other places where the same error occurs. It should also help to prevent this kind of error from occurring again in the future.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #270: Small I2C
fixups
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#issuecomment-585866363
Yes, there is a problem. But your solution is not correct. It is worse.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] NicholasChin commented on issue #270: Small I2C
fixups
Posted by GitBox <gi...@apache.org>.
NicholasChin commented on issue #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#issuecomment-585857955
@patacongo ARCH_HAVE_I2CRESET is outside the conditional. However, ARCH_HAVE_I2CRESET MUST come after the conditional. If it does not, I2C config options are nested under the "Device Drivers" menu and not under "I2C Driver Support."
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] NicholasChin commented on a change in pull
request #270: Small I2C fixups
Posted by GitBox <gi...@apache.org>.
NicholasChin commented on a change in pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#discussion_r379000645
##########
File path: drivers/i2c/Kconfig
##########
@@ -54,3 +50,7 @@ config I2CMULTIPLEXER_PCA9540BDP
endmenu # I2C Multiplexer Support
endif
Review comment:
@patacongo This is the `endif` for the corresponding `if I2c`
`config ARCH_HAVE_I2CRESET` comes after it. Therefore, `ARCH_HAVE_I2CRESET` does NOT DEPEND on I2C and is unconditionally declared. This 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] NicholasChin commented on a change in pull
request #270: Small I2C fixups
Posted by GitBox <gi...@apache.org>.
NicholasChin commented on a change in pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#discussion_r379001861
##########
File path: drivers/i2c/Kconfig
##########
@@ -54,3 +50,7 @@ config I2CMULTIPLEXER_PCA9540BDP
endmenu # I2C Multiplexer Support
endif
+
+config ARCH_HAVE_I2CRESET
Review comment:
This Kconfig file isn't conditionally brought in when I2C is defined. So technically ARCH_HAVE_I2CRESET is declared at the same level as it was before. If it would be preferred for these to be declared in the larger Kconfig as before, I can change 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request
#270: Small I2C fixups
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#discussion_r378983337
##########
File path: drivers/i2c/Kconfig
##########
@@ -54,3 +50,7 @@ config I2CMULTIPLEXER_PCA9540BDP
endmenu # I2C Multiplexer Support
endif
+
+config ARCH_HAVE_I2CRESET
Review comment:
The reason is that ARCH_HAVE_I2CRESET is selected unconditionally in all architecture configurations that support I2C reset. That is correct, advertising that an architecture supports a feature is conditional. CONFIG_I2C is completely unnecessary to enable architecture I2C support.
If conditions are added to ARCH_HAVE_I2CRESET, the errors referring to unmet dependencies will be generated on all configurations that enabled architecture-specific I2C support but don't need CONFIG_I2C.
This same change was added a few weeks ago and this resulted in several new errors in build testing and had to be fixed by moving ARCH_HAVE_I2CRESET outside of I2C. It must be that way.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #270: Small I2C fixups
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #270: Small I2C fixups
URL: https://github.com/apache/incubator-nuttx/pull/270#discussion_r378932376
##########
File path: drivers/i2c/Kconfig
##########
@@ -53,4 +49,8 @@ config I2CMULTIPLEXER_PCA9540BDP
endmenu # I2C Multiplexer Support
+config ARCH_HAVE_I2CRESET
Review comment:
ARCH_HAVE_xxx should exist unconditionally, please see the discussion here:
https://github.com/apache/incubator-nuttx/pull/217
https://github.com/apache/incubator-nuttx/pull/228
https://github.com/apache/incubator-nuttx/pull/232
----------------------------------------------------------------
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
With regards,
Apache Git Services