-
-
Notifications
You must be signed in to change notification settings - Fork 125
Update MBC3.md #637
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
base: master
Are you sure you want to change the base?
Update MBC3.md #637
Conversation
Results from CasualPokePlayer and me, as discussed in the Research channel on Discord on Tuesday. Also make RAM bank count and RAM size consistent and add some more details how it handles wirting invalid values.
ISSOtm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution!
src/MBC3.md
Outdated
| **Help wanted** | ||
|
|
||
| If you have a flashcart and any MBC3 or MBC30 cart (see the print on the chip), please reach out to us on gbdev Discord so you can be given the test ROMs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is help wanted for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all chip variants have been tested extensively for latching quirks. MBC3 (without A/B/0) and MBC30 have not been tested. It'd also be good to verify that there is indeed MBC3A vs MBC3B behavior (since the testing for that so far has been a total of 2 carts, one MBC3A and one MBC3B, not a very good sample size).
src/MBC3.md
Outdated
| this memory space is used to access an 8 KiB external RAM Bank, or a | ||
| single RTC Register. | ||
|
|
||
| The Japanese version of Pokémon Crystal Version is the only official game to have an MBC3 with 8 RAM banks (for a total of 64 KiB). It is sometimes referred to as MBC30, reflecting the print on the chip, although the cartridge type in the header is not different. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved under the initial description paragraph.
|
|
||
| The exact behavior of this register varies depending on hardware: | ||
|
|
||
| MBC3B provides a running clock on power-on and after writing any even value to this register. It is still recommended to latch the clock by writing any odd value. MBC3B can only latch while it provides a running clock, so you must write an even value before you can write an odd value again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the two versions of this chip are mentioned before this point? I'd suggest adding a paragraph explaining this in the intro section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are at least 4 versions of the chip to be clear: MBC3, MBC3A, MBC3B, and MBC30. Only MBC3A and MBC3B have been tested for latching quirks (so far).
src/MBC3.md
Outdated
|
|
||
| Bits that are not required to store the above information will be ignored and always read 0. | ||
|
|
||
| You can write values larger than the ones mentioned above (up to 63 for seconds and minutes, and up to 31 for hours). Invalid values will then continue incrementing like a valid value and will only overflow once the available bits no longer suffice. This overflow however will not cause a carry, neither does writing 60 or 24 directly. For example, if you write 30:59:63 (and clear the Halt Flag), it will be 30:59:00 one second later, and 31:00:00 one minute after that. This behavior has been confirmed on MBC3B. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is behavior tested in rtc3test (and my own MBC3A flashcart passes all tests here perfectly, as expected).
If we're mentioning more quirky behavior, we could mention the "subsecond counter", which is cleared upon writing to the seconds counter (although, it is also halted when the halt flag is set, so for practical usage this detail doesn't matter so much since you'd end up with a reset subsecond counter after writing to all the registers and unhalting)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this behavior is thought to be universal and the last sentence should be removed?
|
We also might want to add the following, which is true on MBC3B, but @CasualPokePlayer please confirm on MBC3A:
|
Co-authored-by: Eldred Habert <[email protected]>
Reflecting suggestions from the discussion
|
Before we merge, we should agree on a consistent capitalization scheme. It is entirely random right now. Looks like it has originally been written by a German. |
|
There's also some inconsistent line wrapping to fix before merging |
|
I thought it would be better to not bloat the diff with retroactive changes of the line wrapping. Unifying the capitalization will create a massive diff for a neclegible change anyway, so I guess we can address the line wrapping at the same time. Which type of line wrapping do you recommend? I would normally go for infinite line length, since it is the software's job to handle this (people on some forums don't understand that). |
quinnyo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your efforts!
I don't think it's quite right to change the supported ROM/RAM size in this way. It seems misleading to say plainly that MBC3 supports 4 MiB ROM / 64 KiB RAM, when only one special variant actually does. MBC30, insofar as it's part of the "MBC3 family", is an anomaly, not a representative.
To aid review and future changes, it's helpful to spread the text over multiple lines. The easiest/obvious place to do this is per-sentence, but it can make sense to breaking up long sentences in some cases. Note that the old text has hard-coded line wrapping, which is undesirable.
src/MBC3.md
Outdated
| RTC requires an external 32.768 kHz Quartz Oscillator, and an external | ||
| battery (if it should continue to tick when the Game Boy is turned off). | ||
| Beside for the ability to access up to 4 MiB ROM (256 banks) and 64 KiB RAM | ||
| (8 banks), the MBC3 also includes a built-in Real Time Clock (RTC), sometimes referred to as the timer. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place I've seen it referred to as the timer is in this file. Adding the alternative terminology makes it less clear what it is, which is an RTC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both Pan Docs and RGBLINK exclusively use "timer". Only this file uses RTC. This is because it isn't a real-time clock as it cannot tell you the date. It has the days past since a certain period in time. It's simply a persistent timer. Using the hours, minutes, seconds counter as a clock is simply a feature that could be added easily.
| until it becomes latched again, by repeating the write $00-\>$01 | ||
| procedure. This provides a way to read the RTC registers while the | ||
| clock keeps ticking. | ||
| Latching makes a static copy of the current timestamp available in the clock counter registers while the clock keeps running in the background. This makes sure that your reads from the counter registers will be consistent, since any counter overflowing while you read the different parts can have you read an incorrect value (e.g. reading the minute at 11:59 and the hour at 12:00 will give 12:59.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These sections should start with the basic technical information about the register itself.
- what the register does -- controls the RTC register latch
- the register interface / how to use it technically -- write value with bit 0 set/clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latching is a rather technical term. I felt that it was more appropriate to describe this first, instead of writing "The latch clock data register can latch clock data." as the first sentence.
If you read on, you will notice that it has no universal behavior, just that writing an even value followed by an odd value will happen to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not and would not suggest including "The latch clock data register can latch clock data." as the first (or any) sentence. You can see what I suggested should be the first thing in the section in my comment above.
I understood what you wrote and I did read the whole thing before choosing to submit the review including this comment.
I agree that an explanation of the purpose of the latching feature is necessary. But I don't think that should be the first thing under this heading.
(Like all the other comments, the relevant line here is the bottom one. The three above came for free.)
src/MBC3.md
Outdated
|
|
||
| MBC3B provides a running clock on power-on and after writing any even value to this register. It is still recommended to latch the clock by writing any odd value. MBC3B can only latch while it provides a running clock, so you must write an even value before you can write an odd value again. | ||
|
|
||
| MBC3A's clock counters are indeterminate by default. Writing any value to this register latches the clock. MBC3A cannot provide a running clock. Naturally, it can latch repeatedly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear what "indeterminate by default" means here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly do you not understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exact thing that I don't understand is the intended meaning of the phrase "indeterminate by default".
The text could be improved by plainly describing the behaviour that this phrase presumably refers to.
e.g. questions that could be answered:
- What is the default state? RTC selected / Clock battery inserted / System power cycle?
- What happens when you read from the registers?
- What about writes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"What is the default state? RTC selected / Clock battery inserted / System power cycle?" - Power-cycle, but I didn't want to repeat the same word again.
"What happens when you read from the registers?" The clock counter registers are indeterminate.
"What about writes?" As clearly explained a few paragraphs further down, latching is optional for writes. Since this register is only used for latching on MBC3A, this means that you can write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
Specifically for this instance, I'd prefer to have the repetition. Using a different term to refer to the same thing suggests a difference in meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we use expressions such as "These registers are left entirely uninitialized" instead of 'indeterminate'
src/MBC3.md
Outdated
|
|
||
| MBC3B provides a running clock on power-on and after writing any even value to this register. It is still recommended to latch the clock by writing any odd value. MBC3B can only latch while it provides a running clock, so you must write an even value before you can write an odd value again. | ||
|
|
||
| MBC3A's clock counters are indeterminate by default. Writing any value to this register latches the clock. MBC3A cannot provide a running clock. Naturally, it can latch repeatedly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably worth explaining the behaviour more specifically rather than relying on the phrase "latches the clock" which seems to use "latch" in an unusual way.
It sounds like a write to the register triggers an automatic sequence in which the latch is disabled and then enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your first remark: That behaviour was just described two paragraphs above.
For the second one: What exactly are you referring to? You selected two full paragraphs.
src/MBC3.md
Outdated
|
|
||
| MBC3A's clock counters are indeterminate by default. Writing any value to this register latches the clock. MBC3A cannot provide a running clock. Naturally, it can latch repeatedly. | ||
|
|
||
| **tl;dr:** Write $00 then $01 to this register to safely trigger latching on all versions of the chip. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of "tl;dr" does not match the style of the document.
Also, this is basic information and should be at the top of the section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @quinnyo here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that all of my comments in the review ended up pointing at more lines than what I anchored them to.
The line I was referring to here was only the "tl;dr" one at the bottom, in case that wasn't clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually didn't want to change it for the same reason, but I did it anyway because it was requested by one of the primary maintainers.
Generally avoided the term MBC3 when not speaking about a certain variant
- Made clearer that the "default state" of the MBC3A refers to the state when powered on - Undid change requested by @ISSOtm based on feedback from others and my own opinion - Changed "RTC registers" to "clock counter registers" (since 4000-5FFF is an RTC register, but is not what the term was referring to) - Unified capitalization: lowercase in text (those are no proper nouns!) and title case in titles - Unified line breaks (infinte length)
Results from CasualPokePlayer and me, as discussed in the Research channel on Discord on Tuesday. Also make RAM bank count and RAM size consistent and add some more details how it handles wirting invalid values.