Skip to content

Conversation

@bbbbbr
Copy link
Contributor

@bbbbbr bbbbbr commented Apr 30, 2021

Also helps to slightly disambiguate the STAT Vblank Interrupt from the regular Vblank Interrupt

…sociated with

Also helps to slightly disambiguate the STAT Vblank Interrupt from the regular Vblank Interrupt
@avivace avivace requested review from ISSOtm and avivace April 30, 2021 06:48
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.

I fully agree in principle, but I think a different wording should be used.

As I read this, I understand "Oh, there are several types of STAT interrupts", but that's not really what it is. Thus, I suggest talking about "interrupt source"s; don't we have a paragraph explaining those, esp. in relation to e.g. STAT int blocking?

@bbbbbr
Copy link
Contributor Author

bbbbbr commented Apr 30, 2021

"interrupt source" seems like a good clarification. Would this be ok, or does the description become too long?

Bit 6 - LYC=LY STAT Interrupt source         (1=Enable) (Read/Write)
Bit 5 - Mode 2 OAM STAT Interrupt source     (1=Enable) (Read/Write)
Bit 4 - Mode 1 VBlank STAT Interrupt source  (1=Enable) (Read/Write)
Bit 3 - Mode 0 HBlank STAT Interrupt source  (1=Enable) (Read/Write)

The STAT Interrupt section alludes to there being multiple sources, but doesn't enumerate them. The phrasing there is also somewhat vague- It might be better to use separate terminology for "interrupt sources" vs "uses", whereas currently "reasons" seems like it's being used for both meanings.

For example:
There are various sources which can trigger this interrupt as described by the STAT register ($FF41). One very popular use is to indicate to the user when the video hardware is about to redraw a given LCD line. This can be useful for dynamically controlling the SCX/SCY registers ($FF43/$FF42) to perform special video effects.

BTW, since "STAT interrupt blocking" seems to be a commonly(?) used term, it might also be useful to mention it explicitly in the Warning section which describes the behavior. (if my understanding of what you are referring to is right)

@ISSOtm
Copy link
Member

ISSOtm commented Apr 30, 2021

"interrupt source" seems like a good clarification. Would this be ok, or does the description become too long?

Bit 6 - LYC=LY STAT Interrupt source         (1=Enable) (Read/Write)
Bit 5 - Mode 2 OAM STAT Interrupt source     (1=Enable) (Read/Write)
Bit 4 - Mode 1 VBlank STAT Interrupt source  (1=Enable) (Read/Write)
Bit 3 - Mode 0 HBlank STAT Interrupt source  (1=Enable) (Read/Write)

LGTM.

The STAT Interrupt section alludes to there being multiple sources, but doesn't enumerate them. The phrasing there is also somewhat vague- It might be better to use separate terminology for "interrupt sources" vs "uses", whereas currently "reasons" seems like it's being used for both meanings.

For example:
There are various sources which can trigger this interrupt as described by the STAT register ($FF41). One very popular use is to indicate to the user when the video hardware is about to redraw a given LCD line. This can be useful for dynamically controlling the SCX/SCY registers ($FF43/$FF42) to perform special video effects.

Again, LGTM. I would make "the STAT register ($FF41)" be a link to the section, and would also link to DeadCScroll for the last sentence, as it is designed to be a clear example of this kind of effect.

BTW, since "STAT interrupt blocking" seems to be a commonly(?) used term, it might also be useful to mention it explicitly in the Warning section which describes the behavior. (if my understanding of what you are referring to is right)

Right.

-As mentioned in the description of the STAT register, the PPU cycles through the different modes in a fixed order. If we set the STAT bits in a way that they would interrupt the CPU at two consecutive modes, then the second interrupt will not trigger. So for example, if we enable the interrupts for Mode 0 and Mode 1, the Mode 1 interrupt will not trigger.
+As mentioned in the description of the STAT register, the PPU cycles through the different modes in a fixed order. If we set the STAT bits in a way that they would interrupt the CPU at two consecutive modes, then the second interrupt will not trigger. So for example, if we enable the interrupts for Mode 0 and Mode 1, the Mode 1 interrupt will not trigger. This phenomenon is known as "STAT blocking".

@bbbbbr
Copy link
Contributor Author

bbbbbr commented Apr 30, 2021

I think that's all updated now. The link rendering on gbdev.io appears to be different than markdown link rendering on github, so I think I've got the STAT register link working I'm not 100% sure. (I haven't looked into whatever is used to build/render the web output)

@bbbbbr bbbbbr requested a review from ISSOtm April 30, 2021 22:43
@ISSOtm
Copy link
Member

ISSOtm commented May 1, 2021

I think that's all updated now. The link rendering on gbdev.io appears to be different than markdown link rendering on github, so I think I've got the STAT register link working I'm not 100% sure. (I haven't looked into whatever is used to build/render the web output)

Our CI script checks for that. It normally runs automatically on each push, but since this is your first time contributing to this repo, a security feature requires maintainers to manually start it.

@avivace
Copy link
Member

avivace commented May 1, 2021

I think that's all updated now. The link rendering on gbdev.io appears to be different than markdown link rendering on github, so I think I've got the STAT register link working I'm not 100% sure. (I haven't looked into whatever is used to build/render the web output)

Our CI script checks for that. It normally runs automatically on each push, but since this is your first time contributing to this repo, a security feature requires maintainers to manually start it.

Correct, CI is running on pushes to this branch, so @bbbbbr you can check the results on the "Checks" tab on this PR.

@bbbbbr bbbbbr requested a review from ISSOtm May 2, 2021 17:46
@ISSOtm ISSOtm requested a review from avivace May 2, 2021 21:56
@avivace avivace changed the title Add STAT phrasing to clarify & emphasize the interrupt they are associated with Add STAT phrasing to clarify and emphasize the interrupt they are associated with May 4, 2021
@avivace avivace merged commit d244a38 into gbdev:develop May 4, 2021
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.

3 participants