Skip to content
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

Sending USB reports to set keyboard LEDs? #6

Open
epsilon537 opened this issue Jan 9, 2024 · 38 comments
Open

Sending USB reports to set keyboard LEDs? #6

epsilon537 opened this issue Jan 9, 2024 · 38 comments

Comments

@epsilon537
Copy link
Contributor

I'm testing this core with a USB keyboard. It's working great! I was wondering if you're considering adding support for sending reports to the keyboard to set the num_lock/caps_lock/... LEDs.

@epsilon537
Copy link
Contributor Author

I added keyboard LED control to the core: https://github.com/epsilon537/usb_hid_host

@nand2mario
Copy link
Owner

That is nice. I'll merge it.

@emard
Copy link
Contributor

emard commented Apr 2, 2024

hi [epsilon537]
we have accidentally merged but it seems that asukp is still outputting
4-bit code, I extended it's hex width but something is not working for me
can you check in your rep does it compile and work

@epsilon537
Copy link
Contributor Author

Is this the version you merged: https://github.com/epsilon537/usb_hid_host/blob/boxlambda/src/usb_hid_host/asukp?
When I run it against this ukp.s, https://github.com/epsilon537/usb_hid_host/blob/boxlambda/src/usb_hid_host/ukp.s, it produces a .hex file where I can see a few opcodes > 10 (hex).
The generated usb_hid_host_rom.v also has 5 bits wide data.

@emard
Copy link
Contributor

emard commented Apr 2, 2024 via email

@epsilon537
Copy link
Contributor Author

I'll take a look, but I'm in transit right now. It might be a day or two before I can report back.

@emard
Copy link
Contributor

emard commented Apr 2, 2024 via email

@epsilon537
Copy link
Contributor Author

OK, I might do that (the ones I understand at least ;-)
You mentioned enumeration isn't working for you. Which board are you building?

@emard
Copy link
Contributor

emard commented Apr 2, 2024 via email

@epsilon537
Copy link
Contributor Author

Thanks. I'm not familiar with the Lattice toolchain, but this looks suspect in the ulx3s Makefile:

ecpbram --width 4 --depth $(ROMDEPTH512) --seed 0 --generate $(SRC)/usb_hid_host_rom_random_new.hex

Shouldn't that width be 5?

Also, it looks like top.v in the ulx3s board directory hasn't been updated to match the new usb_hid_host module interface.
Aside from the additional inputs and outputs for led control, I also split the usb inouts into separate inputs, outputs, and an output enable. That makes it easier to use the core in Verilator.
(In my fork of the repo, I haven't modified any of the board files. I just pull in the repo as a submodule and synthesize only the usb_hid_host core into my own design, BoxLambda).

What I don't get is that without fixing up top.v to match the new usb_hid_host interface, it shouldn't even synthesize. The port names have changed.

Anyway, if it helps, I can make a best-effort modification of the ulx3s top.v, but I won't be able to test since I don't have that board.

@emard
Copy link
Contributor

emard commented Apr 3, 2024 via email

@epsilon537
Copy link
Contributor Author

OK, so then width should be set to 8 in your Makefile, not 4.
In your top.v, where you instantiated usb_hid_host, can you add .update_leds_stb(1'b0) to the port list?

@emard
Copy link
Contributor

emard commented Apr 4, 2024 via email

@epsilon537
Copy link
Contributor Author

I don't think we're on the same page. What do you mean with the led updater module? There's no separate module, just a bit of extra logic in the usb_hid_host_core and the ukp module, and a bit of extra code in the ukp firmware.

I don't know if verilog, or your toolchain, has a concept of a default value for input ports that are left unspecified. I think it's best to be explicit and specify a value (or wire) for all input ports when you instantiate the usb_hid_host core in top.v.

If we were in the same room we probably would have sorted this out in 5 minutes, but as it is (and with me severly jetlagged) it'll take a bit longer for us to understand each other.

@emard
Copy link
Contributor

emard commented Apr 4, 2024 via email

@emard
Copy link
Contributor

emard commented Apr 4, 2024 via email

@emard
Copy link
Contributor

emard commented Apr 4, 2024 via email

@epsilon537
Copy link
Contributor Author

In the version before you reverted you had ".update_leds_stb(btn[2])" in your top.v.
I don't know if btn[2] is active high or active low on your board. It would be good to test with ".update_leds_stb(1'b0)".
A button there is no good anyway because the strobe should only be asserted for one clock cycle when you want to update the leds.

Another good test would be to see if enumeration works with the new usb_hid_host core running the old ukp.s firmware compiled with the new asukp.

@emard
Copy link
Contributor

emard commented Apr 5, 2024 via email

@emard
Copy link
Contributor

emard commented Apr 5, 2024 via email

@epsilon537
Copy link
Contributor Author

I found an error in your top.v (before you reverted) that would explain why enumeration isn't working:

.usb_dm_i(usb_dm_i), .usb_dp_i(usb_dp_i),
.usb_dm_o(usb_dm_o), .usb_dp_i(usb_dp_o),

That last one should be .usb_dp_o(usb_dp_o).

@emard
Copy link
Contributor

emard commented Apr 5, 2024 via email

@emard
Copy link
Contributor

emard commented Apr 5, 2024 via email

@emard
Copy link
Contributor

emard commented Apr 5, 2024 via email

@epsilon537
Copy link
Contributor Author

In your 'typo but it doesn't help' commit you only changed one statement between comments. I assume this is not what you actually tested.
Can you check in the code that you tested (i.e. with the fix but still not working) and push to Github? If you don't want to commit to main branch, maybe create a separate branch?

@emard
Copy link
Contributor

emard commented Apr 6, 2024 via email

@emard
Copy link
Contributor

emard commented Apr 6, 2024 via email

@epsilon537
Copy link
Contributor Author

I would be a bit more comfortable if in askup, you change this line:

\talways @(posedge clk) data <= mem[adr];

to:

\talways @(posedge clk) data <= mem[adr][4:0];

Nothing else is catching my eye at the moment.

@epsilon537
Copy link
Contributor Author

In my project, there's an explicit power-on-reset phase. In your project there isn't which means that the initial values in usb_hid_host.v are more important.
With the current code base, can you press and release reset (btn[1]) after startup? Does that help?

@emard
Copy link
Contributor

emard commented Apr 6, 2024 via email

@emard
Copy link
Contributor

emard commented Apr 6, 2024 via email

@epsilon537
Copy link
Contributor Author

I reviewed my usb_hid_host.v changes and noticed a coupled of things I don't like. I'll correct them and get back to you.

@epsilon537
Copy link
Contributor Author

I checked in my changes. I made sure ukp initial state is same as after reset. Please give it a try.

@emard
Copy link
Contributor

emard commented Apr 8, 2024 via email

@epsilon537
Copy link
Contributor Author

Bummer. I have spent a fair amount of time looking at my code, your code, synthesis results, and will continue to do so, but at the moment, I'm not seeing it. My go-to to dig deeper is simulation, but my simulation test bench is just working fine.
We may have to move on to an on-FPGA debug session. Is an embedded logic analyzer an option for you? I would love to see a trace of the ukp_prom input (adr/pc) and output (data/inst).

@emard
Copy link
Contributor

emard commented Apr 8, 2024 via email

@epsilon537
Copy link
Contributor Author

I wonder if the issue is with the inference of the tristate logic.
According to this doc: https://github.com/FPGAwars/iceIO/wiki
this how the verilog should be written:

//-- Generic InOut
//-- Number of data bits
localparam N = 1;

//-- Configuration as input:
//---- Read from pin --> din
assign din = oe ? {N{1'b0}} : pin;

//-- Configuration as output
//-- din is always 0!
//-- dout --> pin
assign pin = oe ? dout : {N{1'bZ}};

i.e. the if oe then-else should be applied to both the input and the output direction.

@emard
Copy link
Contributor

emard commented Apr 9, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants