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

Adding generic display driver #118

Merged
merged 6 commits into from
Mar 10, 2023
Merged

Adding generic display driver #118

merged 6 commits into from
Mar 10, 2023

Conversation

Ellerbach
Copy link
Member

Description

Adding generic display driver

Motivation and Context

How Has This Been Tested?

On real devices with real screen

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
  • I have added new tests to cover my changes.

@nfbot nfbot added Type: enhancement New feature or request Breaking-Change Type: documentation Improvements or additions to documentation labels Mar 8, 2023
Copy link
Contributor

@TerryFogg TerryFogg left a comment

Choose a reason for hiding this comment

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

I guess this is a work in progress as I can't see the calls to a native method.
One thing to note, I found that the use of properties when compiled into native stubs causes every property to have two NULL entries on the method call which I assume takes 8 bytes.
Using fields eliminates this.
I usually use properties as it allows extra code checking if required and I'm sure the compiler optimises it all, but, in this situation the stubs become much much larger than required.

Its a shame that everything needs to be cast as byte on the call since we can call the init code with different drivers which have different enums and they can't by definition be related.
An alternate, maybe a little cleaner is to use a class instead of an enum and create a series of public const byte fields. This way you can reference the codes without the case

Also, would this be a good time to start the split up into DisplayController, Primitives and optionally nanoPresentation, Touch?
So the DisplayController could also have a basic embedded font for a minimal system, requiring no bitmaps etc.
I have a basic font and writing direct to display working

@Ellerbach
Copy link
Member Author

I guess this is a work in progress as I can't see the calls to a native method.

There is no call to native methods. There is a generic driver on the native side. See the native PR.

An alternate, maybe a little cleaner is to use a class instead of an enum and create a series of public const byte fields. This way you can reference the codes without the case

It's a matter of optimization. 1 class, few arrays and all is there. Then the magic happens fully transparently on the native side. With a normal driver or a generic ones, there is no difference at all. No breaking changes neither from existing code (OK, I renamed the orientation enum but should not be too damageable on the managed side.

For the rest of the remarks, it's in the backlog to be optimized.

@Ellerbach
Copy link
Member Author

For some reasons, the pragma did not solve the code smell which is only on auto properties. So I'll leave it like this!

Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

LGTM!

@josesimoes
Copy link
Member

Worth noting that there are several enums being passed to the native declaration that aren't used there.
We can (should) make those go away to simplify maintenance and future updates.

@Ellerbach check this out on how to exclude enums that are not used in native.
With this they are not added to the stubs. 

I'm not familiar with the code, so please check if any of these should indeed be there. And, of course, do add all the others that don't need to be there.
@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 30 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Ellerbach Ellerbach merged commit f7a9399 into main Mar 10, 2023
@Ellerbach Ellerbach deleted the adjust-graphics branch March 10, 2023 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking-Change Type: documentation Improvements or additions to documentation Type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants