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

The big cycle unit rework #534

Merged
merged 8 commits into from
Feb 15, 2024
Merged

The big cycle unit rework #534

merged 8 commits into from
Feb 15, 2024

Conversation

SonoSooS
Copy link
Contributor

I've read all pages, searching for instances where clock units in particular were either confusing, ambiguous, misleading, or simply just used wrong. This PR aids to fix all of these.

Notes:

  • Four Player Adapter: even though serial runs locked to M-cycle boundary, I did not edit the bits-per-second formula, as I can't reword it due to not being able to test it myself to verify why the formula is that. Also 6 % 4 != 0, and that is definitely wrong.
  • IR: Few hundred what cycles? With no other units of reference nearby, and not having a real CGB to test it myself, I can't fix this alone.
  • OAM DMA Transfer: I edited all instances of cycle to M-cycle, but it looks bad tbh
  • Power Up Sequence: what one cycle faster for SGB?
  • Rendering: my wording may be a bit awkward (standardize the rest of this page too? dots vs. dot cycles)
  • Timer and Divider Registers: I had to alter the table, I'm not happy how I had to do it, but considering how ridiculously ambiguous the "CPU Clock" phrase is, I'd rather write it this way. Having to clarify around the table instead of in the table I think fails approachability. As for the DIV register part, personally I think it's out of the scope of this PR to rewrite it completely, so I've put temporarily warnings there during the transitional period. Annoyingly, there is an annoying line break in the table I don't know how to fix. Must be fixed definitely for sure, I don't think it should stay like that... Oh also, I've updated the rest of the page to the current typographic standards while at it.

TODO (perhaps for other PRs if these are outside of the scope of this PR):

  • Timer Obscure Behavior: DIV ticks at M-cycle intervals, those lower two bits don't exist. A fix to the diagrams is needed.
  • Timer and Divider Registers: [$FF04] is only the visible region of DIV, real DIV increases every M-cycle. Too much detail? Not enough detail? At least make it so that the visible portion is increased every 64 M-cycle, instead of having to clarify paragraphs later that it runs twice as fast in Double Speed mode.

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.

Thanks a lot, Sono! I appreciate that you took the time to survey the whole doc, and corrected some inconsistencies.

Arguably, what's left is explaining those two terms somewhere, but that would be part of a "overview"/"introduction" PR, so it's fine not to deal with it here.

src/CGB_Registers.md Outdated Show resolved Hide resolved
src/Rendering.md Outdated Show resolved Hide resolved
src/Timer_Obscure_Behaviour.md Outdated Show resolved Hide resolved
src/Timer_and_Divider_Registers.md Outdated Show resolved Hide resolved
src/Timer_and_Divider_Registers.md Outdated Show resolved Hide resolved
src/Timer_and_Divider_Registers.md Outdated Show resolved Hide resolved
src/Timer_and_Divider_Registers.md Outdated Show resolved Hide resolved
src/Timer_Obscure_Behaviour.md Outdated Show resolved Hide resolved
@avivace
Copy link
Sponsor Member

avivace commented Jan 17, 2024

What's left is explaining those two terms somewhere, but that would be part of a "overview"/"introduction" PR, so it's fine not to deal with it here.

This should go in the "Glossary" (see: #194)

@avivace avivace mentioned this pull request Jan 17, 2024
Copy link
Sponsor 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.

Are those handwritten?
As a general practice, to have some maintainability, those graphs should be drawn on something like draw.io and both the "project" file (more easily editable) and the svg should be uploaded and kept in the repository.

Also, there are lot of useless arrow "diversion" caused by elements not aligned, such as:
image

image

@ISSOtm
Copy link
Member

ISSOtm commented Jan 17, 2024

Those are, indeed, hand-written. No editing tool I'm aware of supports "themed" SVGs. I understand the theoretical maintenance loss, but hear me out:

  • Embedded text is still easy to modify (including to localize),
  • SVG is a well-known format,
  • Worst case the "next person" can open such a SVG in Inkscape or other editing program, and lose the theming from the conversion but keep the information otherwise intact.1
  • Finally, those diagrams rarely get edited; and when they do, they can be redone from scratch: even hand-crafting those took me only a day each, tops, so someone using a GUI should have far less trouble. (And I've been thinking of ways to simplify them further, which would make them more maintainable and faster to create.)

All of these are why I personally consider the tradeoff acceptable when weighed against the gains, chiefly in accessibility and hackability.

Footnotes

  1. Existing SVGs did not have that until 5eaf6f8 because I did not think of providing "fallback" values to the var() directives; a few diagrams in outstanding PRs do not yet have this, though; I'll have to get to that.

@ISSOtm
Copy link
Member

ISSOtm commented Jan 17, 2024

Also, there are lot of useless arrow "diversion" caused by elements not aligned, such as:

I paid a lot of attention trying to avoid bending the arrows, which required a lot of tweaking overall. Both of these are actually intentional, so I will lay out my rationale for you to judge.

image

This is actually so that the "Timer tick" bubble lies at the same place on both the DMG and CGB diagrams. This mattered more in a tentative design I made where the two diagrams were shown on separate (responsive) columns, side by side. Now that this design has been dropped (the diagrams are too horizontal, they were terribly squished), I guess the bubble can be moved down a little?

image

This was done because the two elements the arrows originate from are too close, necessitating at least one bend. Then, I felt that having only a single bend looked awkward, and made the two actions look asymmetrical, and thus having different priority. (Am I reading too much into it?)

I considered another workaround in making one of the arrows arrive to the register from the top or bottom instead of the side; but looking at the diagrams, vertical arrows designate single bits of registers, whereas horizontal ones designate the register as a whole... which made such a workaround impractical.

Another design I considered was pointing either of the two arrows (probably the "Reset" one) at the register title itself; only problem is, DIV specifically resists such a design, since the low bits are really unnamed1, so there's nothing to point to.
That said, now that I thought more about it, it seems possible to perhaps convey that name within the diagram itself (making the right slanted edge of DIV dotted, and otherwise extending the grouping to all 14 bits and inserting the name). What do you think?

Footnotes

  1. The "main text" calls them and DIV "System counter", though I think "M-cycle counter" is more appropriate due to what it's counting; cc @SonoSooS for opinion. Scratch that, it would make too many sentences awkward. "System counter" is fitting if arbitrary, but what name isn't that?

@ISSOtm
Copy link
Member

ISSOtm commented Jan 17, 2024

And to answer a question that was asked on IM: yes, I'd say this PR should be considered content-complete; there is one last diagram not converted on the "Timer Obscure Behavior" page, but during the conversion process I realised that it is plain wrong, and fell into a rabbit hole of researching and correcting behaviour.

Which is to say, I'd rather keep those corrections to a separate PR, working on it after this one is deemed acceptable. And I'd rather not expend the effort to convert a diagram before modifying it into something correct, even if it means temporarily having a style discrepancy and keeping some wrong info. (We already have many differing styles throughout the document anyway; big whoop.)

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.

Alright, as far as my opinion goes, there's only the single TODO left to take care of. I expect someone other than me will want to review the few changes I have introduced, though.

src/Timer_Obscure_Behaviour.md Outdated Show resolved Hide resolved
src/Rendering.md Outdated Show resolved Hide resolved
src/Timer_Obscure_Behaviour.md Outdated Show resolved Hide resolved
src/Timer_Obscure_Behaviour.md Outdated Show resolved Hide resolved
src/Timer_Obscure_Behaviour.md Outdated Show resolved Hide resolved
SonoSooS and others added 8 commits February 15, 2024 11:47
The M-cycle crusade!
Fixes gbdev#492 by moving the DIV-APU bits from 5 and 6 to 4 and 5.
Also applies the "TODO: correct the diagram" note.

Also tries to reorganise the diagram and reword some bits to be
more consistent with the rest of the document, and improve clarity.
Also restore the CGB version of that diagram
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.

Alright, now this PR looks good to me!

Considering the last month of inactivity, and that the last change had some agreement from @avivace, I'll be merging this PR. Follow-ups can happen in separate ones.

@ISSOtm ISSOtm dismissed avivace’s stale review February 15, 2024 13:10

SVGs have been accepted (on IM), if reluctantly, as a good enough compromise for now

@ISSOtm ISSOtm merged commit 7a8bc4a into gbdev:master Feb 15, 2024
2 checks passed
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.

None yet

3 participants