Skip to content

Overhaul audio section #350

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

Merged
merged 2 commits into from
Nov 2, 2022
Merged

Overhaul audio section #350

merged 2 commits into from
Nov 2, 2022

Conversation

ISSOtm
Copy link
Member

@ISSOtm ISSOtm commented Sep 1, 2021

Waaay not finished, only posting for discussion and early review. (Anything changed and not marked with a TODO is intended to be final.)

I won't be requesting any reviews until it's finished, but please feel free to chime in so as to make the process piecemeal as possible :)

TODO:

  • Rename channel "frequency" to "wavelength" or "period", since it's an unit of time and not a frequency.
  • Merge CH2 descriptions into CH1? All CH2 stuff would either be duplicates or "c.f.", so why not merge them? (How, though?)

Fixes #347 and fixes #119.

An issue that @LIJI32 raised is the time units. Currently, I only use "APU ticks", being 2 MiHz (the smallest common denominator), and provide measurements in multiples of those. Liji instead suggested introducing separate units whenever necessary (e.g. "1 128 Hz tick" instead of "16 [APU] ticks"). The pro is that the units make more sense (no multiples of powers of 2), the con is potentially more mental complexity.

I'm not attached to either, so I used the solution I originally came up with in this PR, please feel free to discuss and provide feedback in the comments.

Other names for the time unit were suggested: "sample" (given that 2 MiHz is the final sample rate), "boop", maybe others I missed?

@ISSOtm ISSOtm added content Improvements or additions to documentation work in progress labels Sep 1, 2021
@ISSOtm ISSOtm force-pushed the audio branch 2 times, most recently from 71ee168 to 5b39681 Compare September 4, 2021 17:50
@allkern
Copy link
Member

allkern commented Sep 14, 2021

I think the "Common concepts" section should precede everything else, otherwise, really good job!

@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 14, 2021

Thanks!

It's not possible to talk about any of these concepts without introducing the architecture (e.g. channels). That said, maybe moving the "APU" section before the architecture would indeed be better? I also want to introduce the diagram early, so that it can serve as a visual aid to go back to for the rest of the page.

@aaaaaa123456789
Copy link
Member

Minor trivial feedback: use either "wave length" or "wavelength" consistently; you're currently using both.

@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 25, 2021

As per #350 (comment), it should be "wavelength". That should be added to the document style, and it'll have to be caught during review due to size of the change.

@avivace
Copy link
Member

avivace commented May 2, 2022

Hi @ISSOtm did you have any chance to come back on this? I will be fixing the conflict/rebasing, splitting it and merging what we have if no activity is expected in the near future as I don't want this to become blocking or get lost in time

@ISSOtm
Copy link
Member Author

ISSOtm commented May 2, 2022

I am not planning to work on this in the foreseeable future due to being busy on many other things (as you know), but I can provide guidance to anyone else willing to pick this up. I don't think it's production-ready in its current state due to how incomplete it is—it doesn't have all the info the page it's meant to replace has.

Most of the info is available though not necessarily readily, if only from the amount of people writing GB emulators by the day.

@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 20, 2022

Let's not let it get lost to time. :)

@ISSOtm ISSOtm force-pushed the audio branch 2 times, most recently from df312e9 to d163cf8 Compare September 21, 2022 18:14
@ISSOtm ISSOtm requested a review from nitro2k01 September 21, 2022 18:15
@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 21, 2022

I fixed issues and feedback on the "Audio" and "Sound registers" pages, I'll work on the "Audio details" now, but feedback on the first two would be appreciated. Thanks!

@ISSOtm ISSOtm force-pushed the audio branch 4 times, most recently from 3468fde to 6180889 Compare September 22, 2022 07:03
Co-authored-by: Antonio Vivace <avivace4@gmail.com>
@ISSOtm
Copy link
Member Author

ISSOtm commented Sep 26, 2022

Pushed changes we agreed upon, and rendered the draft at http://eldred.fr/pandocs/Audio.html.

@ISSOtm
Copy link
Member Author

ISSOtm commented Oct 26, 2022

Bump. I'd really like to merge this soon, please. It's going to become a blocker for other projects of mine soon.

@avivace
Copy link
Member

avivace commented Oct 26, 2022

Bump. I'd really like to merge this soon, please. It's going to become a blocker for other projects of mine soon.

I can see some threads here still waiting for your follow up.. can you take a look?

@ISSOtm
Copy link
Member Author

ISSOtm commented Oct 26, 2022

I don't see any of the open threads currently waiting on me, can you give me some links?

@avivace
Copy link
Member

avivace commented Oct 27, 2022

#350 (comment)

@ISSOtm
Copy link
Member Author

ISSOtm commented Oct 27, 2022

This one has been changed:

Except for pulse channels, whose phase position is only ever reset by turning the APU off.

...and I'm waiting for approval or further modification requests on the updated text.

@avivace
Copy link
Member

avivace commented Nov 2, 2022

A number of very interesting threads were opened by @nitro2k01, for some of which I feel they should've been blocking, but this PR has been sitting here for more than a year and I really don't feel like it's progressing in any sensible way since months.

Exceptionally, I "moved" those conversations into issues #444 #445 #446 (and in a minor fashion #194) and I'm going to merge what we have now in this chapter, which is really a block of awesome work and information previously scattered unorganically around.

Thanks @ISSOtm and @nitro2k01 ! Let's try to keep the conversation going in those issues and open more PRs to iterate upon this content.

@avivace avivace merged commit 1a6ca71 into gbdev:master Nov 2, 2022
@ISSOtm ISSOtm deleted the audio branch November 2, 2022 20:36
ISSOtm added a commit that referenced this pull request Nov 2, 2022
I had forgotten to force-push, oops :(
@ISSOtm ISSOtm mentioned this pull request Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document APU DAC behavior CH2 register descriptions are copy-pasted from CH1
6 participants