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

Replace gbhw.asm with hardware.inc #479

Merged
merged 2 commits into from
Nov 16, 2022
Merged

Replace gbhw.asm with hardware.inc #479

merged 2 commits into from
Nov 16, 2022

Conversation

tobiasvl
Copy link
Collaborator

@tobiasvl tobiasvl commented Nov 7, 2022

hardware.inc is the modern de facto standard include file for hardware constants with RGBDS. It's actively maintained, and used by some other disassembly projects (and mgbdis). This PR pulls it in as a git submodule and replaces gbhw.asm. Not sure if we should, but I wanted to see if we could.

However, currently it doesn't build because hardware.inc defines joypad button bits in a different nybble order than gbhw.asm and I'm not sure why.

gbhw.asm:

; Joypad buttons mask
J_RIGHT  EQU 1 << 0     ;  1
J_LEFT   EQU 1 << 1     ;  2
J_UP     EQU 1 << 2     ;  4
J_DOWN   EQU 1 << 3     ;  8
J_A      EQU 1 << 4     ; 10
J_B      EQU 1 << 5     ; 20
J_SELECT EQU 1 << 6     ; 40
J_START  EQU 1 << 7     ; 80

; Joypad button bits
J_BIT_RIGHT  EQU 0
J_BIT_LEFT   EQU 1
J_BIT_UP     EQU 2
J_BIT_DOWN   EQU 3
J_BIT_A      EQU 4
J_BIT_B      EQU 5
J_BIT_SELECT EQU 6
J_BIT_START  EQU 7

hardware.inc:

DEF PADF_DOWN   EQU $80
DEF PADF_UP     EQU $40
DEF PADF_LEFT   EQU $20
DEF PADF_RIGHT  EQU $10
DEF PADF_START  EQU $08
DEF PADF_SELECT EQU $04
DEF PADF_B      EQU $02
DEF PADF_A      EQU $01

DEF PADB_DOWN   EQU $7
DEF PADB_UP     EQU $6
DEF PADB_LEFT   EQU $5
DEF PADB_RIGHT  EQU $4
DEF PADB_START  EQU $3
DEF PADB_SELECT EQU $2
DEF PADB_B      EQU $1
DEF PADB_A      EQU $0

@tobiasvl
Copy link
Collaborator Author

tobiasvl commented Nov 14, 2022

The reason for this is that the Game Boy represents the D-pad as the exact same (lower-nybble) values as the other buttons, but it depends which button matrix you read from. Most games combine these nybbles into one byte when reading from the joypad, and LADX (and gbhw.asm) chose a different nybble order from hardware.inc.

See gbdev/hardware.inc#37.

@kemenaran
Copy link
Collaborator

Hmm, interesting mismatch.

I suppose having the issue addressed upstream in hardware.inc would be nice, but meanwhile we can always derive our own J_ constants in some constants file (like input.asm).

@tobiasvl tobiasvl force-pushed the migrate-to-hardware.inc branch 2 times, most recently from 0e03d7e to 0de7920 Compare November 15, 2022 14:38
@@ -774,7 +774,7 @@ UpdateEntityTimers::
; When the flash countdown is active, invert the palette every 4 frames
sla a ; $4DE4: $CB $27
sla a ; $4DE6: $CB $27
and OAM_DMG_PAL_1 ; $4DE8: $E6 $10
and OAMF_PAL1 ; $4DE8: $E6 $10
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I still need to re-align a million billion comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

tools/align_comments.py might help?

@kemenaran
Copy link
Collaborator

(For what is worth, I think setting up submodules makes setting up the disassembly on a local machine significantly harder: it adds an extra step to the README, without this step nothing compiles…

Could simply copying the latest hardware.inc (with a version number) be a good compromise between standardization and simplicity of setting things up?)

@tobiasvl
Copy link
Collaborator Author

tobiasvl commented Nov 15, 2022

Yes, sure, that's a good idea (and also simplifies the include "hardware.inc/hardware.inc" line, hehe). I had the same thought but decided to do it since there already was a submodule in the repo (mgbdis), but obviously that one isn't needed to build.

@kemenaran
Copy link
Collaborator

Yup, I should definitely remove the existing submodule, which is no longer needed :)

@tobiasvl tobiasvl force-pushed the migrate-to-hardware.inc branch 2 times, most recently from d961320 to 88a5922 Compare November 16, 2022 13:58
@tobiasvl tobiasvl marked this pull request as ready for review November 16, 2022 13:59
Makefile Outdated Show resolved Hide resolved
src/constants/gfx.asm Outdated Show resolved Hide resolved
src/constants/mbc3.asm Outdated Show resolved Hide resolved
Copy link
Collaborator

@kemenaran kemenaran left a comment

Choose a reason for hiding this comment

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

Nitpick: I'd suggest maybe squashing all these commits together in a single one – but in either case, this is good to me.

@kemenaran kemenaran changed the title WIP: Replace gbhw.asm with hardware.inc Replace gbhw.asm with hardware.inc Nov 16, 2022
@tobiasvl
Copy link
Collaborator Author

tobiasvl commented Nov 16, 2022

Yeah, I approved your suggestions on the phone, but I think I would've been able to batch them in a browser. I'll squash them with the first commit when I get home.

As for the alignment commit, I made that a separate one because the script (which I didn't know about until now, haha) uncovered some unrelated misalignments, and I thought maybe this would look less messy in git blame.

@tobiasvl tobiasvl merged commit afb9a11 into main Nov 16, 2022
@tobiasvl tobiasvl deleted the migrate-to-hardware.inc branch November 16, 2022 21:31
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