Reenable debugger

[sa666666]

This is now partially done, but the TIA stuff is still broken. I will update it as I go, but the code can at least compile and run with the debugger enabled.

[sa666666]

@DirtyHairy, I'm having an issue with implementing scanline tracing in the debugger. The current code will advance one scanline each time the "Scan +1" button or Ctrl+l is pressed, but the issue is how it proceeds from one scanline to the next.

Currently, it tracks the current scanline, and stops advancing when it reaches the next line. But what's happening is that when @ scanline 0, advancing immediately goes to scanline 34 (or whatever the first rendered line will be). So it seems to skip all lines that aren't rendered. That may be fine for rendering the image, but in the debugger needs to actually go through every line when advancing by frame, step or scanline. The two former ones seem to be working, but the latter isn't.

Compare to the old core to see what I mean. Also, for now, ignore the white square position, as it's incorrectly placed, and getting it working correctly depends on how we proceed with the aforementioned problem.

[DirtyHairy]

@sa666666 I'll look into it the next days. I guess we'll need to add some additional logic to FrameManager to do this properly.

[sa666666]

No problem, I'm working on other parts of the code ATM.

[DirtyHairy]

@sa666666 I just took a look; the problem is the semantics of FrameManager::scanlines(): at the moment, this will give the current frame scanline if the current line is in the visible range of the frame --- in all other cases, it returns the last total frame line count.

This is trivial to fix by changing the method to always return the current frame scanline, but TIA::scanlines() is used in several different places, so I'd like to ask you first of whether this should indeed be the correct behavior :smirk:

[sa666666]

The old semantics of scanline() was to return the actual scanline at the time of the method call, whether it was being drawn or not. In other words, if I enter the debugger with a ROM of 262 scanlines, repeatedly stepping by scanline would go 0, 1, 2 ..., 262. This assumption is used everywhere scanline() is currently used in the codebase, so nothing should break if you modify it to work this way (famous last words).

EDIT: I also need a way to determine when (a) the drawing starts (ie, the scanline at which drawing first happens, or IOW what scanline now does), and (b) similarly when the drawing stops. These would be used by the debugger TIA image to know when to start/stop drawing the frame in black&white.

[DirtyHairy]

Commit 0efbbde fixes TIA::scanline to always return the current total scanline, as we discussed. This fixes stepping line by line. In addition, I have fixed TIA::scanlinePos, and the framebuffer preview seems to work properly now, including the "cursor".

@sa666666 , you can access the information you require by either

• FrameManager::isRendering: is true while drawing is in progress
• FrameManager::getY: gets the current visible line (aka scanlines() - frame offset) As an alternative, we could also add callbacks to FrameManager (similarly to what I am doing for frame start and stop).

[sa666666]

Thanks @DirtyHairy, the scanline issues in the debugger are now fixed in https://github.com/stella-emu/stella/commit/c99cb33cda234940d5c04f1e8cc09a415405bac9.

[sa666666]

I just realized that rewind functionality in the debugger depends on state saving working, so this issue can't be closed until issue https://github.com/stella-emu/stella/issues/12 is done.

[DirtyHairy]

Hmm, I haven't touched anything related to state saving until now as I was expecting that the internal structure of the sprites would still be evolving and changing. However, it could well be that no more changes are needed and, in any case, stability of the serialized representation is not an issue for rewind, so I might take a look at what still needs to be done there in the future.

[sa666666]

The comment I left above was more for myself than for you. I was simply stating that currently the state save/load is returning false, which doesn't allow rewind to work (it only functions when the current state is accurately saved and returns true). I am waiting until you finish the implementation details before I get to it; you don't have to do anything with the state save stuff yourself.

[sa666666]

@DirtyHairy, is it possible to modify TIA::ystart() to return the actual ystart value being calculated, and not just the 0 when autodetect is used? Or maybe introduce another method??

Basically, I need a method that returns the actual scanline number based on the TIA image shown in the debugger. For example, the first line shown in the debugger is probably 34, 35, etc, depending on what the frame offset it. For the first line (line 0 shown), I need a method that tells me that it is (for example) scanline 34. I'm guessing this is calculated somewhere inside FrameManager, but is it actually ystart that I'm looking for? And would modifying the ystart to be that value when it was initially 0 throw off something else in your algorithm?

[DirtyHairy]

Hi @sa666666 ! It sounds like FrameManager::scanlines is what you are looking for --- it returns the current 'total' scanline. I.e, if autodetect is of (nonzero ystart), it will return ystart on the first visible line, otherwise the autodetected first scanline.

The autodetected first scanline is kept separate from ystart in VblankManager::myLastVblankLines, but exposing it wouldn't be a problem. From the top of my head, switching ystart to that value shouldn't cause any issues.

[sa666666]

I've just experimented, and it's actually VblankManager::myLastVblankLines that I need. FrameManager::scanlines() won't work, since before we do a 'fill to scanline' command, that method will return the actual # of lines, not the first line drawn (ie, it will change based on if a partial frame is drawn, which is exactly what I don't want in this case).

I will experiment further to make sure that changing ystart() method to return myLastVblankLines won't cause any regressions.

[DirtyHairy]

I see --- I misunderstood you there :smirk: I am pretty sure that my code doesn't rely on ystart() anywhere, so changing its semantics should be fine from my point of view.

[sa666666]

Selecting the specific scanline in the TIA display with the mouse now works correctly, fixed in https://github.com/stella-emu/stella/commit/b5b058c615c4cf8a39dc1ed03bc4f7ccc4634b01.

[thrust26]

Were are we here?

IMO, as soon as the debugger is working again (including save states), a final Stella 5.0 can be released. Or do I miss something? (PAL color loss would be nice)

[sa666666]

There are still a few things left TODO for the debugger. Save states are easy; it's just that the TIA classes need to be finalized before I do it. But there are still some issues I'd like to see closed before a final 5.0 release, including https://github.com/stella-emu/stella/issues/63, https://github.com/stella-emu/stella/issues/83, https://github.com/stella-emu/stella/issues/93 and https://github.com/stella-emu/stella/issues/108.

The second one in particular is a regression from Stella 4.7.3. In general, I have no problem with releasing 5.0 with the new TIA not quite complete, but I'd like to not have any regressions.

[thrust26]

Are the TODOs for the debugger listed somewhere? When would it make sense to start testing the debugger?

[sa666666]

Only in the code :smile: In particular, there's a bunch of FIXME's in https://github.com/stella-emu/stella/blob/master/src/debugger/TIADebug.cxx. To fix them, I need answers to questions posed in https://github.com/stella-emu/stella/issues/73.

[sa666666]

I'd also like to have the BUS and CDF bankswitch schemes in before the 5.0 release.

[DirtyHairy]

The second one in particular is a regression from Stella 4.7.3. In general, I have no problem with releasing 5.0 with the new TIA not quite complete, but I'd like to not have any regressions.

My five cents: I do not think that TIA emulation will ever be complete :smirk: There will always be edge cases in which the emulation will differ from real hardware. A gate level simulation would be required to get even close to that, and even then, there would be effects (e.g. due to leak currents) that are not part of the emulation (just look at the documented cases where temperature changes what the TIA displays). I think that the new core is already much closer to hardware than the old core was, and there is still alot potential for getting even closer, but it will never achieve a 100% match :smile:

Philosophy aside, I agree about #83: I would very much like to get this closed before a release of 5.0. I am less enthusiastic about the other issues, though :smirk: #63 and #93 do not have any defined scope. I opened them to track the efforts of getting closer to real hardware in these cases, but there is no point at which you could add the tag "complete", there's always going to be the other odd case that we didn't examine. As for #108, I am very curious about the solution of this puzzle, but I don't think it is a show-stopper for a release, I guess the difference is mostly academic.

[sa666666]

I agree. I'd like to see https://github.com/stella-emu/stella/issues/83 done, and BUS/CDF bankswitching added. And the debugger stuff needs to be completed. IMO, those are the real showstoppers for 5.0. But depending on how hard https://github.com/stella-emu/stella/issues/83 is to fix, I'm not opposed to releasing 5.0 with it not fixed.

Overall, I'm pretty flexible with all this. My main concern right now is finishing the debugger. So if you both wouldn't mind looking at the FIXME's mentioned above and advising on the semantics of some of the functions ...

I'm going to be spending this weekend on finishing the debugger (with your feedback) and getting state loading/saving working again.

[DirtyHairy]

83 is hopefully history :smirk: I would opt for ignoring #63 and #93: unless we have any issues connected to them, they are purely academic.

@sa666666 I'll try too look at those FIXMEs this weekend. As a sidenote, I think I have spotted a bug in the debugger: the "cursor" starts at hclock 0, not at 68 or 76. To check this, just look for a WSYNC; the cursor will immediately start moving right as you step over it.

[sa666666]

I've removed a few of the FIXME's in https://github.com/stella-emu/stella/commit/90aa152f596189c0b4e1eed362434fdaec9ee882 and https://github.com/stella-emu/stella/commit/4b56a178d8e7589da3a46256f04a3b08f448c759. The remaining ones deal specifically with cases where one can't just change a register in the TIA with a POKE, but need to access the TIA sub classes directly. That's the ones I need help with. In particular, I'm not sure how the implement the ones that set the graphical objects position.

[DirtyHairy]

Unfortunately, I am rather low on time to spare at the moment, but I have finally managed to take a look at TIADebug.cxx and at the TODO.

Concerning querying / setting the sprite positions: this information is not directly available, but instead encoded in the phase between the sprite counters and the playfield counter (myHctr). I can easily provide getters and the setters on the sprites for this. Concerning semantics: setting the position would set the sprite counter to its proper value and be esentially similar to strobing RESx on a suitable clock, with the same effects on draw counter decoding. In case of the players, I propose define the position as referring to NUSIZ 0 --- this will lead to a one pixel shift for double and quadruple width players, but this just reflects the fact that there is no well defined player position for players that is independent of NUSIZ :smirk: @sa666666 , @thrust26, what do you think?

Concerning register peeks / pokes (including those that are disabled atm): reading back the register values the way it is implemented now is problematic and may prove misleading as there usually is a clocking delay between the poke on the bus and the actual effect. I.e., the current implementation will not reflect effect of a STA GRP0 after the instruction has finished, but only another pixel clock (which amounts to stepping another instruction). I would propose to implement a 128 byte "register file" that records all pokes and that can be read at any time to reflect the value of the last write. As a plus, we could drop all register-specific getters from the TIA and sprite classes and instead have a single getter paramterized by the register enum. Again, @sa666666 , @thrust26 , what do you think?

[sa666666]

I have no issues with anything you said. In particular, having all the registers (whether read-only or not) in a separate array is similar to how 4.x worked, except in that case they were all in separate variables.

I defer to Thomas at this point, since he is a 2600 developer that would likely be using the debugger during development, so whatever makes sense from that POV should be what we do.

One question, though. Wouldn't we want to know the most current value of each register at any point in time? So would this 'register file' reflect that? I'm assuming it would, since you'd be using it to replace all getters in the TIA class.

Would you mind doing an outline of this, or perhaps implementing it entirely? When you have time, of course.

[thrust26]

I think I would prefer if the sprite position value represents the actual position, especially because it represents not a register, but the result of positioning the sprites.

Regarding the clocking delay, how about making this transparent? E.g. by displaying the current delay value and/or by making a poke a two step transition. So when value of a register is changed, the new pattern and the old pattern are displayed (e.g. by different shading) in parallel.

Also, how about allowing a pixel clock step additional to a CPU clock step?

[DirtyHairy]

One question, though. Wouldn't we want to know the most current value of each register at any point in time? So would this 'register file' reflect that? I'm assuming it would, since you'd be using it to replace all getters in the TIA class.

I'm not sure that I understand your point correctly, but I think the "register file" would do exactly that: track the last write to each register.

Would you mind doing an outline of this, or perhaps implementing it entirely? When you have time, of course.

I think I can implement it this week. I've got long train rides coming up on wednesday and thursday, and I should have ample time for coding there :smirk:

I think I would prefer if the sprite position value represents the actual position, especially because it represents not a register, but the result of positioning the sprites.

Then I'll implement it this way :smile: For the players, this will imply that the position shifts when player width is changed between normal and wide.

Regarding the clocking delay, how about making this transparent?

That's an interesting idea. The necessary information could be extracted by examining the delay queue. I think that this would make a nice feature in one of the next releases. @sa666666 , what do you think?

Also, how about allowing a pixel clock step additional to a CPU clock step?

Unfortunately, I don't see any way to do this in Stella (either pixel clocks or CPU / RIOT clocks). In 6502.ts, the emulation is driven in a loop cycle by cycle, with each component receiving clocks in turn. This makes stepping very easy as it just means single stepping the loop, but it makes cycle exact bus access in CPU emulation a nightmare (which is why this currently not implemented properly). In Stella, the emulation is driven by the CPU whose smallest unit of execution is a single instruction. Each bus access increments the system clock by one, and if hardware registers are accessed, the peripherials fast-forward their internal state to reflect the current system clock. This is more efficient (and you get cycle exact bus timings for free), but the smallest unit by which the emulation can advance is a single instruction.

[sa666666]

WRT the part you asked me about, I would just need a mockup of what you'd like the UI to look like. Again, from the POV of a 2600 developer, who would find it most useful.

As for the current code, I think I will do a test6 release in the next day or so, since we now have BUS/CDF bankswitching and state saving working. Any objections to this?

[DirtyHairy]

I'm fine with a new prerelease :smirk: As for the UI, I'd like to defer this to @thrust26 as he is much more of an 2600 developer than me.

[thrust26]

Are we talking about the clock delays here?

For the delay value, I would prefer it to be positioned directly before or behind the respective register (e.g. the GRP0 pattern or the M0 enable checkbox).

For the two step transition:

• The GRPx and PFx pattern could get two intermediate pattern; one for getting enabled and one for getting disabled, e.g. a small rectangle in the middle when getting enabled and the inversed graphics when getting disabled.
• The checkboxes could get intermediate colors; again one for getting enabled and one for getting disabled or the same graphics as for the pattern.

BTW: Regarding the current UI:

1. IMO the GR: prefix is quite superfluous.
2. We should try to get rid of the colons. The layout (spacing, alignment etc.) should make it obvious where a label belongs to.
3. The TIA and Audio tab could be merged (I prefer as much information at once as possible).
4. A small vertical gap between BL and PF would structure things a bit nicer
5. The PF and GRPx pattern are handling the background differently. IMO it should be transparent for both. Else you have to check the colors first, before you know which PF registers are enabled.
6. The bits of the PF registers should be annotated, because I always have to look up the info :) (e.g. 4..7, 7..0, 0...7). Either inside the boxes or above as labels.

[sa666666]

If possible, please draw how you'd like it to look and provide a picture. Doesn't have to be neat, but I have problems picturing stuff from text descriptions only.

Perhaps draw something on paper and take a picture of it, do a mockup in a graphics editing program, etc.

[DirtyHairy]

I'm not sure that this is the right tool for this purpose, but the 30 days trial is free, and it usually does the job when we need it at work: https://balsamiq.com/ :smirk:

[thrust26]

I took the easy way and partially modified the TIA tab (mainly P0, M0 and PF).

Maybe we should rename Delay into VDEL?

[ale-79]

Would it be possible to show the content of both the normal and delayed registers for each player (and the delayed "enable" bit for the ball) and have a visual indication of which one will be displayed?

I think the vertical delay is one of the aspect of the TIA that's particulary difficult to understand while learning 2600 programming and a visual representation would greatly help.

[thrust26]

That's a good idea. We probably should put the delayed registers directly below the normal ones. For GRPx this is straightforward, but I think it would be a waste doing the same for BL.

Another thing we should fix (not fixed in the mockup yet): It should be obvious to the user, which values can be changed and which not. Non changeable values should have a different e.g. background.

[DirtyHairy]

I must admit that I have a hard time imagining out how to squeeze the current value, the pending value and the delay for all registers (into the widget). @thrust26 , I like your suggestions for checkboxes and bit masks; perhaps we can do something similar for numeric quantities by showing their upcoming value next to the current one in a different color (in the same field)?

As for the delay, perhaps we can skip it for now? The highest value is 6 color clocks (HMOVE), all others are one or two clocks, so it's only visible immediately after the corresponding instruction anyway.

As for visualizing the "old" and "new" GRPx registers: I think this is an excellent idea.

[thrust26]

I have no idea how we should display two values in the same field (besides each other? alternating display?)

As for the delay, since it is (usually) only valid for the current TIA write, how about displaying one, labeled value instead? So when e.g. a "sta GRP0" has been executed, this would be displayed as "GRP0: 1 px delay". And at the same spot on the UI, a change e.g. of PF2 would be displayed. And when there is no pending change, nothing (or an alternative text) is displayed.

[sa666666]

We do have some amount of vertical space, so extra widgets can be added. I'd rather do that than try to cram everything into one widget, and make it very difficult to understand what it's actually implying.

Again, I will need pictures before I can implement any of this. So I leave it to you to discuss until you come to an agreement ...

[DirtyHairy]

@thrust26 I was think of displaying values next to each other, but I think your second suggestion is much better. In fact, this matches what is going on below the hood: there is a delay queue which, for each of the next few cycles, holds the writes that are queued to execute in this cycle. We can just display the contents of the delay queue below the existing widgets, something like

 Queued writes:
     * 0x47 -> GRP0, 1 pixel delay

As the maximum delay is 6 clocks, the maximum possible queue depth is limited to two entries (if HMOVE is hit with a RMW instruction), so two lines should suffice. For starters, we can just display the hex value of the write and the delay, but we can evolve this to showing a processed version of the induced change, e.g. the new pixel mask.

@sa666666 , if we agree on this, I think I can try to create an initial version of such a delay queue widget (as I know the delay queue code best :smirk:).

[sa666666]

If you wish to tackle the UI code yourself, then I won't object. But it can get somewhat tedious, so I offer to do it if you find it too much work.

[thrust26]

Here is my latest mockup. Either we have individual delays or one combined one. I put in both, the combined one is at the bottom, maybe we find a better place.

Other changes:

• darkened read only fields
• moved the labels closer to the checkbox
• removed all colons
• shortened, removed and fixed some labels

I like the "Queued write" prefix. Not sure if have to annotate the new value.

BTW: With Busstuffing working, I think the queue could have up to three entries.

[DirtyHairy]

@thrust26 I think we should add the new value to the queue widget. It's redundant for the bitfields if we annotate them this way, but we need the value for the other registers which are not visualized this ways. Also, I must admit that I am not fully convinced yet that this way of annotating the bitfields will not prove to be more confusing than helpful :smirk: If this does not work out well, then we could always add the upcoming bitfield to the queue widget.

As for the individual Delay values next to the bitfields, I think we can dump those. As for the two different bitfields for each player, I think we should annotate them with 'old' and 'new' and highlight the one currently active (depending on vdel). Also, only the 'new' register can be the target of a delayed write. I'll try to build a mockup later on the train.

@sa666666 I'll try my hands on the queue widget and see how it goes. If I get lost, I'll come running to you for help :smirk:

[thrust26]

Let's see what you can deliver :)

[sa666666]

The major issue you may run into is that the UI very much exists only to service current needs. IOW, it's not a general purpose UI toolkit, and you will probably have to create some items from scratch, modify others, etc. So it's not as easy as looking up the item to use and adding it; you may need to design it first :smile:

[DirtyHairy]

With commit 988b360, all FIXMEs from TiaDebug should be addressed --- shadow registers and positioning are implemented and work. There still is something funky with double buffering, but that'll have to wait until my trip home tomorrow :smiley:

[sa666666]

Read-only fields are now coloured differently (currently using the above colour, but this can be changed) in https://github.com/stella-emu/stella/commit/3e5df9c7bc4485f5472f409e0909004051d655ea.

[thrust26]

Probably a color in the middle between the bright input field background and the tab background would be perfect.

[sa666666]

Feel free to experiment and send me the colour you want :smile: When I said "above colour", I meant the grey-ish one you used in the mockup. It is very easy to change, just let me know what you want (the hex triple for RGB would be fine).