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
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
Followed the developer (style) guidelines.
Updated existing doc of or add new doc to README file(s). aciotest doc didn't provide such level of detail
Updated development documentation. didn't find aciodrv details in the docs
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
<!-- Make sure you covered all items, which apply, of the checklist below. -->
<!-- Strikethrough items that do not apply and provide a brief description why. -->
* [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
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 completed
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: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
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 development documentation.~~ didn't find aciodrv details in the docs** as completed
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: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;
Nit: Naming-wise, I would stick to an `out` prefix for out data like the names of the other structs in this union
```suggestion:-0+0
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;
};
```
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.
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.
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.
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.
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.
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.
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.
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 205](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-1c1a2ad8deb60fa4bb473a3e4f9e4732R205)
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.
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:54
Commented on [src/main/aciodrv/device.c line 91](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-1c1a2ad8deb60fa4bb473a3e4f9e4732R91)
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)
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, 20:55
Commented on [src/main/aciodrv/device.c line 405](https://github.com/djhackersdev/bemanitools/compare/d3026cd540f38e4313c9102425a9d1f9fd006a27..f88ffdb8548d63b59c8a2df469c1b9d2dc804c6f#diff-1c1a2ad8deb60fa4bb473a3e4f9e4732R405)
they have to be exposed due to the way panb works (more on that in the comment about spawning a thread)
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:05
Commented on [src/main/aciodrv/panb.h line 7](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..b3cde6f6697cb86dfacf8168cc4843fa4b65a986#diff-a64e8e92fada8e639e87fc96c2ecf4dbR7)
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...
oh, interesting... I will try and see if this works, would indeed be useful
In GitLab by @shtokopep on May 2, 2021, 21:07
Commented on [src/main/acio/panb.h line 17](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
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.
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.
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
In GitLab by @shtokopep on May 2, 2021, 21:18
Commented on [src/main/aciodrv/panb.h line 7](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..b3cde6f6697cb86dfacf8168cc4843fa4b65a986#diff-a64e8e92fada8e639e87fc96c2ecf4dbR7)
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.
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.
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 ?
In GitLab by @shtokopep on May 2, 2021, 21:49
Commented on [src/main/acio/panb.h line 17](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
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;
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;
```
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..)
In GitLab by @shtokopep on May 2, 2021, 22:14
Commented on [src/main/acio/panb.h line 17](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
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)
```
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]
In GitLab by @shtokopep on May 2, 2021, 22:26
Commented on [src/main/acio/panb.h line 17](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
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:
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
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/acio/panb.h line 17](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
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
@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/device.c line 405](https://github.com/djhackersdev/bemanitools/compare/d3026cd540f38e4313c9102425a9d1f9fd006a27..f88ffdb8548d63b59c8a2df469c1b9d2dc804c6f#diff-1c1a2ad8deb60fa4bb473a3e4f9e4732R405)
@icex2 the internal send / recv functions remain static, it's just these outer ones have been split, for the reasons they mentioned.
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 @xyen on May 2, 2021, 22:32
Commented on [src/main/aciodrv/panb.h line 7](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..b3cde6f6697cb86dfacf8168cc4843fa4b65a986#diff-a64e8e92fada8e639e87fc96c2ecf4dbR7)
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.
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 @shtokopep on May 2, 2021, 22:34
Commented on [src/main/acio/panb.h line 17](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
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](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
Yeah, big endian, just swap the struct around
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 @xyen on May 2, 2021, 22:38
Commented on [src/main/aciodrv/panb.h line 7](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..b3cde6f6697cb86dfacf8168cc4843fa4b65a986#diff-a64e8e92fada8e639e87fc96c2ecf4dbR7)
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](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
ok done, thanks :) working now
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.
In GitLab by @xyen on May 2, 2021, 22:43
Commented on [src/main/aciodrv/panb.h line 7](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..b3cde6f6697cb86dfacf8168cc4843fa4b65a986#diff-a64e8e92fada8e639e87fc96c2ecf4dbR7)
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.
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on [src/main/acio/acio.h line 84](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-2e52a04245193c383aaacc3b14b371beR84)
changed this line in [version 2 of the diff](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1640&start_sha=19464bef5bb489a7215098c3a85d109408e964f8#4221f5f6f381c6122f2e150657a6036d9e22c926_84_84)
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on [src/main/acio/panb.h line 17](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR17)
changed this line in [version 2 of the diff](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1640&start_sha=19464bef5bb489a7215098c3a85d109408e964f8#b6a7bed3eb267b9e11b61ab6ebcf6a04c451a9a7_17_24)
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on [src/main/acio/panb.h line 28](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-6b4bf785b4f84ed9d0bf1f08bc3cbddeR28)
changed this line in [version 2 of the diff](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1640&start_sha=19464bef5bb489a7215098c3a85d109408e964f8#b6a7bed3eb267b9e11b61ab6ebcf6a04c451a9a7_28_34)
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on [src/main/aciodrv/device.c line 91](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-1c1a2ad8deb60fa4bb473a3e4f9e4732R91)
changed this line in [version 2 of the diff](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1640&start_sha=19464bef5bb489a7215098c3a85d109408e964f8#c04cec926c4bc2a62496efc8522ee127583ae23a_91_91)
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on [src/main/aciodrv/device.c line 205](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..19464bef5bb489a7215098c3a85d109408e964f8#diff-1c1a2ad8deb60fa4bb473a3e4f9e4732R205)
changed this line in [version 2 of the diff](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1640&start_sha=19464bef5bb489a7215098c3a85d109408e964f8#c04cec926c4bc2a62496efc8522ee127583ae23a_205_205)
In GitLab by @shtokopep on May 2, 2021, 22:44
added 1 commit
<ul><li>b3cde6f6 - review part 1</li></ul>
[Compare with previous version](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1640&start_sha=19464bef5bb489a7215098c3a85d109408e964f8)
This would provide a clear distinction / note to anyone reading the code, that there is something different going on here.
That is my primary intention with this approach.
> This would provide a clear distinction / note to anyone reading the code, that there *is* something different going on here.
That is my primary intention with this approach.
In GitLab by @shtokopep on May 3, 2021, 01:18
added 2 commits
<ul><li>335b27f6 - aciodrv-proc module</li><li>ecb3859a - review part 2</li></ul>
[Compare with previous version](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1642&start_sha=b3cde6f6697cb86dfacf8168cc4843fa4b65a986)
In GitLab by @shtokopep on May 3, 2021, 01:18
Commented on [src/main/aciodrv/panb.h line 7](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..b3cde6f6697cb86dfacf8168cc4843fa4b65a986#diff-a64e8e92fada8e639e87fc96c2ecf4dbR7)
changed this line in [version 3 of the diff](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1642&start_sha=b3cde6f6697cb86dfacf8168cc4843fa4b65a986#28b3e1c1f6d6eb9625ff0e235f7ec7a89b3a3a3b_7_7)
A new aciodrv-proc module has been added (even though @xyen confirmed that PANB was the only device from libacio that would require threading as of now, and there's little chance that konami adds new ones in the future, I feel panb-proc might become ambiguous when the acioemu part of panb will come into play)
In GitLab by @shtokopep on May 3, 2021, 01:26
Commented on [src/main/aciodrv/panb.h line 7](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..b3cde6f6697cb86dfacf8168cc4843fa4b65a986#diff-a64e8e92fada8e639e87fc96c2ecf4dbR7)
A new `aciodrv-proc` module has been added (even though @xyen confirmed that PANB was the only device from libacio that would require threading as of now, and there's little chance that konami adds new ones in the future, I feel `panb-proc` might become ambiguous when the acioemu part of panb will come into play)
I think right now it is fine to use btools "built-in" thread abstraction. I would suggest to rather have this provided as a parameter like on all the btools APIs to give flexibility to the user regarding which threading implementation to use. I just don't see the use-case needing this right now.
So good as is, but maybe something to keep in mind for a small refactoring step in the future.
I think right now it is fine to use btools "built-in" thread abstraction. I would suggest to rather have this provided as a parameter like on all the btools APIs to give flexibility to the user regarding which threading implementation to use. I just don't see the use-case needing this right now.
So good as is, but maybe something to keep in mind for a small refactoring step in the future.
lgtm. I suggest to do a full sqash to a single commit and provide an elaborate message of what you did and some reasoning for some key aspects, e.g. the threading part.
lgtm. I suggest to do a full sqash to a single commit and provide an elaborate message of what you did and some reasoning for some key aspects, e.g. the threading part.
it's already a static (above) he just didn't carry over the static declaration down. The rest of the locks and stuff though should also be static.
In GitLab by @xyen on May 3, 2021, 18:28
Commented on [src/main/aciodrv-proc/panb.c line 20](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..ecb3859ab83dd0c3aae5bd1b3706a342e751d434#diff-dd2580e6a5ce21b849d1afb199c83eb9R20)
it's already a static (above) he just didn't carry over the static declaration down. The rest of the locks and stuff though should also be static.
Actually, this should be changed thread_create lest we forget later. (it's already bound to crt_thread_create by default).
Having the threads functions be passed in is NOT the solution here I think, it should only be passed when it crosses a memory space boundary, since otherwise the actual IO DLL itself should be receiving the thread functions and calling thread_api_init.
In GitLab by @xyen on May 3, 2021, 18:28
Commented on [src/main/aciodrv-proc/panb.c line 52](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..ecb3859ab83dd0c3aae5bd1b3706a342e751d434#diff-dd2580e6a5ce21b849d1afb199c83eb9R52)
Actually, this should be changed `thread_create` lest we forget later. (it's already bound to `crt_thread_create` by default).
Having the threads functions be passed in is NOT the solution here I think, it should only be passed when it crosses a memory space boundary, since otherwise the actual IO DLL itself should be receiving the thread functions and calling `thread_api_init`.
In GitLab by @shtokopep on May 3, 2021, 20:06
added 1 commit
<ul><li>12ab02df - review part 3</li></ul>
[Compare with previous version](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1646&start_sha=ecb3859ab83dd0c3aae5bd1b3706a342e751d434)
In GitLab by @shtokopep on May 3, 2021, 20:06
Commented on [src/main/aciodrv-proc/panb.c line 52](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..ecb3859ab83dd0c3aae5bd1b3706a342e751d434#diff-dd2580e6a5ce21b849d1afb199c83eb9R52)
changed this line in [version 4 of the diff](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1646&start_sha=ecb3859ab83dd0c3aae5bd1b3706a342e751d434#7a472b160be3343313680eb5283b37c15beeb481_52_52)
In GitLab by @shtokopep on May 3, 2021, 20:06
Commented on [src/main/aciodrv-proc/panb.c line 20](https://github.com/djhackersdev/bemanitools/compare/c3453adc42796e1a22533a987a5f6a472726af30..ecb3859ab83dd0c3aae5bd1b3706a342e751d434#diff-dd2580e6a5ce21b849d1afb199c83eb9R20)
changed this line in [version 4 of the diff](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1646&start_sha=ecb3859ab83dd0c3aae5bd1b3706a342e751d434#7a472b160be3343313680eb5283b37c15beeb481_20_20)
In GitLab by @shtokopep on May 3, 2021, 20:15
added 4 commits
<ul><li>12ab02df...d3026cd5 - 3 commits from branch <code>djhackers:master</code></li><li>f88ffdb8 - aciodrv: add PANB support (+ aciotest handler)</li></ul>
[Compare with previous version](/djhackers/bemanitools/-/merge_requests/96/diffs?diff_id=1648&start_sha=12ab02df41b1333ffe8be6fc1768f2a7243f1d3a)
Thanks for the feedback, I think I took care of everything, rebased and squashed, should be good to go :)
In GitLab by @shtokopep on May 3, 2021, 20:16
Thanks for the feedback, I think I took care of everything, rebased and squashed, should be good to go :)
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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 :
Checklist
Tested with the following games:aciodrv cannot be used with the gameUpdated existing doc of or add new doc to README file(s).aciotest doc didn't provide such level of detailUpdated development documentation.didn't find aciodrv details in the docsIn 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 completedIn 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 incompleteIn 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 completedIn 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 completedNit: Naming-wise, I would stick to an
outprefix for out data like the names of the other structs in this unionMinor improvement: You could use a struct to actually put this comment into code, e.g.
Actually, maybe you can even align this nicely with the total number of keys avoid this key pair construct:
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 28Same for the magic number 14 above that:Not needed anymore if my above suggestion is do-able.#define AC_IO_PANB_MAX_KEY_PAIRS 14Do 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 :
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
panbmodule stick to how the other modules have done it so far and add anotherpanb-procmodule that takes care of threading and synchronisation. Documentation on thepanbmodule should include stating the issue and advice to use thepanb-procmodule.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 ?
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
valuefield since this is wrapped in a struct. I guess what you want is: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:
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 :
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
1in#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: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.
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on src/main/acio/acio.h line 84
changed this line in version 2 of the diff
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on src/main/acio/panb.h line 17
changed this line in version 2 of the diff
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on src/main/acio/panb.h line 28
changed this line in version 2 of the diff
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on src/main/aciodrv/device.c line 91
changed this line in version 2 of the diff
In GitLab by @shtokopep on May 2, 2021, 22:44
Commented on src/main/aciodrv/device.c line 205
changed this line in version 2 of the diff
In GitLab by @shtokopep on May 2, 2021, 22:44
added 1 commit
Compare with previous version
Clear. Thanks for clarifying.
That is my primary intention with this approach.
In GitLab by @shtokopep on May 3, 2021, 01:18
added 2 commits
Compare with previous version
In GitLab by @shtokopep on May 3, 2021, 01:18
Commented on src/main/aciodrv/panb.h line 7
changed this line in version 3 of the diff
In GitLab by @shtokopep on May 3, 2021, 01:26
Commented on src/main/aciodrv/panb.h line 7
A new
aciodrv-procmodule has been added (even though @xyen confirmed that PANB was the only device from libacio that would require threading as of now, and there's little chance that konami adds new ones in the future, I feelpanb-procmight become ambiguous when the acioemu part of panb will come into play)I guess that is fine. If things can be iterated/refactored easily, that's better than leaving things in a difficult to manage/static state.
Nit: I don't see that one exposed, so I guess you can make it a static function.
I think right now it is fine to use btools "built-in" thread abstraction. I would suggest to rather have this provided as a parameter like on all the btools APIs to give flexibility to the user regarding which threading implementation to use. I just don't see the use-case needing this right now.
So good as is, but maybe something to keep in mind for a small refactoring step in the future.
lgtm. I suggest to do a full sqash to a single commit and provide an elaborate message of what you did and some reasoning for some key aspects, e.g. the threading part.
In GitLab by @xyen on May 3, 2021, 18:28
Commented on src/main/aciodrv-proc/panb.c line 20
it's already a static (above) he just didn't carry over the static declaration down. The rest of the locks and stuff though should also be static.
In GitLab by @xyen on May 3, 2021, 18:28
Commented on src/main/aciodrv-proc/panb.c line 52
Actually, this should be changed
thread_createlest we forget later. (it's already bound tocrt_thread_createby default).Having the threads functions be passed in is NOT the solution here I think, it should only be passed when it crosses a memory space boundary, since otherwise the actual IO DLL itself should be receiving the thread functions and calling
thread_api_init.In GitLab by @shtokopep on May 3, 2021, 20:03
resolved all threads
In GitLab by @shtokopep on May 3, 2021, 20:06
added 1 commit
Compare with previous version
In GitLab by @shtokopep on May 3, 2021, 20:06
Commented on src/main/aciodrv-proc/panb.c line 52
changed this line in version 4 of the diff
In GitLab by @shtokopep on May 3, 2021, 20:06
Commented on src/main/aciodrv-proc/panb.c line 20
changed this line in version 4 of the diff
In GitLab by @shtokopep on May 3, 2021, 20:15
added 4 commits
djhackers:masterf88ffdb8- aciodrv: add PANB support (+ aciotest handler)Compare with previous version
In GitLab by @shtokopep on May 3, 2021, 20:16
Thanks for the feedback, I think I took care of everything, rebased and squashed, should be good to go :)
yeah, that's lgtm. Great first contribution and happy to see others getting on-board.
approved this merge request
resolved all threads