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

Let kernels use the Logger #218

Closed
wants to merge 10 commits into from

Conversation

kennystrawnmusic
Copy link
Contributor

Since A, the next post in Edition 3 of phil-opp/blog_os is supposed to be all about text output, and B, the bootloader already defines a built-in way to do just that ― time to take the Logger out of its binary cocoon and let everyone use it.

@kennystrawnmusic
Copy link
Contributor Author

kennystrawnmusic commented Jan 30, 2022

To explain this further: unresolved import compiler errors are currently thrown when a kernel tries to use bootloader::binary::logger because binary only works when features = ["binary"] is added to Cargo.toml — ah, but simply making that change to its own Cargo.toml is not something a kernel can do without causing the bootloader to panic because it's hardcoded to require the bootloader's built-in custom targets. Hence why I proposed this PR which does 5 things:

  1. Move the crates that the Logger depends on (conquer_once, noto_sans_mono_bitmap, spinning_top and of course log) out of binary and into default in the bootloader's Cargo.toml
  2. Move src/binary/logger.rs up one level — to the crate root
  3. Move pub mod logger from src/binary/mod.rs to the crate root's lib.rs
  4. Add a pub use crate::logger line to src/binary/mod.rs for backward compatibility
  5. (By accident) duplicate the changes that were originally supposed to go into maybe_uninit_extra is no longer feature-gated in Rust 1.60 #215

This approach would allow kernels to simply use bootloader::logger instead of needing to reinvent the wheel. Any issues with this, @phil-opp?

@kennystrawnmusic
Copy link
Contributor Author

Made a few more changes to push this Logger API into a more robust state. Still have yet to hear from the maintainer(s).

@phil-opp
Copy link
Member

phil-opp commented Feb 3, 2022

Thanks for the PR, @kennystrawnmusic! I like the idea to give people a way to quickly add some debug printing to their kernels. However, I don't think that we should export the Logger/LockedLogger API, mainly because I want to avoid the maintenance overhead. There are already good crates for drawing to framebuffers such as embedded-graphics, so I see no reason to duplicate that functionality here.

So I propose the following:

  • Add an optional logger feature to this crate that is disabled by default.
  • If enabled, make the init_logger function public (and move it from the binary module to the root).
  • Keep the logger module private, even if the logger feature is enabled.

This way, we provide an easy way to get some output on the screen when getting started, without needing to provide and maintain a full framebuffer writer implementation. Does this sound good to you?

(I also want to mention that I'm currently working on a rewrite of this crate in the next branch, which will split off a bootloader-api crate. Kernels will only depend on this api crate then, so I'm not sure if we can/should share a logger implementation between the api crate and the main bootloader crate then.)

@kennystrawnmusic
Copy link
Contributor Author

Closing now — going to open a couple of new PRs in the next couple of hours to make the recommended changes.

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.

None yet

2 participants