Reenable debugger

[DirtyHairy]

No description provided.

[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 😏

[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 c99cb33.

[sa666666]

I just realized that rewind functionality in the debugger depends on state saving working, so this issue can't be closed until issue #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 😏 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 b5b058c.

[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 #63, #83, #93 and #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 😄 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 #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 😏 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 😄

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 😏 #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 #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 #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 😏 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 90aa152 and 4b56a17. 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 😏 @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 knew about issue #114, and tried to keep the order still intact. Just read the debug colors line by line.

But let's hear what the others say.

BTW: Why did you chose to use mixed case names for the buttons (e.g. CxClr) initially?

The constants for these registers are all completely uppercase (within source code and also Stella disassembler). I got long used to it, but it still looks a bit odd.

[sa666666]

I agree, lets hear more input. I can align the ResXY buttons right now, and also change to all uppercase in the buttons (I don't recall the reason for doing it with mixed case). But then I want to do a pre7 release.

[thrust26]

Do whatever doesn't delay the next pre-release. :)

[SpiceWare]

I really like Thomas' suggestion for moving/relabeling the debug colors. A minor change would restore ROYGBIV:

[thrust26]

Oops, looks like I swapped BL and PF. That was not intentional.

Or, alternatively we could keep my wrong order and just swap colors between BL and PF.

[SpiceWare]

The order of PF BL is logical as the ball color would be just below the missile colors.

[thrust26]

Agreed. Probably that was the reason for my mistake.

So, how about just swapping the two colors? Would that cause a problem?

[sa666666]

Since the colours have been redefined since Stella 4.x, I see no reason why we can't swap PF and BL ones. And I too really like the layout of having them next to the COLUxx registers, since it reinforces the association between players and missiles, etc. So the only difficult thing for me is aligning CxClr with the rest of the ResXY buttons. But I'll see what I can do. In any event, I want to do pre7 release this evening.

[thrust26]

That alignment with the other buttons is really only a 2nd thought, since I didn't know where to put the collision segment. So I decided it would look best aligned.

So absolutely no big deal if it is not aligned.

[sa666666]

Latest changes in 1d30f1d. Every suggestion made is done, except for the second 'maybe' and the alignment of CXCLR. I'll be doing pre7 in a moment.

[thrust26]

One small wish for the next release (I missed this until testing now, sorry):
The active bits in the inactive P0/P1/BL registers should be displayed darker (not brighter) than the background (see my small mockup 28 days ago). IMO that makes the disabled state more obvious.

And in the end, all (maybe with a very few exceptions) colons should be removed from all tabs and other parts of the GUI (e.g. Options menu). And the labeled elements should be moved a few pixel closer to the label then.

[sa666666]

I simply used the background colour of non-editable widgets. I guess it's easy enough to add another colour.

As for the colons, yes, I am in the process of removing them. Perhaps you didn't notice a recent commit "the great colon purge"? 😄

[sa666666]

Colour is changed in 847e910 to match one in post from 28 days ago.

If there are no other items, I want to close this issue soon. The debugger has be re-enabled for quite some time, and what we've done here has greatly surpassed the original point of the issue.

[thrust26]

Yup, lets open a new topic for "beautification" or so :)

[sa666666]

Closing this since its goal was reached long ago.

[thrust26]

There is a small bug when enabling M0 and M1. Unlike for P0, P1 and BL, the real color is not displayed in the (former) checkbox immediately. The update only happens after the next instruction

BTW: Until now I missed that the checkbox was replaced with a color box, which works as a checkbox too. This is pretty unusual, and maybe irritating especially because there is no "Enable" label anymore. I would prefer to have at least the label back (I think the tab has enough horizontal space). And the behavior of the color box should be documented.

[sa666666]

Arrgh, not this issue again (it already has 138 replies)

As for the removal of the checkbox and label, that was at your insistence 👿
As is normal, I need a test case/specific way to trigger the bug, and a brief description of what should be happening.

Also, all the graphical registers are color boxes, so I'm not sure why it's more confusing for missiles.

[thrust26]

Sorry, I have learned that GUI refinements tend to be very tedious. Especially with multiple people with multiple ideas and preferences involved. But in our case, IMO the result was well worth it.
And no, it wasn't my "insistence". It was Alex idea and I didn't really notice the result until now. My bad, ignore me here. :) (and I think I will soon get used to it)

Anyway, back to the bug:

• start e..g River Raid and enter the debugger. Initially both missiles are disabled, displayed by the tabs background color. Fine.
• now double click* on the color box for M0. The color changes into a darker brown. This is not OK, I would expect the color to change into yellow (the color of P0) immediately.
• when you now step the CPU, THEN the expected color shows in color box for M0.

*BTW: why a double click? A single click would do. (please, don't kill me for this! :D)

[sa666666]

Insistence is perhaps a strong word. In any event, I will look into it.

As for the single vs. double-clicking, I don't know if there's any technical reason for one or the other. It should be easy enough to change it.

[ale-79]

Using color boxes for all the graphic objects allows to uniform the way the active register is visualized in objects with vertical delay.
The enable bit for missiles and ball sets/unsets the single pixel of the graphic object, and you can think the GRPx adn PFx registers as containing several enable bits, one for each of the pixels. So I think it makes sense using the same layout for all the objects. An "enable" label wouldn't provide additional information IMO, but I wouldn't mind if it's added.
As Thomas said, personal preferences play a large role here.

I confirm the bug and the fact that it only happens for missiles, not players, ball or playfield.

[thrust26]

For me the color boxes are fine. Already got used to them. :)

[sa666666]

The last bugs mentioned by @thrust26 above are fixed in 43c22af, so we can finally close this issue.