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

WIP: Port "Gameboy sound hardware" from the old wiki #562

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

remind-me-later
Copy link

Closes #552. Each section from the old wiki corresponds to a commit.
I encountered the following issues while porting some sections:

  • The Channels section diagram is pretty ugly, I would like recommendations on how to redo it.
  • The Frame Sequencer section diagram has been skipped, since it
    doesn't match with SameBoy source, and I think that's more accurate.
  • The VIN mixing section is pretty bare and I don't know if the information in there
    is useful, so I skipped it.
  • The obscure behavior section could probably be refactored to be in Audio Registers,
    and Audio Details, but I don't know if that would make the rest of the section more bothersome
    to read.
  • I don't think a lot of version differences are mentioned in the rest of the page,
    so I skipped them as well.

@avivace avivace requested review from ISSOtm and avivace August 28, 2024 16:29
Copy link
Member

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

Thank you!

This looks largely like a straight port, correct? If so, I anticipate that we'll want to check some of the statements. Will you want to help with that?

src/Audio.md Outdated Show resolved Hide resolved
src/Audio_Registers.md Outdated Show resolved Hide resolved
Comment on lines 316 to 311
:::warning RETRIGGERING CAUTION

On monochrome consoles only, retriggering CH3 while it's about to read a byte from wave RAM causes wave RAM to be corrupted in a generally unpredictable manner.
On monochrome consoles only, retriggering CH3 while it's about to read a byte from wave RAM causes wave RAM to be corrupted in a generally unpredictable manner.

:::
:::

:::warning PLAYBACK DELAY
:::warning PLAYBACK DELAY

Triggering the wave channel does not immediately start playing wave RAM; instead, the *last* sample ever read (which is reset to 0 when the APU is off) is output until the channel next reads a sample.
([Source](https://github.com/LIJI32/SameSuite/blob/master/apu/channel_3/channel_3_delay.asm))
Triggering the wave channel does not immediately start playing wave RAM; instead, the *last* sample ever read (which is reset to 0 when the APU is off) is output until the channel next reads a sample.
([Source](https://github.com/LIJI32/SameSuite/blob/master/apu/channel_3/channel_3_delay.asm))

:::

:::
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't unindenting those break the list in two?

Copy link
Author

Choose a reason for hiding this comment

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

With the last commit the warnings are nested 2 levels deep in the sublist.

src/Audio_Registers.md Outdated Show resolved Hide resolved
@avivace
Copy link
Member

avivace commented Sep 1, 2024

@ISSOtm would you say we can start merging and afterwards "check" the statements ?

For "critical ones" we could directly add notes/earnings asking for help to verify the statements

@ISSOtm
Copy link
Member

ISSOtm commented Sep 1, 2024

I don't know. The risk of adding false statements to Pan Docs always exists, and is a regression of sorts.

I'd like to fix them from the get-go if the resources (primarily motivation) are available, but if not, then yes we'll merge this and improve later.

@remind-me-later
Copy link
Author

Thank you!

This looks largely like a straight port, correct? If so, I anticipate that we'll want to check some of the statements. Will you want to help with that?

Sure, for now I've been using SameBoy source as a reference but I don't know if there's a better source for checking the statements of the old wiki.

@ISSOtm
Copy link
Member

ISSOtm commented Sep 2, 2024

SameBoy's source is one of the best references, since it's accurate, well-tested, and often well commented; but since it's code, it's still subject to interpretation. That's why we prefer citing test ROMs (when possible).

But “best effort“ remains in effect, so only verifying against SameBoy's source is perfectly fine, and hey, still miles better than just blindly copy-pasting some text! ^^

@remind-me-later
Copy link
Author

Ok, I'll see if I can find some test ROMs, specially for the "obscure behavior" claims.

Copy link
Member

@avivace avivace left a comment

Choose a reason for hiding this comment

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

Just started to leave some possible minor improvements after a read

@@ -202,7 +204,15 @@ Period changes (written to `NR13` or `NR14`) only take effect after the current
"NR14" 7:"Trigger" 6:"Length enable" 2-0:"Period"
}}

- **Trigger** (*Write-only*): Writing any value to `NR14` with this bit set [triggers](<#Triggering>) the channel.
- **Trigger** (*Write-only*): Writing any value to `NR14` with this bit set [triggers](<#Triggering>) the channel, causing the
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can improve this wording a bit? "triggering a channel"

src/Audio_Registers.md Outdated Show resolved Hide resolved
src/Audio_Registers.md Outdated Show resolved Hide resolved
src/Audio_details.md Outdated Show resolved Hide resolved
optionally negating the value, depending on the [direction](<#FF10 — NR10: Channel 1 sweep>), and summing this with the frequency "shadow register" to produce a new frequency.
What is done with this new frequency depends on the context.

The _overflow check_ simply calculates the new frequency and if it is greater than 2047, or $7FF, CH1 is disabled.
Copy link
Member

Choose a reason for hiding this comment

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

minor: this section may enjoy a flow diagram?

@ISSOtm
Copy link
Member

ISSOtm commented Sep 18, 2024

Have you been able to find any test ROMs?

If not, that's fine, then SameBoy would be quite sufficient.

@avivace
Copy link
Member

avivace commented Sep 20, 2024

@remind-me-later do you have any update on this? It's fine to just mention SameBoy for the time being, we can open an issue for the test roms

@remind-me-later
Copy link
Author

Have you been able to find any test ROMs?

If not, that's fine, then SameBoy would be quite sufficient.

I've seen that the blargg test roms check for some of the behavior of the sweep (cgb-sound tests 04-sweep and 05-sweep details) which seem to coincide with the behavior described. The test 03-trigger also matches the added text about triggering a channel. The parts I'm less sure of are some claims in the obscure behavior section, for example the zombie step paragraph, I've tried reading SameBoy's source about this but couldn't quite get how this mode works.

Copy link
Member

@avivace avivace left a comment

Choose a reason for hiding this comment

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

I'd like to start merging what we have, if that's okay @ISSOtm ?

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.

Port "Gameboy sound hardware" from the old wiki
3 participants