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/04/07 21:20:04 UTC
[GitHub] [incubator-nuttx] gustavonihei opened a new pull request #3475: drivers/input: Add TOUCHSCREEN config and add NX_XYINPUT dependencies
gustavonihei opened a new pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475
## Summary
This PR intends to provide a new CONFIG_TOUCHSCREEN config. This configuration should be enabled in case any touchscreen input device is selected.
Related:
- https://github.com/apache/incubator-nuttx-apps/pull/666
- https://github.com/apache/incubator-nuttx/pull/3443
## Impact
No impact to existing board configurations, since CONFIG_TOUCHSCREEN is a hidden config.
## Testing
Verified dependencies via `menuconfig`. No impact to build process.
--
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] gustavonihei edited a comment on pull request #3475: drivers/input: Add TOUCHSCREEN config and add NX_XYINPUT dependencies
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#issuecomment-815300125
> I think that I tried to enforce was to include the name of the directory where a configuration is defined OR part of the name of the configuration that enables this one. You will see that most but not all configurations follow this convention. CONFIG_INPUT, for example, does not. Per that convention, it should be CONFIG_DRIVERS_INPUT. Then all of the top level configurations defined in that directory should be CONFIG_INPUT_*. So for example, CONFIG_TOUCHSCREEN should be CONFIG_INPUT_TOUCHSCREEN.
>
> This has the advantage that (1) by the name of the configuration, you (usually) know which Konfig file it is defined in and (2) it prevents configuration naming collisions. Kconfig variable name collisions are especially nasty because usually not error is generated when the redundant names are used.
I usually find myself spending some time to discover where a given config is defined, so I agree that we can improve this with a convention.
Although it may not reflect the directory structure, I believe that following the `CONFIG_<SUBSYSTEM>_*` template is more intuitive, like `CONFIG_INPUT_*` and `CONFIG_NX_*`.
Drivers could be classified as SUBSYSTEM, so we'd have `CONFIG_DRIVERS_<CLASS>_`. As an example, `MAX11802` could be redefined as `CONFIG_DRIVERS_INPUT_MAX11802`.
It would be a big refactor of our Kconfig system, probably affecting every subsystem.
--
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] gustavonihei commented on pull request #3475: drivers/input: Add INPUT_TOUCHSCREEN config
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#issuecomment-815634135
I created https://github.com/apache/incubator-nuttx/pull/3482 for renaming the `MOUSE` and the other classes.
--
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] gustavonihei edited a comment on pull request #3475: drivers/input: Add TOUCHSCREEN config and add NX_XYINPUT dependencies
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#issuecomment-815300125
> I think that I tried to enforce was to include the name of the directory where a configuration is defined OR part of the name of the configuration that enables this one. You will see that most but not all configurations follow this convention. CONFIG_INPUT, for example, does not. Per that convention, it should be CONFIG_DRIVERS_INPUT. Then all of the top level configurations defined in that directory should be CONFIG_INPUT_*. So for example, CONFIG_TOUCHSCREEN should be CONFIG_INPUT_TOUCHSCREEN.
>
> This has the advantage that (1) by the name of the configuration, you (usually) know which Konfig file it is defined in and (2) it prevents configuration naming collisions. Kconfig variable name collisions are especially nasty because usually not error is generated when the redundant names are used.
I usually find myself spending some time to discover where a given config is defined, so I agree that we can improve this with a convention.
Although it may not reflect the directory structure, I believe that following the `CONFIG_<SUBSYSTEM>_*` template is more intuitive, like `CONFIG_INPUT_*` and `CONFIG_NX_*`.
Drivers could be classified as SUBSYSTEM, so we'd have `CONFIG_DRIVERS_<CLASS>_`. As an example, `MAX11802` could be redefined as `CONFIG_DRIVERS_INPUT_MAX11802`.
It will be a big refactor of our Kconfig system.
--
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] patacongo commented on pull request #3475: drivers/input: Add TOUCHSCREEN config
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#issuecomment-815452389
This kind of short, unadorned naming will always lead to problems and name collisions. Again, I recommend that you consider CONFIG_INPUT_TOUCHSCREEN and CONFIG_INPUT_MOUSE instead of CONFIG_TOUCHSCREEN and CONFIG_MOUSE. That is must less likely to conflict.
--
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 #3475: drivers/input: Add TOUCHSCREEN config
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#discussion_r609234720
##########
File path: drivers/input/Kconfig
##########
@@ -30,17 +30,23 @@ config MOUSE_WHEEL
endif # MOUSE
+config TOUCHSCREEN
Review comment:
Ok, it's reasonable. @btashton could you take a look?
--
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] gustavonihei commented on pull request #3475: drivers/input: Add TOUCHSCREEN config
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#issuecomment-815577782
> This kind of short, unadorned naming will always lead to problems and name collisions. Again, I recommend that you consider CONFIG_INPUT_TOUCHSCREEN and CONFIG_INPUT_MOUSE instead of CONFIG_TOUCHSCREEN and CONFIG_MOUSE. That is must less likely to conflict.
Sorry, I may have not made myself clear. I agree with using `CONFIG_INPUT_TOUCHSCREEN` as well as `CONFIG_INPUT_MOUSE`, which would fit to what I described as `CONFIG_<SUBSYSTEM>_*`. It is just that I was intending to this renaming in a later PR.
For now, since TOUCHSCREEN is being created, although it won't follow the style from the other configs, it may be created in the most suitable template. I'll change it to `CONFIG_INPUT_TOUCHSCREEN`.
--
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 #3475: drivers/input: Add TOUCHSCREEN config
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#discussion_r609220109
##########
File path: drivers/input/Kconfig
##########
@@ -30,17 +30,23 @@ config MOUSE_WHEEL
endif # MOUSE
+config TOUCHSCREEN
Review comment:
should we apply the similar change to MOUSE and JOYSTICK?
--
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] davids5 merged pull request #3475: drivers/input: Add INPUT_TOUCHSCREEN config
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475
--
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] patacongo commented on pull request #3475: drivers/input: Add TOUCHSCREEN config and add NX_XYINPUT dependencies
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#issuecomment-815284115
One thing that I tried to do in the past was establish some conventions for configuration naming. I was not very successful in that as there are many, many configuration naming consistencies.
I think that I tried to enforce was to include the name of the directory where a configuration is defined OR part of the name of the configuration that enables this one. You will see that most but not all configurations follow this convention. CONFIG_INPUT, for example, does not. Per that convention, it should be CONFIG_DRIVERS_INPUT. Then all of the top level configurations defined in that directory should be CONFIG_INPUT_*. So for example, CONFIG_TOUCHSCREEN should be CONFIG_INPUT_TOUCHSCREEN.
This has the advantage that (1) by the name of the configuration, you (usually) know which Konfig file it is defined in and (2) it prevents configuration naming collisions. Kconfig variable name collisions are especially nasty because usually not error is generated when the redundant names are used.
We should come to an agreement as a group about configuration naming. Should we continue to try to follow this convention? Should we abandon it altogether? Should we establish a different convention for configuration names?
--
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] gustavonihei commented on a change in pull request #3475: drivers/input: Add TOUCHSCREEN config
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#discussion_r609229382
##########
File path: drivers/input/Kconfig
##########
@@ -30,17 +30,23 @@ config MOUSE_WHEEL
endif # MOUSE
+config TOUCHSCREEN
Review comment:
Yes, it makes sense to extend this approach to the other input classes.
For JOYSTICK should have no impact, since CONFIG_JOYSTICK currently does not exist.
But CONFIG_MOUSE will have an impact to existing configurations, because it will change the current behavior.
So my recommendation is that we address each one in a dedicated PR.
--
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] gustavonihei commented on pull request #3475: drivers/input: Add TOUCHSCREEN config and add NX_XYINPUT dependencies
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #3475:
URL: https://github.com/apache/incubator-nuttx/pull/3475#issuecomment-815300125
> I think that I tried to enforce was to include the name of the directory where a configuration is defined OR part of the name of the configuration that enables this one. You will see that most but not all configurations follow this convention. CONFIG_INPUT, for example, does not. Per that convention, it should be CONFIG_DRIVERS_INPUT. Then all of the top level configurations defined in that directory should be CONFIG_INPUT_*. So for example, CONFIG_TOUCHSCREEN should be CONFIG_INPUT_TOUCHSCREEN.
>
> This has the advantage that (1) by the name of the configuration, you (usually) know which Konfig file it is defined in and (2) it prevents configuration naming collisions. Kconfig variable name collisions are especially nasty because usually not error is generated when the redundant names are used.
I usually find myself spending some time to discover where a given config is defined, so I agree that we can improve this with a convention.
Although it may not reflect the directory structure, I believe that following the CONFIG_<SUBSYSTEM>_* template is more intuitive, like `CONFIG_INPUT_*` and `CONFIG_NX_*`.
Drivers could be classified as SUBSYSTEM, so we'd have `CONFIG_DRIVERS_<CLASS>_`. As an example, `MAX11802` could be redefined as `CONFIG_DRIVERS_INPUT_MAX11802`.
It will be a big refactor of our Kconfig system.
--
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