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.

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

Jump to bottom

framebuf driver for embedded graphics #35

Merged
merged 15 commits into from
Jul 30, 2022
Merged

framebuf driver for embedded graphics #35

merged 15 commits into from
Jul 30, 2022

Conversation

arunvijayshankar
Copy link
Contributor

Added a framebuf driver for embedded graphics

@netlify
Copy link

netlify bot commented Jul 27, 2022 •
edited

✅ Deploy Preview for merry-scone-cc7a60 ready!

Name Link
🔨 Latest commit 97153c6
🔍 Latest deploy log https://app.netlify.com/sites/merry-scone-cc7a60/deploys/62e4aa96d323e50008c40382
😎 Deploy Preview https://deploy-preview-35--merry-scone-cc7a60.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

hawkw
hawkw reviewed Jul 27, 2022
View reviewed changes
source/melpomene/src/main.rs Outdated Show resolved Hide resolved
@jamesmunns
Copy link
Contributor

Hey @arunvijayshankar, I took a look and this looks really good!

One question - is it possible to move the simulator components to ONLY exist in melpo, not the kernel?

The kernel needs to be no_std, which means it should only hold the embedded-graphics dependency.

hawkw
hawkw reviewed Jul 29, 2022
View reviewed changes
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this is really cool! i left a few minor suggestions, i hope they're helpful.

source/kernel/Cargo.toml Outdated Show resolved Hide resolved
source/kernel/src/drivers/emb_display.rs Outdated Show resolved Hide resolved
source/kernel/src/drivers/emb_display.rs Outdated Show resolved Hide resolved
source/kernel/src/drivers/emb_display.rs Outdated Show resolved Hide resolved
source/kernel/src/drivers/emb_display.rs Outdated Show resolved Hide resolved
source/kernel/src/drivers/emb_display.rs
const DISP_WIDTH: u32 = 319;
const DISP_HEIGHT: u32 = 239;

// Registered driver
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this supposed to be a doc comment?

Suggested change
// Registered driver
/// Registered driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a comment I think. I retained the same style of comments from serial_mux.rs

source/kernel/src/drivers/emb_display.rs
kernel: &'static Kernel,
}

// FrameChunk is recieved after client has sent a request for one
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is this supposed to be a doc comment?

Suggested change
// FrameChunk is recieved after client has sent a request for one
/// FrameChunk is recieved after client has sent a request for one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also just a standard comment. I should probably put in document comments, come to think of it

source/kernel/src/drivers/emb_display.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Contributor

hawkw commented Jul 29, 2022

also, iit looks like rustfmt needs to be run on this branch to make CI happy.

arunvijayshankar and others added 9 commits July 29, 2022 15:43
@jamesmunns
Copy link
Contributor

As a note @arunvijayshankar, you can run cargo fmt --all from the root of the project, and it'll format all of the crates :)

@jamesmunns jamesmunns merged commit 1a0b819 into tosc-rs:main Jul 30, 2022
@jamesmunns jamesmunns mentioned this pull request Jul 30, 2022
Closed
@jamesmunns jamesmunns added platform: melpomene Specific to the Melpomene simulator platform area: drivers Related to driver implementation for hardware devices. labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hawkw hawkw hawkw approved these changes

Assignees
No one assigned
Labels
area: drivers Related to driver implementation for hardware devices. platform: melpomene Specific to the Melpomene simulator platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@arunvijayshankar @jamesmunns @hawkw