In GitLab by @darkstar on Nov 12, 2019, 24:48
_Merges sdvx-tyfp -> master_
Stops SDVX hanging at the thankyou for playing screen
This matches what ACrealIO does.
Fixes #21
Not sure what determines whether the status code 1 or 4 should be returned and I don't have access to any real hardware to see what it does.
Tested this change on SDVX 4, IIDX23, IIDX26
Presumably bemanitools v4 did this too though I've no idea why its different in v5, whether the change was intentional and whether changing it like this will break anything else.
In GitLab by @darkstar on Nov 12, 2019, 01:17
Not sure what determines whether the status code 1 or 4 should be returned and I don't have access to any real hardware to see what it does.
Tested this change on SDVX 4, IIDX23, IIDX26
Presumably bemanitools v4 did this too though I've no idea why its different in v5, whether the change was intentional and whether changing it like this will break anything else.
This is great. I am curious, how did you figure this out?
I would like to get this tested at least on IIDX13 as well because this was one of the first games introducing the old slotted reader and ACIO. The acio client implementation was more likely to fail with errors on IIDX13 though everything was fine on newer versions.
Afaik, bemanitools 4 used a re-implementation of the higher level acio library functions and did not implement its hooks on the acio protocol level. Therefore, this source of error was not even possible there.
This is great. I am curious, how did you figure this out?
I would like to get this tested at least on IIDX13 as well because this was one of the first games introducing the old slotted reader and ACIO. The acio client implementation was more likely to fail with errors on IIDX13 though everything was fine on newer versions.
Afaik, bemanitools 4 used a re-implementation of the higher level acio library functions and did not implement its hooks on the acio protocol level. Therefore, this source of error was not even possible there.
After a while reading and debugging and seeing that it basically intercepts the calls which would go through the serial port, I wanted to compare it to what actual hardware would do. Not having any actual hardware, I compared it with what ACrealIO would do and saw that it returns a 4 rather than 1 when no card is present.
I tried that change and found it worked.
In GitLab by @darkstar on Nov 12, 2019, 19:27
After a while reading and debugging and seeing that it basically intercepts the calls which would go through the serial port, I wanted to compare it to what actual hardware would do. Not having any actual hardware, I compared it with what ACrealIO would do and saw that it returns a `4` rather than `1` when no card is present.
I tried that change and found it worked.
Ahhhhh so that's what it was. I was planning on taking a look once I got access to my cab again, but this makes sense, nice catch.
Echoing what icex2 said, BT4 doesn't emulate the device itself which is why this is a "new" issue.
Another thing to note is that ACrealIO actually set it to 1 when using old readers, (which, given the emulation code, is how the old slotted readers actually responded), so it'd be best to change the response based on what kinda reader is detected.
Basically, add the new enum as AC_IO_ICCA_STATUS_IDLE_NEW, and change line 321 to:
In GitLab by @xyen on Nov 12, 2019, 20:02
Ahhhhh so that's what it was. I was planning on taking a look once I got access to my cab again, but this makes sense, nice catch.
Echoing what icex2 said, BT4 doesn't emulate the device itself which is why this is a "new" issue.
Another thing to note is that ACrealIO actually set it to `1` when using old readers, (which, given the emulation code, is how the old slotted readers actually responded), so it'd be best to change the response based on what kinda reader is detected.
Basically, add the new enum as AC_IO_ICCA_STATUS_IDLE_NEW, and change line 321 to:
```cpp
if (icca->detected_new_reader) {
body->status_code = AC_IO_ICCA_STATUS_IDLE_NEW;
} else {
body->status_code = AC_IO_ICCA_STATUS_IDLE;
}
```
@xyen's proposal is important to have the code running as either slotted or wave pass reader as far as I can tell by looking at the code? Just a minor nit here to make this part clear in the future: name the "detected_new_reader" flag to "detected_wave_pass_reader" as there are also other reader types like ICCB, ICCC (?) which I would have considered as a "new" reader. The ICCA type is kinda mixed here and, to me, caused quite some confusion in the past already.
@xyen's proposal is important to have the code running as either slotted or wave pass reader as far as I can tell by looking at the code? Just a minor nit here to make this part clear in the future: name the "detected_new_reader" flag to "detected_wave_pass_reader" as there are also other reader types like ICCB, ICCC (?) which I would have considered as a "new" reader. The ICCA type is kinda mixed here and, to me, caused quite some confusion in the past already.
So ICCB being treated differently is actually kinda a lie, it's that jubeat talks to the reader slightly differently, realistically icca should be renamed / split into iccx-slotted and iccx-wavepass or something, and iccb to iccx-jubeat or merged into iccx-wavepass.
ICCA/B/C all support the same messages from what I can tell. (the SDVX cab I have in fact, has an ICCB, that is talked to as listed as we emulate ICCA, but with encryption on).
In GitLab by @xyen on Nov 12, 2019, 20:13
So ICCB being treated differently is actually kinda a lie, it's that jubeat talks to the reader slightly differently, realistically icca should be renamed / split into iccx-slotted and iccx-wavepass or something, and iccb to iccx-jubeat or merged into iccx-wavepass.
ICCA/B/C all support the same messages from what I can tell. (the SDVX cab I have in fact, has an ICCB, that is talked to as listed as we emulate ICCA, but with encryption on).
@icex2 I'd leave the naming as-is for now, and just request the change to returning different statuses based on detected type.
In GitLab by @xyen on Nov 12, 2019, 20:13
@icex2 I'd leave the naming as-is for now, and just request the change to returning different statuses based on detected type.
Oh, interesting. That sounds like the whole acio card reader related would need some slight refactoring to split this nicely (with some shared/duplicate code).
Yeah, then leave the naming as it does not solve the root issue we are facing with properly seperating this. Just have the branch to reply with a different status code for now.
I just opened an issue to keep that documented: #38
Oh, interesting. That sounds like the whole acio card reader related would need some slight refactoring to split this nicely (with some shared/duplicate code).
Yeah, then leave the naming as it does not solve the root issue we are facing with properly seperating this. Just have the branch to reply with a different status code for now.
I just opened an issue to keep that documented: #38
In GitLab by @darkstar on Nov 12, 2019, 23:27
added 1 commit
<ul><li>dceb763b - ICCA - set idle status code to 0x04 for wavepass readers</li></ul>
[Compare with previous version](/djhackers/bemanitools/merge_requests/10/diffs?diff_id=1072&start_sha=13282c0d1ff16792b8883f131cf33295b6d17bd8)
In GitLab by @darkstar on Nov 12, 2019, 23:30
added 1 commit
<ul><li>8288d1f8 - ICCA - set idle status code to 0x04 for new readers</li></ul>
[Compare with previous version](/djhackers/bemanitools/merge_requests/10/diffs?diff_id=1073&start_sha=dceb763b0f1d59ad53bd57f0ef99621f583e8578)
The formatting is broken. Please fix this by using 4 spaces as one tab. You can also just run clang-format with the included formatter file, e.g. make clang-format.
The formatting is broken. Please fix this by using 4 spaces as one tab. You can also just run clang-format with the included formatter file, e.g. make clang-format.
In GitLab by @darkstar on Nov 13, 2019, 01:53
Commented on [src/main/acioemu/icca.c line 35](https://github.com/djhackersdev/bemanitools/compare/6da3732968a35ea192df8fb174ed93602a2a16ba..8288d1f828c73b5deb973464dfb2e8e1f829f99b#diff-affab87058fe7f10f8cdc694a83c62e0R35)
changed this line in [version 4 of the diff](/djhackers/bemanitools/merge_requests/10/diffs?diff_id=1074&start_sha=8288d1f828c73b5deb973464dfb2e8e1f829f99b#afe0e349cab0b6e39bf4435fdcb044da5be5b82e_35_35)
In GitLab by @darkstar on Nov 13, 2019, 01:53
Commented on [src/main/acioemu/icca.c line 323](https://github.com/djhackersdev/bemanitools/compare/6da3732968a35ea192df8fb174ed93602a2a16ba..8288d1f828c73b5deb973464dfb2e8e1f829f99b#diff-affab87058fe7f10f8cdc694a83c62e0R323)
changed this line in [version 4 of the diff](/djhackers/bemanitools/merge_requests/10/diffs?diff_id=1074&start_sha=8288d1f828c73b5deb973464dfb2e8e1f829f99b#afe0e349cab0b6e39bf4435fdcb044da5be5b82e_323_322)
In GitLab by @darkstar on Nov 13, 2019, 01:53
added 1 commit
<ul><li>fd9b8439 - Stop SDVX hanging on thankyou for playing</li></ul>
[Compare with previous version](/djhackers/bemanitools/merge_requests/10/diffs?diff_id=1074&start_sha=8288d1f828c73b5deb973464dfb2e8e1f829f99b)
Just spent a while working out why my fix didn't work any more. Turns out that when I was undoing all my debugging and experiements to get it ready to merge, I'd undone another important part. I assume I must have messed up when I tested the final code.
In addition to changing the idle status code, I'd also hard coded keypad_status to 0x03, even if keypad_started wasn't set.
From my log file, it looks like SDVX4 doesn't ever send the AC_IO_ICCA_CMD_BEGIN_KEYPAD command so this flag is never set.
Given the feedback on the first change, I've also limited the second change to when the detected_new_reader flag is set.
Both changes are required, neither alone is sufficient.
In GitLab by @darkstar on Nov 13, 2019, 02:00
Just spent a while working out why my fix didn't work any more. Turns out that when I was undoing all my debugging and experiements to get it ready to merge, I'd undone another important part. I assume I must have messed up when I tested the final code.
In addition to changing the idle status code, I'd also hard coded `keypad_status` to `0x03`, even if `keypad_started` wasn't set.
From my log file, it looks like SDVX4 doesn't ever send the `AC_IO_ICCA_CMD_BEGIN_KEYPAD` command so this flag is never set.
Given the feedback on the first change, I've also limited the second change to when the `detected_new_reader` flag is set.
Both changes are required, neither alone is sufficient.
In GitLab by @darkstar on Nov 13, 2019, 02:01
Commented on [src/main/acioemu/icca.c line 35](https://github.com/djhackersdev/bemanitools/compare/6da3732968a35ea192df8fb174ed93602a2a16ba..8288d1f828c73b5deb973464dfb2e8e1f829f99b#diff-affab87058fe7f10f8cdc694a83c62e0R35)
Now fixed
In GitLab by @darkstar on Nov 13, 2019, 02:01
Commented on [src/main/acioemu/icca.c line 323](https://github.com/djhackersdev/bemanitools/compare/6da3732968a35ea192df8fb174ed93602a2a16ba..8288d1f828c73b5deb973464dfb2e8e1f829f99b#diff-affab87058fe7f10f8cdc694a83c62e0R323)
Fixed
So, when I was testing out the wavepass readers for aciodrv, it turns out the status codes mean something slightly different for them:
1 - BUSY
2 - UID READY
4 - IDLE (or no card read)
This explains why the game gets stuck probably, as it probably thinks the reader is stuck trying to poll a card.
In GitLab by @xyen on Mar 17, 2021, 24:26
So, when I was testing out the wavepass readers for aciodrv, it turns out the status codes mean something slightly different for them:
1 - BUSY
2 - UID READY
4 - IDLE (or no card read)
This explains why the game gets stuck probably, as it probably thinks the reader is stuck trying to poll a card.
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 @darkstar on Nov 12, 2019, 24:48
Merges sdvx-tyfp -> master
Stops SDVX hanging at the thankyou for playing screen
This matches what ACrealIO does.
Fixes #21
In GitLab by @darkstar on Nov 12, 2019, 01:17
Not sure what determines whether the status code 1 or 4 should be returned and I don't have access to any real hardware to see what it does.
Tested this change on SDVX 4, IIDX23, IIDX26
Presumably bemanitools v4 did this too though I've no idea why its different in v5, whether the change was intentional and whether changing it like this will break anything else.
This is great. I am curious, how did you figure this out?
I would like to get this tested at least on IIDX13 as well because this was one of the first games introducing the old slotted reader and ACIO. The acio client implementation was more likely to fail with errors on IIDX13 though everything was fine on newer versions.
Afaik, bemanitools 4 used a re-implementation of the higher level acio library functions and did not implement its hooks on the acio protocol level. Therefore, this source of error was not even possible there.
In GitLab by @darkstar on Nov 12, 2019, 19:27
After a while reading and debugging and seeing that it basically intercepts the calls which would go through the serial port, I wanted to compare it to what actual hardware would do. Not having any actual hardware, I compared it with what ACrealIO would do and saw that it returns a
4rather than1when no card is present.I tried that change and found it worked.
In GitLab by @xyen on Nov 12, 2019, 20:02
Ahhhhh so that's what it was. I was planning on taking a look once I got access to my cab again, but this makes sense, nice catch.
Echoing what icex2 said, BT4 doesn't emulate the device itself which is why this is a "new" issue.
Another thing to note is that ACrealIO actually set it to
1when using old readers, (which, given the emulation code, is how the old slotted readers actually responded), so it'd be best to change the response based on what kinda reader is detected.Basically, add the new enum as AC_IO_ICCA_STATUS_IDLE_NEW, and change line 321 to:
@xyen's proposal is important to have the code running as either slotted or wave pass reader as far as I can tell by looking at the code? Just a minor nit here to make this part clear in the future: name the "detected_new_reader" flag to "detected_wave_pass_reader" as there are also other reader types like ICCB, ICCC (?) which I would have considered as a "new" reader. The ICCA type is kinda mixed here and, to me, caused quite some confusion in the past already.
In GitLab by @xyen on Nov 12, 2019, 20:13
So ICCB being treated differently is actually kinda a lie, it's that jubeat talks to the reader slightly differently, realistically icca should be renamed / split into iccx-slotted and iccx-wavepass or something, and iccb to iccx-jubeat or merged into iccx-wavepass.
ICCA/B/C all support the same messages from what I can tell. (the SDVX cab I have in fact, has an ICCB, that is talked to as listed as we emulate ICCA, but with encryption on).
In GitLab by @xyen on Nov 12, 2019, 20:13
@icex2 I'd leave the naming as-is for now, and just request the change to returning different statuses based on detected type.
Oh, interesting. That sounds like the whole acio card reader related would need some slight refactoring to split this nicely (with some shared/duplicate code).
Yeah, then leave the naming as it does not solve the root issue we are facing with properly seperating this. Just have the branch to reply with a different status code for now.
I just opened an issue to keep that documented: #38
In GitLab by @darkstar on Nov 12, 2019, 23:27
added 1 commit
Compare with previous version
In GitLab by @darkstar on Nov 12, 2019, 23:30
added 1 commit
Compare with previous version
Same here, formatting.
The formatting is broken. Please fix this by using 4 spaces as one tab. You can also just run clang-format with the included formatter file, e.g. make clang-format.
In GitLab by @darkstar on Nov 13, 2019, 24:15
Ah rats, sorry, I'll fix that.
In GitLab by @darkstar on Nov 13, 2019, 01:53
Commented on src/main/acioemu/icca.c line 35
changed this line in version 4 of the diff
In GitLab by @darkstar on Nov 13, 2019, 01:53
Commented on src/main/acioemu/icca.c line 323
changed this line in version 4 of the diff
In GitLab by @darkstar on Nov 13, 2019, 01:53
added 1 commit
fd9b8439- Stop SDVX hanging on thankyou for playingCompare with previous version
In GitLab by @darkstar on Nov 13, 2019, 02:00
Just spent a while working out why my fix didn't work any more. Turns out that when I was undoing all my debugging and experiements to get it ready to merge, I'd undone another important part. I assume I must have messed up when I tested the final code.
In addition to changing the idle status code, I'd also hard coded
keypad_statusto0x03, even ifkeypad_startedwasn't set.From my log file, it looks like SDVX4 doesn't ever send the
AC_IO_ICCA_CMD_BEGIN_KEYPADcommand so this flag is never set.Given the feedback on the first change, I've also limited the second change to when the
detected_new_readerflag is set.Both changes are required, neither alone is sufficient.
In GitLab by @darkstar on Nov 13, 2019, 02:01
Commented on src/main/acioemu/icca.c line 35
Now fixed
In GitLab by @darkstar on Nov 13, 2019, 02:01
resolved all threads
In GitLab by @darkstar on Nov 13, 2019, 02:01
Commented on src/main/acioemu/icca.c line 323
Fixed
In GitLab by @darkstar on Nov 13, 2019, 02:04
Oh, and I've tested this change against IIDX(13,23,26) and SDVX(3,4)
In GitLab by @xyen on Nov 13, 2019, 14:56
merged
In GitLab by @xyen on Nov 13, 2019, 14:56
lgtm, merging.
Well done @darkstar, thanks a lot for the fix.
In GitLab by @xyen on Mar 17, 2021, 24:26
So, when I was testing out the wavepass readers for aciodrv, it turns out the status codes mean something slightly different for them:
1 - BUSY
2 - UID READY
4 - IDLE (or no card read)
This explains why the game gets stuck probably, as it probably thinks the reader is stuck trying to poll a card.