aciodrv: add PANB support (+ aciotest handler) - [merged] #197

Closed icex2 closed 1 year ago

In GitLab by @shtokopep on May 2, 2021, 15:16

Merges nostalgia -> master

Summary

add PANB support (nostalgia piano) to aciodrv as well as an aciotest handler for it

Description

this allows to use a nostalgia piano through aciodrv

Related Issue

https://dev.s-ul.net/djhackers/bemanitools/-/issues/69

How Has This Been Tested?

The aciotest handler :

displays the current velocity of held keys onscreen
lights pressed keys with that same greenish tone from AC game
can trigger the device close/reset from a key combination

Checklist

[x] Implemented (unit) test(s) which prove that the introduced changes are working as expected.
~~Tested with the following games:~~ aciodrv cannot be used with the game
[x] Followed the developer (style) guidelines.
[x] ~~Updated existing doc of or add new doc to README file(s).~~ aciotest doc didn't provide such level of detail
[x] ~~Updated development documentation.~~ didn't find aciodrv details in the docs

In GitLab by @shtokopep on May 2, 2021, 16:01

marked the checklist item ~~Updated existing doc of or add new doc to README file(s).~~ aciotest doc didn't provide such level of detail as completed

In GitLab by @shtokopep on May 2, 2021, 16:01

marked the checklist item ~~Updated existing doc of or add new doc to README file(s).~~ aciotest doc didn't provide such level of detail as incomplete

In GitLab by @shtokopep on May 2, 2021, 16:29

marked the checklist item ~~Updated development documentation.~~ didn't find aciodrv details in the docs as completed

In GitLab by @shtokopep on May 2, 2021, 16:29

marked the checklist item ~~Updated existing doc of or add new doc to README file(s).~~ aciotest doc didn't provide such level of detail as completed

Nit: Naming-wise, I would stick to an out prefix for out data like the names of the other structs in this union

                struct ac_io_panb_poll_out panb_poll_out;

Minor improvement: You could use a struct to actually put this comment into code, e.g.

struct keypair {
    uint8_t key1_velocity : 4;
    uint8_t key2_velocity : 4;
};

Actually, maybe you can even align this nicely with the total number of keys avoid this key pair construct:

struct ac_io_panb_key_velocity {
    uint8_t value : 4;
};

#pragma pack(push, 1)
struct ac_io_panb_poll_in {
    // ... other stuff

    struct ac_io_panb_key_velocity key_velocity[AC_IO_PANB_MAX_KEYS];
};
#pragma pack(pop)

You need to verify if this turns out properly packed. Otherwise, the compiler might fill-up each velocity item to a full byte which might break the whole thing.

Nit magic number 28: I assume that's the total number of keys on the input device? Suggestion: #define AC_IO_PANB_MAX_KEYS 28

~~Same for the magic number 14 above that: #define AC_IO_PANB_MAX_KEY_PAIRS 14~~ Not needed anymore if my above suggestion is do-able.

Do you mind elaborating the reason for this change?

Same here. This is in a module shared with other acio devices. I am worried this change breaks something else. Please let me know your thoughts behind this.

Nice refactoring. Do we need to expose the split send and receive methods in the module or could these even be static?

Unfortunately, this breaks the entire architecture of aciodrv modules. Each module should work independent of any thread management which a user of the module should take care of on their level of abstraction. I understand the reasoning for that but I am wondering if there is something else wrong for this root cause. Why is the serial buffer overlapping itself? If you don't drive the hardware, I would expect nothing to happen like that. Furthermore, sends and receives are typically executed in sync, i.e. receive input data, do stuff with that, set output data derived from inputs and send it.

It would be great if you can elaborate on that particaular issue a bit because I would like to understand the need for this implementation.

Nice contribution. There are a bunch of open points that I would like to clarify. Please let me know if you got any questions.

In GitLab by @shtokopep on May 2, 2021, 20:54

Commented on src/main/aciodrv/device.c line 205

oh! as I was typing the explanation I realized we are not reading one byte at a time here. I'm lucky this works with panb, you are entirely right it might break something else, will revert this.

In GitLab by @shtokopep on May 2, 2021, 20:54

Commented on src/main/aciodrv/device.c line 91

I think it was a leftover oversight from when the device parameter has been added.

I kept getting compilation warnings about it because the calls to this function in the file are with the parameters in this order, so I switched them back and now it's happy... (note that this only happens when AC_IO_MSG_LOG is on so that might explain why it wasn't caught before)

In GitLab by @shtokopep on May 2, 2021, 20:55

Commented on src/main/aciodrv/device.c line 405

they have to be exposed due to the way panb works (more on that in the comment about spawning a thread)

In GitLab by @shtokopep on May 2, 2021, 21:05

Commented on src/main/aciodrv/panb.h line 7

in fact, the way PANB works, you send the start_auto_input command only once, and from there the piano keeps spamming auto_poll messages, it's not in sync, not even a "send one command get one reply" thing, unlike all other acio devices I came across.

(this is also why we had to split the "send" and "receive parts", and why I don't bother checking the command number in the reply because in panb case it's not even the same command number (you send 01 15 once, and you get a lot of 01 10 messages which keep piling...)

when you send a lamp update command, you don't get a reply at all either. You just see the command has been processed because the lamp do light up, and because in the 01 10 messages you'll get from that point, the first subsequence number will be incremented to reflect what was in that last send_lamp command you sent.

basically the piano has no time to send you anything else than 01 10 messages, and it does so indefinitely.

Now, in the case of aciotest, I tried not to use a thread and rather just read the incoming packet at each handler_update, but doing this gave me the following behavior :

very slow update (if I held some keys before starting the update loop, i would see these keys as being pressed, but releasing them would not produce any effect... neither would pressing new keys)
after a (short) while, i would get checksum errors and then aciotest would remove the node from its update loop

Discussing the issue with @xyen we came up with this conclusion that it was probably due to too much data being sent on the serial line without us reading them to empty the buffer...

In GitLab by @shtokopep on May 2, 2021, 21:07

Commented on src/main/acio/panb.h line 17

oh, interesting... I will try and see if this works, would indeed be useful

thanks for the detailed explanation. well, that sucks that konmai is not sticking to their own "standards" they defined, again. I suggest to reflect that in the architecture using another layer. Make the panb module stick to how the other modules have done it so far and add another panb-proc module that takes care of threading and synchronisation. Documentation on the panb module should include stating the issue and advice to use the panb-proc module.

What do you think?

Also want to loop-in feedback by @xyen regarding that.

In GitLab by @shtokopep on May 2, 2021, 21:18

Commented on src/main/aciodrv/panb.h line 7

I guess it's possible to move all the thread management into the aciotest handler (or through a panb-proc module indeed, then we get a place to document all this I agree) rather than keeping it inside the aciodrv module if I expose the recv_poll and the start_auto_input functions

Just checked the code and you are correct. The code using that function is removed using ifdef-blocks. Therefore, this mistake did not have any effect so far on standard builds.

In that case, I would stick to the old signature because all other functions always have the context as the first parameter which is common to what I have seen if you pass around state. Instead, I suggest to fix the parameter order on the code calling this function.

In GitLab by @shtokopep on May 2, 2021, 21:49

Commented on src/main/acio/panb.h line 17

I have a question about using bitfields... how am I supposed to go about splicing this to separate values for aciotest ?

src/main/aciodrv/panb.c:142:13: error: used struct type value where scalar is required
  142 |         if (key_velocity[i]) {
      |             ^~~~~~~~~~~~
src/main/aciodrv/panb.c:143:27: error: incompatible types when initializing type 'uint8_t' {aka 'unsigned char'} using type 'struct ac_io_panb_key_velocity'
  143 |             uint8_t but = (key_velocity[i]);
      |                           ^

should I recast the key_velocity into uint8_t and then perform the bitshift myself, or is there another way ?

You forgot to access the actual value field since this is wrapped in a struct. I guess what you want is:

uint8_t but = poll_in.key_velocity[i].value;

In GitLab by @shtokopep on May 2, 2021, 22:14

Commented on src/main/acio/panb.h line 17

oh, good catch... it indeeds looks like each velocity value gets packed into a full byte unfortunately (on the aciotest thing first button value is updated by 2nd physical key, second button by 4th physical key, and the latter half of the buttons contain garbage..)

I think you have to also wrap the other struct to pack it and avoid that the compilers adds padding, try:

#pragma pack(push, 1)
struct ac_io_panb_key_velocity {
    uint8_t value : 4;
};
#pragma pack(pop)

In GitLab by @shtokopep on May 2, 2021, 22:26

Commented on src/main/acio/panb.h line 17

didn't work either... for reference here's the relevant bit of code :

    /* splice the keypairs into separate button values */
    for (int i=0; i<AC_IO_PANB_MAX_KEYS; i++) {
        button_state[i] = key_velocity[i].value;
    }

and instead of having this, it behaves as if velocity[2*i+1] gets mapped to button_state[i]

I just realized that the parameter 1 in #pragma pack(push, 1) is for defining the alignment to a single byte. Hence, my idea isn't possible. You could however at least split the higher and lower values into:

#pragma pack(push, 1)
struct ac_io_panb_key_pair_velocity {
    uint8_t key_1_value : 4;
    uint8_t key_2_value : 4;
};
#pragma pack(pop)

Not ideal, but at least removes the bit-shifting

In GitLab by @xyen on May 2, 2021, 22:32

Commented on src/main/acio/panb.h line 17

Yeah, bit fields don't compose like that correctly, you'll need to make a struct that just contains the high / low nibble, and access them in pairs if you wanna make use of this for readability

In GitLab by @xyen on May 2, 2021, 22:32

Commented on src/main/aciodrv/device.c line 405

@icex2 the internal send / recv functions remain static, it's just these outer ones have been split, for the reasons they mentioned.

In GitLab by @xyen on May 2, 2021, 22:32

Commented on src/main/aciodrv/panb.h line 7

I'd move kicking off the auto polling to its own function, but arguably the thread logic should just stay here. I don't think splitting it off to another library makes much sense.

In GitLab by @shtokopep on May 2, 2021, 22:34

Commented on src/main/acio/panb.h line 17

using struct ac_io_panb_keypair keypair[AC_IO_PANB_MAX_KEYPAIRS]; works but for some reason each pair of key is reversed? (key1 is actually key2..)

In GitLab by @xyen on May 2, 2021, 22:35

Commented on src/main/acio/panb.h line 17

Yeah, big endian, just swap the struct around

In GitLab by @xyen on May 2, 2021, 22:38

Commented on src/main/aciodrv/panb.h line 7

Actually on second thought, as all the init function does is start the auto polling, and the thread, I think it makes sense without splitting it off to another function.

In GitLab by @shtokopep on May 2, 2021, 22:38

Commented on src/main/acio/panb.h line 17

ok done, thanks :) working now

In GitLab by @xyen on May 2, 2021, 22:43

Commented on src/main/aciodrv/panb.h line 7

Hmmmm, on third thought, I think adding a new module is the right choice, this way the parsing code can live in aciodrv still, and the only thing the panb-proc handles is the threading / last state management.

This would provide a clear distinction / note to anyone reading the code, that there is something different going on here.