jbio-p4io impl and updated jbio API to fully support test menu - [merged] #191

Closed icex2 closed 1 year ago

In GitLab by @mon on Apr 4, 2021, 09:28

Merges feature/jbio-p4io -> master

Summary

This is the required pre-work to get p3io games working on p4io cabs, but it's a big feature on its own so it's been put into its own MR.

Description

Missing functionality had to be implemented in a breaking change to jbio - this was deemed OK due to the lack of pre-existing jbio hooks. Features are

Support individual corners via jb_io_set_panel_mode
Support coin blocker due to it affecting corner testing

In addition, jbio-p4io was added to open a p4io device + H44B over ACIO for input and lights.

Related Issue

!55 depends on this for full cab support.

How Has This Been Tested?

This is fairly standalone and only changes to jubeat hooks were made. Tested that PC based jubeat still works as normal, and my p4io cab based jubeat can work with the new jbio-p4io. Also tested using jbiotest.exe. Using festo.

Checklist

Documentation has been added.
I've tried to stick to generally-good style, but because make code-format makes a huge amount of changes to the rest of the codebase, I can't run it. I've discussed this with Xyen and after this (and !55) are merged, a new MR with code-format run can be merged to simplify your life here.

In GitLab by @mon on Apr 4, 2021, 10:35

added 1 commit

9d27936e - remove spurious warning

Compare with previous version

Makes me wonder, do we have some easy to use bat scripts for that like we have for iidx? If not, might be worth adding them for the different versions and provide commands for the most common use case.

* You can change the port and baudrate of the H44B node by editing the `jbio-h44b.conf` that should be created by default

I don't see a default jbio-h44b.conf in the changeset. Can you add one to the dist files and include it in the dist packages in Module.mk?

I assume this is "empty" because it runs together with the "main IO" on the same bus? Since I would have expect some kind of init code here, could you add a brief comment explaining why you don't have to initialize anything here?

Nit: Add log_assert(lights)

I just crossed checked this with the icca module and it seems like this was missing previously...I assume that means iccb didn't work at all?

Ah, I think that was causing some issues with "unresponsive inputs" back when I looked into jb support for a bit. Nice seeing this implemented properly now :thumbsup:

The magicbox IO doesn't support this and always returns a merged state per panel?

Missing cleanup, e.g. aciomgr_port_fini?

Well, that might be the case, but I personally would not rely on that even if we just copy-pasted the API for now. It will bloat the code, yes, but makes it less error prone to any changes there.

Naming nit:

    uint8_t header_aa;

Suggestion: Have these in enum which is self documenting?

Very good and important comment :thumbsup:

Why? Would that lead to any issues? In general, I would favor to always check errors if this is not causing incompatibility issues due to relying on "broken code".

I really like the separation of concerns here with a low-level usb foo handling module. Makes the code easier to read and understand. Well done.

Nit: Move 0xAA into a macro or constant.

I don't understand that part and it looks super hacky changing the requested amount of bytes. Can you provide some more information on why this is required and what would be the result if you would not do that?

Nit: To be very nitty about that interface, I don't think the USB layer knows about something like "jamma" at this point. Considering you also use a very generic p4io_usb_transfer which fits nicely, I would suggest to align the naming here (is it an interrupt endpoint?).

Ah, now these header files make sense: Sharing them between emulation and real device code. Nice :thumbsup:

Not your change, but magic numbers suck when you have to figure out what they mean years later. Do you happen to know why we need the 8 here and the 32 on P4IO_CMD_DALLAS_READ_MEM?

In GitLab by @mon on Apr 6, 2021, 12:00

Commented on doc/jbhook/jbio-p4io.md line 15

I based this off sdvx-hwio which does not include these configuration files - they are created on first run. Should I still add them?

In GitLab by @mon on Apr 6, 2021, 12:02

Commented on src/main/acioemu/iccb.c line 172

It worked fine! The geninput based ic card module has an eam_io_poll that does nothing. Forgetting to call it only affects eamio impls that actually need to be polled, and nobody has ever done that with jubeat before.

In GitLab by @mon on Apr 6, 2021, 12:03

Commented on src/main/jbhook/io.c line 145

Actually, the unresponsive inputs were caused by a backwards-bitmask - all panels were constantly pressed, and pressing the key un-pressed the input. The unresponsive input was caused by inputs coming from key release, not key press!

In GitLab by @mon on Apr 6, 2021, 12:05

Commented on src/main/p4io/cmd.h line 26

Oh! I removed this because the type name ends with header and the member in struct p4io_cmd_package is also header, so I thought the header in the name could be removed. Could you explain your reasoning?

I can change the upper to lowercase, of course.

In GitLab by @mon on Apr 6, 2021, 12:07

Commented on src/main/p4iodrv/device.c line 90

The main reason is because the reset command returns a 0 byte response - not even an "OK" status, 0 bytes in total. That technically should be returning an error, but because reset is a special case it ignores it. If you're happy with that explanation feel free to resolve.

In GitLab by @mon on Apr 6, 2021, 12:08

Commented on src/main/p4ioemu/device.c line 136

I have no idea, I suspect it's just matched to the number of bytes realio happens to return for that request...

In GitLab by @mon on Apr 6, 2021, 12:12

Commented on src/main/jbio-magicbox/jbio.c line 146

I don't know if it can support single panel mode, and I don't care to find out - the test menu certainly doesn't expose anything of the sort. I'll add a comment in my documentation commit.

In GitLab by @mon on Apr 6, 2021, 12:15

Commented on src/main/p4iodrv/usb.c line 139

This comment was updated in a later commit to read: // must be 65 bytes or requests can stall - only 64 will ever be returned Acceptable detail, or needs more?

I think we started including them because people were not reading the readme files and wondering where the configuration files are. Therefore, I suggest we stick to what we have been doing for the hooks so far. As for the sdvx-hwio one, that should be added as well (can be done in a follow-up MR).

Fair point with the "header in header" part. Then just lowercase is fine.

In GitLab by @mon on Apr 6, 2021, 12:19

added 1 commit

a3e248ea - H44B drv: add missed deinit code, comments

Compare with previous version

Yes, I am. Could you add this detail "because the reset command returns a 0 byte response - not even an "OK" status, 0 bytes in total. That technically should be returning an error, but because reset is a special case it ignores it." to the comment? I think this is quite essential for anyone picking this up to understand the actual reasoning behind that.

In GitLab by @mon on Apr 6, 2021, 12:20

Commented on src/main/aciodrv/h44b.c line 18

Added doc in a3e248ea

In GitLab by @mon on Apr 6, 2021, 12:20

Commented on src/main/aciodrv/h44b.c line 28

Added in a3e248ea

Nope, I think the new comment makes this part clear. Forgot to remove this comment after seeing it on one of the newer commits.

In GitLab by @mon on Apr 6, 2021, 12:23

Commented on src/main/p4io/cmd.h line 26

changed this line in version 4 of the diff

In GitLab by @mon on Apr 6, 2021, 12:23

Commented on src/main/p4iodrv/usb.c line 121

changed this line in version 4 of the diff

In GitLab by @mon on Apr 6, 2021, 12:23

added 1 commit

45d67ab1 - p4io: change magic AA to P4IO_SOF

Compare with previous version

In GitLab by @mon on Apr 6, 2021, 12:23

Commented on src/main/p4io/cmd.h line 26

I changed my mind with the other review comment about 0xAA being a magic number - with a P4IO_SOF #define I've changed this member to be sof. Solves all issues! Commit 45d67ab1

In GitLab by @mon on Apr 6, 2021, 12:25

added 1 commit