-
Notifications
You must be signed in to change notification settings - Fork 4
Preliminary Giga Touch support on Zephyr #14
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
base: main
Are you sure you want to change the base?
Conversation
@per1234 @leonardocavagnis and all... Not sure what to do with this clang format failure? Looks like you added this something like two years ago, and Not sure that is something that should be done as part of this PR? As for example, I have not touched any of your Thanks |
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.
I haven't tested this yet but I think we should move this to a separate file? For example src/Arduino_GigaDisplayTouchZephyr.cpp
and then move the old one to src/Arduino_GigaDisplayTouchMbed.cpp
Also, please fix your formatting. For example:
- Please pick a style:
Arduino_GigaDisplayTouch::~Arduino_GigaDisplayTouch()
{
}
bool Arduino_GigaDisplayTouch::begin() {
- Fix indentation:
void Arduino_GigaDisplayTouch::end()
{
registerGigaTouchCallback(nullptr);
}
- Remove empty lines such as:
uint8_t Arduino_GigaDisplayTouch::getTouchPoints(GDTpoint_t* points, uint32_t timeout) {
// First wait to see if we get any events.
- Remove empty spaces at end of lines:
// First wait to see if we get any events.
Arduino_GigaDisplayTouch(TwoWire& wire, | ||
uint8_t intPin, | ||
uint8_t rstPin, | ||
#else |
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.
Do we need to duplicate the class?
#else | |
#elif defined(__ZEPHYR__) | |
Arduino_GigaDisplayTouch(); | |
#else |
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.
Note: I pushed up a clang-format of the files and then restricted these changes to the zephyr area.
This is sort of a global question, that we (@mjs513 and I) have been trying to ask. How does Arduino want to handle some of these libraries? There are several different options:
a) Completely different libraries, that potentially part of the Board installs. Like: Camera, Wire, SPI are done now
b) As libraries you install from the library manager. With this there are at least 3 variations that I have seen (and done)
- Try to intersperse all the code the MBED and Zephyr stuff with lots of #ifdef...
- Like I did, where there is like 1 #ifdef or the like with the two different implementations.
- like 2, except that split the files up. like you might have Arduio_GigaDisplayTouch_zephyr.cpp and these have
the same #if stuff like 2, but easier to locate the code and defines.
There are pros and cons to all of these.
So again not sure which way?
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.
I don't think there's a single rule that fits all, this is probably best decided on a case-by-case basis, so please use your better judgement. For example, if you find yourself adding a whole new alternate implementation, then it should probably go in a new file. However, there might be an argument for keeping everything in the same file if support for Mbed and other cores is planned to be removed, that would make it easier to do so. I'm also not sure if this is planned or not, perhaps @pennam knows more about this.
EDIT: No plan to deprecate other cores anytime soon, so the header file should use interspersed ifdef
's, the implementation should be in two separate files: Arduino_GigaDisplayTouchZephyr.cpp
and Arduino_GigaDisplayTouchMbed.cpp
.
Completely different libraries, that potentially part of the Board installs. Like: Camera, Wire, SPI are done now
I think core libraries provide core functionality like Wire and SPI. Libraries like touchscreen, BLE etc.. are not core functionality.
@@ -122,7 +217,7 @@ class Arduino_GigaDisplayTouch { | |||
private: | |||
TwoWire& _wire; | |||
uint8_t _intPin; | |||
mbed::InterruptIn _irqInt; | |||
mbed::InterruptIn _irqInt; |
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.
Same here we just hide all these private functions and variables with #ifdef __MBED__
.
Zephyr: Added in our preliminary support for touch on the display In ArduinoCore-zephyr added a callback to register with the input class. When set it simply forwards the callbacks to this function. Then added some support. We still don't have things like gestures as I don't think the zephyr input class has support for it. Have a simple touch paint sketch (now supporting multiple touches, that appears to work Edit: clang-format zephyr parts.
Zephyr: Added in our preliminary support for touch on the display
In ArduinoCore-zephyr added a callback to register with the input class. When set it simply forwards the callbacks to this function.
Then added some support. We still don't have things like gestures as I don't think the zephyr input class has support for it.
Have a simple touch paint sketch (now supporting multiple touches, that appears to work
Note: This is related to the ArduinoCore-zephyr PR.
arduino/ArduinoCore-zephyr#134