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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ rand = { version = "0.8.4", optional = true, default-features = false }
rand_chacha = { version = "0.3.1", optional = true, default-features = false }

[dependencies.noto-sans-mono-bitmap]
version = "0.1.2"
version = "0.1.6"
default-features = false
features = ["regular", "size_14"]
optional = true
Expand Down
5 changes: 4 additions & 1 deletion src/binary/logger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,10 @@ impl Logger {
if self.y_pos >= (self.height() - BITMAP_LETTER_WIDTH) {
self.clear();
}
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 •
edited
Loading

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.

tho... What?

https://www.urbandictionary.com/define.php?term=tho :)

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 •
edited
Loading

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 •
edited
Loading

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

);
self.write_rendered_char(bitmap_char);
}
}
Expand Down