New TIA code comments missing

[thrust26]

While trying to get a grip on the new TIA code, I am really missing code comments.

I know this is tedious work, but IMO classes, members and methods should be commented. Plus additional code comments where necessary. Else code maintenance and collaboration will become really difficulty.

BTW: IMO the other code could use some more comments too. 😉

[sa666666]

@DirtyHairy mentioned coming back to this and commenting, etc. I would prefer to follow his lead on this, since he wrote most of the code.

Also, what 'other' code is it you speak of?

[thrust26]

I only looked into emucore and a bit into some other directories. They are definitely much better commented than the tia sub directory.

And meanwhile I realized that the class comments are all in the header files. So you can ignore my BTW from above.

[sa666666]

Commenting in the header files is the way I learned it in school, way back many moons ago. There are several justifications for this:

• with code that is to be released commercially, you normally only get the header files as source; the actual source code is a black box
• related to the first one, typically you comment and describe the ADT, not the implementation
• tools to autogenerate documentation (javadoc, doxygen, etc) only process header files

So at least for the code I wrote, most of the commenting is in the header files. Of course, if there is something complicated in the cxx file, it is commented there too. But the overall 'big picture' commenting happens in the header files mostly.

[thrust26]

All valid arguments, looks like I have done too much Java in between.

[DirtyHairy]

Full ACK. I usually prefer clear code with sensible names for entities over verbose documentation. However, I wouldn't claim that the TIA implementation necessarily meets these criteria, and even less in its C++ incarnation :smirk: I'll try to improve this over time.

Other than that, there are mainly two reasons why documentation is currently so sparse:

• The TIA code has been in flux alot, and I hate rewriting documentation during refactoring
• The other Stella code has had more time to get documented :smile:

[thrust26]

Welcome back from mud!

Sure, take the time you need. I just tried to understand the code and (where it makes sense) to optimize some parts and there I struggled. Maybe you can ask some questions:

• why is myTimestamp a double?
• would it be save to move it outside the loop in TIA:cycle? (myTimestamp += colorClocks instead of myTimestamp++)
• what does DelayQueue do exactly?

Or you could just comment it. 😆

[DirtyHairy]

Or I could do my promised git writeup and you could add comments yourself where you find them appropiate :stuck_out_tongue: Just kidding, although this might actually improve the quality of any new comments a lot.

As for your questions:

• myTimestamp counts color clocks and would overflow after some 30 minutes if it were a 32 bit integer. It currently enters the floating point calculation that simulates the paddle readout circuit, that's why I chose to just make it a double.
• While I think that it technically could be moved from the loop at this point, I am not overly enthusiastic about doing so. The whole TIA simulation is build around a cycle-by-cycle simulation, and the loop you reference interfaces this to the Stella way of counting time. Moving the increment outside would complicate things and introduce an error vector if anything that touches myTimestamp is changed.
• DelayQueue is a scheduling queue for pokes that do not take effect instantly, but rather are deferred by one or more clocks. The scheduled pokes are executed during DelayQueue::execute after the specified number of clocks has passed --- that's what happens in delayedWrite. As an additional twist, there is a similar delay when the contents of the "old" and "new" GRPx registers are swapped; this is also handled by the queue with "writes" to registers that are unused on a real TIA (those are enumerated in the DummyRegisters enum).

[thrust26]

Thanks for the explanations.

What happens if myTimeStamp overflows (several years now 😃 )? I found it only used in paddle code, correct? And why double, why not a 64 bit integer instead?

[DirtyHairy]

Long before it actually overflows, the paddle will start getting jumpy as the relative accuracy will become smaller than one clock. Considering the 52 bits of fraction precision in double, this will happen in about 40.8 years (not considering leap years). Depending on the game, I would think there are some more three to four powers of two headroom, so there won't be any epic, 640 year (with hotswapped hardware) breakout highscores with Stella :smile:

As for why it is a double:

• There's hardware support for doubles in more current CPUs as there is support for 64 bit integers
• It will be cast to double anyway in the paddle code
• The code is ported from Javascript which only knows 32 (actually more like 31 in all current VMs) bit integers and doubles :smiling_imp:

[thrust26]

Too many valid reasons to disagree. 😄

[thrust26]

@DirtyHairy

Happy birthday to you, Happy birthday to you, Happy birthday dear issue 186, Happy birthday to you. 🎂

😈

[sa666666]

I didn't want to be pushy, so I pushed the deadline to 6.1. Of course, whatever is added from now to the 6.0 release would be appreciated too. (hint-hint)

[thrust26]

Missed the 2nd birthday by a few days. 😈

@DirtyHairy Where are we with this? What is done and what not?

[DirtyHairy]

About 50% done iirc. More to come anytime soon 😛