Skip to content
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

display: Support arbitrary resolutions #311

Closed
wants to merge 1 commit into from

Conversation

sp1187
Copy link
Contributor

@sp1187 sp1187 commented Oct 15, 2022

This PR adds support for custom resolutions by changing the replacing the existing resolution_t enum type with a struct type of the same name that includes width, height and interlacing as its values, which is then sent as an argument to display_init like before. The new implementation includes some asserts to protect against invalid settings that do not work on real hardware.

There is still one bug left to resolve. When changing resolutions frequently, it seems that some resolutions will break the VI output in real hardware and leave it in a corrupt state that remains even after soft reset. This can be reproduced that applying this patch to the test example that I made for testing the feature and then press D-pad right, then D-pad down (which should correspond to 322x241).

diff --git a/examples/test/test.c b/examples/test/test.c
index b211a01e..b25fd846 100644
--- a/examples/test/test.c
+++ b/examples/test/test.c
@@ -70,10 +70,8 @@ int main(void)
         /*Fill the screen */
         graphics_fill_screen( disp, 0 );
 
-        sprintf(tStr, "Video mode: %lu\n", *((uint32_t *)0x80000300));
+        sprintf(tStr, "Size: %lux%lu\n", res.width, res.height);
         graphics_draw_text( disp, 20, 20, tStr );
-        sprintf(tStr, "Status: %08lX\n", *((uint32_t *)0xa4400000));
-        graphics_draw_text( disp, 20, 30, tStr );
 
         /* Full bright color */
         graphics_draw_box(disp, 20, 40, 20, 20, graphics_make_color(255, 0, 0, 255));
@@ -125,32 +123,28 @@ int main(void)
         if( keys.c[0].up )
         {
             display_close();
-
-            res = RESOLUTION_640x480;
+            res.height--;
             display_init( res, bit, 2, GAMMA_NONE, ANTIALIAS_RESAMPLE );
         }
 
         if( keys.c[0].down )
         {
             display_close();
-
-            res = RESOLUTION_320x240;
+            res.height++;
             display_init( res, bit, 2, GAMMA_NONE, ANTIALIAS_RESAMPLE );
         }
 
         if( keys.c[0].left )
         {
             display_close();
-
-            bit = DEPTH_16_BPP;
+            res.width -= 2;
             display_init( res, bit, 2, GAMMA_NONE, ANTIALIAS_RESAMPLE );
         }
 
         if( keys.c[0].right )
         {
             display_close();
-
-            bit = DEPTH_32_BPP;
+            res.width += 2;
             display_init( res, bit, 2, GAMMA_NONE, ANTIALIAS_RESAMPLE );
         }
     }

Copy link
Collaborator

@rasky rasky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sp1187, I'll play with the sample and see if I can pinpoint the crashing configurations

static const resolution_t RESOLUTION_512x480 = {512, 480, true};
/** @brief 640x480 mode, interlaced */
static const resolution_t RESOLUTION_640x480 = {640, 480, true};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static members are not exposed in documentation, which would make the API a bit harder to understand. One alternative is to declare them here, and define them in display.c.

include/display.h Show resolved Hide resolved
include/display.h Show resolved Hide resolved
@sp1187 sp1187 force-pushed the resolution branch 2 times, most recently from 988feec to 5738384 Compare October 27, 2022 16:44
@sp1187
Copy link
Contributor Author

sp1187 commented Oct 27, 2022

Seems that waiting for vblank before updating the VI settings was enough to avoid the VI crash, I updated the PR to have waits in display_init and display_close.

@sp1187 sp1187 marked this pull request as ready for review October 27, 2022 16:46
@rasky
Copy link
Collaborator

rasky commented Nov 13, 2022

Merged manually, thanks

@rasky rasky closed this Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants