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 boot.asm #18

Merged
merged 5 commits into from
Jul 26, 2024
Merged

Update boot.asm #18

merged 5 commits into from
Jul 26, 2024

Conversation

franzageek
Copy link
Contributor

  • Refactor the code (split it into .text and .data section);
  • Improve label naming;
  • Remove jumps before 'ret's.

- Refactor the code (.text and .data section);
- Improve label naming;
- Remove jumps before 'ret's;

Signed-off-by: franzageek <[email protected]>
Copy link
Owner

@adamperkowski adamperkowski left a comment

Choose a reason for hiding this comment

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

Hey. Thanks for contributing again!
The code looks good but there's one small issue. It doesn't boot. I'll look into it later today.

@franzageek
Copy link
Contributor Author

Oop so sorry, lemme try to fix it. It could be that no entry point was specified, I'll give it a try.

Signed-off-by: franzageek <[email protected]>
@adamperkowski
Copy link
Owner

I'll add a boot check to this assembly CI, because now it only compiles and that could be misleading.

Copy link
Owner

@adamperkowski adamperkowski left a comment

Choose a reason for hiding this comment

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

Still doesn't boot for me in QEMU.

@franzageek
Copy link
Contributor Author

franzageek commented Jul 26, 2024

Adding an extra boot check sounds like a good idea. I'm on a Mac rn so I couldn't test the code before committing, thinking that because the changes were not invasive, it should've booted just fine. I've just updated it - lmk if it works fine :)
EDIT: didn't see the previous comment - my bad.
Working on a fix rn.
EDIT2: omw to install QEMU rn.

@franzageek
Copy link
Contributor Author

I think I've found the bug, trying to fix it.

@adamperkowski
Copy link
Owner

Added a job that checks if a DOS boot sector is present. It's not a full "boot check" but it'll do.

- name: Check DOS/MBR boot sector
id: bootsector
run: echo "::set-output name=check::$(file ./asm/boot.bin)"
- name: Validate the check
run: 'echo "${{ steps.bootsector.outputs.check }}" | grep -q "./asm/boot.bin: DOS/MBR boot sector"'

@adamperkowski
Copy link
Owner

I'll update your branch.

adamperkowski and others added 2 commits July 26, 2024 16:42
- Fix a bug: BIN file was not booting (caused by an accidentally deleted line💀);
- Remove code sections (got too much used to program in Protected Mode assembly...).

Signed-off-by: franzageek <[email protected]>
@franzageek
Copy link
Contributor Author

franzageek commented Jul 26, 2024

There you go, fixed. I forgot that 16 bit Real Mode assembly does not support splitting code in text/data sections. I'm used to program in 32/64 bit Protected mode. Very sorry, should be fixed now.

While debugging, I found another bug - after pressing enter the program flow remained trapped in an infinite loop, because I replaced a jump with a ret.

Fixed that as well.

Try it out for yourself and let me know if it sticks!

@adamperkowski
Copy link
Owner

adamperkowski commented Jul 26, 2024

Seems like you forgot about this bc369ce .
Added.

input wasn't looping because of that

@adamperkowski adamperkowski self-requested a review July 26, 2024 14:57
@franzageek
Copy link
Contributor Author

franzageek commented Jul 26, 2024

Actually no - the reason was the jmp shell line on line 116/117.
EDIT: yeah, forgot abt both. I'm sorry man, bit tired these days.

@adamperkowski
Copy link
Owner

No problem at all. Happens. So yeah thanks for taking your time and contributing again. Much appreciated. I'll merge this if you don't want to add anything.

@franzageek
Copy link
Contributor Author

Sure! Really like the vision & the direction of this project.
Let me review the code - will add anything if necessary.

@adamperkowski
Copy link
Owner

Message me privately please through Discord, Signal, e-mail or whatever you like. Merging.

@adamperkowski adamperkowski merged commit b9488d2 into adamperkowski:main Jul 26, 2024
1 check passed
@franzageek
Copy link
Contributor Author

Alright - just sent you a DM request on Discord.

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.

2 participants