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

Implement proper font support #76

Merged
merged 4 commits into from
Mar 29, 2023
Merged

Implement proper font support #76

merged 4 commits into from
Mar 29, 2023

Conversation

nia-e
Copy link
Collaborator

@nia-e nia-e commented Mar 27, 2023

Goal of this PR is to enable support for custom fonts. This fixes #70 and closes #43. Current status and goals:

  • Support all builtin LVGL fonts
  • Support the C-encoded font that LVGL can generate
  • Document!
  • Go off-topic and bundle in a bunch of documentation additions and write a changelog apparently

@nia-e nia-e added the enhancement New feature or request label Mar 27, 2023
@nia-e
Copy link
Collaborator Author

nia-e commented Mar 27, 2023

Currently I'm thinking of shipping the built-in LVGL fonts as nightly-only, since they depend on the cfg_accessible feature which isn't stable. I probably could write macros to get this working otherwise like I did for drivers, or move it into lvgl_codegen (since I can just read the constants defined in lv_conf.h). This would also enable lv_drivers being usable without macro spam (there were so many fonts here I couldn't justify the same kind of macro spam to myself...) However, I'll probably do that Later™️

@nia-e
Copy link
Collaborator Author

nia-e commented Mar 28, 2023

I tested out the idea of reverting #75 and reborrowing the pointer in display.rs as a &mut T but it did not seem to work and I think I get why. The body of the function makes relatively heavy use of typical rust references, and mixing references and pointers does tend to be somewhat frowned upon, so I'm guessing we're causing UB somehow by simply accessing the LVGL display driver as a normal reference - regardless, moving out and back in seems to be fine and conceptually this makes sense as it enforces a hard limit between raw pointer land and reference land. I'd love to run miri on everything and be able to point a finger at the problem but alas miri doesn't support FFI, so in my opinion I'd say we should stick to this for now. It's a small enough copy that it shouldn't be a huge problem, as the deref only performs a shallow copy so it's just a few hundred bytes of pointers being copied.

We're probably causing potential UB all over the place in lvlg-rs and I definitely didn't make this any better after jury-rigging the hack that is #67 but keeping a move boundary between rust and C bits should be a durable solution (I hope).

@nia-e nia-e changed the title [WIP] Implement proper font support Implement proper font support Mar 28, 2023
@nia-e nia-e mentioned this pull request Mar 28, 2023
10 tasks
@nia-e
Copy link
Collaborator Author

nia-e commented Mar 28, 2023

This is ready to be merged, just needs commits squashed. I added some docs to the driver usage as well. Let me know if I missed anything (and especially if you have better ideas for this)

@nia-e nia-e marked this pull request as ready for review March 28, 2023 15:26
@nia-e nia-e requested a review from rafaelcaricio March 28, 2023 15:26
@nia-e
Copy link
Collaborator Author

nia-e commented Mar 28, 2023

Also: the demo example works as intended again :D
image

@nia-e
Copy link
Collaborator Author

nia-e commented Mar 28, 2023

Squashed commits. This is ready to merge for real I promise I won't add any more documentation on this PR

@nia-e
Copy link
Collaborator Author

nia-e commented Mar 28, 2023

Bah, I couldn't help myself. We have a changelog now 😅

@rafaelcaricio
Copy link
Collaborator

rafaelcaricio commented Mar 29, 2023

@nia-e feel free to merge your PRs. At this point I thrust you know what you are doing, and I don't want to slow you down. 😉 I will be looking sporadically when I get some spare time.

@nia-e
Copy link
Collaborator Author

nia-e commented Mar 29, 2023

Thanks so much, I really appreciate the trust :D I still need to request a review every time since I can't merge without an approval, though

@nia-e nia-e merged commit c0263a2 into master Mar 29, 2023
@nia-e nia-e deleted the fonts branch March 29, 2023 11:43
@rafaelcaricio
Copy link
Collaborator

@nia-e You shouldn't need any more approvals to merge PRs now. PRs are still needed, but you should be able to merge them right away.

@nia-e
Copy link
Collaborator Author

nia-e commented Mar 29, 2023

Woo, thanks! I have another one I should be opening in a hot minute regarding #69

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix regression in font support Improved custom font support
2 participants