Refactor mmio_mux

[sy2002]

@MJoergen let's use this issue to get the refactored and much more elegant mmio_mux working.

Originally, there was pull request #16, but this lead to very strange behaviours of the keyboard. So I reverted the PR back to the old design. Here is the problem description that can also be found in PR #16:

Problem description

I merged the PR, synthesized with Vivado and then run into a strange effect, when performing some additional tests on my loccal Nexys4DDR hardware.

Let's start first with what works fine:

• You already tested eae.asm, cpu_test.asm and ise.asm: I retested: Works.
• To check if the VGA registers and the SD card registers and the PS2/USB keyboard stills work, I loaded Q-TRIS from qbin/q-tris.out from the SD card and played it: Works only partially: The keyboard behaves very strange: see description below (But VGA and SD card seem to work fine)
• To test if the TIL leds still work, I ran regbank.asm: Works.
• To check the UART, I switched STDIN/OUT and connected by a serial program and I used qtransfer: Works.

PS2/USB Keyboard does not work reliably any more:

Here is the strange effect I observe on the PS2/USB keyboard: Many times, I need to hit keys 3-4 times until they are registered. Example: When trying to load Q-Tris from the SD Card, I need to enter normally F L qbin/q-tris.out but now I need to enter F LL qqqbin///q--trisss...outtt, i.e. I sometimes (not predictable when and which key) need to repeat the keys.

When I revert the PR to the old design and synthesize again, then it works fine.

So for now, I reverted the develop branch back to the old design.

Since I consider your design as much more elegant, it would be great, if we could hunt down this bug together :-)

At this moment in time I am stunned:

• Is your new elegant design maybe faster than my old one and uncovered a timing problem or other kind of glitch that was sleeping in our code since years?

• Or do we all just overlook some obvious typo?

Always fascinating how hard FPGA development is compared to software development 😈

[MJoergen]

I've reproduced the behaviour with occasional missing key presses, with a keyboard connected to the Nexys4DDR board, using Vivado.

I'm quite stumped by this problem, and just want to give an update on my progress so far.

After reproducing, I reviewed my changes to mmio_mux, but found nothing obvious. I then changed tactics and decided to make use of the Internal Logic Analyzer (ILA) built into the FPGA. This tool is a bit cumbersome and the process is partly manual. However, it does give the ability to watch how the different signals in the design change for every clock cycle.

The plan was to watch the kbd_en and cpu_data signals in the keyboard.vhd module. However, apparently this is not possible and leads to an error with multiple drivers (like before). I assume this is once again related to using tristate buffers. I also tried to connect the ADDR output signal from the CPU to the ILA, but that failed too. I don't know why that should be a problem, but I assume this is related to the tristate buffers as well, and possibly to the keep_hierarchy option (which I'm so far keeping at the default value 'rebuilt').

On the upside, I successfully connected the ILA to the ascii_code signal of the module ps2_keyboard_to_ascii, as well as fsmCpuAddr of the CPU and write_addr in register_file.vhd. I indeed see that sometimes the ascii_code from the keyboard is updated, but then the incorrect value (0004) is read from address FF13, instead of the expected value (0005). So this narrows down the problem as to why is bit 0 of FF13 sometimes not asserted when the keyboard correctly detects a key press ?

Then I looked more closely in the file keyboard.vhd and noticed the use of latches (?) in the process ff_ascii_new_handler, where the process is triggered on another signal (and not a regular clock signal). I'm struggling to reason about how this code is supposed to behave. Furthermore, the ILA expects all debug signals to be clocked by the same clock signal, so I'm unsure whether the ILA can be trusted to show the correct value of ff_ascii_new.

Things to try:

• Connect ascii_new (in ps2_keyboard_to_ascii.vhd) to see if it behaves as expected.
• Change keep_hierarchy from 'rebuilt' (which is the default) to 'none', and see if that solves the issue with multiple drivers.
• Rewrite keyboard.vhd to make use of only a single clock in all flip-flops. This is not as simple as it sounds, because I have to ensure that the behaviour of the module doesn't change. But that requires I can figure out the expected behaviour from looking at the existing code.
• A last-ditch attempt could be to go straight to the branch dev-io-addr and refactor the mmio_mux.vhd module there (which is a much simpler task) and hope the problem with the keyboard does not re-appear. Not a satisfactory solution as long as the root cause is unknown. 

Lessons learned so far:

• How to use the ILA in project-flow-mode in Vivado. I'm used to working in non-project flow and using tcl-scripts to automate the process.
• It is non-trivial to contribute to a foreign code base with different coding styles etc.
• I have still a lot to learn about VHDL programming :-)

Comments, ideas, suggestions etc. are most welcome.

[sy2002]

WOW!

Thank you for this thorough analysis! I did not even know, that something like the "Internal Logic Analyzer" (ILA) is existing at all. :-) Very interessting. One day, you need to show me how to use it.

After reading through your process and revisiting the five year old code of keyboard.vhd I need to admit: This code can and should be refactored. Furtheremore (also as discussed in Issue #4) at the right moment in time (when we are not having so many loose ends open in parallel) - we should get rid of the tristate logic in the overall QNICE-FPGA project.

So my view after reading your comment is: For some reason, the code in keyboard.vhd is stable in the old way mmio_mux works - but the code seems to allow race conditions and/or glitches that surface to daylight as soon as we implement some faster / more elegant mmio_mux.

I am volunteering to refactor it (but this might take a while as I first want to finish the interrupt topic in dev-int).

So in case you want to refactor it: Maybe you can combine refactoring keyboard.vhd with improving mmio_mux inside the branch dev-io-addr while adjusting the hardware to work with the new addresses specified in dev-io-addr?

When we get rid of the latches and only work with one central clock, then I am pretty optimistic, that we will solve the glitches.

I am describing here how keyboard.vhd is meant to work and give you some more background on how the software accesses the keyboard.

• Bit 0 in $FF13 signals: New ASCII character available for reading while bit 1 signals new special key. Look at the constants in kbd_constants.vhd to learn what we mean by "special key".
• The underlying logic makes sure, that there is always either a new ASCII key waiting or a new special key.
• When $FF13 signals a new key then it can be read in $FF14
• keyboard.vhd is just a wrapper of ps2_keyboard_to_ascii.vhd: The latter one is pulling up ascii_new for one system clock cycle, if a new ASCII key is waiting in ascii_code and it is pulling up spec_new, if a new special key is waiting in spec_code.
• Furthermore, the currently pressed modifier keys are available in realtime (independend from ascii_new and spec_new) via the modifiers signal.
• The semantics that are implemented in keyboard.vhd are: If a new ASCII or special key comes in, we use the ascii_new or spec_new signal (which is only there for one clock cycle) as a clock to trigger a flip/flop to store the news, that there is a new key waiting. This news is directly wired to $FF13.
• When now the monitor checks $FF13, it sees: new ASCII key or new special key
• When the monitor READS the new key in $FF14, then an important action takes place: The "new" flag is deleted. And by the way I guess (when I look at this code from my 5 years older and wiser perspective), I assume, that here lies some potential for a race condition and/or glitch

So I guess there are many way to refactor this.

A simple state machine, cloked by the system clock might be a way forward:

• Idle: Wait for either ascii_new or spec_new to be strobed: remember which one it was
• Wait for data being read via MMIO: Provide keypress in $FF14 and the news which key type it was in $FF13
• Reset: As soon as the CPU has read the data in $FF14, reset the flags

Have explicitly three states might make things more stable. But maybe you find a more elegant way to do it.

Here is an extra idea that goes a bit beyond this simple refactoring:

Right now, there is no keyboard buffer (FIFO): If the Monitor is too slow in collecting the key-strokes, then we loose key presses. But this is for sure at this moment not the reason for our challenge, because when the Monitor waits for a keypress, it currently does nothing else - see the code here But in future, when we might have interrupt driven IO or some VGA games, then a keyboard buffer (FIFO) might be helpful for not to miss keys.

So depending on your motivation and "refactoring-energy", you might even want to go as far and add a keyboard FIFO. There is a generic FIFO that we use for UART in the develop branch in vhdl/fifo.vhd. An idea might be: Create a RAM_WIDTH, which is 16 bit (SPEC + ASCII) + 2 bits for the flag which one is valid + 3 bit for the modifier keys. Then you connect wr_en with something like ascii_new or spec_new. I think no state machine would be needed any more because on the one side, the signals from ps2_keyboard_to_ascii.vhd are filling the FIFO and reading via MMIO shrinks the FIFO automatically, this is how FIFOs work in general. So with the FIFO, maybe the refactoring might be even easier than with the state machine, but I am not sure...

What are you thoughts?

[MJoergen]

Well, I've made some progress.

I made some small changes to keyboard.vhd to make all processes run on the same clock. And that appears to solve the issue! I had to make some small changes to ps2_keyboard_to_ascii, because the signal ascii_new was held high for many clock cycles after a key press (presumably until the next PS2 event from the keyboard).

So with these changes, ascii_new is only high for one clock cycle, and the signals ff_ascii_new and ff_spec_new are clocked by the main clock. There was no need for a state machine.

I still don't fully understand the root cause, and agree with your guess that there must be a race condition.

So far I've tested the changes together with my old PR on the develop branch.

I assume that this fix should be a part of the dev-io-addr branch, right ?

Adding a keyboard FIFO sounds like a good idea. That should probably be a separate issue.

I'm starting to feel there are a lot of loose ends, and it would be nice to make a (small) list of issues / changes that are to be a part of the next release. With the understanding that all other issues are for an even later release.

I will now see if I can get the dev-io-addr branch to work with the new IO memory map and a refactored mmio_mux and a new keyboard.vhd.

[sy2002]

This is excellent news! 👏 👏 👏 I am totally excited, because as written in another issue: Thanks to you the quality of our code base is strongly improving 😃

So yes:

Please work in branch dev-io-addr and commit the changes to the keyboard as well as your new mmio_mux there in dev-io-addr. Please test on real hardware and do the smoke-tests from CONTRIBUTING.md.

While updating dev-io-addr to your new mmio_mux and the new addresses from the monitor/sysdef.asm of branch dev-io-addr: Would be great if you could also sift through the VHDL files and adjust the comments in the VHDL files of the various devices that are still mentioning the old addresses.

And you are totally right: So far, the loose ends were all in my head. But now, that we collaborate, it makes 100% sense to write them down. I did it: You find everything in issue #28 "Release 1.6: Scope"

EDIT: And please don't be shy to add your name to the header comments of all the files you improved :-)

[sy2002]

I looked at your code: Looks good.

I assigned this issue to you: If it "survived" your re-test on real hardware: Please close the issue. (I will retest it also, at a later moment in time, when we merge dev-io-addr, but then more in the form of the overall post-merge, pre-release testing)

[MJoergen]

Verified on Nexys4DDR board. Tested with both keyboard and USB connection.