Skip to content

Commit

Permalink
Sound driver: fixing labels and other things
Browse files Browse the repository at this point in the history 
Working towards [sonicretro#32](sonicretro#32); thankfully, it's close enough to Sonic 1's code that I can take names from the S1 disassembly. Some comments have been changed accordingly.

I also fixed a few typos and inconsistent tab character uses, as well as converted all uses of "branch" to "jump" to better reflect Z80 assembly nomenclature.
  • Loading branch information
@Brainulator9
Brainulator9 authored Aug 9, 2021
1 parent c50f92d commit 055363d
Showing 1 changed file with 265 additions and 233 deletions.
Loading

2 comments on commit 055363d

@Awuwunya
Copy link

@Awuwunya Awuwunya commented on 055363d Aug 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Had a quick look at the commit, and a few things came to mind while looking

1: imo, each label should have at least 1 empty line between it and code above (just like how the 68k code does)
2: You reference a label named .bgmfmloadloop, which is named differently as the label itself
3: you named some standard library functions, like zDACStopTrack to be local labels instead. This may be a bad idea for people who want to modify the driver.
4: did you ensure bit perfect builds succeed?

Otherwise this looks good as far as I can tell

edit: I thought this was a pr on the main repo lol, ignore in that case

@Brainulator9
Copy link
Owner Author

Choose a reason for hiding this comment

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

@NatsumiFox I do intend for this whole thing to eventually be a pull request; I just wanted to take care of the smaller of the two ASM files first as a proof of concept.

  1. I agree; I was too concerned with taking a conservative approach to alterations lest I go all crazy. I'll fix this next time.
  2. Oops. This is what happens when you change your labels mid-commit.
  3. Well, this is a problem. I probably should undo those next time I work on these labels... part of the problem was that the original setup relied on absolute labels not descoping nameless temporary labels the same way local labels do (unless you want to use the absolute.local syntax, which IMO is too clunky and unlike the source code to bother with for a public disassembly).
  4. Admittedly, I made this edit with the idea that I would work on bits at a time. That said, there should hopefully be very little, if any, issues as far as this is concerned; I made sure to cross-check everything (besides that one label) before I saved this.

Thanks for the feedback!

Please sign in to comment.