Skip to content

[Logger] Don't panic if unsupported char #256

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

Conversation

asensio-project
Copy link
Contributor

Hello,

Currently, when we print a char in the screen if it is unsupported on noto-sans-mono-bitmap it panics.
Now the noto-sans-mono-bitmap supports the '■' char, we can use it instead of panicking.

Changes included in this PR:
- Update noto-sans-mono-bitmap (Cargo.toml)
- Don't panic if an unsupported char is found (src/binary/logger.rs)

Thanks!

let bitmap_char = get_bitmap(c, FontWeight::Regular, BitmapHeight::Size14).unwrap();
let bitmap_char = get_bitmap(c, FontWeight::Regular, BitmapHeight::Size14)
.unwrap_or(
get_bitmap('■', FontWeight::Regular, BitmapHeight::Size14).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe � (Replacement Character)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are you referring? � it's not in the noto-sans-mono-bitmap crate yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's not in the noto-sans-mono-bitmap crate yet.

Really? I would have expected it to be. Guess it is fine to leave it like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, later we can ask to @phip1611 for adding that char. Two days ago the other char was not.
See this issue for more information.

Copy link
Member

@phip1611 phip1611 Sep 3, 2022

Choose a reason for hiding this comment

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

Really? I would have expected it to be. Guess it is fine to leave it like this.

Maintainer here: I'm about to add more unicode ranges as selectable cargo features. Thus, it will be available soon, hopefully. Currently, this specific symbol is not included.

However, having a reliable default as fall back is important and a good. One can argue whether this should be a space or the ■, tho

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I never listened that. I'm from Spain and English in Spain is quite bad.
Thanks, now I have one word more.

Copy link
Contributor Author

@asensio-project asensio-project Sep 3, 2022

Choose a reason for hiding this comment

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

@bjorn3 So we merge it with the ■, or we wait for the other char (�) which is more descriptive?

Copy link
Member

@phip1611 phip1611 Sep 25, 2022

Choose a reason for hiding this comment

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

Update regarding unicode features in the noto-sans-mono-bitmap crate: I experimented with it but even adding a "reasonable amount" of unicode ranges results in a 12 GB large crate, as all the characters must be pre-rasterized and shipped with the crate.

This is unacceptable. I decided that noto-sans-mono-bitmap has to strive for minimality. If you want the whole unicode range @asensio-project , switch to user-mode in your kernel project and use the floating point unit for dynamic font rendering. You will be able to use dynamic rendering of all the chars you can think of.

I'll keep you updated when I have something release-ready

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will create a PR for adding the � char to noto-sans-mono-bitmap-rs and then we can merge it for the main branch and the next branch of the bootloader. @phip1611

@phip1611
Copy link
Member

phip1611 commented Oct 1, 2022

I opened #267 to bump the noto-sans-mono-bitmap to 0.2.0 (unreleased yet). This will make this PR obsolete.

@asensio-project
Copy link
Contributor Author

Closing this in favor of #267.

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.

3 participants