-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for XPT2046 touch screens as own module #1242
Conversation
A relevant point might be, that part of the source code I used is GPL at the moment. If this is a problem I would ask the creator to change it to enable an integration. |
uint16_t _cal_vi1, _cal_vj1; | ||
|
||
// Average pair with least distance between each | ||
static int16_t ICACHE_FLASH_ATTR besttwoavg( int16_t x , int16_t y , int16_t z ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ICACHE_FLASH_ATTR
is redundant since the default mapping for nodemcu firmware is flash. Please remove these attributes.
So far I checked just some formal coding aspects. PR is missing an API documentation in |
I just added the documentation and modified the source code. |
The GPL aspect is unfortunate, let's see what their response is. @TerryE I guess this is another reason why it would be good to have a separate repo for non-core modules. |
I also don't like a contaminating nature of GPL. Anyone who uses a GPL component has to adopt GPL for their own product licence. Maybe we need an I note that the PHP project takes a harder line: they don't allow GPL source in the core code base for this reason. |
As far as I understand, the API suggests that the user has to poll for position updates. What I'm missing is a callback mechanism so that the application gets triggered when the screen is touched. This would better fit the event-driven approach of the Lua firmware in my opinion. What is your view here - Let the user handle interrupt callbacks or does this need to go into the module (as it's already dealing with IRQ pin)? |
``` | ||
|
||
|
||
## xpt2046.getPositionMeaned() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
averaged instead of meaned?
On a general aspect, this PR adds a controller of what I'd call the touchscreen or pointer class. How about having this class as a single module and specific controllers derive from it? These would then all implement methods of a common API. What's the position of other contributors? |
To me this strongly implies that it should be internal to the module.
The theory sounds good. I haven't got any driver-level hands-on experience with even a single touch-screen controller, so I'm not in a good position to assess feasibility of it. |
Needs to go into the module.
Thanks for that feedback, I had meant to raise that as well but you beat me to it. From a Lua developer's perspective touch support should IMO work like U8G/UCG - you select the module and the appropriate driver. |
@Starofall does the feedback from us committers make sense to you? Do you think you can incorporate the requested changes? |
@marcelstoer I first wanted to wait for an ok from the library creator to fix the licensing problem. |
I'm glad the licensing issue got resolve so effectively. |
No feedback from PR author in a long time, pity -> closing |
I integrated the source from the referenced github projects into the nodemcu-firmware.
This offers fast and efficient access to the touch display on a nodemcu.
Especially as many ILI9341 (which is supported by the ucg lib) displays uses the XPT2046 for touch.