fixed extends for dynamic rotation, added simple rotate function, added example

[espHorst]

Hi ImpulseAdventure,

as discussed in #68 here a PR trying to fix the dynamic
bool gslc_DrvRotateSwapFlip(gslc_tsGui* pGui, uint8_t nRotation, uint8_t nSwapXY, uint8_t nFlipX, uint8_t nFlipY )
function.

Furthermore I have added the function
bool gslc_DrvRotate(gslc_tsGui* pGui, uint8_t nRotation)
This function auto sets nSwapXY, nFlipX and nFlipY by inspecting the change of nRotation to the previous pGui->nRotation.

Finally this was tested on a STM32F1 with ILI9341 and XPT2046. The according
example is added as gslc_ex17_ard.ino

For me it works perfect, and I assume that it works with every combination display / touch.
But I was only able to test it on my hardware.

It only requires

#define ADATOUCH_SWAP_XY  
#define ADATOUCH_FLIP_X   
#define ADATOUCH_FLIP_Y   

in GUIslice_config_ard.h to be set correctly for the
#define GSLC_ROTATE
that is defined in GUIslice_config_ard.h

[ImpulseAdventure]

Thanks @espHorst !

I have been doing further testing of this PR but wondered if you could confirm for me the default config you started with in your tests on the ILI9341? ie.

DRV_DISP_*
DRV_TOUCH_*
GSLC_ROTATE
ADATOUCH_SWAP_XY
ADATOUCH_FLIP_X
ADATOUCH_FLIP_Y

Thanks!

[espHorst]

I did not change the default config in the commited GUIslice_config_ard.h file.
For my personal configuration (ILI9341 and XTP2046) I use the following configuration:

#define DRV_DISP_ADAGFX_AS
#define DRV_TOUCH_XPT2046
#define STM32_NOTATION
#define GSLC_ROTATE 1
#define ADATOUCH_SWAP_XY 0
#define ADATOUCH_FLIP_X 1
#define ADATOUCH_FLIP_Y 1

[ImpulseAdventure]

The code looks good, and I like the partitioning of functionality between DrvRotate() and DrvRotateSwapFlip(). The new example gslc_ex17 demonstrates the dynamic rotation nicely.

When testing the PR, I did notice that the nRotation relative transformation that occurs on nSwapXY, nFlipX and nFlipY appears to require different logic on my STMPE610 versus what you have tested on the XPT2046, otherwise I'll see the X touch handling flipped in certain orientations (as confirmed by DBG_TOUCH).

Empirically, it looks like ILI9341+STMPE610 might expect the following as a function of GSLC_ROTATE:

GSLC_ROTATE=0: Swap=0; FlipX=0; FlipY=0;
GSLC_ROTATE=1: Swap=1; FlipX=0; FlipY=1; (default in GUIslice_config_ard.h)
GSLC_ROTATE=2: Swap=0; FlipX=1; FlipY=1;
GSLC_ROTATE=3: Swap=1; FlipX=1; FlipY=0;

On the basis of your XPT2046 config and the new transform logic in DrvRotate, I assume ILI9341+XPT2046 might require: (this was done by hand, so hopefully I got it right)

GSLC_ROTATE=0: Swap=1; FlipX=0; FlipY=1;
GSLC_ROTATE=1: Swap=0; FlipX=1; FlipY=1;
GSLC_ROTATE=2: Swap=1; FlipX=1; FlipY=0;
GSLC_ROTATE=3: Swap=0; FlipX=0; FlipY=0;

If the above is correct, then I'm starting to wonder if:

1. Whether we need to provide per-touch driver relative transforms (by relative, I mean calculating nSwapXY, nFlip* from the old pGUI->nRotation and the new nRotation)
2. Or, if it might even be practical to hardcode the Swap/Flip settings on the basis of touch driver & nRotation alone (ie. like in the above tables, as opposed to computing a transform).

I generally don't like hardcoding, but IF the mapping from GSLC_ROTATE/nRotation to the Swap/FlipX/FlipY tuple is constant / only a function of the touch driver, then it might make sense to encode this in DrvRotate. One reason I'm somewhat interested in this approach is that it could mean the elimination of the ADATOUCH_SWAP_XY / FLIP_X / FLIP_Y config settings. (which are probably confusing for new users).

However, I suspect the mapping from GSLC_ROTATE/nRotation to Swap/FlipX/FlipY might be dependent on the combination of display module and touch driver (DRV_TOUCH_*). If so, providing user configuration (ADATOUCH_*) is probably the most practical approach.

Given the difference in behavior I've noted here between touch controllers, do you have an opinion on an approach that makes sense?

thanks again

[espHorst]

I have made a mistake in the calculations. Indeed the calculations depend on the state of the swap, not only on the difference between the current and the next rotation. Thus the false behaviour for your touch. It just starts with another swap setting. I will work on a fix.

If all touch screens have in their default setting a correct x and y direction, i. e. x and y are not flipped, then a simple rotation will do the job for all different touch displays. Then it is sufficient to specify a "GLSC_TOUCH_ROTATION". This could be specified by the user. So at maximum the user has to try four settings. I hope this is easier for the user as to try eight settings for flipX,Y and swap. If the GLSC_TOUCH_ROTATION is specified correctly then a bug fixed version of gslc_DrvRotate should do the job for dynamic rotation.

I will commit a fix into the PR within the next day(s).

[ImpulseAdventure]

Thanks for identifying the difference -- seems to explain the observations. I agree that it appears likely that we could get away with a touch controller orientation setting (ie. 4 settings) in the config. As for defining the config settings: I'm thinking it should be relatively straightforward to create a training example that instructs the user what touch setting(s) to use: reset the Swap/FlipX/FlipY and then ask the user to press a button in each nRotation mode (maybe facilitated by #24). Assuming the button was placed well to the interior of a given quadrant (and with layout scaled to screen size), one should be able to detect the mapping based on what quadrants were actually clicked.

BTW -- later on, I intend to create a gslc_GuiRotate() function that invokes your DrvRotate() function (per display driver) since it would be ideal if users don't need to call driver-scope functions directly.

[espHorst]

I have pushed a bug fix. I have successfully tested it for STM32F1+ILI9341+XPT2046 and furthermore I have printed the settings with default values for STMPE610 - the results looks promising, i.e. this are the values that @ImpulseAdventure you have evaluated empirically:

s,x,y=0,0,0
s,x,y=1,0,1
s,x,y=0,1,1
s,x,y=1,1,0

Moreover in the GUIslice_config_ard.h you can configure now both ways:
a) setting a #define GSLC_TOUCH_ROTATE x AND
b) setting the swap/flip values as before
Both values are combined - thus the code should be fully backwards compatible.
There is a proposal in the GUIslice_config_ard.h to set swap/flip values to zero and just to
use #define GSLC_TOUCH_ROTATE - but it also works the old way if GSLC_TOUCH_ROTATE is defined as zero - which is the default.
@ImpulseAdventure :
GSLC_TOUCH_ROTATE for your setup should be the same value as GSLC_ROTATE.
For my setup it is GSLC_ROTATE + 1.
If this works on your setup, then I can add an additional dynamic glsc_touch_rotate_offset parameter. This can be used for a automatic configuration as you described (asking the user to press a button).

By the way: Maybe this can be combined with a touch screen calibration.
I think it is possible to define some "calibration" pages within one example.
On four successive displayed pages a cross is displayed in one corner on each page.
The user just touches the cross and then we can output

• the GSLC_TOUCH_ROTATE
• the four touch screen x/y min/max calibration values
  on the screen.

[espHorst]

One further comment:
I have added some macros to the GUIslice_config_ard.h file, I think it would be better to place them in a include file which is common to all drivers. But I was not sure about this.

  // TODO: maybe those macros should be moved to one include file which is included by all drivers
  #define TOUCH_ROTATION_DATA 0x6350
  #define TOUCH_ROTATION_SWAPXY(rotation) ((( TOUCH_ROTATION_DATA >> ((rotation&0x03)*4) ) >> 2 ) & 0x01 )
  #define TOUCH_ROTATION_FLIPX(rotation)  ((( TOUCH_ROTATION_DATA >> ((rotation&0x03)*4) ) >> 1 ) & 0x01 )
  #define TOUCH_ROTATION_FLIPY(rotation)  ((( TOUCH_ROTATION_DATA >> ((rotation&0x03)*4) ) >> 0 ) & 0x01 )
 

[ImpulseAdventure]

I have tested your pull request with my setup and so far it has been working nicely.

I made a couple additional updates:

• Added GuiRotate() as wrapper for DrvRotate(). Only supports DRV_DISP_ADAGFX for now, and will alert user with error if attempted in unsupported mode.
• Moved the TOUCH_ROTATION_* macros into GUIslice.h. I think it will be safe to keep the transformation matrix (TOUCH_ROTATION_DATA) hardcoded. If users report that changing GSLC_TOUCH_ROTATE is insufficient to get correct mapping, then we can move these macros back into the editable config file.
• Detection & warning for out-of-date config

Config Breaking Change

One important point about the PR is that it would normally be a breaking change. If users have not updated their config files to the latest version, compilation would normally fail due to the missing GSLC_TOUCH_ROTATE definition. As a large number of users are likely to encounter this, I added a warning preprocessor directive to alert users of the out-of-date config but then proceed with a default (=GSLC_ROTATE). One of the benefits of relocating the TOUCH_ROTATION_* defines into GUIslice.h is that it becomes easier to provide a compiler fallback.

Thankfully, in most cases the minimal config update for users should just require the addition of the following line to their GUIslice_config_*.h file: (with potentially a different number from 0..3)

#define GSLC_TOUCH_ROTATE 1

Config Patching

On the topic of changes to config files, I have considered creating a "patch" flow for users. As the config files are moderately complex, it may require moderate effort to update one's config file if many parts have changed. Ideally, a user could always update to the latest config without losing their customizations by using a "patch" script... something to consider for later.

Next Steps

Assuming this update works for people, I will proceed to implement the equivalent functionality for other display drivers.

Creating a rotation detection & calibration example might be worth considering later.

[ImpulseAdventure]

Note that I have filed PR #72 to incorporate the combination of changes in this PR as well as the additional edits mentioned above. @espHorst -- if you could have a look at my proposed changes, that would be great.

I wasn't sure of the best way to enable you to test my additional changes without pushing to your master branch, so I created a separate branch (& PR) with the merged changes. If there was a better approach without effectively duplicating the PR, I'd be interested in learning :)