Add bi2a-iidx support for aciotest #263

Merged
MatryoshkaLJY merged 5 commits from master into master 2023-10-19 00:06:11 +03:00
MatryoshkaLJY commented 2023-10-12 15:26:33 +03:00 (Migrated from github.com)
No description provided.
icex2 (Migrated from github.com) reviewed 2023-10-12 22:07:18 +03:00
icex2 (Migrated from github.com) commented 2023-10-12 22:07:18 +03:00

I guess that works, but compared to the other ways it detects different devices, it feels a bit off.
Do we ever have multiple acio devices on the same bus with any BI2A device? To my knowledge, the BI2A device is always on its own bus exclusively (please correct me here if I am wrong).
Assuming that's the case, I suggest to extend the tool rather to allow selecting different "kinds" of devices.

Examples how to call aciotest:

  • aciotest.exe COM1 57600 for a "generic" acio device that does the entire "go by product code" thing
  • aciotest.exe COM1 57600 bi2a-iidx for the iidx BI2A mode
  • aciotest.exe COM1 57600 bi2a-sdvx for the sdvx BI2A mode

Mind tweaking the tool a little to have it work like that?

The actual extension of the tool already looks good to me.

I guess that works, but compared to the other ways it detects different devices, it feels a bit off. Do we ever have multiple acio devices on the same bus with any BI2A device? To my knowledge, the BI2A device is always on its own bus exclusively (please correct me here if I am wrong). Assuming that's the case, I suggest to extend the tool rather to allow selecting different "kinds" of devices. Examples how to call `aciotest`: * `aciotest.exe COM1 57600` for a "generic" acio device that does the entire "go by product code" thing * `aciotest.exe COM1 57600 bi2a-iidx` for the iidx BI2A mode * `aciotest.exe COM1 57600 bi2a-sdvx` for the sdvx BI2A mode Mind tweaking the tool a little to have it work like that? The actual extension of the tool already looks good to me.
MatryoshkaLJY (Migrated from github.com) reviewed 2023-10-13 11:29:13 +03:00
MatryoshkaLJY (Migrated from github.com) commented 2023-10-13 11:29:13 +03:00

Sorted!

Sorted!
icex2 (Migrated from github.com) reviewed 2023-10-14 11:10:33 +03:00
icex2 (Migrated from github.com) commented 2023-10-14 11:10:33 +03:00

Non-blocking remark to take away as a learning: An enum would improve readability here, e.g.

enum BI2A_MODE {
    BI2A_MODE_INVALID = 0,
    BI2A_MODE_SDVX = 1,
    BI2A_MODE_IIDX = 2,
};

If you want, you can follow up with another small refactoring PR to improve this. The current solution is fine though considering it's just two modes now and scoped to a single file.

Non-blocking remark to take away as a learning: An enum would improve readability here, e.g. ``` enum BI2A_MODE { BI2A_MODE_INVALID = 0, BI2A_MODE_SDVX = 1, BI2A_MODE_IIDX = 2, }; ``` If you want, you can follow up with another small refactoring PR to improve this. The current solution is fine though considering it's just two modes now and scoped to a single file.
icex2 (Migrated from github.com) approved these changes 2023-10-14 11:11:19 +03:00
icex2 (Migrated from github.com) left a comment

Looking good, thanks for following up on the suggestion.

Looking good, thanks for following up on the suggestion.
icex2 commented 2023-10-14 11:13:12 +03:00 (Migrated from github.com)

"This branch cannot be rebased due to conflicts"

Not sure where the conflicts are. It doesn't mention any specific files. Can you check on your end if you are up to date with djhackersdev:master and fix that if needed?

Otherwise, I can't merge it.

"This branch cannot be rebased due to conflicts" Not sure where the conflicts are. It doesn't mention any specific files. Can you check on your end if you are up to date with [djhackersdev:master](https://github.com/djhackersdev/bemanitools/tree/master) and fix that if needed? Otherwise, I can't merge it.
MatryoshkaLJY (Migrated from github.com) reviewed 2023-10-14 13:02:25 +03:00
MatryoshkaLJY (Migrated from github.com) commented 2023-10-14 13:02:25 +03:00

Sure, I'll do it later

Sure, I'll do it later
icex2 commented 2023-10-19 00:06:23 +03:00 (Migrated from github.com)

Thanks for following up. Merged.

Thanks for following up. Merged.
Sign in to join this conversation.