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

Update new hardware interrupts post #473

Merged
merged 20 commits into from
Oct 17, 2018
Merged

Update new hardware interrupts post #473

merged 20 commits into from
Oct 17, 2018

Conversation

acheronfail
Copy link
Contributor

@acheronfail acheronfail commented Oct 15, 2018

Hey there, this PR updates the new hardware-timer branch with the following:

  • rebases it off of master so it pulls in those extra changes
  • updates the source code to match the new post (it builds successfully)
  • adds in some screenshots and a gif of QEMU showing the hardware timer

EDIT: I just noticed that some more content has been added recently (I hadn't seen your updates from 5 days ago). So, this basically gets everything working before those updates.

If you'd like, I'm happy to continue through and provide all the other missing pieces (screenshots, etc) from your more recent updates as well. Just say the word. 🙂

@acheronfail acheronfail changed the title Hardware interrupts Update new hardware interrupts post Oct 15, 2018
phil-opp added a commit that referenced this pull request Oct 15, 2018
@phil-opp phil-opp force-pushed the hardware-interrupts branch 2 times, most recently from 164e9a8 to 87f6e73 Compare October 15, 2018 17:36
@phil-opp
Copy link
Owner

Thanks a lot for helping to get this over the finish line! Do you have any feedback or suggestions for the post?

I'm still not happy with the main.rs/interrupts.rs split. Ideally, I would like all interrupt related code in interrupt.rs, including the IDT and the exception handlers. The question is whether I should add a refactoring step to this guide or update the previous exception posts to use interrupts.rs right from the start…

@phil-opp
Copy link
Owner

I'm having trouble merging this because it would clutter the history. I tried to merge it manually, but github does not mark this PR as merged then.

Could you rebase your PR on top of the current hardware-interrupts branch to solve this? (If you're fine with manually merging and closing this PR, that's fine for me too.)

@acheronfail
Copy link
Contributor Author

acheronfail commented Oct 15, 2018

The question is whether I should add a refactoring step to this guide or update the previous exception posts to use interrupts.rs right from the start…

If you're asking me, I'd probably prefer to place everything to do with interrupts in interrupts.rs. For me, it makes it clearer, and also prepares well for the future if there's heaps of code related to the interrupts.

Could you rebase your PR on top of the current hardware-interrupts branch to solve this?

Sure thing, I'll rebase the changes and update this PR. 👍

@acheronfail
Copy link
Contributor Author

As I'm moving on and testing the code, I've found that this section isn't totally accurate:

We now see that a k appears whenever we press or release a key. The keyboard controller generates continuous interrupts if the key is hold down, so we see a series of ks on the screen.

When I run this in QEMU, it only prints the first k and nothing else. I believe this is because the keyboard won't send another interrupt if the data hasn't first been read out of its port.

I'll update this PR with a few updates and finish it off. If you have any objections let me know. 🙂

@acheronfail
Copy link
Contributor Author

acheronfail commented Oct 15, 2018

That's all the gifs and changes needed to finish the post as it is.

If we want to re-write the previous exception posts to use interrupts.rs from the start, should we first merge this and then open another PR? I'm happy to re-write them if that's the case.

@acheronfail
Copy link
Contributor Author

acheronfail commented Oct 16, 2018

I'm still not happy with the main.rs/interrupts.rs split. Ideally, I would like all interrupt related code in interrupt.rs, including the IDT and the exception handlers. The question is whether I should add a refactoring step to this guide or update the previous exception posts to use interrupts.rs right from the start…

@phil-opp for when you get the chance to have a look, here's the current diff of the re-write which places all exception code in src/interrupts.rs as soon as possible: https://github.com/acheronfail/blog_os/pull/1/files (note that this branch is currently pointing to my fork's hardware-interrupts branch. Once this PR is merged, I can re-point it to your hardware-interrupts instead).

@phil-opp
Copy link
Owner

Sorry for the delay, I was busy yesterday. Looks good to me!

@phil-opp phil-opp merged commit 48548fa into phil-opp:hardware-interrupts Oct 17, 2018
@phil-opp
Copy link
Owner

Thanks so much for your contributions!

@acheronfail
Copy link
Contributor Author

Sorry for the delay, I was busy yesterday. Looks good to me!

Haha no worries mate, there's no rush! 👍

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.

6 participants